Bug 235271

Summary: [JSC] Fix Date functions' argument coercion
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: 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 Flags
Patch ashvayka: review+

Description Yusuke Suzuki 2022-01-15 12:37:09 PST
[JSC] Fix Date functions' argument coercion
Comment 1 Yusuke Suzuki 2022-01-15 12:39:17 PST
Created attachment 449265 [details]
Patch
Comment 2 Alexey Shvayka 2022-01-15 12:44:38 PST
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.
Comment 3 Yusuke Suzuki 2022-01-15 12:58:40 PST
(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.
Comment 4 Yusuke Suzuki 2022-01-15 13:35:12 PST
Committed r288066 (246086@trunk): <https://commits.webkit.org/246086@trunk>
Comment 5 Radar WebKit Bug Importer 2022-01-15 13:36:16 PST
<rdar://problem/87641688>
Comment 6 Darin Adler 2022-01-16 13:53:49 PST
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 7 Yusuke Suzuki 2022-01-21 18:59:55 PST
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.
Comment 8 Yusuke Suzuki 2022-01-21 19:03:12 PST
Committed r288399 (246290@trunk): <https://commits.webkit.org/246290@trunk>
Comment 9 Darin Adler 2022-01-22 13:47:55 PST
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 10 Yusuke Suzuki 2022-01-22 16:21:50 PST
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 :)