Closed Bug 992274 Opened 6 years ago Closed 6 years ago

Assertion failure: i < mLength, at ../../dist/include/mozilla/Vector.h:379

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- verified
firefox31 --- verified
firefox-esr24 30+ verified
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
seamonkey2.26 --- fixed

People

(Reporter: h4writer, Assigned: njn)

References

(Blocks 2 open bugs)

Details

(Keywords: csectype-bounds, csectype-oom, sec-high, Whiteboard: [adv-main30+][adv-esr24.6+])

Attachments

(4 files)

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.
Because the code under frame 1 is adjusted in http://hg.mozilla.org/integration/mozilla-inbound/rev/1f3587e02361 (bug 747831), calling for needinfo.
Flags: needinfo?(n.nethercote)
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
> 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.
Flags: needinfo?(n.nethercote)
Attached patch OOM fix 1.Splinter Review
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).
Attachment #8402456 - Flags: review?(jorendorff)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attached patch OOM fix 2.Splinter Review
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.
Attachment #8402457 - Flags: review?(jorendorff)
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 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.
Attachment #8402456 - Flags: review?(jorendorff) → review+
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...
Attachment #8402457 - Flags: review?(jorendorff)
Attachment #8402857 - Flags: review?(n.nethercote)
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 :(
Attachment #8402857 - Flags: review?(n.nethercote) → review-
Attachment #8402457 - Flags: review?(jorendorff)
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.
Attachment #8402457 - Flags: review?(jorendorff) → review+
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?
Whiteboard: [leave open]
> 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.
landed on central https://hg.mozilla.org/mozilla-central/rev/9e0e519805ab
Flags: in-testsuite?
Target Milestone: --- → mozilla31
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?
Oh, sorry! Landing without sec-approval was my fault :(

AFAICT this goes back to bug 678037, which landed in June 2013.
Blocks: LazyBytecode
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 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.
Attachment #8402456 - Flags: sec-approval?
I doubt we'll take this on Beta and ESR24 since we're making final builds next week. Sylvestre?
Flags: needinfo?(sledru)
Since we have this issue since several versions, if I don't have to take it, I won't.
Flags: needinfo?(sledru)
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.
Attachment #8402456 - Flags: sec-approval? → sec-approval+
This never had approval to land on Aurora. sec-approval is not the same as branch approval.
Flags: needinfo?(n.nethercote)
Argh.
Flags: needinfo?(n.nethercote)
Were you going to backout or should I?
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
Attachment #8402456 - Flags: approval-mozilla-aurora?
Attachment #8402456 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Ok, now it has approval, so there shouldn't be a need to back out the patch.
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open] → [land on esr24 after 5/15]
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #28)
> Can we get an esr24 approval request as well?
Flags: needinfo?(n.nethercote)
Attachment #8402456 - Flags: approval-mozilla-esr24+
Flags: needinfo?(n.nethercote)
RyanVM, will you do the ESR24 landing or is that something I have to do? Thanks.
(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.
Al already approved it. I'll land it in May per the whiteboard.
Thanks! And sorry for all the chaos.
Actually, this is going to need some rebasing for esr24. Nick, can you please post a branch patch? Thanks :)
Flags: needinfo?(n.nethercote)
Now that it isn't b2g, let's just land this everywhere. We need an ESR24 patch.
Whiteboard: [land on esr24 after 5/15]
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.
Attachment #8422129 - Flags: review+
Attachment #8422129 - Flags: approval-mozilla-esr24?
Flags: needinfo?(n.nethercote)
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 :)
Attachment #8422129 - Flags: approval-mozilla-esr24?
Whiteboard: [adv-main30+][adv-esr24.6+]
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
Group: core-security
You need to log in before you can comment on or make changes to this bug.