Bug 163937 - JSStringJoiner::joinedLength() should limit joined string lengths to INT_MAX.
Summary: JSStringJoiner::joinedLength() should limit joined string lengths to INT_MAX.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 164125
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-24 23:45 PDT by Mark Lam
Modified: 2016-10-28 06:20 PDT (History)
9 users (show)

See Also:


Attachments
proposed patch. (4.72 KB, patch)
2016-10-24 23:53 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (4.74 KB, patch)
2016-10-24 23:59 PDT, Mark Lam
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-yosemite (1.80 MB, application/zip)
2016-10-25 01:38 PDT, Build Bot
no flags Details
proposed patch: with improvements. (19.37 KB, patch)
2016-10-25 13:47 PDT, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-10-24 23:45:46 PDT
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.
Comment 1 Mark Lam 2016-10-24 23:46:26 PDT
<rdar://problem/28828676>
Comment 2 Mark Lam 2016-10-24 23:47:26 PDT
(In reply to comment #1)
> <rdar://problem/28828676>

Wrong radar.
Comment 3 Mark Lam 2016-10-24 23:47:38 PDT
<rdar://problem/28642990>
Comment 4 Mark Lam 2016-10-24 23:53:37 PDT
Created attachment 292726 [details]
proposed patch.
Comment 5 WebKit Commit Bot 2016-10-24 23:54:56 PDT
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.
Comment 6 Mark Lam 2016-10-24 23:59:09 PDT
Created attachment 292727 [details]
proposed patch.
Comment 7 Build Bot 2016-10-25 01:38:33 PDT
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
Comment 8 Build Bot 2016-10-25 01:38:36 PDT
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
Comment 9 JF Bastien 2016-10-25 08:29:09 PDT
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?
Comment 10 Saam Barati 2016-10-25 09:22:37 PDT
(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?
Comment 11 Geoffrey Garen 2016-10-25 09:44:46 PDT
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.
Comment 12 Mark Lam 2016-10-25 13:47:55 PDT
Created attachment 292816 [details]
proposed patch: with improvements.
Comment 13 Geoffrey Garen 2016-10-25 13:55:05 PDT
Comment on attachment 292816 [details]
proposed patch: with improvements.

r=me
Comment 14 Mark Lam 2016-10-25 15:23:12 PDT
Thanks for the reviews.  Landed in r207849: <http://trac.webkit.org/r207849>.