Bug 190836 – [BigInt] JSBigInt::createWithLength should throw when length is greater than JSBigInt::maxLength

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

[Caio Lima]

Current implementation ignores JSBigInt::maxLength, but we should follow this definition, since some algorithms rely on this vale.

[Caio Lima]

Created attachment 353034 [details]
WIP - Patch

This starts. There are changes and tests missing.

[Caio Lima]

Created attachment 353097 [details]
WIP - Patch

[Yusuke Suzuki]

If the factory function may fail, I like to name it "tryCreateXXX" instead of "createXXX". It encourages us to check the result in the caller side.

[Caio Lima]

Created attachment 353664 [details]
WIP - Patch

[Caio Lima]

(In reply to Yusuke Suzuki from comment #3)
> If the factory function may fail, I like to name it "tryCreateXXX" instead
> of "createXXX". It encourages us to check the result in the caller side.

I changed this. Thx for the comment!

[Caio Lima]

Created attachment 354237 [details]
WIP - Patch

[Caio Lima]

Created attachment 354572 [details]
Patch

[Caio Lima]

Created attachment 354573 [details]
Patch

[Saam Barati]

Comment on attachment 354573 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.cpp:98
> +    if (length > maxLength) {

nit: Maybe do UNLIKELY here?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:414
> +        JSBigInt* y1 = absoluteSubOne(exec, y, y->length());

Don't you also need an exception check after this?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:416
> +        return absoluteAddOne(exec, result, SignOption::Signed);

Do you need to release the scope before this?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:425
> +    return absoluteAndNot(vm, x, absoluteSubOne(exec, y, y->length()));

You don't need an exception check here? Can't this return nullptr?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1190
> +JSBigInt* JSBigInt::absoluteAddOne(ExecState* exec, JSBigInt* x, SignOption signOption)

Don't you need to make all callers check for an exception here?

[Caio Lima]

Comment on attachment 354573 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:414
>> +        JSBigInt* y1 = absoluteSubOne(exec, y, y->length());
> 
> Don't you also need an exception check after this?

Yes. Actually I realised this is the wrong version of the Patch.

[Caio Lima]

Comment on attachment 354573 [details]
Patch

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

Thx for the review and sorry for the confusion in the previous Patch.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:98
>> +    if (length > maxLength) {
> 
> nit: Maybe do UNLIKELY here?

Yes. Done.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:416
>> +        return absoluteAddOne(exec, result, SignOption::Signed);
> 
> Do you need to release the scope before this?

Ditto.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:425
>> +    return absoluteAndNot(vm, x, absoluteSubOne(exec, y, y->length()));
> 
> You don't need an exception check here? Can't this return nullptr?

yes. done.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:1190
>> +JSBigInt* JSBigInt::absoluteAddOne(ExecState* exec, JSBigInt* x, SignOption signOption)
> 
> Don't you need to make all callers check for an exception here?

I don't understand this question. Do you mean the caller of absoluteAddOne? It is always the last operation of functions we use, and IIUC, we don't need to check for exceptions in such case. Is it.right?

[Caio Lima]

Created attachment 354602 [details]
Patch

[Caio Lima]

Created attachment 354604 [details]
Patch

[Saam Barati]

Comment on attachment 354604 [details]
Patch

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

r=me w/ comments.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:112
> +    RELEASE_ASSERT(bigInt);

No need for this RELEASE_ASSERT. This will never return null. If you can't allocate the cell, it'll crash. It could return nullptr if you used tryAllocateCell.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:419
> +        result = absoluteAddOne(exec, result, SignOption::Signed);
> +        RETURN_IF_EXCEPTION(scope, nullptr);
> +        return result;

This could be:
scope.release()
return absoluteAddOne(exec, result, SignOption::Signed)

> Source/JavaScriptCore/runtime/JSBigInt.cpp:498
> +    result = absoluteAddOne(exec, result, SignOption::Signed);
> +    RETURN_IF_EXCEPTION(scope, nullptr);
> +    return result;

this could be:
scope.release();
return absoluteAddOne(exec, result, SignOption::Signed);

> Source/JavaScriptCore/runtime/Operations.cpp:75
> +            JSBigInt* result = JSBigInt::add(callFrame, WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric));
> +            RETURN_IF_EXCEPTION(scope, { });
> +            return result;

ditto w/ scope.release() before calling add

> Source/JavaScriptCore/runtime/Operations.h:354
> +            JSBigInt* result = JSBigInt::sub(exec, WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric));
> +            RETURN_IF_EXCEPTION(scope, { });
> +            return result;

ditto

> Source/JavaScriptCore/runtime/Operations.h:377
> +            JSBigInt* result = JSBigInt::multiply(state, WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric));
> +            RETURN_IF_EXCEPTION(scope, { });
> +            return result;

ditto

[Caio Lima]

Created attachment 354670 [details]
Patch

[Caio Lima]

Comment on attachment 354670 [details]
Patch

Thank you very much for the review.

[WebKit Commit Bot]

Comment on attachment 354670 [details]
Patch

Clearing flags on attachment: 354670

Committed r238132: <https://trac.webkit.org/changeset/238132>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46028371>

[Saam Barati]

Comment on attachment 354670 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.cpp:249
> +    auto scope = DECLARE_CATCH_SCOPE(vm);

Sorry for missing this earlier, but this should be a throw scope, not a catch scope

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1237
> +    auto scope = DECLARE_CATCH_SCOPE(vm);

ditto

[Caio Lima]

(In reply to Saam Barati from comment #20)
> Comment on attachment 354670 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354670&action=review
> 
> > Source/JavaScriptCore/runtime/JSBigInt.cpp:249
> > +    auto scope = DECLARE_CATCH_SCOPE(vm);
> 
> Sorry for missing this earlier, but this should be a throw scope, not a
> catch scope
> 
> > Source/JavaScriptCore/runtime/JSBigInt.cpp:1237
> > +    auto scope = DECLARE_CATCH_SCOPE(vm);
> 
> ditto

Sorry for this. I'm going to handle this change into https://bugs.webkit.org/show_bug.cgi?id=190264 then.

[Ryan Haddad]

The test added with this change (stress/big-int-out-of-memory-tests.js) is timing out on Debug JSC bots:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/1742
https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/2922

[Ryan Haddad]

Reverted r238132 for reason:

The test added with this change is timing out on Debug JSC bots.

Committed r238156: <https://trac.webkit.org/changeset/238156>

[Caio Lima]

Created attachment 354755 [details]
Patch

[Caio Lima]

Created attachment 354797 [details]
Patch

[Guillaume Emont]

Before it got rolled out, the test stress/big-int-out-of-memory-tests.js was being killed (probably OOM-killed) on MIPS, and timed out on MIPS. Is there an easy way to make this test run on these slower, memory restricted platforms? If not, the test should be skipped for them (at least arm and mips on linux), maybe on debug bots too (since that seems to be the reason for the rollout)?

Disregard this message if your new patch already addresses this.

[Caio Lima]

Created attachment 355242 [details]
Patch

[Caio Lima]

The big difference from approved version is the change into test to avoid timeout.

[Yusuke Suzuki]

Comment on attachment 355242 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.cpp:111
>      JSBigInt* bigInt = new (NotNull, allocateCell<JSBigInt>(vm.heap, allocationSize(length))) JSBigInt(vm, vm.bigIntStructure.get(), length);

Let's have an assertion on `length`!

ASSERT(length <= maxLength);

[Caio Lima]

Created attachment 355422 [details]
Patch

[Caio Lima]

(In reply to Yusuke Suzuki from comment #29)
> Comment on attachment 355242 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355242&action=review
> 
> > Source/JavaScriptCore/runtime/JSBigInt.cpp:111
> >      JSBigInt* bigInt = new (NotNull, allocateCell<JSBigInt>(vm.heap, allocationSize(length))) JSBigInt(vm, vm.bigIntStructure.get(), length);
> 
> Let's have an assertion on `length`!
> 
> ASSERT(length <= maxLength);

Done. Thanks for the review!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 355422 [details]:

http/wpt/css/css-animations/start-animation-001.html bug 190903 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 355422 [details]
Patch

Clearing flags on attachment: 355422

Committed r238425: <https://trac.webkit.org/changeset/238425>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.