Summary: | Make string MaxLength for all WTF and JS strings consistently equal to INT_MAX. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, ews-watchlist, fpizlo, jfbastien, keith_miller, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 190392 | ||||||||
Attachments: |
|
Description
Mark Lam
2018-10-01 23:44:06 PDT
*** Bug 165790 has been marked as a duplicate of this bug. *** Created attachment 351437 [details]
proposed patch.
Let's get some EWS testing first.
Created attachment 351441 [details]
proposed patch.
Comment on attachment 351441 [details]
proposed patch.
I think this is ready for a code review. Let's get some feedback.
Comment on attachment 351441 [details]
proposed patch.
How did you chose INT_MAX over UINT_MAX?
(In reply to Darin Adler from comment #5) > Comment on attachment 351441 [details] > proposed patch. > > How did you chose INT_MAX over UINT_MAX? 1. Most of the existing code (and tests) are already configured to check and test for a limit of INT_MAX. 2. We don't gain much by switching to UINT_MAX, because I don't think anyone would reasonably expect to work with a 2G (let alone 4G) length string except for realistic workloads. The only times when one would typically work with strings that large are either for testing edge cases, or for malicious purposes in an attempt to break the system. 3. I ran the choices by Filip, and he chose INT_MAX. We can certainly make UINT_MAX work, but I suspect it will be a larger change (possibly with greater risk of bugs) and it doesn't buy us much. (In reply to Mark Lam from comment #6) > 2. We don't gain much by switching to UINT_MAX, because I don't think anyone > would reasonably expect to work with a 2G (let alone 4G) length string > except for realistic workloads. Sorry, that's a typo: the "except" should not be there. I meant to say "I don't think anyone would reasonably expect to work with a 2G (let alone 4G) length string for realistic workloads." Comment on attachment 351441 [details]
proposed patch.
r=me
Comment on attachment 351441 [details]
proposed patch.
Thanks for the review. Landing now.
Comment on attachment 351441 [details] proposed patch. Clearing flags on attachment: 351441 Committed r236804: <https://trac.webkit.org/changeset/236804> All reviewed patches have been landed. Closing bug. Comment on attachment 351441 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=351441&action=review > Source/WTF/wtf/text/StringConcatenate.h:145 > + RELEASE_ASSERT(length <= String::MaxLength); This seems not quite right. Either we are safe and no one could ever pass a super-colossal string, so we don’t need this RELEASE_ASSERT, or we aren’t safe, in which case we need to make sure that length didn’t overflow above, because some super-colossal strings could overflow. Same thinking applies above in the LChar version of this adapter. Comment on attachment 351441 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=351441&action=review >> Source/WTF/wtf/text/StringConcatenate.h:145 >> + RELEASE_ASSERT(length <= String::MaxLength); > > This seems not quite right. > > Either we are safe and no one could ever pass a super-colossal string, so we don’t need this RELEASE_ASSERT, or we aren’t safe, in which case we need to make sure that length didn’t overflow above, because some super-colossal strings could overflow. > > Same thinking applies above in the LChar version of this adapter. Good catch. The length check was never correct to begin with. This RELEASE_ASSERT is bug compatible with the previous code. I'll create a new patch with a fix. Thanks again for the catch. (In reply to Darin Adler from comment #12) > Comment on attachment 351441 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=351441&action=review > > > Source/WTF/wtf/text/StringConcatenate.h:145 > > + RELEASE_ASSERT(length <= String::MaxLength); > > This seems not quite right. I'll fix this in https://bugs.webkit.org/show_bug.cgi?id=190392. |