Crash [@ js::UnwindIteratorForException] or Assertion failure: moreAllocations(), at jit/JitFrameIterator.h:454 or heap-use-after-free [@ js::jit::CompactBufferReader::readByte]

VERIFIED FIXED in Firefox 39

Status

()

defect
--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks 1 bug, 6 keywords)

Trunk
mozilla41
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.2?, firefox37 wontfix, firefox38- wontfix, firefox39+ verified, firefox38.0.5 wontfix, firefox40+ verified, firefox41 verified, firefox-esr3139+ fixed, firefox-esr3839+ verified, b2g-v1.4 wontfix, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [jsbugmon:ignore][adv-main39+][adv-esr31.8+][adv-esr38.1+], crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

4 years ago
The following testcase crashes on mozilla-central revision 436686833af0 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

function foo() {
  function gen() {
    try {
      yield 1;
    } finally {
      bar();
    }
  }
  for (i in gen()) {
    for each (var i in this) 
	return false;   
  }
} foo();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::UnwindIteratorForException (cx=cx@entry=0x1734b60, obj=obj@entry=...) at js/src/jsobj.h:128
#0  js::UnwindIteratorForException (cx=cx@entry=0x1734b60, obj=obj@entry=...) at js/src/jsobj.h:128
#1  0x000000000071d64f in CloseLiveIterator (localSlot=<optimized out>, frame=..., cx=0x1734b60) at js/src/jit/JitFrames.cpp:388
#2  HandleExceptionIon (overrecursed=0x7fffffffb7f0, rfe=0x7fffffffc280, frame=..., cx=0x1734b60) at js/src/jit/JitFrames.cpp:464
#3  js::jit::HandleException (rfe=0x7fffffffc280) at js/src/jit/JitFrames.cpp:782
#4  0x00007ffff7fe815d in ?? ()
[...]
#24 0x0000000000000000 in ?? ()
rax	0xfff9000000000000	-1970324836974592
rbx	0x1734b60	24333152
rcx	0xf	15
rdx	0x1734b78	24333176
rsi	0x16eab40	24030016
rdi	0x1743810	24393744
rbp	0x7fffffffb880	140737488337024
rsp	0x7fffffffb720	140737488336672
r8	0x17cef60	24964960
r9	0x3bb1eea0	1001516704
r10	0x17cef60	24964960
r11	0x7fffffffb720	140737488336672
r12	0x1	1
r13	0x7fffffffb8d0	140737488337104
r14	0x7fffffffb870	140737488337008
r15	0x1818938	25266488
rip	0x870b49 <js::UnwindIteratorForException(JSContext*, JS::Handle<JSObject*>)+105>
=> 0x870b49 <js::UnwindIteratorForException(JSContext*, JS::Handle<JSObject*>)+105>:	mov    (%rcx),%rdx
   0x870b4c <js::UnwindIteratorForException(JSContext*, JS::Handle<JSObject*>)+108>:	mov    (%rdx),%rdx


S-s and sec-critical because on ASan builds this shows up as a heap-use-after-free.
Reporter

Updated

4 years ago
Group: core-security
Reporter

Updated

4 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect,ignore]
Reporter

Comment 1

4 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 008b3f65a7e0).
Reporter

Updated

4 years ago
Whiteboard: [jsbugmon:update,bisect,ignore] → [jsbugmon:bisectfix]
Did the bisectfix get stuck?  How long should we expect these bisections will take?
Flags: needinfo?(choller)
Reporter

Comment 3

4 years ago
The problem seems to be that the testcase is intermittent. Bisecting won't work well in this case I assume. gkw, could you bisect this manually?
Flags: needinfo?(choller) → needinfo?(gary)
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:ignore]
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/35038c3324ee
user:        Jan de Mooij
date:        Thu Jul 24 11:56:41 2014 +0200
summary:     Bug 1031529 part 1 - Add a --no-threads shell flag. r=bhackett

I don't think this is correct, but I tried going back further than this (using the old --enable-threadsafe compile flag) - I didn't get a conclusive answer either.

I guess, Jan could take a look at the root cause and then he can see who's a better person to look at this.
Flags: needinfo?(gary) → needinfo?(jdemooij)
Any ideas here?
Tracking for 38+ since this is sec-critical.
It is a fuzzing bug, no known exploit using it, not tracking.
Assignee

Comment 8

4 years ago
Posted patch Part 1 - Fix (obsolete) — Splinter Review
The problem is that we have a JSOP_ENDITER that throws an exception (see the outer for-in in the testcase). We're closing a legacy generator that throws in a finally block.

Then we end up in HandleExceptionIon where we try to close the iterator on the stack, but it's not there because the resume point after MIteratorEnd contains the stack state *after* we pop the iterator.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8599472 - Flags: review?(shu)
Assignee

Comment 9

4 years ago
We can land this after part 1 is on all branches.
Attachment #8599482 - Flags: review?(shu)
Assignee

Comment 10

4 years ago
Actually I'm not sure the analysis is correct; let me double check..
Assignee

Comment 11

4 years ago
Hm yeah, my analysis wasn't entirely correct. We have:

for (var x in y) {
    for (var i in j) {
        return;
    }
}

For the |return|, we emit two JSOP_ENDITER ops (one for each loop). The problem is that if the *second* one throws (for the outer loop), it tries to close *both* iterators (based on the try notes), but we already closed the inner iterator and popped it.

Shu, do you remember why TryNoteIterIon uses IgnoreStackDepthOp? It seems like the stack depth check is how the interpreter and Baseline handle this.
Flags: needinfo?(shu)
(In reply to Jan de Mooij [:jandem] from comment #11)
> Hm yeah, my analysis wasn't entirely correct. We have:
> 
> for (var x in y) {
>     for (var i in j) {
>         return;
>     }
> }
> 
> For the |return|, we emit two JSOP_ENDITER ops (one for each loop). The
> problem is that if the *second* one throws (for the outer loop), it tries to
> close *both* iterators (based on the try notes), but we already closed the
> inner iterator and popped it.
> 
> Shu, do you remember why TryNoteIterIon uses IgnoreStackDepthOp? It seems
> like the stack depth check is how the interpreter and Baseline handle this.

I don't. When I refactored the TryNoteIter stuff I blindly copied the old logic for Ion, which didn't take the stack depth into consideration. Sounds like it should, though.
Flags: needinfo?(shu)
Assignee

Comment 13

4 years ago
Posted patch Part 1 - FixSplinter Review
This patch makes TryNoteIterIon use the actual number of stack slots it has instead of UINT32_MAX.
Attachment #8599472 - Attachment is obsolete: true
Attachment #8599482 - Attachment is obsolete: true
Attachment #8599472 - Flags: review?(shu)
Attachment #8599482 - Flags: review?(shu)
Attachment #8599748 - Flags: review?(shu)
Assignee

Comment 14

4 years ago
Attachment #8599749 - Flags: review?(shu)
Comment on attachment 8599748 [details] [diff] [review]
Part 1 - Fix

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

Great! The Ion exception code is less sketchy now.
Attachment #8599748 - Flags: review?(shu) → review+

Updated

4 years ago
Attachment #8599749 - Flags: review?(shu) → review+
Group: javascript-core-security
Is this ready to land?  Be sure to ask for sec-approval as it may not actually be time to land yet due to where we are in the current cycle.
Flags: needinfo?(jdemooij)
B2G 2.2 code complete is june 8th. Would be great if we can get this in before that.
blocking-b2g: --- → 2.2?
Assignee

Comment 18

4 years ago
Comment on attachment 8599748 [details] [diff] [review]
Part 1 - Fix

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily, but maybe with some fuzzing help.

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

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

> How likely is this patch to cause regressions; how much testing does it need?
I don't expect any regressions but this is pretty complicated code; the more testing it gets the better.
Flags: needinfo?(jdemooij)
Attachment #8599748 - Flags: sec-approval?
Comment on attachment 8599748 [details] [diff] [review]
Part 1 - Fix

sec-approval=dveditz

Fine to check in the patch, would prefer if you hang on to the test for a bit before checking in.
Attachment #8599748 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/aca710f1a14a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee

Updated

4 years ago
Flags: needinfo?(jdemooij)
Looks like this may need uplift to aurora and beta as well as esr 38.
   
Jan, you mention this may be complicated/risky; what are you looking for to verify the fix?
Assignee

Comment 23

4 years ago
(In reply to Liz Henry (:lizzard) from comment #22)
> Jan, you mention this may be complicated/risky; what are you looking for to
> verify the fix?

A few Nightlies and fuzzing should be enough I think. IMHO we can and should backport this; the patch is not trivial but safe enough to uplift.
Assignee

Comment 24

4 years ago
Comment on attachment 8599748 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/regressing bug #]: Unknown, old bug.
[User impact if declined]: Crashes, security bugs.
[Describe test coverage new/current, TreeHerder]: Patch is in today's nightly and fixes the reported crash.
[Risks and why]: Some risk but I think this should be safe to uplift after a few nightlies and some days of fuzzing.
[String/UUID change made/needed]: None.
Attachment #8599748 - Flags: approval-mozilla-esr38?
Attachment #8599748 - Flags: approval-mozilla-esr31?
Attachment #8599748 - Flags: approval-mozilla-beta?
Attachment #8599748 - Flags: approval-mozilla-aurora?
Comment on attachment 8599748 [details] [diff] [review]
Part 1 - Fix

Taking it aurora to increase the coverage.
Once it is green, we will take it on the other releases.
Attachment #8599748 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter

Updated

4 years ago
Status: RESOLVED → VERIFIED
Reporter

Comment 27

4 years ago
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx40
Comment on attachment 8599748 [details] [diff] [review]
Part 1 - Fix

Approved for uplift to beta, esr31 and esr38.
Attachment #8599748 - Flags: approval-mozilla-esr38?
Attachment #8599748 - Flags: approval-mozilla-esr38+
Attachment #8599748 - Flags: approval-mozilla-esr31?
Attachment #8599748 - Flags: approval-mozilla-esr31+
Attachment #8599748 - Flags: approval-mozilla-beta?
Attachment #8599748 - Flags: approval-mozilla-beta+
This needs rebasing for beta uplift. Preferably today so it can make tomorrow's beta5 go to build.
Assignee

Comment 30

4 years ago
Landed rebased patch on beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/c5327254125d

(I also verified it fixes the assertion failure.)
Group: javascript-core-security
Ryan,

Is there some way we can easily get tracking set to the upcoming release when we check bugs like this into ESR branches? We mark it as "fixed" for them but you have to look at the checkin date to know which ESR release since tracking isn't set for the releases.
Flags: needinfo?(ryanvm)
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][adv-main39+][adv-esr31.8+][adv-esr38.1+]
I don't have the ability to set them myself, but I try to remember to set them to ? at least figuring they'll be triaged by someone who can.
Flags: needinfo?(ryanvm)
Reporter

Comment 34

4 years ago
JSBugMon: This bug has been automatically verified fixed on Fx39
(In reply to Christian Holler (:decoder) from comment #34)
> JSBugMon: This bug has been automatically verified fixed on Fx39

Christian, can we get this verified also on ESR 31.8.0 and 38.1.0? I've never used JSBugMon before, so I don't know how that can be done.
Flags: needinfo?(choller)
Reporter

Comment 36

4 years ago
JSBugMon doesn't support the ESR channel right now, so you'd best manually verify it since it's unlikely I can implement it in the next few days.
Flags: needinfo?(choller)
Cristian, do I need to build Firefox with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug in order to reproduce the initial issue?

I tried using Asan build from 2015-03-16 but was unable to reproduce the issue in order to verify the fix in ESR.
Flags: needinfo?(choller)
Reporter

Comment 38

4 years ago
This issue was found with the jsshell so you might have to build that, not Firefox.
Flags: needinfo?(choller)
This seems complicated to verify, and since it was already verified on Firefox 39, 40 and 41, we'll just skip verification on ESR to spend our time with other high priority tasks.
Verified on ESR 38:

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   260530:5f3e963eae50
user:        Jan de Mooij
date:        Wed Jun 10 18:01:09 2015 +0200
summary:     Bug 1143679 - Make TryNoteIterIon behave more like Baseline/interpreter iterators. r=shu, a=lizzard
Assignee

Comment 41

4 years ago
Added the test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/12722aeb2499
Flags: needinfo?(jdemooij)
Flags: in-testsuite? → in-testsuite+

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.