As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 992274 - Assertion failure: i < mLength, at ../../dist/include/mozilla/Vector.h:379
: Assertion failure: i < mLength, at ../../dist/include/mozilla/Vector.h:379
Status: RESOLVED FIXED
[adv-main30+][adv-esr24.6+]
: csectype-bounds, csectype-oom, sec-high
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla31
Assigned To: Nicholas Nethercote [:njn]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 912928 988953 LazyBytecode
  Show dependency treegraph
 
Reported: 2014-04-04 09:14 PDT by Hannes Verschore [:h4writer]
Modified: 2015-08-30 12:12 PDT (History)
10 users (show)
cbook: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
verified
verified
30+
verified
unaffected
unaffected
fixed
fixed
fixed
fixed
fixed
fixed


Attachments
OOM fix 1. (5.79 KB, patch)
2014-04-06 21:31 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
abillings: approval‑mozilla‑aurora+
abillings: approval‑mozilla‑esr24+
abillings: sec‑approval+
Details | Diff | Splinter Review
OOM fix 2. (1.93 KB, patch)
2014-04-06 21:32 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Splinter Review
OOM fix 2, alternate approach (17.40 KB, patch)
2014-04-07 13:06 PDT, Jason Orendorff [:jorendorff]
n.nethercote: review-
Details | Diff | Splinter Review
(part 1) - Tweak an edge case in line number handling (5.80 KB, patch)
2014-05-13 17:48 PDT, Nicholas Nethercote [:njn]
n.nethercote: review+
Details | Diff | Splinter Review

Description User image Hannes Verschore [:h4writer] 2014-04-04 09:14:29 PDT
Running: 

js -e oomAfterAllocations\(447\) -f ./tests/shell.js -f ./tests/js1_7/geniter/shell.js -f ./tests/js1_7/geniter/regress-349331.js

on a linux 32bit debug build gives:
Assertion failure: i < mLength, at ../../dist/include/mozilla/Vector.h:379

(gdb) bt
> #0  0x08093ba5 in mozilla::VectorBase<unsigned int, 128u, js::TempAllocPolicy, js::Vector<unsigned int, 128u, js::TempAllocPolicy >::operator[] (this=0xbfffe4e0, i=256)    at ../../dist/include/mozilla/Vector.h:379
> #1  0x081d1ef3 in js::frontend::TokenStream::SourceCoords::lineIndexOf (this=0xbfffe4e0, offset=7444) at /home/h4writer/Build/mozilla-inbound/js/src/frontend/TokenStream.cpp:194
> #2  0x08178176 in js::frontend::TokenStream::SourceCoords::lineNum (this=0xbfffe4e0, offset=7444) at /home/h4writer/Build/mozilla-inbound/js/src/frontend/TokenStream.cpp:233
> #3  0x081242ee in UpdateLineNumberNotes (cx=0x8ac7f60, bce=0xbfffdc34, offset=7444) at /home/h4writer/Build/mozilla-inbound/js/src/frontend/BytecodeEmitter.cpp:377
> #4  0x081243c8 in UpdateSourceCoordNotes (cx=0x8ac7f60, bce=0xbfffdc34, offset=7444) at /home/h4writer/Build/mozilla-inbound/js/src/frontend/BytecodeEmitter.cpp:410
> #5  0x0813972d in js::frontend::EmitTree (cx=0x8ac7f60, bce=0xbfffdc34, pn=0x8afbf38) at /home/h4writer/Build/mozilla-inbound/js/src/frontend/BytecodeEmitter.cpp:6606
> #6  0x081220ac in js::frontend::CompileScript (cx=0x8ac7f60, alloc=0x8abb62c, scopeChain=..., evalCaller=..., options=...,     chars=0x8b89018 u"/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-\n * This Source Code Form is subject to the terms of the Mozilla Public\n * License, v. 2.0. If a copy of the MPL was not dis"..., length=19878, source_=warning: (Internal error: pc 0x0 in read in psymtab, but not in symtab.) 0x0, staticLevel=0, extraSct=warning: (Internal error: pc 0x0 in read in psymtab, but not in symtab.)0x0) at /home/h4writer/Build/mozilla-inbound/js/src/frontend/BytecodeCompiler.cpp:376
> #7  0x084acc9a in JS::Compile (cx=0x8ac7f60, obj=..., options=...,     chars=0x8b89018 u"/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-\n * This Source Code Form is subject to the terms of the Mozilla Public\n * License, v. 2.0. If a copy of the MPL was not dis"..., length=19878) at /home/h4writer/Build/mozilla-inbound/js/src/jsapi.cpp:4449
> #8  0x084acd98 in JS::Compile (cx=0x8ac7f60, obj=..., options=...,     bytes=0x8b81010 "/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-\n * This Source Code Form is subject to the terms of the Mozilla Public\n * License, v. 2.0. If a copy of the MPL was not dis"..., length=19878) at /home/h4writer/Build/mozilla-inbound/js/src/jsapi.cpp:4464
> #9  0x084ace62 in JS::Compile (cx=0x8ac7f60, obj=..., options=..., fp=0x8af91f8) at /home/h4writer/Build/mozilla-inbound/js/src/jsapi.cpp:4476
> #10 0x0804c8d3 in RunFile (cx=0x8ac7f60, obj=..., filename=0xbffff1ca "./tests/shell.js", file=0x8af91f8, compileOnly=false) at /home/h4writer/Build/mozilla-inbound/js/src/shell/js.cpp:447
> #11 0x0804d25f in Process (cx=0x8ac7f60, obj_=0xb5d2e040, filename=0xbffff1ca "./tests/shell.js", forceTTY=false) at /home/h4writer/Build/mozilla-inbound/js/src/shell/js.cpp:590
> #12 0x0805fea6 in ProcessArgs (cx=0x8ac7f60, obj_=0xb5d2e040, op=0xbfffeeb0) at /home/h4writer/Build/mozilla-inbound/js/src/shell/js.cpp:5718
> #13 0x08060a69 in Shell (cx=0x8ac7f60, op=0xbfffeeb0, envp=0xbffff01c) at /home/h4writer/Build/mozilla-inbound/js/src/shell/js.cpp:5931
> #14 0x08061b1e in main (argc=9, argv=0xbfffeff4, envp=0xbffff01c) at /home/h4writer/Build/mozilla-inbound/js/src/shell/js.cpp:6197

In frame 1:
> #1  0x081d1ef3 in js::frontend::TokenStream::SourceCoords::lineIndexOf (this=0xbfffe4e0, offset=7444) at /home/h4writer/Build/mozilla-inbound/js/src/frontend/TokenStream.cpp:194
> 194             if (offset < lineStartOffsets_[lastLineIndex_ + 1])

(gdb) p lineStartOffsets_.length()
$1 = 256
(gdb) p lastLineIndex_
$2 = 255

So we are reading out of bounds of the vector.
Comment 1 User image Hannes Verschore [:h4writer] 2014-04-04 09:15:59 PDT
Because the code under frame 1 is adjusted in http://hg.mozilla.org/integration/mozilla-inbound/rev/1f3587e02361 (bug 747831), calling for needinfo.
Comment 2 User image Hannes Verschore [:h4writer] 2014-04-04 09:17:52 PDT
Updated run command (since copy did something wrong):

js -e "oomAfterAllocations(447)" -f ./tests/shell.js -f ./tests/js1_7/geniter/shell.js -f ./tests/js1_7/geniter/regress-349331.js
Comment 3 User image Nicholas Nethercote [:njn] 2014-04-06 19:53:50 PDT
> js -e "oomAfterAllocations(447)" -f ./tests/shell.js -f
> ./tests/js1_7/geniter/shell.js -f ./tests/js1_7/geniter/regress-349331.js

I had to change the 447 to 449, but I can reproduce it now.
Comment 4 User image Nicholas Nethercote [:njn] 2014-04-06 21:31:52 PDT
Created attachment 8402456 [details] [diff] [review]
OOM fix 1.

For some reason, SourceCoords::fill() deliberately ignores OOMs. I guess I
thought this was safe, but it's not. This patch makes it not do that, bubbling
any failures up through TokenStream::seek() (but only the two-argument
overloading)

This fixes the assertion failure. I now get a different assertion failure in
the JS shell, which seems less important (and I got it multiple times when
experimenting with different oomAfterAllocations values).
Comment 5 User image Nicholas Nethercote [:njn] 2014-04-06 21:32:58 PDT
Created attachment 8402457 [details] [diff] [review]
OOM fix 2.

This patch fixes things so the "don't do anything on OOM" path in
SourceCoords::add() actually doesn't do anything on OOM.

I'll merge the two patches before landing, to make things marginally more
difficult for security-bug snoopers.
Comment 6 User image Jason Orendorff [:jorendorff] 2014-04-07 12:33:58 PDT
I have a patch that's very similar to part 1 here up for review in bug 990787. Let's scuttle mine and take yours. Reviewing (really: comparing) now.
Comment 7 User image Jason Orendorff [:jorendorff] 2014-04-07 12:39:41 PDT
Comment on attachment 8402456 [details] [diff] [review]
OOM fix 1.

Review of attachment 8402456 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this is a strict sub-patch of part 9 in bug 990787; I'll resubmit the remaining junk there.
Comment 8 User image Jason Orendorff [:jorendorff] 2014-04-07 12:43:16 PDT
Comment on attachment 8402457 [details] [diff] [review]
OOM fix 2.

Review of attachment 8402457 [details] [diff] [review]:
-----------------------------------------------------------------

Oh. The other half of my patch there is just a bigger version of this; instead of sweeping the OOM under the rug and producing slightly wrong answers, my patch propagated oom out of getChar. It's not as much effort as you'd think; something like 57 lines touched.

Let me post that one here, and you tell me which is better...
Comment 9 User image Jason Orendorff [:jorendorff] 2014-04-07 13:06:13 PDT
Created attachment 8402857 [details] [diff] [review]
OOM fix 2, alternate approach
Comment 10 User image Nicholas Nethercote [:njn] 2014-04-07 18:15:16 PDT
Comment on attachment 8402857 [details] [diff] [review]
OOM fix 2, alternate approach

Review of attachment 8402857 [details] [diff] [review]:
-----------------------------------------------------------------

getChar() is super-hot and called from quite a few places. I'd really prefer to not have to handle the failure case :(
Comment 11 User image Jason Orendorff [:jorendorff] 2014-04-08 09:10:54 PDT
Comment on attachment 8402457 [details] [diff] [review]
OOM fix 2.

Review of attachment 8402457 [details] [diff] [review]:
-----------------------------------------------------------------

The perf argument doesn't seem right to me. The hottest call site uses 'switch(getChar())' and most of the rest are infallible: we'd only check for errors in debug-mode assertions. One call site (TokenStream::advance) probably doesn't need to touch SourceCoords at all (?). In all the remaining call sites, the check for 'c == GETCHAR_ERROR' could be combined with 'c == EOF' to say 'c < 0'.

Reviewing your patch anyway as I'm pessimistic of my chances here ;-)

I wonder if inlining the fastest path through getChar() would win anything...

::: js/src/frontend/TokenStream.cpp
@@ +151,3 @@
>          uint32_t maxPtr = MAX_PTR;
> +        if (lineStartOffsets_.append(maxPtr))
> +            lineStartOffsets_[lineIndex] = lineStartOffset;

If append fails, we'll have reported an error on lineStartOffsets_.cx_, so there should be an else branch here that calls cx->recoverFromOutOfMemory().

(No getter for cx_ in the base class where it's declared, js::TempAllocPolicy, but there could be.)

r=me with that.
Comment 12 User image Nicholas Nethercote [:njn] 2014-04-13 15:42:35 PDT
Part 1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e0e519805ab


> If append fails, we'll have reported an error on lineStartOffsets_.cx_, so
> there should be an else branch here that calls cx->recoverFromOutOfMemory().
> 
> (No getter for cx_ in the base class where it's declared,
> js::TempAllocPolicy, but there could be.)

TempAllocPolicy only has a ContextFriendFields*, so I can't see how call recoverOutOfMemory without some kind of unsafe cast. Am I missing something?
Comment 13 User image Nicholas Nethercote [:njn] 2014-04-13 19:27:28 PDT
> I wonder if inlining the fastest path through getChar() would win anything...

getChar() isn't actually called much. getCharIgnoreEOL() is used most of the time, and it already gets inlined.
Comment 14 User image Carsten Book [:Tomcat] 2014-04-14 06:59:12 PDT
landed on central https://hg.mozilla.org/mozilla-central/rev/9e0e519805ab
Comment 15 User image Al Billings [:abillings] 2014-04-17 13:16:53 PDT
Is this trunk only? I assume it isn't so it should have gotten a sec-approval? before landing, Carsten. See https://wiki.mozilla.org/Security/Bug_Approval_Process .

How far back does this affect things?
Comment 16 User image Nicholas Nethercote [:njn] 2014-04-17 14:58:44 PDT
Oh, sorry! Landing without sec-approval was my fault :(

AFAICT this goes back to bug 678037, which landed in June 2013.
Comment 17 User image Al Billings [:abillings] 2014-04-17 15:23:24 PDT
Can you get the questions from the sec-approval? template and answer them? We need to figure out if we have to backport this to branches now that we've exposed it on trunk.
Comment 18 User image Nicholas Nethercote [:njn] 2014-04-17 15:32:44 PDT
Comment on attachment 8402456 [details] [diff] [review]
OOM fix 1.

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

I'd guess very difficult. (Before pwn2own this year, I would have thought "almost impossible", but an exploit was crafted that involved a missing OOM check, so who knows.)

> Do comments in the patch, the check-in comment, or tests included in the 
> patch paint a bulls-eye on the security problem?

No.

> Which older supported branches are affected by this flaw?

All of them, AFAIK -- the problem was introduced in June 2013.

> If not all supported branches, which bug introduced the flaw?

Bug 678037.

> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?

This will probably apply as-is. If not, rebasing should be easy.

> How likely is this patch to cause regressions; how much testing does it need?

Very unlikely, because it just adds some missing OOM checks. That's not something we really test for, though decoder does do some fuzzing of allocation failures.
Comment 19 User image Al Billings [:abillings] 2014-04-17 15:52:40 PDT
I doubt we'll take this on Beta and ESR24 since we're making final builds next week. Sylvestre?
Comment 20 User image Sylvestre Ledru [:sylvestre] 2014-04-18 06:20:19 PDT
Since we have this issue since several versions, if I don't have to take it, I won't.
Comment 21 User image Al Billings [:abillings] 2014-04-18 09:55:44 PDT
Sec-approval+ for trunk. Let's get this on Aurora too. 

I would say that we check this into ESR24 around 5/15, just to get it there too but we can revisit then.
Comment 22 User image Nicholas Nethercote [:njn] 2014-04-22 20:28:43 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d1b4d36eef9
Comment 23 User image Ryan VanderMeulen [:RyanVM] 2014-04-22 20:40:32 PDT
This never had approval to land on Aurora. sec-approval is not the same as branch approval.
Comment 24 User image Nicholas Nethercote [:njn] 2014-04-22 21:15:03 PDT
Argh.
Comment 25 User image Ryan VanderMeulen [:RyanVM] 2014-04-23 03:54:21 PDT
Were you going to backout or should I?
Comment 26 User image Andrew McCreight [:mccr8] 2014-04-23 09:37:16 PDT
Comment on attachment 8402456 [details] [diff] [review]
OOM fix 1.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 678037.
User impact if declined: security problems
Testing completed (on m-c, etc.): it has been on m-c for a week or so
Risk to taking this patch (and alternatives if risky): very low, according to comment 18.
String or IDL/UUID changes made by this patch: none
Comment 27 User image Andrew McCreight [:mccr8] 2014-04-23 09:40:18 PDT
Ok, now it has approval, so there shouldn't be a need to back out the patch.
Comment 28 User image Ryan VanderMeulen [:RyanVM] 2014-04-23 10:46:15 PDT
Given that we're really close to the uplift, let's track any future patches in a new bug for saner tracking. Can we get an esr24 approval request as well? Per comment 21, I'm adding a whiteboard comment now about waiting to uplift it once approved.
Comment 29 User image Ryan VanderMeulen [:RyanVM] 2014-04-24 08:27:51 PDT
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #28)
> Can we get an esr24 approval request as well?
Comment 30 User image Nicholas Nethercote [:njn] 2014-04-24 12:20:11 PDT
RyanVM, will you do the ESR24 landing or is that something I have to do? Thanks.
Comment 31 User image Andrew McCreight [:mccr8] 2014-04-24 12:33:13 PDT
(In reply to Nicholas Nethercote [:njn] from comment #30)
> RyanVM, will you do the ESR24 landing or is that something I have to do?
> Thanks.

You need to request esr24 approval on the patch, then Ryan will land it once it has approval.
Comment 32 User image Ryan VanderMeulen [:RyanVM] 2014-04-24 12:42:46 PDT
Al already approved it. I'll land it in May per the whiteboard.
Comment 33 User image Nicholas Nethercote [:njn] 2014-04-24 15:21:46 PDT
Thanks! And sorry for all the chaos.
Comment 34 User image Ryan VanderMeulen [:RyanVM] 2014-05-07 08:19:03 PDT
Actually, this is going to need some rebasing for esr24. Nick, can you please post a branch patch? Thanks :)
Comment 36 User image Al Billings [:abillings] 2014-05-09 11:41:29 PDT
Now that it isn't b2g, let's just land this everywhere. We need an ESR24 patch.
Comment 37 User image Nicholas Nethercote [:njn] 2014-05-13 17:48:35 PDT
Created attachment 8422129 [details] [diff] [review]
(part 1) - Tweak an edge case in line number handling

ESR24. The rebase was simple.
Comment 38 User image Nicholas Nethercote [:njn] 2014-05-13 17:51:20 PDT
Comment on attachment 8422129 [details] [diff] [review]
(part 1) - Tweak an edge case in line number handling

The previous version was already approved for ESR24. This is just a minor rebase.
Comment 39 User image Ryan VanderMeulen [:RyanVM] 2014-05-13 17:55:05 PDT
Comment on attachment 8422129 [details] [diff] [review]
(part 1) - Tweak an edge case in line number handling

Minor rebases don't need re-approval. Thanks :)
Comment 40 User image Ryan VanderMeulen [:RyanVM] 2014-05-14 08:23:02 PDT
https://hg.mozilla.org/releases/mozilla-esr24/rev/268e2f1ea097
Comment 41 User image Matt Wobensmith [:mwobensmith][:matt:] 2014-06-03 17:17:22 PDT
Confirmed crash on 2014-04-04.
Confirmed fixed on 2014-06-03 for Fx24esr, Fx30 and Fx31.

However, I noticed that it still crashes Fx32. Similar output to original problem:

Assertion failure: [unhandlable oom] Could not add to pending GC helpers list, at /builds/slave/m-cen-osx64-d-0000000000000000/build/js/src/jscntxt.cpp:1391
Hit MOZ_CRASH() at /builds/slave/m-cen-osx64-d-0000000000000000/build/js/src/jscntxt.cpp:1392
Segmentation fault: 11

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