Bug 190836

Summary: [BigInt] JSBigInt::createWithLength should throw when length is greater than JSBigInt::maxLength
Product: WebKit Reporter: Caio Lima <ticaiolima>
Component: JavaScriptCoreAssignee: Caio Lima <ticaiolima>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, guijemont, guijemont+jsc-armv7-ews, keith_miller, mark.lam, msaboff, ryanhaddad, sbarati, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 179001, 186233    
Attachments:
Description Flags
WIP - Patch
none
WIP - Patch
none
WIP - Patch
none
WIP - Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Caio Lima 2018-10-23 11:15:58 PDT
Current implementation ignores JSBigInt::maxLength, but we should follow this definition, since some algorithms rely on this vale.
Comment 1 Caio Lima 2018-10-24 07:06:43 PDT
Created attachment 353034 [details]
WIP - Patch

This starts. There are changes and tests missing.
Comment 2 Caio Lima 2018-10-25 11:39:57 PDT
Created attachment 353097 [details]
WIP - Patch
Comment 3 Yusuke Suzuki 2018-10-25 20:47:10 PDT
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.
Comment 4 Caio Lima 2018-11-01 17:13:40 PDT
Created attachment 353664 [details]
WIP - Patch
Comment 5 Caio Lima 2018-11-01 17:14:47 PDT
(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!
Comment 6 Caio Lima 2018-11-08 06:59:07 PST
Created attachment 354237 [details]
WIP - Patch
Comment 7 Caio Lima 2018-11-12 11:26:00 PST
Created attachment 354572 [details]
Patch
Comment 8 Caio Lima 2018-11-12 11:39:58 PST
Created attachment 354573 [details]
Patch
Comment 9 Saam Barati 2018-11-12 11:48:13 PST
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?
Comment 10 Caio Lima 2018-11-12 12:35:49 PST
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.
Comment 11 Caio Lima 2018-11-12 13:06:18 PST
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?
Comment 12 Caio Lima 2018-11-12 15:46:53 PST
Created attachment 354602 [details]
Patch
Comment 13 Caio Lima 2018-11-12 15:54:08 PST
Created attachment 354604 [details]
Patch
Comment 14 Saam Barati 2018-11-12 18:06:40 PST
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
Comment 15 Caio Lima 2018-11-13 06:28:22 PST
Created attachment 354670 [details]
Patch
Comment 16 Caio Lima 2018-11-13 07:20:22 PST
Comment on attachment 354670 [details]
Patch

Thank you very much for the review.
Comment 17 WebKit Commit Bot 2018-11-13 07:46:41 PST
Comment on attachment 354670 [details]
Patch

Clearing flags on attachment: 354670

Committed r238132: <https://trac.webkit.org/changeset/238132>
Comment 18 WebKit Commit Bot 2018-11-13 07:46:43 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2018-11-13 07:48:53 PST
<rdar://problem/46028371>
Comment 20 Saam Barati 2018-11-13 09:21:15 PST
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
Comment 21 Caio Lima 2018-11-13 10:25:08 PST
(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.
Comment 22 Ryan Haddad 2018-11-13 17:35:15 PST
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
Comment 23 Ryan Haddad 2018-11-13 17:47:36 PST
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>
Comment 24 Caio Lima 2018-11-13 19:27:53 PST
Created attachment 354755 [details]
Patch
Comment 25 Caio Lima 2018-11-14 04:50:00 PST
Created attachment 354797 [details]
Patch
Comment 26 Guillaume Emont 2018-11-14 10:01:09 PST
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.
Comment 27 Caio Lima 2018-11-18 16:10:22 PST
Created attachment 355242 [details]
Patch
Comment 28 Caio Lima 2018-11-20 16:04:46 PST
The big difference from approved version is the change into test to avoid timeout.
Comment 29 Yusuke Suzuki 2018-11-20 19:00:57 PST
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);
Comment 30 Caio Lima 2018-11-21 09:45:28 PST
Created attachment 355422 [details]
Patch
Comment 31 Caio Lima 2018-11-21 09:48:23 PST
(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!
Comment 32 WebKit Commit Bot 2018-11-21 10:37:32 PST
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: dino@apple.com, fred.wang@free.fr, and graouts@apple.com)
The commit-queue is continuing to process your patch.
Comment 33 WebKit Commit Bot 2018-11-21 10:38:19 PST
Comment on attachment 355422 [details]
Patch

Clearing flags on attachment: 355422

Committed r238425: <https://trac.webkit.org/changeset/238425>
Comment 34 WebKit Commit Bot 2018-11-21 10:38:21 PST
All reviewed patches have been landed.  Closing bug.