Summary: | [JSC] Fix Date functions' argument coercion | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ashvayka, darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Yusuke Suzuki
2022-01-15 12:37:09 PST
Created attachment 449265 [details]
Patch
Comment on attachment 449265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449265&action=review Super neat! > Source/JavaScriptCore/ChangeLog:9 > + arguments to Number since it has observable side effect. We might want to link spec PR: https://github.com/tc39/ecma262/pull/2136. (In reply to Alexey Shvayka from comment #2) > Comment on attachment 449265 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449265&action=review > > Super neat! > > > Source/JavaScriptCore/ChangeLog:9 > > + arguments to Number since it has observable side effect. > > We might want to link spec PR: https://github.com/tc39/ecma262/pull/2136. Nice, done. Committed r288066 (246086@trunk): <https://commits.webkit.org/246086@trunk> Comment on attachment 449265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449265&action=review Love patches like this one, beautifully done. > Source/JavaScriptCore/runtime/DatePrototype.cpp:113 > +static void applyToNumbersToTrashedArguments(JSGlobalObject* globalObject, CallFrame* callFrame, unsigned maxArgs) Terminology nitpicks: I would have called these "otherwise ignored" arguments, not "trashed" arguments. Also, should be "ToNumber" not "ToNumbers". > Source/JavaScriptCore/runtime/DatePrototype.cpp:118 > + unsigned numArgs = std::min<unsigned>(callFrame->argumentCount(), maxArgs); Not a problem in practice, but a subtle point about what <unsigned> means here is that that we *chop* argumentCount from size_t to unsigned, ignoring the high bits. So if we had a colossal argument count (which I suppose is impossible?) we could overflow and not apply toNumber to some of the arguments (which I suppose is relatively harmless). Not sure why size_t was the right type for argumentCount and yet unsigned is the right type for maxArgs, but this is the kind of thing that worries me when I see it, in principle. > Source/JavaScriptCore/runtime/DatePrototype.cpp:136 > + unsigned numArgs = std::min<unsigned>(callFrame->argumentCount(), maxArgs); Same thing here. > Source/JavaScriptCore/runtime/DatePrototype.cpp:185 > + unsigned numArgs = std::min<unsigned>(callFrame->argumentCount(), maxArgs); Here too. > Source/JavaScriptCore/runtime/DatePrototype.cpp:191 > + ok = (ok && std::isfinite(years)); Not sure the parentheses add clarity. Comment on attachment 449265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449265&action=review Thanks! >> Source/JavaScriptCore/runtime/DatePrototype.cpp:113 >> +static void applyToNumbersToTrashedArguments(JSGlobalObject* globalObject, CallFrame* callFrame, unsigned maxArgs) > > Terminology nitpicks: I would have called these "otherwise ignored" arguments, not "trashed" arguments. Also, should be "ToNumber" not "ToNumbers". Right, changed. >> Source/JavaScriptCore/runtime/DatePrototype.cpp:118 >> + unsigned numArgs = std::min<unsigned>(callFrame->argumentCount(), maxArgs); > > Not a problem in practice, but a subtle point about what <unsigned> means here is that that we *chop* argumentCount from size_t to unsigned, ignoring the high bits. So if we had a colossal argument count (which I suppose is impossible?) we could overflow and not apply toNumber to some of the arguments (which I suppose is relatively harmless). > > Not sure why size_t was the right type for argumentCount and yet unsigned is the right type for maxArgs, but this is the kind of thing that worries me when I see it, in principle. Actually, argumentCount() is stored in 32bit field, so it is unsigned (since it is never negative) :) >> Source/JavaScriptCore/runtime/DatePrototype.cpp:136 >> + unsigned numArgs = std::min<unsigned>(callFrame->argumentCount(), maxArgs); > > Same thing here. Ditto. >> Source/JavaScriptCore/runtime/DatePrototype.cpp:185 >> + unsigned numArgs = std::min<unsigned>(callFrame->argumentCount(), maxArgs); > > Here too. Ditto. >> Source/JavaScriptCore/runtime/DatePrototype.cpp:191 >> + ok = (ok && std::isfinite(years)); > > Not sure the parentheses add clarity. Removed. Committed r288399 (246290@trunk): <https://commits.webkit.org/246290@trunk> Comment on attachment 449265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449265&action=review >>> Source/JavaScriptCore/runtime/DatePrototype.cpp:118 >>> + unsigned numArgs = std::min<unsigned>(callFrame->argumentCount(), maxArgs); >> >> Not a problem in practice, but a subtle point about what <unsigned> means here is that that we *chop* argumentCount from size_t to unsigned, ignoring the high bits. So if we had a colossal argument count (which I suppose is impossible?) we could overflow and not apply toNumber to some of the arguments (which I suppose is relatively harmless). >> >> Not sure why size_t was the right type for argumentCount and yet unsigned is the right type for maxArgs, but this is the kind of thing that worries me when I see it, in principle. > > Actually, argumentCount() is stored in 32bit field, so it is unsigned (since it is never negative) :) Maybe we should use unsigned or uint32_t instead of size_t for the return type for the argumentCount() function, then? Comment on attachment 449265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449265&action=review >>>> Source/JavaScriptCore/runtime/DatePrototype.cpp:118 >>>> + unsigned numArgs = std::min<unsigned>(callFrame->argumentCount(), maxArgs); >>> >>> Not a problem in practice, but a subtle point about what <unsigned> means here is that that we *chop* argumentCount from size_t to unsigned, ignoring the high bits. So if we had a colossal argument count (which I suppose is impossible?) we could overflow and not apply toNumber to some of the arguments (which I suppose is relatively harmless). >>> >>> Not sure why size_t was the right type for argumentCount and yet unsigned is the right type for maxArgs, but this is the kind of thing that worries me when I see it, in principle. >> >> Actually, argumentCount() is stored in 32bit field, so it is unsigned (since it is never negative) :) > > Maybe we should use unsigned or uint32_t instead of size_t for the return type for the argumentCount() function, then? Yeah, it should be. But we need to carefully review whether the existing code is effectively using size_t to handle some cases in that change :) |