The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 30, Firefox OS v1.2

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: h4writer, Assigned: njn)

Tracking

(Blocks: 2 bugs, {csectype-bounds, csectype-oom, sec-high})

unspecified
mozilla31
csectype-bounds, csectype-oom, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

SeaMonkey Tracking Flags

(firefox28 wontfix, firefox29 wontfix, firefox30 verified, firefox31 verified, firefox-esr2430+ 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)

Details

(Whiteboard: [adv-main30+][adv-esr24.6+])

Attachments

(4 attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
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)
(Reporter)

Comment 2

3 years ago
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
(Assignee)

Comment 3

3 years ago
> 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)
(Assignee)

Comment 4

3 years ago
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).
Attachment #8402456 - Flags: review?(jorendorff)
(Assignee)

Updated

3 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 5

3 years ago
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.
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.
Blocks: 912928, 988953
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)
Created attachment 8402857 [details] [diff] [review]
OOM fix 2, alternate approach
Attachment #8402857 - Flags: review?(n.nethercote)
(Assignee)

Comment 10

3 years ago
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-
(Assignee)

Updated

3 years ago
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+
Keywords: csectype-bounds, csectype-oom, sec-high
(Assignee)

Comment 12

3 years ago
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]
(Assignee)

Comment 13

3 years ago
> 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
status-firefox31: --- → fixed
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?
(Assignee)

Comment 16

3 years ago
Oh, sorry! Landing without sec-approval was my fault :(

AFAICT this goes back to bug 678037, which landed in June 2013.
Blocks: 678037
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.
status-firefox28: --- → wontfix
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox-esr24: --- → affected
(Assignee)

Comment 18

3 years ago
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)
status-b2g18: --- → unaffected
status-b2g-v1.1hd: --- → unaffected
status-b2g-v1.2: --- → affected
status-b2g-v1.3: --- → affected
status-b2g-v1.3T: --- → affected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → fixed
Since we have this issue since several versions, if I don't have to take it, I won't.
Flags: needinfo?(sledru)
status-firefox29: affected → wontfix
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+
(Assignee)

Comment 22

3 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d1b4d36eef9
This never had approval to land on Aurora. sec-approval is not the same as branch approval.
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 24

3 years ago
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.
status-firefox30: affected → fixed
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
Last Resolved: 3 years ago
status-b2g-v1.4: affected → fixed
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)
(Assignee)

Comment 30

3 years ago
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.
(Assignee)

Comment 33

3 years ago
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)
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/81319e7fe4ea
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/de68af394882
status-b2g-v1.2: affected → fixed
status-b2g-v1.3: affected → fixed
Now that it isn't b2g, let's just land this everywhere. We need an ESR24 patch.
Whiteboard: [land on esr24 after 5/15]
status-b2g-v1.3T: affected → fixed
(Assignee)

Comment 37

3 years ago
Created attachment 8422129 [details] [diff] [review]
(part 1) - Tweak an edge case in line number handling

ESR24. The rebase was simple.
(Assignee)

Comment 38

3 years ago
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?
https://hg.mozilla.org/releases/mozilla-esr24/rev/268e2f1ea097
status-firefox-esr24: affected → fixed
tracking-firefox-esr24: --- → 30+
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
status-firefox30: fixed → verified
status-firefox31: fixed → verified
status-firefox-esr24: fixed → verified
https://hg.mozilla.org/releases/mozilla-release/rev/3a6b4d996fe7
status-seamonkey2.26: --- → fixed
status-firefox32: --- → affected
status-firefox32: affected → ---
Group: core-security
You need to log in before you can comment on or make changes to this bug.