650621 - Assertion failure: str->length() < JSString::MAX_LENGTH, at jsscopeinlines.h:158

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

[Jeff Walden [:Waldo]]

I think this might just be a boundary failure, and the assertion should be <=.  Still looking.

Comment 3

[Jeff Walden [:Waldo]]

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

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I'm on this.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch-v1

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

[Jeff Walden [:Waldo]]

Comment on attachment 526603 [details] [diff] [review]
patch-v1

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

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch with test to be checked in to TM

Adding test, bringing forward r+.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

http://hg.mozilla.org/tracemonkey/rev/a20192003b39

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/a20192003b39