1888964 - Assertion failure: !done(), at vm/Scope.h:1674

Status: ASSIGNED

(Core :: JavaScript Engine, defect, P1)

Description

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

Attachment: debug stack

interruptIf([]);
stackTest(function () {
  let x = oomAtAllocation(1);
  try {
    return;
  } catch (e) {}
});

(gdb) bt
#0  js::ScopeIter::scope (this=<optimized out>) at /home/ubumain/trees/mozilla-central/js/src/vm/Scope.h:1674
#1  js::WrappedPtrOperations<js::ScopeIter, JS::Rooted<js::ScopeIter>, void>::scope (this=<optimized out>) at /home/ubumain/trees/mozilla-central/js/src/vm/Scope.h:1747
#2  js::EnvironmentIter::scope (this=<optimized out>) at /home/ubumain/trees/mozilla-central/js/src/vm/EnvironmentObject.h:1074
#3  PopEnvironment (cx=cx@entry=0x7ffff6639100, ei=...) at /home/ubumain/trees/mozilla-central/js/src/vm/Interpreter.cpp:1034
#4  0x000055555727732b in js::UnwindEnvironment (cx=cx@entry=0x7ffff6639100, ei=..., pc=<optimized out>) at /home/ubumain/trees/mozilla-central/js/src/vm/Interpreter.cpp:1114
#5  0x0000555557298f02 in SettleOnTryNote (cx=cx@entry=0x7ffff6639100, tn=tn@entry=0x7ffff66546e8, ei=..., regs=...) at /home/ubumain/trees/mozilla-central/js/src/vm/Interpreter.cpp:1154
#6  0x000055555728fa09 in ProcessTryNotes (cx=0x7ffff6639100, ei=..., regs=...) at /home/ubumain/trees/mozilla-central/js/src/vm/Interpreter.cpp:1218
#7  HandleError (cx=0x7ffff6639100, regs=...) at /home/ubumain/trees/mozilla-central/js/src/vm/Interpreter.cpp:1312
#8  js::Interpret (cx=0x7ffff6639100, state=...) at /home/ubumain/trees/mozilla-central/js/src/vm/Interpreter.cpp:4357
#9  0x0000555557272909 in MaybeEnterInterpreterTrampoline (cx=0x7ffff7a1ca60 <_IO_stdfile_2_lock>, cx@entry=0x7ffff6639100, state=...) at /home/ubumain/trees/mozilla-central/js/src/vm/Interpreter.cpp:393
/snip

Run with --fuzzing-safe --no-threads --no-baseline --no-ion, compile with AR=ar sh ../configure --enable-debug --enable-debug-symbols --with-ccache --enable-nspr-build --enable-ctypes --enable-gczeal --enable-rust-simd --disable-tests, tested on m-c rev 26157b52a8c3.

This seems to go back at least 3 months prior, I'll keep trying to bisect.

Comment 1

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

The testcase does not reproduce with the latest debug js shell from FTP (2015-10-21) as stackTest was not available then but reproduces with m-c rev a5887514ddfb (Feb 2022).

I'm going to take a guess - since this has interruptIf and stackTest, I'll set a needinfo? for Jan to take a look as a start.

Comment 2

[Jan de Mooij [:jandem]]

This is an old problem with the C++ interpreter. It does an interrupt check at a JSOp::RetRval op. This interrupt check then terminates execution and when this happens we report a warning. Reporting the warning OOMs, so the interrupt check reports an exception.

The exception handler then gets confused while unwinding environment objects, I think because the scope note for the RetRval op is for the top-level scope and the try-catch scope (and the actual environment chain) has an additional lexical scope for the let x.

The simplest fix may be to align the interrupt checks with the Baseline interpreter and JITs, and not check for interrupts at RetRval ops. The same issue may also affect debugger breakpoints though...

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)

The same issue may also affect debugger breakpoints though...

It does, the debugger test below fails the same assertion and this one also affects the Baseline Interpreter and JITs.

arai, can you take a look at this? I hope we can fix this by changing the scope notes emitted for the RetRval op.

var g = newGlobal({newCompartment: true});
g.evaluate(`
function f() {
  let x = 1;
  try {
    return;
  } catch (e) {}
}
`)
var dbg = new Debugger(g);
dbg.onEnterFrame = function(frame) {
  frame.onStep = function() {
    if (frame.offset === 16) { // RetRval
      return {throw: 1};
    }
  };
};
g.f();

Comment 4

[Tooru Fujisawa [:arai]]

what's the expected behavior for "throw on JSOp::RetRval" ?
should it be caught by catch, or should it be outside of try-catch ?
it's after JSOp::DebugLeaveLexicalEnv (or JSOp::PopLexicalEnv if let x is closed over), so making it outside of try-catch would be simpler.

Comment 5

[Tooru Fujisawa [:arai]]

actually, this is more complex.
the following also hits the issue, but it's not leaving the function but just leaving the loop.

var g = newGlobal({newCompartment: true});
g.evaluate(`
function f() {
  let y = 1;
  foo: while (true) {
    let x = 1;
    function g() { return x + y }
    try {
      break foo;
    } catch (e) {}
  }
  return 10;
}
dis(f);
`);
var dbg = new Debugger(g);
dbg.onEnterFrame = function(frame) {
  frame.onStep = function() {
    if (frame.offset === 68) { // Goto for break foo
      return {throw: 1};
    }
  };
};
g.f();

anyway, try-catch code block would need to be split into multiple parts in term of scope note, having the outer scope in between them, for each non-local jump's target depth.

Comment 6

[Daniel Veditz [:dveditz]]

Could this lead to a security issue or is it just a correctness issue?

Comment 7

[Tooru Fujisawa [:arai]]

on non-debug build, this results in nullptr dereference for EnvironmentIter::si_.ptr.scope_, and just crashes,
given it expects more nested scopes than the actual scopes, and it tries to access the sentinel value (EnvironmentIter::done() == true), which is nullptr.

So, this won't be security issue but just DoS.

Comment 8

[Tooru Fujisawa [:arai]]

Given the try note cannot be disjoint, we could apply the similar technique as TryNoteKind::ForOf+TryNoteKind::ForOfIterClose, to mark the non-local-exit range inside TryNoteKind::Catch or TryNoteKind::Finally, in order to make the range virtually outside of the try-catch and let the error handling not settling to the try.
Then, I guess we should look into for-of+"throw during non-local-exit" as well.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Bug 1888964 - Exclude non-local-exit range from Catch try note. r?jandem!

Comment 10

[Iain Ireland [:iain]]

When dealing with a vaguely similar problem involving try-finally, I had some success moving code that was semantically outside the block to be physically outside the block as well by delaying its generation until later. We speculated a bit at the time that it might help us clean up for-of-iterclose, but I never put any effort into trying it out.

I wonder if a similar approach could help here. Instead of generating extra trynotes to carve out gaps in an existing trynote's scope, can we move the code that would be in that gap to the end of the scope and jump to it? I think it probably make bytecode generation a little trickier, but then unwinding needs fewer special cases. We might also be able do so some sort of deduplication to share more code if there are multiple non-local exits from the same scope?

Comment 11

[Tooru Fujisawa [:arai]]

Yeah, reordering the bytecode can simplify the situation, but the issue here affects JSOp::Goto as well, given it's step-able, and the onStep handler can throw there.
So some kind of technique to exclude the jump itself is necessary, unless we apply another specialization to prevent onStep handler there (perhaps this is more easier?).
Otherwise it's still possible to throw at the point of "TryNote says it's inside, but the stack says it's outside", especially when there are 2 different non-local-exit and one of the JSop::Goto cannot go outside of the try note (even if we put them at the end of the block).

Deduplicating the non-local-exits sounds interesting.

Comment 12

[Tooru Fujisawa [:arai]]

is bug 1890610 meta bug, or some other bug related to try note?
if latter, can anyone CC me there?

Comment 13

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

(In reply to Tooru Fujisawa [:arai] from comment #12)

can anyone CC me there?

Done!

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #11)

Yeah, reordering the bytecode can simplify the situation, but the issue here affects JSOp::Goto as well, given it's step-able, and the onStep handler can throw there.

We currently exit the outer scope before the RetRval, so the RetRval op has a scope note that's different from the op before it. This causes the bug because the scope notes don't match the try note stack. If we reordered the bytecode, wouldn't the JSOp::Goto have the same scope note and try note as the code before it?

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #14)

We currently exit the outer scope before the RetRval, so the RetRval op has a scope note that's different from the op before it. This causes the bug because the scope notes don't match the try note stack. If we reordered the bytecode, wouldn't the JSOp::Goto have the same scope note and try note as the code before it?

Or maybe this just moves the problem to the target of the Goto?

I think the underlying problem with the debugger is that we allow breakpoints/stepping at (almost) any bytecode op. If we restricted this to just bytecode ops that have the breakpoint/stepping source note, the debugger couldn't stop at this RetRval op and get the engine into this state. That's bug 926528.

Comment 16

[Tooru Fujisawa [:arai]]

Yes, the opcode that leaves the try-note's range is the problem, either RetRval for return or Goto for continue/break.
So, indeed, reducing the target of onStep should ultimately solve the issue with debugger.

The testcase in comment #0 needs different consideration, but if JSOp::Goto is not affected there (that I suppose true), moving the non-local-exit outside of try-note should solve this.

Comment 17

[Jan de Mooij [:jandem]]

The test in comment 0 we could fix by not doing an interrupt check at RetRval. I want to make that change also to match the JITs.

Bug 926528 would then fix the debugger issue.

arai, does bug 926528 make sense to you? I'm tempted to just try it to fix these bugs with the debugger stopping at internal JSOps.

Comment 18

[Tooru Fujisawa [:arai]]

yes, that makes sense.
I'll withdraw the patch, and look into the simplification for the non-local-exit in separate bug.

Comment 19

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

The testcase in comment 0 no longer seems to reproduce on a recent m-c rev 0191fbfc9115:

The first good revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/de8b96102f94
user:        Jan de Mooij
date:        Tue Oct 01 11:02:38 2024 +0000
summary:     Bug 1921780 - Improve exception handling assertions in the JS shell. r=arai

Jan, is bug 1921780 a likely fix?

Comment 20

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

The testcase in comment 0 no longer seems to reproduce on a recent m-c rev 0191fbfc9115:

It seems to show a Script terminated by interrupt handler. message instead.

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) from comment #19)

The testcase in comment 0 no longer seems to reproduce on a recent m-c rev 0191fbfc9115:
...
Jan, is bug 1921780 a likely fix?

Yes:

(In reply to Jan de Mooij [:jandem] from comment #2)

It does an interrupt check at a JSOp::RetRval op. This interrupt check then terminates execution and when this happens we report a warning. Reporting the warning OOMs, so the interrupt check reports an exception.

We now call cx->reportUncatchableException() after reporting the warning and this clears any pending exception. This makes the interrupt checks a little more predictable which is a nice side-effect I hadn't realized yet.

The (debugger) test in comment 3 still reproduces though.