Bug 215897

Summary: [JSC] setLength in Array#push could get very large length
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch keith_miller: review+

Description Yusuke Suzuki 2020-08-27 12:25:04 PDT
[JSC] setLength in Array#push could get very large length
Comment 1 Yusuke Suzuki 2020-08-27 12:29:07 PDT
Created attachment 407422 [details]
Patch
Comment 2 Yusuke Suzuki 2020-08-27 12:29:09 PDT
<rdar://problem/67859149>
Comment 3 Keith Miller 2020-08-27 12:34:48 PDT
Comment on attachment 407422 [details]
Patch

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

r=me with nits.

> Source/JavaScriptCore/ChangeLog:10
> +        Before r266215, it was using putLength which throws an error. But it is replaced with setLength,

Nit: But it *was* replaced.

> Source/JavaScriptCore/ChangeLog:11
> +        and JSC::setLength assumes that this never gets such a length with an assertion. We should fix it

Nit: assumes that *it* never gets *a length greater than UINT32_MAX by asserting*.
Comment 4 Yusuke Suzuki 2020-08-27 12:35:41 PDT
Comment on attachment 407422 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:10
>> +        Before r266215, it was using putLength which throws an error. But it is replaced with setLength,
> 
> Nit: But it *was* replaced.

Fixed.

>> Source/JavaScriptCore/ChangeLog:11
>> +        and JSC::setLength assumes that this never gets such a length with an assertion. We should fix it
> 
> Nit: assumes that *it* never gets *a length greater than UINT32_MAX by asserting*.

Fixed.
Comment 5 Yusuke Suzuki 2020-08-27 14:33:03 PDT
Committed r266257: <https://trac.webkit.org/changeset/266257>
Comment 6 Darin Adler 2020-08-27 16:02:02 PDT
Comment on attachment 407422 [details]
Patch

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

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:168
> +        if (UNLIKELY(value > UINT32_MAX)) {

Could this be an maxArrayLength constant instead of UINT32_MAX?
Comment 7 Darin Adler 2020-08-27 17:51:59 PDT
Comment on attachment 407422 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:168
>> +        if (UNLIKELY(value > UINT32_MAX)) {
> 
> Could this be an maxArrayLength constant instead of UINT32_MAX?

Like maybe:

    constexpr uint32_t maxArrayLength = MAX_ARRAY_INDEX + 1;