Bug 190187 - Make string MaxLength for all WTF and JS strings consistently equal to INT_MAX.
Summary: Make string MaxLength for all WTF and JS strings consistently equal to INT_MAX.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
: 165790 (view as bug list)
Depends on:
Blocks: 190392
  Show dependency treegraph
 
Reported: 2018-10-01 23:44 PDT by Mark Lam
Modified: 2018-10-08 21:52 PDT (History)
16 users (show)

See Also:


Attachments
proposed patch. (15.19 KB, patch)
2018-10-02 13:59 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (15.47 KB, patch)
2018-10-02 14:43 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2018-10-01 23:44:06 PDT
<rdar://problem/42512909>
Comment 1 Mark Lam 2018-10-02 13:06:42 PDT
*** Bug 165790 has been marked as a duplicate of this bug. ***
Comment 2 Mark Lam 2018-10-02 13:59:01 PDT
Created attachment 351437 [details]
proposed patch.

Let's get some EWS testing first.
Comment 3 Mark Lam 2018-10-02 14:43:20 PDT
Created attachment 351441 [details]
proposed patch.
Comment 4 Mark Lam 2018-10-02 15:55:22 PDT
Comment on attachment 351441 [details]
proposed patch.

I think this is ready for a code review.  Let's get some feedback.
Comment 5 Darin Adler 2018-10-02 18:29:40 PDT
Comment on attachment 351441 [details]
proposed patch.

How did you chose INT_MAX over UINT_MAX?
Comment 6 Mark Lam 2018-10-02 19:02:21 PDT
(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.
Comment 7 Mark Lam 2018-10-02 19:04:35 PDT
(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 8 Michael Saboff 2018-10-03 11:01:31 PDT
Comment on attachment 351441 [details]
proposed patch.

r=me
Comment 9 Mark Lam 2018-10-03 11:02:53 PDT
Comment on attachment 351441 [details]
proposed patch.

Thanks for the review.  Landing now.
Comment 10 WebKit Commit Bot 2018-10-03 11:29:03 PDT
Comment on attachment 351441 [details]
proposed patch.

Clearing flags on attachment: 351441

Committed r236804: <https://trac.webkit.org/changeset/236804>
Comment 11 WebKit Commit Bot 2018-10-03 11:29:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Adler 2018-10-07 21:24:40 PDT
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 13 Mark Lam 2018-10-07 22:02:32 PDT
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.
Comment 14 Mark Lam 2018-10-08 21:52:42 PDT
(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.