Bug 235271 – [JSC] Fix Date functions' argument coercion

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Fix Date functions' argument coercion

[Yusuke Suzuki]

Created attachment 449265 [details]
Patch

[Alexey Shvayka]

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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Committed r288066 (246086@trunk): <https://commits.webkit.org/246086@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/87641688>

[Darin Adler]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r288399 (246290@trunk): <https://commits.webkit.org/246290@trunk>

[Darin Adler]

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?

[Yusuke Suzuki]

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 :)