Last Comment Bug 735313 - Assertion failure: checkLength(length), at vm/StringBuffer.cpp:50
: Assertion failure: checkLength(length), at vm/StringBuffer.cpp:50
Status: VERIFIED FIXED
[sg:critical maybe] js-triage-done [a...
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla14
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks: langfuzz 733602
  Show dependency treegraph
 
Reported: 2012-03-13 10:41 PDT by Christian Holler (:decoder)
Modified: 2013-01-19 12:02 PST (History)
7 users (show)
choller: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
+
fixed
-
unaffected


Attachments
Patch (2.22 KB, patch)
2012-03-14 12:46 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Aurora patch (687 bytes, patch)
2012-03-14 14:56 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Don't assert in length() either, get rid of StringBuffer-inl.h entirely (12.19 KB, patch)
2012-03-14 16:32 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Aurora patch with small tweak suggested by trunk patch results (1.04 KB, patch)
2012-03-16 14:19 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jwalden+bmo: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-03-13 10:41:01 PDT
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 :)
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-13 11:17:29 PDT
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 Jesse Ruderman 2012-03-13 11:35:31 PDT
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
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-14 12:46:17 PDT
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.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-14 14:55:15 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc72c4a74024
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-14 14:56:54 PDT
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.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-14 15:28:59 PDT
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 Daniel Veditz [:dveditz] 2012-03-14 16:15:38 PDT
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.
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-14 16:32:47 PDT
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.)
Comment 9 Luke Wagner [:luke] 2012-03-14 16:38:28 PDT
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
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-16 12:12:06 PDT
https://hg.mozilla.org/mozilla-central/rev/1788def989ae
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-16 14:19:44 PDT
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.
Comment 12 Alex Keybl [:akeybl] 2012-03-20 13:22:42 PDT
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.
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-21 12:58:35 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/13af0e0fdd41
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-30 18:16:16 PDT
This is fixed on trunk, it's been fixed in aurora for a week, and I think we had backstop code anyway -- unhiding.
Comment 16 Christian Holler (:decoder) 2013-01-19 12:02:59 PST
Guessing that this cannot be tested without causing OOM. Setting in-testsuite- for that.

Note You need to log in before you can comment on or make changes to this bug.