2003463 - Crash [@ JSObject::getClass() const]

Status: RESOLVED DUPLICATE of bug 2044831

(Core :: JavaScript Engine, defect, P3)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20251201-b50fe26f9711 (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --disable-oom-functions --baseline-eager --inlining-entry-threshold=10 --spectre-mitigations=off --wasm-compiler=baseline --ion-warmup-threshold=10 --blinterp-warmup-threshold=1 --baseline-warmup-threshold=0 --ion-warmup-threshold=0 --ion-check-range-analysis --trial-inlining-warmup-threshold=10 --ion-gvn=off):

See attachment.

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555556f87a64 in JSObject::getClass() const ()
#1  0x00005555573534b6 in js::LookupPropertyPure(JSContext*, JSObject*, JS::PropertyKey, js::NativeObject**, js::PropertyResult*) ()
#2  0x0000555557efe0de in CanAttachNativeGetProp(JSContext*, JSObject*, JS::PropertyKey, js::NativeObject**, mozilla::Maybe<js::PropertyInfoBase<unsigned int> >*, unsigned char*) ()
#3  0x0000555557f58081 in js::jit::CloseIterIRGenerator::tryAttachNoReturnMethod() ()
#4  0x0000555557f5898c in js::jit::CloseIterIRGenerator::tryAttachStub() ()
#5  0x0000555557c0b96b in js::jit::DoCloseIterFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, JS::Handle<JSObject*>) ()
#6  0x000030897cf22f8b in ?? ()
[...]
#21 0x0000000000000000 in ?? ()
rax	0xc17bf99951a84700	-4504732565358754048
rbx	0x7fffed2fbf48	140737172717384
rcx	0x7fffed2fbf48	140737172717384
rdx	0xab9c742bbc0	11793028266944
rsi	0x7000000000000	1970324836974592
rdi	0x7000000000000	1970324836974592
rbp	0x7fffed2fbed0	140737172717264
rsp	0x7fffed2fbed0	140737172717264
r8	0x7fffed2fbf50	140737172717392
r9	0x7fffe3a59997	140737012668823
r10	0xfff8800000000000	-2111062325329920
r11	0x70	112
r12	0x7fffee23de00	140737188716032
r13	0xab9c742bbc0	11793028266944
r14	0x7000000000000	1970324836974592
r15	0x7fffed2fbf50	140737172717392
rip	0x555556f87a64 <JSObject::getClass() const+4>
=> 0x555556f87a64 <_ZNK8JSObject8getClassEv+4>:	mov    (%rdi),%rax
   0x555556f87a67 <_ZNK8JSObject8getClassEv+7>:	test   $0x7,%al

I've been working on this test for quite a while to get it smaller and more reliable but the result attached here still runs for about 30 seconds and is slightly intermittent. I'm going to try and reduce it further but I think we should start looking into this now because it looks security-sensitive.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 2003463 using build mozilla-central 20251201102759-b50fe26f9711. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

I'll bet there's something funky with explicit resource management...

Iain, could you take a first pass?

Comment 5

[Iain Ireland [:iain]]

I've been poking at this in rr. Somewhere in the mess of bytecode we generate for for (using d4 of iterator) {...}, while constantly tiering up into Ion and bailing out, we're getting our expression stack mixed up. It's somewhat finicky to figure out exactly where it's going wrong. Will continue poking.

Comment 6

[Iain Ireland [:iain]]

The reason this takes so long and is hard to reduce is that it depends on allocating a ton of jitcode, running out of executable memory, and then throwing an exception while trying to tier up to Ion at a loop header.

The key code here is this:

function testDisposalThrowsAreThrown() {
  for (using d4 of iterator) {
    if (d4.val == 2000) break;
  }
}

We generate a synthetic try/catch block covering the body of the for-of loop to ensure that we close the iterator on the way out if we throw an exception. We also generate a big chunk of code for the break to close the iterator and call the dispose method on d4. Some parts of that break code (closing the iterator, calling the dispose method) invoke user code, and are wrapped in their own try-catch blocks. However, there are also parts of the dispose machinery that are not covered by any other try block. In particular, we generate a loop to iterate through the list of objects that need to be disposed. If we try to do OSR at the top of that loop, but we fail, then we will be caught by the outer try-catch. However, since we've already cleaned things up, the stack at that point does not match the stack that we expect when we enter the catch block.

We have ForOfIterClose trynotes in an attempt to fix this when closing iterators, and it does look like that trynote is correctly positioned here. However, we don't have an equivalent for the dispose code.

I think this will probably end up not being exploitable, but there's a lot of finicky moving pieces. We can keep it hidden for now.

I will attach an annotated copy of the relevant bytecode.

Comment 7

[Iain Ireland [:iain]]

Attachment: for-using-of.log

We generate a lot of code for this, so I've annotated the key points.

Comment 8

[Iain Ireland [:iain]]

It's not immediately obvious to me what the best fix is here. I think the fix probably has to be in the frontend. I'm cc'ing Debadree and Arai, who wrote/reviewed this code.

Right now, we track disposables dynamically in a list in the environment, and then loop over that list when exiting a scope. The silly switch loophole was fixed, so now it should be possible to statically schedule each disposal, desugaring each using into a synthetic try-finally. I already thought that would be a good change because it should improve performance. It's worth noting that it would also avoid this particular bug, because we would no longer be generating loops that can OSR+throw while breaking out of a for-of.

(In bug 1766730 I simplified try/finally by delaying the generation of non-local exit code until the finally block. In that case, the goal was to avoid jumping backwards after executing the finally to finish the non-local exit. I wonder if we could simplify this case, and the ForOfIterClose mess in general, by similarly moving the break logic outside of the for-of itself.)

Comment 9

[Debadree Chatterjee]

Hello thank you for cc'ing I have been working on the static disposal thing on the side but have not been able to complete it, trying to get back to it again

Comment 10

[Daniel Veditz [:dveditz]]

I think this will probably end up not being exploitable, but there's a lot of finicky moving pieces. We can keep it hidden for now.

so sec-audit for now. Please clear that for re-triage or set it to something more definite if you figure out whether this is or isn't exploitable

Comment 11

[Jan de Mooij [:jandem]]

I think this is a duplicate of bug 2044831. We also implemented the non-local exit cleanup Iain mentioned in comment 8, in bug 2066996.