Bug 212847 – Make CodeBlockHash robust against unreasonably long source code.

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

[Mark Lam]

<rdar://problem/64024279>

[Mark Lam]

Created attachment 401200 [details]
proposed patch.

[Saam Barati]

Comment on attachment 401200 [details]
proposed patch.

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

> Source/JavaScriptCore/bytecode/CodeBlockHash.cpp:51
> +    constexpr int maxSourceCodeLengthToHash = 500 * MB;

should be size_t

> Source/JavaScriptCore/bytecode/CodeBlockHash.cpp:59
> +        m_hash = sourceCode.length() ^ 0x2d5a93d0;

could you alternatively just sample 500mb of the source code?

[Mark Lam]

Comment on attachment 401200 [details]
proposed patch.

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

>> Source/JavaScriptCore/bytecode/CodeBlockHash.cpp:51
>> +    constexpr int maxSourceCodeLengthToHash = 500 * MB;
> 
> should be size_t

Sadly, sourceCode.length() is an int, which is what made this an int.  But I'll be changing this anyway.

>> Source/JavaScriptCore/bytecode/CodeBlockHash.cpp:59
>> +        m_hash = sourceCode.length() ^ 0x2d5a93d0;
> 
> could you alternatively just sample 500mb of the source code?

Will do.

[Mark Lam]

Created attachment 401204 [details]
proposed patch.

[Mark Lam]

Created attachment 401210 [details]
proposed patch.

[Saam Barati]

Comment on attachment 401210 [details]
proposed patch.

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

> Source/JavaScriptCore/bytecode/CodeBlockHash.cpp:67
> +            UChar character = str[index];

what guarantees this doesn't read OOB?

Why not just make this look at 500*MB characters? Seems more straight forward than this.

[Mark Lam]

Comment on attachment 401210 [details]
proposed patch.

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

>> Source/JavaScriptCore/bytecode/CodeBlockHash.cpp:67
>> +            UChar character = str[index];
> 
> what guarantees this doesn't read OOB?
> 
> Why not just make this look at 500*MB characters? Seems more straight forward than this.

Spoke with Saam offline.  Just to make it clear for others, index is guaranteed to be < length, and we're using StringView::length() and StringView::operator[] here.  There's no risk of overflow.  The initial index is always 0, and we know length > 500 MB.  So, no overflow there either.

[Mark Lam]

Comment on attachment 401210 [details]
proposed patch.

Thanks for the review.  Landing now.

[EWS]

Committed r262677: <https://trac.webkit.org/changeset/262677>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401210 [details].