1762770 - Crash [@ js::NativeObject::setFixedSlot] or Assertion failure: frame.isGeneratorFrame(), at vm/GeneratorObject.cpp:227

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20220402-3a5752d614ce (opt build, run with --fuzzing-safe --ion-offthread-compile=off --ion-warmup-threshold=0):

function* a() {
  try {
    yield b;
  } finally {
    for (c = 0; c < 100; c++);
  }
}
(b = a()).next().value.return()

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555555c98949 in js::NativeObject::setFixedSlot(unsigned int, JS::Value const&) ()
#1  0x0000555556084ab8 in js::AbstractGeneratorObject::setClosed() ()
#2  0x0000555555c9fef7 in Interpret(JSContext*, js::RunState&) ()
[...]
#8  0x0000555556021e49 in main ()
rax	0x8dccb589e741b00	638608828655737600
rbx	0x0	0
rcx	0x8dccb589e741b00	638608828655737600
rdx	0x7fffffffc338	140737488339768
rsi	0x0	0
rdi	0x0	0
rbp	0x7fffffffc320	140737488339744
rsp	0x7fffffffc290	140737488339600
r8	0x1	1
r9	0x7ffff602bb80	140737320762240
r10	0x91389d62150	9979521540432
r11	0x7ffff4ef9f24	140737302732580
r12	0x0	0
r13	0xe8	232
r14	0x0	0
r15	0x7fffffffc338	140737488339768
rip	0x555555c98949 <js::NativeObject::setFixedSlot(unsigned int, JS::Value const&)+41>
=> 0x555555c98949 <_ZN2js12NativeObject12setFixedSlotEjRKN2JS5ValueE+41>:	mov    0x18(%rdi,%rbx,8),%rdi
   0x555555c9894e <_ZN2js12NativeObject12setFixedSlotEjRKN2JS5ValueE+46>:	mov    %rdi,%rax

Marking s-s until triaged since this is JIT related and the crash signature is also potentially dangerous (looks like a null deref but crashes around NativeObject slots are potentially more dangerous than that).

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20220403215202-2d8724cbbddd.
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 4

[Iain Ireland [:iain]]

I believe the problem here is that the exception handling code for Ion frames is missing the equivalent of this code in HandleExceptionBaseline for closing generators when generator.return() is called. We end up leaking the GeneratorClosing magic value to the caller. I think the caller is always the self-hosted generator resume function, but I'm not 100% sure yet.

Comment 5

[Iain Ireland [:iain]]

Looked into this in a bit more detail. Here's a testcase with some of the extraneous details removed:

function* a() {
  try {
    yield 1;
  } finally {
    for (c = 0; c < 100; c++);
  }
}

with ({}) {}
var b = a();
b.next();
b.return(0)

We call next on a generator function to get to a yield point inside a try block, then call return on it. This is implemented by throwing the JS_GENERATOR_CLOSING magic value. We have to execute the finally block, so we save the magic value on the stack and resume execution. Because try-finally is now supported in Warp, we can OSR inside the finally block.

When we're done with the finally, we see that we're in the middle of throwing an exception, and rethrow the magic value. HandleExceptionIon doesn't have any special handling for JS_GENERATOR_CLOSING, so we unwind to the next frame, which is the self-hosted GeneratorReturn function. In HandleExceptionBaseline, the code that handles closing generators asserts because GeneratorReturn is not a generator function.

In non-debug builds, it looks like we will end up returning nullptr here because GeneratorReturn doesn't have/need an initial environment, and then segfaulting safely when we try to call setClosed on a nullptr. So I tentatively conclude that this isn't a security problem, although I could be missing something.

The fix is to handle this case in HandleExceptionIon.

Comment 6

[Daniel Veditz [:dveditz]]

If you're confident it's not a security bug you should have the ability to unset the flag, and you're welcome to do so. If you're still a bit unsure and you want to leave it hidden a bit longer that's fine, too.

Comment 7

[Iain Ireland [:iain]]

Attachment: Bug 1762770: Bail out to handle JS_GENERATOR_CLOSING in Ion r=jandem

My initial attempt to fix this bug involved adding support for closing generators in HandleExceptionIon. However, we don't currently have machinery in place to force a return from an Ion frame. In baseline, we reuse the machinery for debugger-induced forced returns. The simplest solution is to treat this case in the same way.

Comment 8

[Daniel Veditz [:dveditz]]

If you discover that this might be exploitable after all please clear the sec-audit keyword so we can re-triage this

Comment 9

[Iain Ireland [:iain]]

Attachment: Bug 1762770: Use an enum class for exception resume kind r=jandem

Comment 10

[Iain Ireland [:iain]]

Attachment: Bug 1762770: Add offsetOf methods to ResumeFromException r=jandem

Depends on D143670

Comment 11

[Iain Ireland [:iain]]

Attachment: Bug 1762770: Add ExceptionResumeKind::ForcedReturnIon r=jandem

We already had a testcase for forced return of a non-inlined frame; I modified it to verify the return value and added a second testcase for inlined frames.

Depends on D143671

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Use an enum class for exception resume kind r=jandem
https://hg.mozilla.org/integration/autoland/rev/52bbdbc9b7842f95e9770409ab6ccbfa856827dd
https://hg.mozilla.org/mozilla-central/rev/52bbdbc9b784

Add offsetOf methods to ResumeFromException r=jandem
https://hg.mozilla.org/integration/autoland/rev/d3933c9bce1ec7940d031f99754a657f6c6549a5
https://hg.mozilla.org/mozilla-central/rev/d3933c9bce1e

Add ExceptionResumeKind::ForcedReturnIon r=jandem
https://hg.mozilla.org/integration/autoland/rev/130d070c77d888001e84b6a7c272f869dd4b68e9
https://hg.mozilla.org/mozilla-central/rev/130d070c77d8

Comment 13

[Bugmon [:jkratzer for issues]]

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