Bug 212512 - [JSC] JSBigInt allocation should be graceful for OOM
Summary: [JSC] JSBigInt allocation should be graceful for OOM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-28 23:40 PDT by Yusuke Suzuki
Modified: 2020-05-30 20:01 PDT (History)
12 users (show)

See Also:

Attachments
Patch (89.66 KB, patch)
2020-05-28 23:57 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (89.29 KB, patch)
2020-05-29 00:06 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (89.50 KB, patch)
2020-05-29 00:12 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (88.71 KB, patch)
2020-05-29 00:22 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (88.71 KB, patch)
2020-05-29 00:33 PDT, Yusuke Suzuki 		mark.lam: review+
Details | Formatted Diff | Diff
Patch (90.99 KB, patch)
2020-05-29 01:50 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (91.30 KB, patch)
2020-05-29 03:07 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch for landing (91.27 KB, patch)
2020-05-29 17:27 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-05-28 23:40:47 PDT 
<rdar://problem/63600600>
Comment 1 Yusuke Suzuki 2020-05-28 23:57:40 PDT 
Created attachment 400553 [details]
Patch
Comment 2 Yusuke Suzuki 2020-05-29 00:06:55 PDT 
Created attachment 400554 [details]
Patch
Comment 3 Yusuke Suzuki 2020-05-29 00:12:53 PDT 
Created attachment 400555 [details]
Patch
Comment 4 Yusuke Suzuki 2020-05-29 00:22:41 PDT 
Created attachment 400556 [details]
Patch
Comment 5 Yusuke Suzuki 2020-05-29 00:33:03 PDT 
Created attachment 400557 [details]
Patch
Comment 6 Mark Lam 2020-05-29 00:35:46 PDT 
Comment on attachment 400554 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.cpp:85
> +inline JSBigInt* JSBigInt::createZero(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm)

Use Optional?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:95
> +inline JSBigInt* JSBigInt::createWithLength(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, unsigned length)

Is there a good reason to not use Optional<JSGlobalObject> here?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:126
> +inline JSBigInt* JSBigInt::createFrom(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, int32_t value)

Ditto: use Optional here?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:318
> +JSValue JSBigInt::parseInt(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, StringView s, uint8_t radix, ErrorParseMode parserMode, ParseIntSign sign)

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1541
> +bool JSBigInt::absoluteDivWithDigitDivisor(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, BigIntImpl x, Digit divisor, JSBigInt** quotient, Digit& remainder)

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2155
> +String JSBigInt::toStringBasePowerOfTwo(VM& vm, JSGlobalObject* nullOrGlobalObjectForOOM, JSBigInt* x, unsigned radix)

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2304
> +JSBigInt* JSBigInt::rightTrim(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm)

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2345
> +JSBigInt* JSBigInt::allocateFor(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, unsigned radix, unsigned charcount)

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2427
> +JSValue JSBigInt::parseInt(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, CharType* data, unsigned length, unsigned startIndex, unsigned radix, ErrorParseMode errorParseMode, ParseIntSign sign, ParseIntMode parseMode)

Ditto.
Comment 7 Mark Lam 2020-05-29 00:38:07 PDT 
Comment on attachment 400557 [details]
Patch

r=me if EWS bots are green.
Comment 8 Yusuke Suzuki 2020-05-29 01:48:39 PDT 
Comment on attachment 400554 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:85
>> +inline JSBigInt* JSBigInt::createZero(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm)
> 
> Use Optional?

For now, I would like to keep the consistency with JSString's `JSGlobalObject* nullOrGlobalObjectForOOM`. We should consider introducing this, but I would like to do that at once.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:95
>> +inline JSBigInt* JSBigInt::createWithLength(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, unsigned length)
> 
> Is there a good reason to not use Optional<JSGlobalObject> here?

Ditto.
Comment 9 Yusuke Suzuki 2020-05-29 01:50:14 PDT 
Created attachment 400559 [details]
Patch
Comment 10 Yusuke Suzuki 2020-05-29 03:07:29 PDT 
Created attachment 400563 [details]
Patch
Comment 11 Caio Lima 2020-05-29 08:43:40 PDT 
Comment on attachment 400563 [details]
Patch

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

LGTM as well. I pointed out the issue breaking on 32-bits.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2558
> +                    RELEASE_AND_RETURN(scope, JSBigInt::createFrom(nullOrGlobalObjectForOOM, static_cast<int32_t>(maybeResult)));

This call will cause segfault when globalObject is `nullptr`. This happens when `parseInt` is called  from `BytecodeGenerator.cpp`, for instance. This is one of the reasons of failures on ARMv7 and MIPS bots.
Comment 12 Yusuke Suzuki 2020-05-29 17:13:35 PDT 
Comment on attachment 400563 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:2558
>> +                    RELEASE_AND_RETURN(scope, JSBigInt::createFrom(nullOrGlobalObjectForOOM, static_cast<int32_t>(maybeResult)));
> 
> This call will cause segfault when globalObject is `nullptr`. This happens when `parseInt` is called  from `BytecodeGenerator.cpp`, for instance. This is one of the reasons of failures on ARMv7 and MIPS bots.

Nice catch. We should call `JSBigInt::createFrom(nullOrGlobalObjectForOOM, vm, static_cast<int32_t>(maybeResult))` instead.
Comment 13 Yusuke Suzuki 2020-05-29 17:27:38 PDT 
Created attachment 400639 [details]
Patch for landing
Comment 14 Yusuke Suzuki 2020-05-30 00:46:59 PDT 
Committed r262342: <https://trac.webkit.org/changeset/262342>
Comment 15 Yusuke Suzuki 2020-05-30 20:01:14 PDT 
Committed r262353: <https://trac.webkit.org/changeset/262353>