1767181 - Crash [@ js::PrimitiveToObject] or Assertion failure: !val.isMagic(), at vm/JSObject.cpp:2625

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20220430-17796248a0e0 (opt build, run with --fuzzing-safe --ion-offthread-compile=off --ion-warmup-threshold=0 --baseline-eager):

function* a() {
  try {
    try {
      yield;
    } finally {
      for (b = 0; b < 20; b++);
    }
  } catch ([]) {}
}
c = a();
c.next();
c.return();

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555555d51c02 in js::PrimitiveToObject(JSContext*, JS::Value const&) ()
#0  0x0000555555d51c02 in js::PrimitiveToObject(JSContext*, JS::Value const&) ()
#1  0x00005555560f8be3 in js::ToObjectSlowForPropertyAccess(JSContext*, JS::Handle<JS::Value>, int, JS::Handle<JS::Value>) ()
#2  0x0000555555ed08bd in js::jit::DoGetElemFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) ()
#3  0x000026eeed551508 in ?? ()
[...]
#23 0x0000000000000000 in ?? ()
rax	0x5555564dcec3	93825008520899
rbx	0xfffa800000000002	-1548112371908606
rcx	0x55555765b068	93825026863208
rdx	0x5555564ee58c	93825008592268
rsi	0x7fffffffbd80	140737488338304
rdi	0x7ffff601e100	140737320706304
rbp	0x7fffffffbac0	140737488337600
rsp	0x7fffffffba20	140737488337440
r8	0x7fffffffbd88	140737488338312
r9	0x7fffffffbd80	140737488338304
r10	0x7fffffffbb70	140737488337776
r11	0xfff9800000000000	-1829587348619264
r12	0xffff800000000000	-140737488355328
r13	0x7ffff601e100	140737320706304
r14	0x1	1
r15	0x7ffff601e100	140737320706304
rip	0x555555d51c02 <js::PrimitiveToObject(JSContext*, JS::Value const&)+898>
=> 0x555555d51c02 <_ZN2js17PrimitiveToObjectEP9JSContextRKN2JS5ValueE+898>:	movl   $0x9e4,0x0
   0x555555d51c0d <_ZN2js17PrimitiveToObjectEP9JSContextRKN2JS5ValueE+909>:	callq  0x5555556f3660 <abort>

Marking s-s because this is a JIT-related failure.

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 20220501190841-4326a9c7fd28.
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]]

The problem here is that HandleExceptionIon doesn't have an equivalent to this code from baseline exception handling, which skips catch blocks if we are propagating the closing of a generator. We got away with it before adding Warp support for finally, because there was (AFAICT) no way to create an Ion frame that was throwing JS_GENERATOR_CLOSING. Now, if a yield is wrapped in a try-finally, we can resume in the finally block, use OSR to get back into Warp, and then re-throw JS_GENERATOR_CLOSING in an Ion frame.

This bug leaks the generator-closing magic value to script. I'm not sure how exploitable this is: it can't be confused with any other magic value, because of this release assert, but it's possible that there's something else you can do with it.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1767181: Skip catch blocks when closing generators in Ion r=jandem

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1767181: Add testcase r=jandem

Depends on D145280

Comment 7

[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 8

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

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

Comment 9

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

Skip catch blocks when closing generators in Ion r=jandem
https://hg.mozilla.org/integration/autoland/rev/f49179d738d04f316b2c70d2700853a081241aa9
https://hg.mozilla.org/mozilla-central/rev/f49179d738d0

Comment 10

[Bugmon [:jkratzer for issues]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 12

[Iain Ireland [:iain]]

Comment on attachment 9274734 [details]
Bug 1767181: Skip catch blocks when closing generators in Ion r=jandem

Beta/Release Uplift Approval Request

• User impact if declined: Leak of a magic value to script. We have mitigations in place that block the obvious ways to abuse this, so most of the time we should just crash with a near-null dereference, but it's possible we've missed something.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The bug is well-understood, and the fix makes one codepath (which was made reachable by the regressing bug) more similar to another well-tested codepath.
• String changes made/needed:
• Is Android affected?: Yes

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9274734 [details]
Bug 1767181: Skip catch blocks when closing generators in Ion r=jandem

Approved for 101.0b5.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c760cf2f245d

Comment 15

[Alex Finder|PTO, back on Nov 25th]

Hello! We encountered the following regression for talos on mozilla-beta. Would it be possible for this push to have triggered that regression ?

Comment 16

[Iain Ireland [:iain]]

Almost certainly not. Any code that hit this path would have crashed before the patch, so if talos wasn't crashing, this patch should have no effect.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61cfd500721f
Add testcase r=jandem

Comment 18

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/61cfd500721f