1535626 - Crash [@ js::jit::MachineState::read] with generator function and Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 4d15e90af575 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --more-compartments --ion-eager):

function* wrapNoThrow() {
    let iter = {
      [Symbol.iterator]() {
        return this;
      },
      next() {
        return { value: 10, iter: false };
      },
      return() { throw "nonsense"; }
    };
    for (const i of iter)
      yield i;
  }
function foo() {
    try {
      l2:
        for (j of wrapNoThrow())
          for (i of [1,2,3])
            break l2;
    } catch (e) {}
}
foo();
var g = newGlobal();
g.parent = this;
g.eval("new Debugger(parent).onExceptionUnwind = function () {};");
foo();

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00005555562ea200 in js::jit::MachineState::read (reg=..., this=<optimized out>) at js/src/jit/Registers.h:261
#0  0x00005555562ea200 in js::jit::MachineState::read (reg=..., this=<optimized out>) at js/src/jit/Registers.h:261
#1  js::jit::SnapshotIterator::fromRegister (this=0x7fffffffad90, reg=...) at js/src/jit/JSJitFrameIter.h:413
#2  js::jit::SnapshotIterator::allocationValue (this=this@entry=0x7fffffffad90, alloc=..., rm=rm@entry=js::jit::SnapshotIterator::RM_Normal) at js/src/jit/JitFrames.cpp:1650
#3  0x00005555563d19c9 in js::jit::SnapshotIterator::read (this=this@entry=0x7fffffffad90) at js/src/jit/JSJitFrameIter.h:523
#4  0x000055555665a6c8 in InitFromBailout (excInfo=0x7fffffffb350, nextCallee=..., startFrameFormals=..., builder=..., invalidate=true, iter=..., script=..., fun=..., frameNo=0, cx=<optimized out>) at js/src/jit/BaselineBailouts.cpp:1026
#5  js::jit::BailoutIonToBaseline (cx=<optimized out>, cx@entry=0x7ffff5f17000, activation=<optimized out>, iter=..., invalidate=invalidate@entry=true, bailoutInfo=bailoutInfo@entry=0x7fffffffaea8, excInfo=excInfo@entry=0x7fffffffb350) at js/src/jit/BaselineBailouts.cpp:1794
#6  0x000055555665e673 in js::jit::ExceptionHandlerBailout (cx=0x7ffff5f17000, frame=..., rfe=rfe@entry=0x7fffffffb8a0, excInfo=...) at js/src/jit/Bailouts.cpp:214
#7  0x00005555562f8e41 in js::jit::HandleExceptionIon (hitBailoutException=<synthetic pointer>, rfe=0x7fffffffb8a0, frame=..., cx=<optimized out>) at js/src/jit/JitFrames.cpp:188
#8  js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:634
#9  0x00003a59f2e05556 in ?? ()
#10 0x0000000000000000 in ?? ()
rax	0x101	257
rbx	0x7fffffffad90	140737488334224
rcx	0x555556c79420	93825016501280
rdx	0x7fffffffaf00	140737488334592
rsi	0x7fffffffa904	140737488333060
rdi	0x6	6
rbp	0x7fffffffa8f0	140737488333040
rsp	0x7fffffffa8b0	140737488332976
r8	0x7ffff5faa648	140737320232520
r9	0xfff9800000000000	-1829587348619264
r10	0x7ffff59abc00	140737313946624
r11	0x7ffff59abee0	140737313947360
r12	0x5	5
r13	0x7ffff5f17000	140737319628800
r14	0x7fffffffad90	140737488334224
r15	0x7fffffffabf0	140737488333808
rip	0x5555562ea200 <js::jit::SnapshotIterator::allocationValue(js::jit::RValueAllocation const&, js::jit::SnapshotIterator::ReadMethod)+688>
=> 0x5555562ea200 <js::jit::SnapshotIterator::allocationValue(js::jit::RValueAllocation const&, js::jit::SnapshotIterator::ReadMethod)+688>:	mov    (%rax),%rax
   0x5555562ea203 <js::jit::SnapshotIterator::allocationValue(js::jit::RValueAllocation const&, js::jit::SnapshotIterator::ReadMethod)+691>:	jmpq   0x5555562e9fc8 <js::jit::SnapshotIterator::allocationValue(js::jit::RValueAllocation const&, js::jit::SnapshotIterator::ReadMethod)+120>

Comment 1

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 2

[Neha Kochar [:neha]]

This is a JS fuzzing bug and sdetar said the team will fix as many as they can in 67 but will not be able to get to all. Marking them fix-optional.

Comment 3

[Nicolas B. Pierron [:nbp]]

Matthew are you interested in fixing this issue?
If you need help you can needinfo me, or forward it.

Comment 4

[Iain Ireland [:iain]]

This testcase looks familiar. I'm pretty sure I wrote it (modulo the bits that the fuzzer changed).

Let me take a look at this bug. It will probably take me less time to get back up to speed on iterators.

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Ok, sounds good :)

Comment 6

[Iain Ireland [:iain]]

I think I understand what's going on here, although I am starting to wish I didn't.

This bug is a variant of bug 1480390. It has to do with bad iterators throwing as we break out of nested loops.

Oversimplifying the testcase, we have:

var bad_iter = IteratorThatWillThrowAnErrorOnReturn;
loop: for (var i of bad_iter) {
   for (var j of [1,2,3]) {
      break loop;
   }
}

When we hit |break|, we leave the outer loop. We have to clean up the iterators on our way out, so we emit code to do so at the point of the |break| itself. However, if an exception is thrown during cleanup, a try-catch inside the loop shouldn't catch it. To make this work, we use for-of-iterclose try notes. Consider the following code:

for (var i of bad_iter) {
  try {
    break;
  } catch {}
} 

With some irrelevant cruft removed, the trynotes look like this:

|-------------------------| for-of
 |----------------------| catch
        |--------|  for-of-iterclose

The for-of iterclose covers the bytecode range that is responsible for closing the iterator. In TryNoteIter, when we see this for-of-iterclose, we "skip ahead" until we see the corresponding for-of, ignoring any other trynotes that might match. As a result, the catch block inside the loop doesn't see the error that occurs as we leave.

The equivalent over-simplified ASCII diagram for the failing testcase looks like this:

 |----------------------| for-of (outer)
   |-----------------|  for-of (inner)
     |-----| |-----|
        ^       ^
        |       |
        |       for-of-iterclose for outer
        for-of-iterclose for inner

Note that the for-of-iterclose trynotes do not overlap. If we throw inside the for-of-iterclose for the outer iterator, TryNoteIter has no way of knowing that it should skip two for-of loops, not just one. As a result, according to the trynotes, the exception is being thrown inside the outer for-of.

In this case, the problem happens when we bail out. The bailout code determines from trynotes that it should have values on the stack corresponding to the outer for-of. However, those values aren't present in the recovery instructions, and we crash. A similar problem occurs in this testcase:

function* wrapNoThrow() {
    let iter = {
      [Symbol.iterator]() {
        return this;
      },
      next() {
        return { value: 10, iter: false };
      },
      return() { throw "nonsense"; }
    };
    for (const i of iter)
      yield i;
  }

function foo() {
    try {
	var badIter = wrapNoThrow();
	loop: for (var i of badIter) {
	    try {
		for (i of [1,2,3]) {
		    break loop;
		}
	    } catch (e) { print("Bad catch: " + e); }
	}
    } catch (e) { }
}

foo();

With --baseline-eager or --ion-eager, this testcase prints "Bad catch: nonsense". This is incorrect.

I think this will require changes in bytecode generation to fix.

Comment 7

[Iain Ireland [:iain]]

Attachment: Bug 1535626: Extend range of FOR_OF_ITERCLOSE trynotes until scope exit r=arai

When breaking out of nested for-of loops, we emit code to close each of the iterators, and we cover each of those code segments with an individual FOR_OF_ITERCLOSE trynote. When TryNoteIter walks through the trynotes, it sees a single FOR_OF_ITERCLOSE trynote at the current pc, and skips past one enclosing for-of. This is insufficient for the case where an outer iterator throws an exception while being closed.

This patch fixes that problem. The new semantics for FOR_OF_ITERCLOSE trynotes are: a FOR_OF_ITERCLOSE trynote begins as soon as we clear ther iterator slot on the stack, and continues until we leave the scope. It indicates that the innermost unclosed for-of iterator has been closed. Nested FOR_OF_ITERCLOSE trynotes indicate that nested for-of iterators have been closed.

This is my first patch touching the frontend, so it's probably horribly unidiomatic. I am open to suggestions for better ways to structure this.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68ca7447ef62
Extend range of FOR_OF_ITERCLOSE trynotes until scope exit r=arai

Comment 9

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/68ca7447ef62