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, saam, 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

Caio Lima
Reported 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.
Attachments
WIP - Patch (39.02 KB, patch)
2018-10-24 07:06 PDT, Caio Lima 		no flags
DetailsFormatted DiffDiff
WIP - Patch (38.57 KB, patch)
2018-10-25 11:39 PDT, Caio Lima 		no flags
DetailsFormatted DiffDiff
WIP - Patch (38.55 KB, patch)
2018-11-01 17:13 PDT, Caio Lima 		no flags
DetailsFormatted DiffDiff
WIP - Patch (38.87 KB, patch)
2018-11-08 06:59 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (38.53 KB, patch)
2018-11-12 11:26 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (38.94 KB, patch)
2018-11-12 11:39 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (39.01 KB, patch)
2018-11-12 15:46 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (39.02 KB, patch)
2018-11-12 15:54 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (38.61 KB, patch)
2018-11-13 06:28 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (38.60 KB, patch)
2018-11-13 19:27 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (38.99 KB, patch)
2018-11-14 04:50 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (38.99 KB, patch)
2018-11-18 16:10 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (39.08 KB, patch)
2018-11-21 09:45 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.
Caio Lima
Comment 1 2018-10-24 07:06:43 PDT
Created attachment 353034 [details] WIP - Patch This starts. There are changes and tests missing.
Caio Lima
Comment 2 2018-10-25 11:39:57 PDT
Created attachment 353097 [details] WIP - Patch
Yusuke Suzuki
Comment 3 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.
Caio Lima
Comment 4 2018-11-01 17:13:40 PDT
Created attachment 353664 [details] WIP - Patch
Caio Lima
Comment 5 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!
Caio Lima
Comment 6 2018-11-08 06:59:07 PST
Created attachment 354237 [details] WIP - Patch
Caio Lima
Comment 7 2018-11-12 11:26:00 PST
Created attachment 354572 [details] Patch
Caio Lima
Comment 8 2018-11-12 11:39:58 PST
Created attachment 354573 [details] Patch
Saam Barati
Comment 9 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?
Caio Lima
Comment 10 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.
Caio Lima
Comment 11 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?
Caio Lima
Comment 12 2018-11-12 15:46:53 PST
Created attachment 354602 [details] Patch
Caio Lima
Comment 13 2018-11-12 15:54:08 PST
Created attachment 354604 [details] Patch
Saam Barati
Comment 14 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
Caio Lima
Comment 15 2018-11-13 06:28:22 PST
Created attachment 354670 [details] Patch
Caio Lima
Comment 16 2018-11-13 07:20:22 PST
Comment on attachment 354670 [details] Patch Thank you very much for the review.
WebKit Commit Bot
Comment 17 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>
WebKit Commit Bot
Comment 18 2018-11-13 07:46:43 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 19 2018-11-13 07:48:53 PST
<rdar://problem/46028371>
Saam Barati
Comment 20 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
Caio Lima
Comment 21 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.
Ryan Haddad
Comment 22 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
Ryan Haddad
Comment 23 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>
Caio Lima
Comment 24 2018-11-13 19:27:53 PST
Created attachment 354755 [details] Patch
Caio Lima
Comment 25 2018-11-14 04:50:00 PST
Created attachment 354797 [details] Patch
Guillaume Emont
Comment 26 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.
Caio Lima
Comment 27 2018-11-18 16:10:22 PST
Created attachment 355242 [details] Patch
Caio Lima
Comment 28 2018-11-20 16:04:46 PST
The big difference from approved version is the change into test to avoid timeout.
Yusuke Suzuki
Comment 29 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);
Caio Lima
Comment 30 2018-11-21 09:45:28 PST
Created attachment 355422 [details] Patch
Caio Lima
Comment 31 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!
WebKit Commit Bot
Comment 32 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.
WebKit Commit Bot
Comment 33 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>
WebKit Commit Bot
Comment 34 2018-11-21 10:38:21 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.