| Summary: | Make CodeBlockHash robust against unreasonably long source code. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||
| Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | ews-watchlist, keith_miller, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Mark Lam
2020-06-05 14:15:20 PDT
Created attachment 401200 [details]
proposed patch.
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? 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. Created attachment 401204 [details]
proposed patch.
Created attachment 401210 [details]
proposed patch.
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. 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. Comment on attachment 401210 [details]
proposed patch.
Thanks for the review. Landing now.
Committed r262677: <https://trac.webkit.org/changeset/262677> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401210 [details]. |