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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

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

Comment 2

[Andrew McCreight [:mccr8]]

Did the bisectfix get stuck?  How long should we expect these bisections will take?

Comment 3

[Christian Holler (:decoder)]

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?

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 5

[Andrew McCreight [:mccr8]]

Any ideas here?

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 38+ since this is sec-critical.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

It is a fuzzing bug, no known exploit using it, not tracking.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix

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.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Add assert, comment, test

We can land this after part 1 is on all branches.

Comment 10

[Jan de Mooij [:jandem]]

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

Comment 11

[Jan de Mooij [:jandem]]

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.

Comment 12

[Shu-yu Guo [: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.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix

This patch makes TryNoteIterIon use the actual number of stack slots it has instead of UINT32_MAX.

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Add jit-test

Comment 15

[Shu-yu Guo [: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.

Comment 16

[Andrew McCreight [:mccr8]]

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.

Comment 17

[Gregor Wagner [:gwagner]]

B2G 2.2 code complete is june 8th. Would be great if we can get this in before that.

Comment 18

[Jan de Mooij [:jandem]]

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.

Comment 19

[Daniel Veditz [:dveditz]]

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.

Comment 20

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aca710f1a14a

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aca710f1a14a

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 23

[Jan de Mooij [:jandem]]

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

Comment 24

[Jan de Mooij [:jandem]]

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.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/58f9c004fc05

Comment 27

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx40

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Approved for uplift to beta, esr31 and esr38.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing for beta uplift. Preferably today so it can make tomorrow's beta5 go to build.

Comment 30

[Jan de Mooij [:jandem]]

Landed rebased patch on beta:

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

(I also verified it fixes the assertion failure.)

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/5f3e963eae50
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/de95964cc6da
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/7d767fc15126
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/7d767fc15126
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/57d01f476529
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/57d01f476529
https://hg.mozilla.org/releases/mozilla-esr31/rev/33b5104416a2

Comment 32

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

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.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 34

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx39

Comment 35

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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

Comment 36

[Christian Holler (:decoder)]

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.

Comment 37

[Bogdan Maris, Desktop Test Engineering]

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.

Comment 38

[Christian Holler (:decoder)]

This issue was found with the jsshell so you might have to build that, not Firefox.

Comment 39

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.

Comment 40

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 41

[Jan de Mooij [:jandem]]

Added the test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/12722aeb2499

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/12722aeb2499