Bug 190187 – Make string MaxLength for all WTF and JS strings consistently equal to INT_MAX.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

<rdar://problem/42512909>

[Mark Lam]

*** Bug 165790 has been marked as a duplicate of this bug. ***

[Mark Lam]

Created attachment 351437 [details]
proposed patch.

Let's get some EWS testing first.

[Mark Lam]

Created attachment 351441 [details]
proposed patch.

[Mark Lam]

Comment on attachment 351441 [details]
proposed patch.

I think this is ready for a code review.  Let's get some feedback.

[Darin Adler]

Comment on attachment 351441 [details]
proposed patch.

How did you chose INT_MAX over UINT_MAX?

[Mark Lam]

(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.

[Mark Lam]

(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."

[Michael Saboff]

Comment on attachment 351441 [details]
proposed patch.

r=me

[Mark Lam]

Comment on attachment 351441 [details]
proposed patch.

Thanks for the review.  Landing now.

[WebKit Commit Bot]

Comment on attachment 351441 [details]
proposed patch.

Clearing flags on attachment: 351441

Committed r236804: <https://trac.webkit.org/changeset/236804>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Mark Lam]

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.

[Mark Lam]

(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.