Bug 181182 – [JSC] NumberPrototype::extractRadixFromArgs incorrectly cast double to int32_t

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

current implementation is returning ```static_cast<int32_t>(radixValue.toInteger(exec))``` when radixValue isn't int32 or undefined. It can generate undesired behavior when ```radixValue.toInteger``` returns a value that is not in ```int32_t``` range.

[Caio Lima]

Created attachment 330384 [details]
Patch

[Caio Lima]

Created attachment 330388 [details]
Patch

[Darin Adler]

Comment on attachment 330388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330388&action=review

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:114
> +        if (tempRadixValue >= minSafeInteger() && tempRadixValue <= maxSafeInteger())
> +            radix = static_cast<int64_t>(tempRadixValue);
> +        else
> +            return throwVMError(state, scope, createRangeError(state, ASCIILiteral("toString() radix argument must be between 2 and 36")));

Why minSafeInteger and maxSafeInteger? Why not just compare with 2 and 36?

Also, this gets rid of the reason for radix to be int64_t, so please change it to int32_t and get rid of the static_cast below.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:596
> +        double tempRadixValue = radixValue.toInteger(state);
> +        RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +        if (tempRadixValue >= minSafeInteger() && tempRadixValue <= maxSafeInteger())
> +            radix = static_cast<int64_t>(tempRadixValue);
> +        else
> +            return throwVMError(state, scope, createRangeError(state, ASCIILiteral("toString() radix argument must be between 2 and 36")));

Same comment as above. But also, it’s not good to have repeated code. Please refactor this in a future patch so we can share all the identical code. It is not good to have two copies.

[Caio Lima]

Comment on attachment 330388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330388&action=review

Thank you for the review

>> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:114
>> +            return throwVMError(state, scope, createRangeError(state, ASCIILiteral("toString() radix argument must be between 2 and 36")));
> 
> Why minSafeInteger and maxSafeInteger? Why not just compare with 2 and 36?
> 
> Also, this gets rid of the reason for radix to be int64_t, so please change it to int32_t and get rid of the static_cast below.

I'm worried about double precision, however, I'm going to follow your comment.

[Caio Lima]

Created attachment 330962 [details]
Patch

[Caio Lima]

(In reply to Caio Lima from comment #5)
> Created attachment 330962 [details]
> Patch

Changed Patch based on last Darin's comment. Since I changed a lot the code last Patch, I'm asking for a new Review.

[EWS Watchlist]

Comment on attachment 330962 [details]
Patch

Attachment 330962 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6026998

New failing tests:
webgl/1.0.2/conformance/uniforms/uniform-default-values.html

[EWS Watchlist]

Created attachment 330992 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Darin Adler]

Comment on attachment 330962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330962&action=review

Please add a test case for when the valueOf function throws an exception, making sure that the proper exception is propagated, not a different one. I believe this patch breaks that code path and causes it to throw a RangeError instead of propagating the existing exception, so we should make sure we have that tested.

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:102
> +    std::optional<int32_t> radix = extractRadix(state, state->argument(0));

We need a RETURN_IF_EXCEPTION here. Otherwise, we will call throwVMError and create a new exception rather than propagating the one that already happened.

Also, I suggest using auto instead of writing out std::optional<int32_t>.

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:104
> +    if (!radix || *radix < 2 || *radix > 36)

It’s not good  that half of the radix checking is inside extractRadix, and the other half is out here. We should put the range checking entirely inside the function.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:1130
> +inline std::optional<int32_t> extractRadix(ExecState* state, JSValue radixValue)

I suggest taking an ExecState& instead of an ExecState*.

This is the wrong place for this function. JSCJSValue.h and its inlines header, JSCJSValueInlines.h are just for the JSValue class itself. We should not put other arbitrary parts of the runtime in here. I suggest putting this into NumberPrototype.h.

Also not entirely sure this need to be marked inline. If we are so performance sensitive that we need inlining then the extra throw scopes and extra RETURN_IF_EXCEPTION caused by factoring this way might be too much.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:1133
> +        return radixValue.asInt32();

I suggest checking that the number is in the 2-36 range and returning nullopt otherwise.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:1139
> +    double tempRadixValue = radixValue.toInteger(state);

I don’t know why we are using "temp" in this local variable name. I would have called this radixDouble.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:1140
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());

The return value here needs to be a std::optional<int32_t>, so it should be std::nullopt, not encodedJSValue().

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:1141
> +    if (tempRadixValue >= 2 && tempRadixValue <= 32)

This should be 36, not 32.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:584
> +    std::optional<int32_t> radix = extractRadix(state, state->argument(0));

We need a RETURN_IF_EXCEPTION here. Otherwise, we will call throwVMError and create a new exception rather than propagating the one that already happened.

Also, I suggest using auto instead of writing out std::optional<int32_t>.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:587
> +    if (!radix || *radix < 2 || *radix > 36)
> +        return throwVMError(state, scope, createRangeError(state, ASCIILiteral("toString() radix argument must be between 2 and 36")));

Since this code is repeated at both call sites, I suggest changing extractRadix so that it throws this error rather than having the caller check and throw it. And then I would also rename it to extractToStringRadixArgument.

[Darin Adler]

Comment on attachment 330388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330388&action=review

>>> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:114
>>> +            return throwVMError(state, scope, createRangeError(state, ASCIILiteral("toString() radix argument must be between 2 and 36")));
>> 
>> Why minSafeInteger and maxSafeInteger? Why not just compare with 2 and 36?
>> 
>> Also, this gets rid of the reason for radix to be int64_t, so please change it to int32_t and get rid of the static_cast below.
> 
> I'm worried about double precision, however, I'm going to follow your comment.

What is your worry? You should translate your worry into a test case so you can stop worrying.

[Saam Barati]

Comment on attachment 330962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330962&action=review

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:1130
>> +inline std::optional<int32_t> extractRadix(ExecState* state, JSValue radixValue)
> 
> I suggest taking an ExecState& instead of an ExecState*.
> 
> This is the wrong place for this function. JSCJSValue.h and its inlines header, JSCJSValueInlines.h are just for the JSValue class itself. We should not put other arbitrary parts of the runtime in here. I suggest putting this into NumberPrototype.h.
> 
> Also not entirely sure this need to be marked inline. If we are so performance sensitive that we need inlining then the extra throw scopes and extra RETURN_IF_EXCEPTION caused by factoring this way might be too much.

I'm not making a value judgement of ExecState* vs ExecState&, but because we use ExecState* almost unilaterally throughout JSC, I'd vote for ExecState* just for consistency.

[Caio Lima]

Created attachment 331132 [details]
Patch

[Darin Adler]

Comment on attachment 331132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331132&action=review

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:628
> +        return throwVMError(state, scope, createRangeError(state, ASCIILiteral("toString() radix argument must be between 2 and 36")));

Using "else" we could share the two calls to throwVMError and createRangeError.

[Darin Adler]

Comment on attachment 331132 [details]
Patch

Could still add some test cases for other values such as the two infinities, NaN, booleans, strings, null, and negative zero. Not really needed, I guess.

[Caio Lima]

(In reply to Darin Adler from comment #13)
> Comment on attachment 331132 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331132&action=review
> 
> > Source/JavaScriptCore/runtime/NumberPrototype.cpp:628
> > +        return throwVMError(state, scope, createRangeError(state, ASCIILiteral("toString() radix argument must be between 2 and 36")));
> 
> Using "else" we could share the two calls to throwVMError and
> createRangeError.

Thanks for the Review!. I applied suggested changes and also added suggested test cases.

[Caio Lima]

Created attachment 331254 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 331254 [details]
Patch

Clearing flags on attachment: 331254

Committed r226937: <https://trac.webkit.org/changeset/226937>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36496249>

[Darin Adler]

One more idea for a last bit of refinement that I think is still worth it even though this is already landed: There is no need for the helper function to return an EncodedJSValue in the case of an exception. When an exception is present the return value of the host call is ignored; it does not matter what is returned so we don’t need to provide a value to the host functions. So we could simplify and get rid of the need for Variant in one of the two following ways:

Plan A:

    1a) Change the return type of extractToStringRadixArgument to be std::optional<int32_t> instead of Variant<int32_t, EncodedJSValue>.
    2a) Use throwRangeError instead of throwVMError. No need to separately call createRangeError and throw it, that's what the throwRangeError convenience function is for. No need to use throwVMRangeError since that's only a convenience to be used in a function where the return value is EncodedJSValue, and extractToStringRadixArgument no longer falls into that category.
    3a) Return std::nullopt after the call to throwRangeError, ignoring the return value of throwRangeError.
    4a) At the two call sites, return EncodedJSValue() when the radix is not present, since this is the conventional thing to return when letting an exception propagate. Ignored by the caller.

    auto radix = extractToStringRadixArgument(state, state->argument(0));
    if (!radix)
        return EncodedJSValue();

Plan B:

    1b) Change the return type of extractToStringRadixArgument to be int32_t instead of Variant<int32_t, EncodedJSValue>.
    2b) Use throwRangeError instead of throwVMError. Same rationale as 2a above.
    3b) Return 0 after the call to throwRangeError, ignoring the return value of throwRangeError.
    4b) Pass a ThrowScope& into extractToStringRadixArgument; use it instead of doing a DECLARE_THROW_SCOPE inside the function.
    5b) At the two call sites, use RETURN_IF_EXCEPTION before looking at the return value:

    int32_t radix = extractToStringRadixArgument(state, scope, state->argument(0));
    RETURN_IF_EXCEPTION(scope, encodedJSValue());

We don’t have to do either of these, but I think I like the idea of plan B since it is more like the standard idiom for bindings code.

[Ryan Haddad]

The tests added with this change are hitting an assertion failure:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/501

ERROR: Unchecked JS exception:
    This scope can throw a JS exception: extractToStringRadixArgument @ ./runtime/NumberPrototype.cpp:622
        (ExceptionScope::m_recursionDepth was 5)
    But the exception was unchecked as of this scope: bigIntProtoFuncToString @ ./runtime/BigIntPrototype.cpp:96
        (ExceptionScope::m_recursionDepth was 4)

Unchecked exception detected at:
    1   0x102bac313 JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
    2   0x102b883fb JSC::ThrowScope::~ThrowScope()
    3   0x102b887d5 JSC::ThrowScope::~ThrowScope()
    4   0x1028cf210 JSC::bigIntProtoFuncToString(JSC::ExecState*)
    5   0x50632d001178
    6   0x1019736c1 llint_entry
    7   0x1019736c1 llint_entry
    8   0x10196b5d2 vmEntryToJavaScript
    9   0x1026cf62e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
    10  0x1026755bd JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
    11  0x102926ba7 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
    12  0x1018afc9e runWithOptions(GlobalObject*, CommandLine&)
    13  0x10188a9a4 jscmain(int, char**)::$_3::operator()(JSC::VM&, GlobalObject*) const
    14  0x101872f1f int runJSC<jscmain(int, char**)::$_3>(CommandLine, bool, jscmain(int, char**)::$_3 const&)
    15  0x101871a0f jscmain(int, char**)
    16  0x10187196e main
    17  0x7fff68743115 start
    18  0x9

ASSERTION FAILED: !m_needExceptionCheck
./runtime/VM.cpp(1090) : void JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation &)
1   0x102e1f0ad WTFCrash
2   0x102bac469 JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
3   0x102b883fb JSC::ThrowScope::~ThrowScope()
4   0x102b887d5 JSC::ThrowScope::~ThrowScope()
5   0x1028cf210 JSC::bigIntProtoFuncToString(JSC::ExecState*)
6   0x50632d001178
7   0x1019736c1 llint_entry
8   0x1019736c1 llint_entry
9   0x10196b5d2 vmEntryToJavaScript
10  0x1026cf62e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
11  0x1026755bd JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
12  0x102926ba7 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
13  0x1018afc9e runWithOptions(GlobalObject*, CommandLine&)
14  0x10188a9a4 jscmain(int, char**)::$_3::operator()(JSC::VM&, GlobalObject*) const
15  0x101872f1f int runJSC<jscmain(int, char**)::$_3>(CommandLine, bool, jscmain(int, char**)::$_3 const&)
16  0x101871a0f jscmain(int, char**)
17  0x10187196e main
18  0x7fff68743115 start
19  0x9

[Ryan Haddad]

Reverted r226937 for reason:

Tests added with this change are failing due to a missing exception check.

Committed r227004: <https://trac.webkit.org/changeset/227004>

[Caio Lima]

Created attachment 331439 [details]
Patch

[Caio Lima]

Created attachment 331440 [details]
Patch

[Caio Lima]

Ping review.

[WebKit Commit Bot]

Comment on attachment 331440 [details]
Patch

Clearing flags on attachment: 331440

Committed r227271: <https://trac.webkit.org/changeset/227271>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.