| Summary: | JSStringJoiner::joinedLength() should limit joined string lengths to INT_MAX. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||
| Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | commit-queue, fpizlo, ggaren, jfbastien, keith_miller, msaboff, saam, webkit-bug-importer, ysuzuki | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Local Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | 164125 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Mark Lam
2016-10-24 23:45:46 PDT
(In reply to comment #1) > <rdar://problem/28828676> Wrong radar. Created attachment 292726 [details]
proposed patch.
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.
Created attachment 292727 [details]
proposed patch.
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 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 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? (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 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. Created attachment 292816 [details]
proposed patch: with improvements.
Comment on attachment 292816 [details]
proposed patch: with improvements.
r=me
Thanks for the reviews. Landed in r207849: <http://trac.webkit.org/r207849>. |