Last Comment Bug 650621 - Assertion failure: str->length() < JSString::MAX_LENGTH, at jsscopeinlines.h:158
: Assertion failure: str->length() < JSString::MAX_LENGTH, at jsscopeinlines.h:158
[sg:nse] fixed-in-tracemonkey
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- critical (vote)
: ---
Assigned To: Gary Kwong [:gkw] [:nth10sd]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: langfuzz 640503
  Show dependency treegraph
Reported: 2011-04-17 03:44 PDT by Christian Holler (:decoder)
Modified: 2011-08-05 00:54 PDT (History)
7 users (show)
gary: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch-v1 (784 bytes, patch)
2011-04-17 10:23 PDT, Gary Kwong [:gkw] [:nth10sd]
jwalden+bmo: review+
Details | Diff | Splinter Review
patch with test to be checked in to TM (1.89 KB, patch)
2011-04-17 20:57 PDT, Gary Kwong [:gkw] [:nth10sd]
gary: review+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2011-04-17 03:44:16 PDT
The following code will assert on TM trunk (rev eee087772f45), tested on 32 bit, no options required (but works with -j -m as well):

for (args = "" ;;) args+=new String(args)+1

Must be a recent regression, will investigate later when I'm at home. S-s because I assume with such a length constraint violation you can do all sorts of fancy things.
Comment 1 User image Gary Kwong [:gkw] [:nth10sd] 2011-04-17 06:47:29 PDT
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   64623:474e167e344a
user:        Jeff Walden
date:        Sun Mar 13 20:38:34 2011 -0700
summary:     Bug 640503 - Convert new String().length to use a regular data property and not a shared-permanent property-op getter/setter.  r=jorendorff
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-17 08:30:33 PDT
I think this might just be a boundary failure, and the assertion should be <=.  Still looking.
Comment 3 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-17 08:35:20 PDT
Ah, yes, "The remaining bits store the string length (which must be less or equal than MAX_LENGTH).[sic]" from jsstr.h.  I misread the meaning of the const when writing the patch, it seems.  This is just a bad assertion, no actual problem at execution time, ergo no security concern.  rs=me for anyone to convert that < to a <= if they want this fixed immediately, or I should be able to push the fix Monday afternoon.
Comment 4 User image Gary Kwong [:gkw] [:nth10sd] 2011-04-17 09:17:55 PDT
I'm on this.
Comment 5 User image Gary Kwong [:gkw] [:nth10sd] 2011-04-17 10:23:01 PDT
Created attachment 526603 [details] [diff] [review]

Running js / jit tests now. Will notify if something goes wrong.

A test should also be needed - which directory should it be placed?

i.e., js/src/tests/js1_8 or js1_8_1?
Comment 6 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-17 10:42:44 PDT
Comment on attachment 526603 [details] [diff] [review]

rs= means rubber-stamp -- i.e. the change seemed reasonable but I didn't necessarily give it a full, in-depth review, but it's good to be committed just the same (the usual requirements for backing out and such applying if something happens).  In the future you should feel free to just make the change and commit it with rs=jwalden in the commit message.

I think ecma_5/String/string-object-length.js is a reasonable location.  Note that the test sort of implicitly relues on the relevant strings being ropes.  I don't think this is a requirement all implementations must follow, but as the final string is "only" 512MB, I'm going to err on the side of over-testing and guess that even non-ropeful implementations will be able to run the test up to that size string.

This does also arguably run into questions about what the minimum maximal string length must be in spec-compliant implementations.  But as I recall, our max length was calibrated with that of the other browsers, so I don't think there are any problems on that front for the browser-based implementations.  If anyone comes along and complains, we can move it to ecma_5/extensions/ then.
Comment 7 User image Gary Kwong [:gkw] [:nth10sd] 2011-04-17 20:57:49 PDT
Created attachment 526652 [details] [diff] [review]
patch with test to be checked in to TM

Adding test, bringing forward r+.
Comment 8 User image Gary Kwong [:gkw] [:nth10sd] 2011-04-17 21:05:43 PDT
Comment 9 User image Chris Leary [:cdleary] (not checking bugmail) 2011-04-26 15:45:41 PDT

Note You need to log in before you can comment on or make changes to this bug.