1878261 - Crash [@ JS::Compartment::wrap] with Debugger and interrupt callback

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20240202-854c462a083b (debug build, run with --fuzzing-safe --ion-offthread-compile=off --baseline-eager test.js):

gczeal(7, 1)
function a(b) {
    c = newGlobal({
        newCompartment: true
    })
    d = new Debugger
    setInterruptCallback(function() {
        d.addDebuggee(c)
        d.getNewestFrame().onStep = function() {
            return b
        }
        return true
    })
    try {
        c.eval("(" + function() {
            invokeInterruptCallback(function() {})
        } + ")()")
    } finally {}
}
a({
    throw: "thrown 42"
})

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555b7e2033db in JS::Compartment::wrap(JSContext*, JS::MutableHandle<JSString*>) ()
#1  0x0000555b7deb61e4 in JS::Compartment::wrap(JSContext*, JS::MutableHandle<JS::Value>) ()
#2  0x0000555b7e2c98ac in JSContext::getPendingException(JS::MutableHandle<JS::Value>) ()
#3  0x0000555b7e2644ed in JS::GetPendingExceptionStack(JSContext*, JS::ExceptionStack*) ()
#4  0x0000555b7e2643df in JS::StealPendingExceptionStack(JSContext*, JS::ExceptionStack*) ()
#5  0x0000555b7de89cd1 in js::shell::AutoReportException::~AutoReportException() ()
#6  0x0000555b7de92c01 in Shell(JSContext*, js::cli::OptionParser*) ()
#7  0x0000555b7de8c459 in main ()
rax	0x6c6a4000001	7450224754689
rbx	0x7f5f9eb44000	140048661233664
rcx	0xfffe2f2f2f2f2f2c	-511070251831508
rdx	0x7fff0a6206e0	140733367584480
rsi	0x7f5f9fd2e100	140048680018176
rdi	0x7f5f9fd03240	140048679842368
rbp	0x7fff0a6206b0	140733367584432
rsp	0x7fff0a620670	140733367584368
r8	0x0	0
r9	0x7fff0a61e301	140733367575297
r10	0x7fff0a7e5080	140733369438336
r11	0x202	514
r12	0x7f5f9fd03240	140048679842368
r13	0x6c6a4000508	7450224755976
r14	0x7fff0a6206e0	140733367584480
r15	0x7f5f9fd2e100	140048680018176
rip	0x555b7e2033db <JS::Compartment::wrap(JSContext*, JS::MutableHandle<JSString*>)+107>
=> 0x555b7e2033db <_ZN2JS11Compartment4wrapEP9JSContextNS_13MutableHandleIP8JSStringEE+107>:	cmp    %rbx,(%rcx)
   0x555b7e2033de <_ZN2JS11Compartment4wrapEP9JSContextNS_13MutableHandleIP8JSStringEE+110>:	jne    0x555b7e20340f <_ZN2JS11Compartment4wrapEP9JSContextNS_13MutableHandleIP8JSStringEE+159>

This involves the debugger so I assume it is sec-moderate at most, however with the poison pattern in the crash, we should confirm that first.

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 20240202094312-dd4c0135beb5.
The bug appears to have been introduced in the following build range:

Start: 82dfbdd770bc54674f82bae256dae683772884af (20240122155520)
End: 75c3c3ed6fe2c33aa435e3a099c5f18be4b4d8d2 (20240122183000)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=82dfbdd770bc54674f82bae256dae683772884af&tochange=75c3c3ed6fe2c33aa435e3a099c5f18be4b4d8d2

Comment 4

[Iain Ireland [:iain]]

I believe the problem is in this code. We store the exception, which is a nursery string, in rfe->exception. Then we store the exception stack. It's from another compartment, so we have to wrap it. Allocating the wrapper triggers a minor GC, which promotes the exception string. However, despite our use of MutableHandleValue::fromMarkedLocation to make a handle out of it, rfe isn't actually traced. The exception value stored in rfe now points to freed nursery space.

I don't immediately see a reason that this couldn't be done without the debugger. It only requires a cross-compartment exception stack, and a very precisely timed GC. I'm going to conservatively bump this up to sec-high.

This is a regression from bug 1843499, although the first dubious use of fromMarkedLocation here appears to date back all the way to bug 951282.

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1878261: Prefer local variables over fromMarkedLocation. r=iain!

Comment 6

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

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

Comment 7

[André Bargull [:anba]]

(In reply to Iain Ireland [:iain] from comment #4)

I believe the problem is in this code.

Yes, that sounds plausible. Explicitly rooting the variables fixes the crash.

Comment 8

[Iain Ireland [:iain]]

Here's a testcase that triggers the bug without needing the debugger:

// |jit-test| --baseline-eager

var g = newGlobal({newCompartment: true});

function foo() {
  try {
    g.eval("gczeal(7,1); throw 'a thrown string'");
  } finally {
    gczeal(0);
  }
}

try {
  foo();
} catch (e) { assertEq(e, 'a thrown string')}

We can land it separately.

Comment 9

[Iain Ireland [:iain]]

Attachment: Bug 1878261: Add testcase r=anba

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df0b9539f560
Prefer local variables over fromMarkedLocation. r=iain

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Bugmon [:jkratzer for issues]]

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