Bug 194957 – String overflow when using StringBuilder in JSC::createError

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dominik Inführ]

String overflow when using StringBuilder in JSC::createError

[Dominik Inführ]

Created attachment 362746 [details]
Patch

[Dominik Inführ]

Created attachment 362751 [details]
Patch

[Dominik Inführ]

Created attachment 362755 [details]
Patch

[Mark Lam]

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.

[Mark Lam]

Comment on attachment 362751 [details]
Patch

I didn't mean to un-obsolete this patch.

[Dominik Inführ]

Created attachment 362757 [details]
Patch

[Dominik Inführ]

Created attachment 362759 [details]
Patch

[Dominik Inführ]

Thanks for catching that! I've updated the patch, it should be ready for review!

[Mark Lam]

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.

[Dominik Inführ]

Created attachment 363879 [details]
Patch

[Dominik Inführ]

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.

[Mark Lam]

Comment on attachment 363879 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 363879 [details]
Patch

Clearing flags on attachment: 363879

Committed r242910: <https://trac.webkit.org/changeset/242910>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48863998>