1768660 - Crash [@ js::jit::DoToBoolFallback] or Assertion failure: v.isObject(), at builtin/Boolean.cpp:172

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20220510-58a6343ab33d (opt build, run with --fuzzing-safe --ion-offthread-compile=off --baseline-eager --more-compartments --ion-warmup-threshold=0):

a = newGlobal();
a.parent = this;
a.eval("Debugger(parent).onExceptionUnwind=function(){}");
b = function() {
  try {} finally {
    try {
      throw 9;
    } catch {}
  }
}();

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555555e96761 in js::jit::DoToBoolFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) ()
#1  0x00001403a06aacb4 in ?? ()
[...]
#17 0x0000000000000000 in ?? ()
rax	0x4800000000009	1266637395197961
rbx	0xfff9000000000000	-1970324836974592
rcx	0xfffe000000000000	-562949953421312
rdx	0xfffa800000000000	-1548112371908608
rsi	0xfffa000000000000	-1688849860263936
rdi	0x7fffffffbc28	140737488337960
rbp	0x7fffffffbda0	140737488338336
rsp	0x7fffffffbbb0	140737488337840
r8	0x7fffffffbbe8	140737488337896
r9	0x0	0
r10	0x7fffffffbde0	140737488338400
r11	0xfff9800000000000	-1829587348619264
r12	0x7fffffffbdf0	140737488338416
r13	0x7fffffffbd18	140737488338200
r14	0x7fffffffbc28	140737488337960
r15	0x7ffff601e100	140737320706304
rip	0x555555e96761 <js::jit::DoToBoolFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>)+929>
=> 0x555555e96761 <_ZN2js3jit16DoToBoolFallbackEP9JSContextPNS0_13BaselineFrameEPNS0_14ICFallbackStubEN2JS6HandleINS7_5ValueEEENS7_13MutableHandleIS9_EE+929>:	mov    (%rax),%rcx
   0x555555e96764 <_ZN2js3jit16DoToBoolFallbackEP9JSContextPNS0_13BaselineFrameEPNS0_14ICFallbackStubEN2JS6HandleINS7_5ValueEEENS7_13MutableHandleIS9_EE+932>:	mov    (%rcx),%rcx

This is likely restricted to the debugger and sec-moderate, but marking s-s because it looks like a type confusion in JIT.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Nicolas B. Pierron [:nbp]]

Ian, you might try to patch it but the fuzzers will always catch bugs.
Finally the day will come where no changes would be necessary.

In the mean time, I think this bug is waiting for you ;)

Comment 4

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20220510095538-58a6343ab33d.
The bug appears to have been introduced in the following build range:

Start: 60080026a143c4f4515557c1704b0b3775607001 (20220330234750)
End: 3358bc3f9c0891fd1e14c7b9903085be32818646 (20220330235837)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=60080026a143c4f4515557c1704b0b3775607001&tochange=3358bc3f9c0891fd1e14c7b9903085be32818646

Comment 5

[Iain Ireland [:iain]]

This is a problem with our debug-mode bailouts.

When bailing out for debug mode, we skip expression stack slots unless they are for iterators we have to close. We do so to avoid a problem where we bail in the middle of a call and can't recover the return value because it hasn't been pushed yet. In this case, we have a try-catch inside a finally block. Because we're inside a finally block, there are two values on the stack telling us what to do when we reach the end of the block. (If this isn't a no-rval script, the cached rval is also on the expression stack, although it's not directly related to this bug.) We replace those slots with JS_OPTIMIZED_OUT, even though the snapshot contains all the information we need to recover the value.

I think the fix is to be more precise about when we skip expression stack slots.

This isn't security sensitive, given that a) it requires the debugger, and b) the leaking magic values aren't directly exposed to script; they're only consumed by the machinery at the end of the finally block, which should always crash in the same way.

Comment 6

[Andrew McCreight [:mccr8]]

Something requiring the debugger does not make it not a security issue, but it does mitigate it a bit. I'll unhide the bug due to (b) though.

Comment 7

[Iain Ireland [:iain]]

Attachment: Bug 1768660: Skip fewer values in buildExpressionStack r=jandem

For the innermost frame of debugger mode bailouts, the current approach effectively uses an allow-list to decide which stack slots need to be recovered. This is fragile to any future bytecode changes that keep values alive on the expression stack. It's also unnecessary: if we just recover slots that are included in the snapshot, and skip slots that don't have allocations, everything works out.

Comment 8

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

:iain, since this bug contains a bisection range, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 9

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

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d86140d2fd38
Skip fewer values in buildExpressionStack r=jandem

Comment 11

[Atila Butkovits]

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

Comment 12

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20220518031437-1e98fd258975.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.