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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

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

[Hannes Verschore [:h4writer]]

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

[Hannes Verschore [:h4writer]]

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

[Nicholas Nethercote [inactive]]

> 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

[Nicholas Nethercote [inactive]]

Attachment: 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

[Nicholas Nethercote [inactive]]

Attachment: 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

[Jason Orendorff [: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 7

[Jason Orendorff [:jorendorff]]

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

[Jason Orendorff [:jorendorff]]

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

[Jason Orendorff [:jorendorff]]

Attachment: OOM fix 2, alternate approach

Comment 10

[Nicholas Nethercote [inactive]]

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

[Jason Orendorff [: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.

Comment 12

[Nicholas Nethercote [inactive]]

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

[Nicholas Nethercote [inactive]]

> 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

[Carsten Book [:Tomcat]]

landed on central https://hg.mozilla.org/mozilla-central/rev/9e0e519805ab

Comment 15

[Al Billings [:abillings - ex-MoCo]]

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

[Nicholas Nethercote [inactive]]

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

AFAICT this goes back to bug 678037, which landed in June 2013.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

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

[Nicholas Nethercote [inactive]]

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

[Al Billings [:abillings - ex-MoCo]]

I doubt we'll take this on Beta and ESR24 since we're making final builds next week. Sylvestre?

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Since we have this issue since several versions, if I don't have to take it, I won't.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

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

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2d1b4d36eef9

Comment 23

[Ryan VanderMeulen [:RyanVM]]

This never had approval to land on Aurora. sec-approval is not the same as branch approval.

Comment 24

[Nicholas Nethercote [inactive]]

Argh.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Were you going to backout or should I?

Comment 26

[Andrew McCreight [:mccr8]]

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

[Andrew McCreight [:mccr8]]

Ok, now it has approval, so there shouldn't be a need to back out the patch.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #28)
> Can we get an esr24 approval request as well?

Comment 30

[Nicholas Nethercote [inactive]]

RyanVM, will you do the ESR24 landing or is that something I have to do? Thanks.

Comment 31

[Andrew McCreight [:mccr8]]

(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

[Ryan VanderMeulen [:RyanVM]]

Al already approved it. I'll land it in May per the whiteboard.

Comment 33

[Nicholas Nethercote [inactive]]

Thanks! And sorry for all the chaos.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Actually, this is going to need some rebasing for esr24. Nick, can you please post a branch patch? Thanks :)

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/81319e7fe4ea
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/de68af394882

Comment 36

[Al Billings [:abillings - ex-MoCo]]

Now that it isn't b2g, let's just land this everywhere. We need an ESR24 patch.

Comment 37

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Tweak an edge case in line number handling

ESR24. The rebase was simple.

Comment 38

[Nicholas Nethercote [inactive]]

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

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/268e2f1ea097

Comment 41

[Matt Wobensmith [:mwobensmith][:matt:]]

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

Comment 42

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-release/rev/3a6b4d996fe7