| Summary: | String overflow when using StringBuilder in JSC::createError | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dominik Inführ <dominik.infuehr> | ||||||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer, xan.lopez, ysuzuki | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Dominik Inführ
2019-02-22 12:07:35 PST
Created attachment 362746 [details]
Patch
Created attachment 362751 [details]
Patch
Created attachment 362755 [details]
Patch
Comment on attachment 362751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362751&action=review > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:213 > + return makeString("object is not a function."); Use makeString("object is not a function." _s) so that we'll create the string using an ASCIILiteral. Comment on attachment 362751 [details]
Patch
I didn't mean to un-obsolete this patch.
Created attachment 362757 [details]
Patch
Created attachment 362759 [details]
Patch
Thanks for catching that! I've updated the patch, it should be ready for review! Comment on attachment 362759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362759&action=review LGTM in general but can you clarify what you meant in the ChangeLog (see my question below). > Source/WTF/ChangeLog:15 > + When calculating the new capacity of a StringBuilder object, > + use a limit of MaxLength instead of MaxLength+1. Allocating > + a string of size MaxLength+1 always fails, this meant that expanding > + a StringBuilder only works when doubling the capacity is smaller > + than that. A character cannot be appended to a String of size 1.4GB, > + since doubling the capacity doesn't fit into MaxLength anymore. > + Changing the maximum capacity to MaxLength allows this operation to > + succeed. Yeah, this is a bug. All string constructs have a max capacity of MaxLength. I don't know what I was thinking when I added that +1 back then. I would also break up this sentence so that it doesn't run on (plus some suggested edits): Allocating a string of size MaxLength+1 always fails. This means that expanding a StringBuilder only works when the newly doubled capacity is less or equal to MaxLength. Can you clarify this part, "A character cannot be appended to a String of size 1.4GB, since doubling the capacity doesn't fit into MaxLength anymore. Changing the maximum capacity to MaxLength allows this operation to succeed.". I'm not sure I understand that the issue here. Can you give a concrete example of what you meant here, perhaps with values in hex form so that it's easier to see what's happening. Created attachment 363879 [details]
Patch
Thanks for the review and your suggestions! About that sentence: I agree, it's quite confusing. I just wanted to give an example for the explanation above. I dropped it now since this example only introduced confusion and your suggestion makes the explanation more clear. Comment on attachment 363879 [details]
Patch
r=me
Comment on attachment 363879 [details] Patch Clearing flags on attachment: 363879 Committed r242910: <https://trac.webkit.org/changeset/242910> All reviewed patches have been landed. Closing bug. |