Closed
Bug 735313
Opened 12 years ago
Closed 12 years ago
Assertion failure: checkLength(length), at vm/StringBuffer.cpp:50
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla14
Tracking | Status | |
---|---|---|
firefox12 | --- | unaffected |
firefox13 | + | fixed |
firefox14 | + | fixed |
firefox-esr10 | - | unaffected |
People
(Reporter: decoder, Assigned: Waldo)
References
Details
(Keywords: assertion, regression, testcase, Whiteboard: [sg:critical maybe] js-triage-done [advisory-tracking+])
Attachments
(3 files, 1 obsolete file)
687 bytes,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
12.19 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
1.04 KB,
patch
|
Waldo
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The following test asserts on mozilla-central revision dfcb11712ec2 (options -m -n): var temp = ''; a=[]; for (var i=0; i<10; i++) { a[a.length] = a; } for (var i=0; i<10; temp ++) { a[a.length] = a.toString(); } This does not crash in GDB when continuing, but I'm marking it S-s anyway until it's triaged because the assert is about length and in StringBuffer, which sounds like a potentially dangerous combo to me :)
Assignee | ||
Comment 1•12 years ago
|
||
inline bool StringBuffer::append(JSLinearString *str) is missing a checkLength before appending, oops. :-( I think adding one there would fix this. But ideally I think we'd do something special to make sure that all appends to the internal vector are always checkLength()'d, and have the type system and/or access controls enforce this.
Comment 2•12 years ago
|
||
Simpler testcase: var a=[12]; while (true) { a.push(a.toString()); } The first bad revision is: changeset: 9d4c267630d4 user: Jeff Walden date: Tue Mar 06 15:28:48 2012 -0800 summary: Bug 733602 - Various StringBuffer cleanups, mostly removing unimplemented methods. r=luke
Blocks: 733602
Keywords: regression
Assignee | ||
Comment 3•12 years ago
|
||
Basically we weren't checking the length of the string we'd generate enough. There are really only two points where this length-checking matters: when finishing a buffer into a string, and when finishing a buffer into an atom. The atom path includes a check in js_AtomizeChars, necessarily. The other path had a check, but I removed it because it looked like every appending path included a check -- but they didn't. As there are a not-entirely-trivial number of ways to append to a string, making sure every one of them contains a check is a little tricky and demonstrably error-prone. So let's just do the check once when attempting to create the final string/atom, and remove the checks from everything else (also coincidentally speeding up appends). If someone buffers up a really huge sequence of characters they'll consume a lot more memory than if we checked on every append, but this is the rare case, so it doesn't seem too bad penalizing it.
Updated•12 years ago
|
Attachment #605904 -
Flags: review?(luke) → review+
Assignee | ||
Comment 4•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc72c4a74024
Assignee | ||
Comment 5•12 years ago
|
||
The trunk patch had a little extra cleanup that's not strictly necessary and makes things trickier to verify. This is the absolute minimal fix. Branch code will be slightly less efficient without the extra changes in the trunk patch, but pretty much negligibly so.
Attachment #605970 -
Flags: review?(luke)
Updated•12 years ago
|
Attachment #605970 -
Flags: review?(luke) → review+
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b24813652b1 Backed out, because the assertion in StringBuffer::length() is now invalid. (It was invalid before, of course, because not all appends were properly checkLength-ing. But only with the landing here did it start biting jstests on tinderbox.) Probably removing that's enough, but I'm going to tryserver to be sure that allowing StringBuffers to grow to whatever size they want doesn't cause OOM failures on tinderbox or whatever.
Comment 7•12 years ago
|
||
There do appear to be later checks for length so maybe this isn't a vulnerability, although maybe we don't check in enough places. Either way let's just fix it.
status-firefox-esr10:
--- → unaffected
status-firefox12:
--- → unaffected
status-firefox13:
--- → affected
status-firefox14:
--- → affected
tracking-firefox13:
--- → +
tracking-firefox14:
--- → +
Updated•12 years ago
|
Whiteboard: js-triage-needed → [sg:critical maybe] js-triage-needed
Assignee | ||
Comment 8•12 years ago
|
||
I removed the assertion from length(). Then I was thinking and realized all the methods that no longer have checkLength() calls could just be defined in the header, too, and things snowballed. To make a long story short, I don't even know why we have StringBuffer-inl.h at all, and with a little work we can just remove it, so I did that. (Branch will obviously take a much smaller patch.)
Attachment #605904 -
Attachment is obsolete: true
Attachment #606022 -
Flags: review?(luke)
Comment 9•12 years ago
|
||
Comment on attachment 606022 [details] [diff] [review] Don't assert in length() either, get rid of StringBuffer-inl.h entirely Review of attachment 606022 [details] [diff] [review]: ----------------------------------------------------------------- Nice ::: js/src/json.cpp @@ +60,4 @@ > > #include "frontend/TokenStream.h" > > +#include "vm/StringBuffer.h" No \n between frontend/TokenStream and this. ::: js/src/jsopcode.cpp @@ +76,2 @@ > #include "vm/Debugger.h" > +#include "vm/StringBuffer.h" No new \n ::: js/src/vm/RegExpObject.cpp @@ +39,5 @@ > * ***** END LICENSE BLOCK ***** */ > > #include "frontend/TokenStream.h" > + > +#include "vm/MatchPairs.h" Ditto
Attachment #606022 -
Flags: review?(luke) → review+
Assignee | ||
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1788def989ae
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
OS: Linux → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [sg:critical maybe] js-triage-needed → [sg:critical maybe] js-triage-done
Target Milestone: --- → mozilla14
Assignee | ||
Comment 11•12 years ago
|
||
[Approval Request Comment] Regression caused by (bug #): Bug 733602. User impact if declined: Not a whole lot, because js_NewString contains the same length-validation check and will prevent the problem there as well, but it'll keep fuzzers happy not to have to tiptoe around this assertion. Testing completed (on m-c, etc.): Fixed on trunk, fix is straightforward to verify. Risk to taking this patch (and alternatives if risky): Very little, simply readding an assertion that had been removed. String changes made by this patch: None.
Attachment #606730 -
Flags: review+
Attachment #606730 -
Flags: approval-mozilla-aurora?
Comment 12•12 years ago
|
||
Comment on attachment 606730 [details] [diff] [review] Aurora patch with small tweak suggested by trunk patch results [Triage Comment] Given the fact that this may be sg:crit, we have a low risk fix in hand, and there's enough time remaining in the cycle to shake out any regressions, approving for Aurora 13.
Attachment #606730 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
tracking-firefox-esr10:
--- → 13+
Assignee | ||
Comment 13•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/13af0e0fdd41
Reporter | ||
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 14•12 years ago
|
||
This is fixed on trunk, it's been fixed in aurora for a week, and I think we had backstop code anyway -- unhiding.
Group: core-security
Updated•12 years ago
|
Keywords: sec-critical
Updated•12 years ago
|
Keywords: sec-critical
Updated•12 years ago
|
Whiteboard: [sg:critical maybe] js-triage-done → [sg:critical maybe] js-triage-done [advisory-tracking+]
Updated•12 years ago
|
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/03c71f6fa4b8
Flags: in-testsuite?
Reporter | ||
Comment 16•11 years ago
|
||
Guessing that this cannot be tested without causing OOM. Setting in-testsuite- for that.
Flags: in-testsuite? → in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•