Bug 192833

Summary: String overflow in JSC::createError results in ASSERT in WTF::makeString
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, guijemont, guijemont+jsc-armv7-ews, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Tadeu Zagallo 2018-12-18 15:27:51 PST
...
Comment 1 Tadeu Zagallo 2018-12-18 15:31:50 PST
<rdar://problem/45706868>
Comment 2 Tadeu Zagallo 2018-12-18 15:42:31 PST
Created attachment 357625 [details]
Patch
Comment 3 Mark Lam 2018-12-18 16:12:23 PST
Comment on attachment 357625 [details]
Patch

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

r=me

> JSTests/stress/string-overflow-createError.js:3
> +bar = '2.3023e-320'
> +foo = bar.padEnd(2147483644, 1);
> +foo(true, 1).value;

Does this test throw an exception?  If so, don't you have to put it in a try catch statement?
Comment 4 EWS Watchlist 2018-12-18 17:33:09 PST
Comment on attachment 357625 [details]
Patch

Attachment 357625 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10465801

New failing tests:
stress/string-overflow-createError.js.dfg-eager-no-cjit-validate
stress/string-overflow-createError.js.no-cjit-validate-phases
stress/string-overflow-createError.js.ftl-eager
stress/string-overflow-createError.js.ftl-no-cjit-no-inline-validate
stress/string-overflow-createError.js.ftl-eager-no-cjit
stress/string-overflow-createError.js.dfg-eager
stress/string-overflow-createError.js.ftl-no-cjit-no-put-stack-validate
stress/string-overflow-createError.js.ftl-no-cjit-validate-sampling-profiler
stress/string-overflow-createError.js.ftl-no-cjit-b3o1
stress/string-overflow-createError.js.dfg-maximal-flush-validate-no-cjit
stress/string-overflow-createError.js.no-llint
stress/string-overflow-createError.js.default
stress/string-overflow-createError.js.ftl-eager-no-cjit-b3o1
stress/string-overflow-createError.js.no-cjit-collect-continuously
stress/string-overflow-createError.js.ftl-no-cjit-small-pool
stress/string-overflow-createError.js.no-ftl
apiTests
Comment 5 Mark Lam 2018-12-18 17:35:46 PST
Comment on attachment 357625 [details]
Patch

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

>> JSTests/stress/string-overflow-createError.js:3
>> +foo(true, 1).value;
> 
> Does this test throw an exception?  If so, don't you have to put it in a try catch statement?

EWS bot is not happy.  It does look like you need to wrap this in a try-catch.  Something like this:

var exception;
try {
    bar = '2.3023e-320'
    foo = bar.padEnd(2147483644, 1);
    foo(true, 1).value;
} catch (e) {
    exception = e;
}

if (exception != "Error: Out of memory")
    throw "FAILED";
Comment 6 Tadeu Zagallo 2018-12-19 02:50:20 PST
Comment on attachment 357625 [details]
Patch

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

>>> JSTests/stress/string-overflow-createError.js:3
>>> +foo(true, 1).value;
>> 
>> Does this test throw an exception?  If so, don't you have to put it in a try catch statement?
> 
> EWS bot is not happy.  It does look like you need to wrap this in a try-catch.  Something like this:
> 
> var exception;
> try {
>     bar = '2.3023e-320'
>     foo = bar.padEnd(2147483644, 1);
>     foo(true, 1).value;
> } catch (e) {
>     exception = e;
> }
> 
> if (exception != "Error: Out of memory")
>     throw "FAILED";

Oops... I should have checked that. I will update, thanks!
Comment 7 Tadeu Zagallo 2018-12-19 02:53:58 PST
Created attachment 357666 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2018-12-19 03:33:19 PST
Comment on attachment 357666 [details]
Patch for landing

Clearing flags on attachment: 357666

Committed r239375: <https://trac.webkit.org/changeset/239375>
Comment 9 WebKit Commit Bot 2018-12-19 03:33:21 PST
All reviewed patches have been landed.  Closing bug.