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)

defect
Not set
critical

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)

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 :)
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.
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
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #605904 - Flags: review?(luke)
Attachment #605904 - Flags: review?(luke) → review+
Attached patch Aurora patchSplinter Review
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)
Attachment #605970 - Flags: review?(luke) → review+
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.
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.
Whiteboard: js-triage-needed → [sg:critical maybe] js-triage-needed
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 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+
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
[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 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+
Status: RESOLVED → VERIFIED
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
Whiteboard: [sg:critical maybe] js-triage-done → [sg:critical maybe] js-triage-done [advisory-tracking+]
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.

Attachment

General

Created:
Updated:
Size: