Bug 163937 – JSStringJoiner::joinedLength() should limit joined string lengths to INT_MAX.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

It was previously limiting it to UINT_MAX.  This is inconsistent with other parts of string code which expects a max length of INT_MAX.

[Mark Lam]

<rdar://problem/28828676>

[Mark Lam]

(In reply to comment #1)
> <rdar://problem/28828676>

Wrong radar.

[Mark Lam]

<rdar://problem/28642990>

[Mark Lam]

Created attachment 292726 [details]
proposed patch.

[WebKit Commit Bot]

Attachment 292726 [details] did not pass style-queue:


ERROR: JSTests/stress/joined-strings-should-not-exceed-max-string-length.js:16:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/joined-strings-should-not-exceed-max-string-length.js:17:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/joined-strings-should-not-exceed-max-string-length.js:19:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/joined-strings-should-not-exceed-max-string-length.js:25:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/joined-strings-should-not-exceed-max-string-length.js:27:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 292727 [details]
proposed patch.

[Build Bot]

Comment on attachment 292727 [details]
proposed patch.

Attachment 292727 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2363652

New failing tests:
js/stringimpl-to-jsstring-on-large-strings-2.html

[Build Bot]

Created attachment 292734 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[JF Bastien]

Comment on attachment 292727 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=292727&action=review

> Source/JavaScriptCore/runtime/JSString.h:108
> +        ASSERT(static_cast<int32_t>(length) >= 0);

Why not length <= std::numeric_limits<int32_t>::max() instead?

[Saam Barati]

(In reply to comment #0)
> It was previously limiting it to UINT_MAX.  This is inconsistent with other
> parts of string code which expects a max length of INT_MAX.

Can we define a constant for like MAX_JSSTRING_LENGTH similar to some of the constants we have for array lengths?

[Geoffrey Garen]

Comment on attachment 292727 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=292727&action=review

>> Source/JavaScriptCore/runtime/JSString.h:108
>> +        ASSERT(static_cast<int32_t>(length) >= 0);
> 
> Why not length <= std::numeric_limits<int32_t>::max() instead?

Yeah, this is better.

> Source/JavaScriptCore/runtime/JSString.h:214
> +    // While length is of type unsigned, the runtime and compilers are all
> +    // expecting that m_length is a positive value <= INT_MAX.
>      unsigned m_length;

A better way to enforce this invariant is to make a helper function for setting m_length, make m_length private, and make the helper function ASSERT that the incoming length is <= std::numeric_limits<int32_t>::max(), with this comment.

[Mark Lam]

Created attachment 292816 [details]
proposed patch: with improvements.

[Geoffrey Garen]

Comment on attachment 292816 [details]
proposed patch: with improvements.

r=me

[Mark Lam]

Thanks for the reviews.  Landed in r207849: <http://trac.webkit.org/r207849>.