[webkit-reviews] review denied: [Bug 181182] [JSC] NumberPrototype::extractRadixFromArgs incorrectly cast double to int32_t : [Attachment 330962] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 181182: [JSC] NumberPrototype::extractRadixFromArgs incorrectly cast double
to int32_t
https://bugs.webkit.org/show_bug.cgi?id=181182

Attachment 330962: Patch

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




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 330962
  --> https://bugs.webkit.org/attachment.cgi?id=330962
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.