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.

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.

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).

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.

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.

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 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 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.

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?

> 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

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.

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.

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

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

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.

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

Argh.

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

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.

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

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

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

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

ESR24. The rebase was simple.

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 on attachment 8422129 [details] [diff] [review] (part 1) - Tweak an edge case in line number handling Minor rebases don't need re-approval. Thanks :)

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