1936757 - Assertion failure: !done(), at js/src/vm/Scope.h:1708 or Crash [@ PopEnvironment]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20241212-a8a3495297c3 (debug build, run with --fuzzing-safe --ion-offthread-compile=off --enable-explicit-resource-management):

function a() {
    using b = {
        [Symbol.dispose]() {
            c;
        }
    }
    with(0) try {
        return;
    } catch {}
}
a()

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555556fd5b35 in PopEnvironment(JSContext*, js::EnvironmentIter&) ()
#1  0x0000555556fd57eb in js::UnwindEnvironment(JSContext*, js::EnvironmentIter&, unsigned char*) ()
#2  0x0000555557007272 in SettleOnTryNote(JSContext*, js::TryNote const*, js::EnvironmentIter&, js::InterpreterRegs&) ()
#3  0x0000555556fe076f in js::Interpret(JSContext*, js::RunState&) ()
[...]
#12 0x0000555556e3480b in main ()
rax	0x5555558e819e	93824995983774
rbx	0x7fffffffd350	140737488343888
rcx	0x5555588dc8a0	93825046268064
rdx	0x1	1
rsi	0x0	0
rdi	0x7ffff7bee7d0	140737349871568
rbp	0x7fffffffd060	140737488343136
rsp	0x7fffffffd040	140737488343104
r8	0x0	0
r9	0x3	3
r10	0x0	0
r11	0x0	0
r12	0x0	0
r13	0x7fffffffd078	140737488343160
r14	0x7ffff4636200	140737293541888
r15	0x30751d866060	53279564652640
rip	0x555556fd5b35 <PopEnvironment(JSContext*, js::EnvironmentIter&)+453>
=> 0x555556fd5b35 <_ZL14PopEnvironmentP9JSContextRN2js15EnvironmentIterE+453>:	movl   $0x6ac,0x0
   0x555556fd5b40 <_ZL14PopEnvironmentP9JSContextRN2js15EnvironmentIterE+464>:	callq  0x555556ed0370 <abort>

This also crashes in some kind without the assert, marking s-s until investigated.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20241212095112-dd0eb56380d4.
The bug appears to have been introduced in the following build range:

Start: c1acf137ed794e8b553c1f40512d21090d1a9b7c (20241114072145)
End: e299ddd844812c1cd97440fd74eb94e0736fbbe9 (20241114100954)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c1acf137ed794e8b553c1f40512d21090d1a9b7c&tochange=e299ddd844812c1cd97440fd74eb94e0736fbbe9

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1927195

:debadree333, since you are the author of the regressor, bug 1927195, could you take a look?

For more information, please visit BugBot documentation.

Comment 5

[Daniel Veditz [:dveditz]]

Guessing sec-high like some of the other regressions. This doesn't appear to have been investigated yet.

Comment 6

[Tooru Fujisawa [:arai]]

This is an issue around with + try + non-local-jump from return + the implicit throw inside dispose.

When performing the return in the following code, we first leave the with scope.
Given the try block doesn't have any variable, it doesn't have dedicate scope and there's no leave operation.

    with(0) try {
        return;
    } catch {}

https://searchfox.org/mozilla-central/rev/70fb2d2a702db4cb455d282be77ecfe9bdcea589/js/src/frontend/EmitterScope.cpp#1065-1066

case ScopeKind::With:
  if (!bce->emit1(JSOp::LeaveWith)) {

Then, after leaving the with scope, the enclosing scope becomes the function's lexical scope.
When leaving the function's lexical scope, we perform the dispose operation before actually leaving the function's lexical scope.

https://searchfox.org/mozilla-central/rev/70fb2d2a702db4cb455d282be77ecfe9bdcea589/js/src/frontend/EmitterScope.cpp#1037,1040-1042,1059-1060

    case ScopeKind::FunctionLexical:
...
#ifdef ENABLE_EXPLICIT_RESOURCE_MANAGEMENT
      if (nonLocal) {
        if (!emitDisposableScopeBodyEndForNonLocalJump(bce)) {
...
      if (!bce->emit1(hasEnvironment() ? JSOp::PopLexicalEnv
                                       : JSOp::DebugLeaveLexicalEnv)) {

and there's implicit throw

https://searchfox.org/mozilla-central/rev/70fb2d2a702db4cb455d282be77ecfe9bdcea589/js/src/frontend/UsingEmitter.cpp#66,76

bool UsingEmitter::emitThrowIfException() {
...
  if (!bce_->emit1(JSOp::Throw)) {

Here, while the scope is already pointing the function's lexical scope, the try note still points the syntactic try-catch inside with.
So, the throw is caught by that try-catch, and the unwind operation tries to unwind the scope to the try-catch's scope, which is inside with, but it fails to find the scope.

This always results in nullptr dereference, and this isn't security sensitive.

Then, we'll need to do either:

• (a) somehow mark the dispose operation outside of the try-catch, by modifying the TryNote
• (b) move the dispose operation bytecode outside of the try-catch, in the same way as try-finally

Comment 7

[Iain Ireland [:iain]]

Opening this up based on Arai's excellent analysis in comment 6.

Comment 8

[Debadree Chatterjee]

Hello everyone! sorry for the delayed response (was on a small vacation :-D ) it seems this test case uncovered a deeper issue with using try-catches and dispose operation we get wrong behavior even when we just have a bare try-catch for example the following script doesnt throw:

function a() {
  using b = {
      [Symbol.dispose]() {
        console.log("shoud throw");
          throw 1
      }
  }
  try {
      return;
  } catch {}
}
a()

I am trying the approach (b) as suggested by arai! lets see how far that goes

Comment 9

[Debadree Chatterjee]

Hello! so with the resolving of https://bugzilla.mozilla.org/show_bug.cgi?id=1943434 on central this test now passes successfully without crashing! hence marking this bug as FIXED !

Thank you!

Comment 10

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20250128090111-fb62e31afd93.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.