Assertion failure: checkLength(length), at vm/StringBuffer.cpp:50

VERIFIED FIXED in Firefox 13

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: Waldo)

Tracking

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

Trunk
mozilla14
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox12 unaffected, firefox13+ fixed, firefox14+ fixed, firefox-esr10- unaffected)

Details

(Whiteboard: [sg:critical maybe] js-triage-done [advisory-tracking+])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.

Comment 2

5 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
Created attachment 605904 [details] [diff] [review]
Patch

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)

Updated

5 years ago
Attachment #605904 - Flags: review?(luke) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc72c4a74024
Created attachment 605970 [details] [diff] [review]
Aurora patch

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

5 years ago
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.
status-firefox-esr10: --- → unaffected
status-firefox12: --- → unaffected
status-firefox13: --- → affected
status-firefox14: --- → affected
tracking-firefox13: --- → +
tracking-firefox14: --- → +
Whiteboard: js-triage-needed → [sg:critical maybe] js-triage-needed
Created attachment 606022 [details] [diff] [review]
Don't assert in length() either, get rid of StringBuffer-inl.h entirely

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

5 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+
https://hg.mozilla.org/mozilla-central/rev/1788def989ae
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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
Created attachment 606730 [details] [diff] [review]
Aurora patch with small tweak suggested by trunk patch results

[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+

Updated

5 years ago
tracking-firefox-esr10: --- → 13+
https://hg.mozilla.org/releases/mozilla-aurora/rev/13af0e0fdd41
status-firefox13: affected → fixed
(Reporter)

Updated

5 years ago
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
status-firefox14: affected → fixed
Keywords: sec-critical
Keywords: sec-critical
Whiteboard: [sg:critical maybe] js-triage-done → [sg:critical maybe] js-triage-done [advisory-tracking+]

Updated

5 years ago
tracking-firefox-esr10: 13+ → -
https://hg.mozilla.org/mozilla-central/rev/03c71f6fa4b8
Flags: in-testsuite?
(Reporter)

Comment 16

4 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.