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




JavaScript Engine
6 years ago
6 years ago


(Reporter: decoder, Assigned: gkw)


(Blocks: 1 bug, {assertion, regression, testcase})

assertion, regression, testcase
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [sg:nse] fixed-in-tracemonkey)


(1 attachment, 1 obsolete attachment)



6 years ago
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

6 years ago
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
Blocks: 640503
I think this might just be a boundary failure, and the assertion should be <=.  Still looking.
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.
Group: core-security
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [sg:nse]

Comment 4

6 years ago
I'm on this.
Assignee: general → gary
Keywords: regression


6 years ago
Summary: TM: Assertion failure: str->length() < JSString::MAX_LENGTH, at jsscopeinlines.h:158 → Assertion failure: str->length() < JSString::MAX_LENGTH, at jsscopeinlines.h:158

Comment 5

6 years ago
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?
Attachment #526603 - Flags: review?(jwalden+bmo)
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.
Attachment #526603 - Flags: review?(jwalden+bmo) → review+

Comment 7

6 years ago
Created attachment 526652 [details] [diff] [review]
patch with test to be checked in to TM

Adding test, bringing forward r+.
Attachment #526603 - Attachment is obsolete: true
Attachment #526652 - Flags: review+

Comment 8

6 years ago
Whiteboard: [sg:nse] → [sg:nse] fixed-in-tracemonkey


6 years ago
Flags: in-testsuite+
Last Resolved: 6 years ago
Resolution: --- → FIXED


6 years ago
Blocks: 676763
You need to log in before you can comment on or make changes to this bug.