1998895 - Hit MOZ_CRASH(ElementAt(aIndex = 2, aLength = 2)) at mfbt/Assertions.cpp:78 or Assertion failure: *callee.wasmJitEntry() != provisionalLazyJitEntryStub, at wasm/WasmInstance.cpp:3340

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: debug stack

oomTest(function() {
  var x = new WebAssembly.Instance(
    new WebAssembly.Module(
      wasmTextToBinary(
        '(module (func $g (result f32) f32.const 1)(table (export "table") 1 funcref)(elem (i32.const 0) $g))',
      ),
    ),
  ).exports.table.get(0);
  try {
    x.apply();
  } catch (e) {}
  x.call();
});

(gdb) bt
#0  0x0000555557246969 in MOZ_CrashSequence (aAddress=0x0, aLine=78)
    at /home/msf2/shell-cache/js-dbg-64-linux-x86_64-7f33a0cc184f-595212/objdir-js/dist/include/mozilla/Assertions.h:237
#1  MOZ_Crash (aFilename=<optimized out>, aLine=78, aReason=0x555558f509b0 <sPrintfCrashReason> "ElementAt(aIndex = 2, aLength = 2)")
    at /home/msf2/shell-cache/js-dbg-64-linux-x86_64-7f33a0cc184f-595212/objdir-js/dist/include/mozilla/Assertions.h:375
#2  mozilla::detail::InvalidArrayIndex_CRASH (aIndex=<optimized out>, aLength=<optimized out>) at /home/msf2/trees/firefox/mfbt/Assertions.cpp:77
#3  0x0000555558767ad7 in js::wasm::Code::lookupLazyInterpEntry (this=this@entry=0x7ffff5231000, guard=..., funcIndex=funcIndex@entry=0)
    at /home/msf2/trees/firefox/js/src/wasm/WasmCode.cpp:847
#4  0x000055555876790c in js::wasm::Code::getOrCreateInterpEntry (this=0x7ffff5231000, funcIndex=0, funcExport=<optimized out>, interpEntry=0x7fffffffb798)
    at /home/msf2/trees/firefox/js/src/wasm/WasmCode.cpp:585
#5  0x00005555587c3e54 in GetInterpEntryAndEnsureStubs (cx=0x7ffff5e3c200, instance=..., funcIndex=0, args=..., interpEntry=0x7fffffffb798,
    funcType=<optimized out>) at /home/msf2/trees/firefox/js/src/wasm/WasmInstance.cpp:3347
/snip

This is probably from before mid-Oct 2025, but I'll keep bisecting.

Run with --fuzzing-safe --no-threads --no-baseline --no-ion, compile with AR=ar sh ~/trees/firefox/js/src/configure --enable-debug --enable-debug-symbols --with-ccache --enable-nspr-build --enable-ctypes --enable-gczeal --enable-rust-simd --disable-tests, tested on gh rev 7f33a0cc184f6372c66281499063bda0f42f9c83.

MOZ_CRASH but I'll still set s-s for now pending a second look.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

291a284a3f9e-538989
291a284a3f9e0ae04546df307c54724ae871344c is the first interesting commit
commit 291a284a3f9e0ae04546df307c54724ae871344c
Author: Jan de Mooij
Date:   Tue Oct 1 11:02:38 2024 +0000

    Bug 1921780 - Improve exception handling assertions in the JS shell. r=arai

    If a JSNative or script execution returns false, we now assert we have a pending exception
    or at some point an uncatchable exception was reported. Bug 1921215 added the `hadUncatchableException_`
    flag to `JSContext`.

    For now enable this only in the JS shell. If that's green we can try enabling it everywhere.

    Differential Revision: https://phabricator.services.mozilla.com/D224128

caused:

Assertion failure: cx->isExceptionPending() || cx->isPropagatingForcedReturn() || cx->hadUncatchableException(), at vm/Interpreter.cpp:440

which then changed to:

Assertion failure: aIndex < mLength, at dist/include/mozilla/Vector.h:585

by:

3a297e732494-548947
3a297e732494e46e863f6b433190183a26dc11dd is the first interesting commit
commit 3a297e732494e46e863f6b433190183a26dc11dd
Author: Jan de Mooij
Date:   Mon Dec 16 15:06:16 2024 +0000

    Bug 1936689 - Add missing ReportOutOfMemory calls to GetInterpEntryAndEnsureStubs. r=rhunt

    Differential Revision: https://phabricator.services.mozilla.com/D232071

and then bug 1445260 probably morphed it into this MOZ_CRASH.

Setting needinfo? from Ryan and Yury as a start, since it seems to involve wasm.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

CC'ing Jan due to possible relation to bug 1921780 and bug 1936689, and Andrew for bug 1445260.

Comment 3

[Jan de Mooij [:jandem]]

We didn't assert or crash before bug 1921780?

It sounds like there was a second issue that was caught by the assertions added in bug 1921780 and then fixed by bug 1936689, unrelated to what's happening here.

Comment 4

[Andrew McCreight [:mccr8]]

Bug 1445260 changed some MOZ_ASSERTs into release crashes, so there should be no net change in debug builds.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Jan de Mooij [:jandem] from comment #3)

We didn't assert or crash before bug 1921780?

Nope, we didn't seem to, in gh rev e12883160c1f.

$ ./js-dbg-64-linux-x86_64-e12883160c1f-538988 --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js
$

Random thought after thinking through possibilities: if we compile with the bug 1936689 patch in every bisection build and set the start revision as 1efb7ceeae33-532305 (far back enough that the bug 1936689 patch applies and compiles properly) and end revision as 3a297e732494-548947 (when the bug 1936689 patch actually landed), we might be able to find the real regressor.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

With bug 1936689's patch, either gh rev 5dba88818b12af079e9ba4c8bc0353922c084109 (the parent, has a compile error) or gh rev 04e79950cb42ff6f3652bed1f2b5e428cdc382ae (the descendant) seems to cause the following:

Assertion failure: *callee.wasmJitEntry() != provisionalLazyJitEntryStub, at wasm/WasmInstance.cpp:3025

And they both are from bug 1910194.

With bug 1936689's patch, somewhere between 291a284a3f9e-538989 and 3a297e732494-548947 caused the assertion to mutate to:

Assertion failure: aIndex < mLength, at dist/include/mozilla/Vector.h:585

but I may want to do this another time.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

With bug 1936689's patch, somewhere between 291a284a3f9e-538989 and 3a297e732494-548947 caused the assertion to mutate to:

Assertion failure: aIndex < mLength, at dist/include/mozilla/Vector.h:585

Found it:

$ ~/shell-cache/js-dbg-64-linux-x86_64-75076165612e-542972/js-dbg-64-linux-x86_64-75076165612e-542972 --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js
Assertion failure: *callee.wasmJitEntry() != provisionalLazyJitEntryStub, at wasm/WasmInstance.cpp:3132

$ ~/shell-cache/js-dbg-64-linux-x86_64-d01fe4dc3711-542973/js-dbg-64-linux-x86_64-d01fe4dc3711-542973 --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js
Assertion failure: aIndex < mLength, at dist/include/mozilla/Vector.h:585

(note that these tested revisions need bug 1936689's patch added prior to compilation)

gh rev d01fe4dc3711 or bug 1913119 caused the assertion failure to mutate.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Thus, either bug 1910194 or bug 1913119 may be the correct regressor.

Comment 9

[Donal Meehan [:dmeehan]]

Updating the affected versions based on the bugs mentioned in Comment 8

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I have had testcases from recent revisions (e.g. as of gh rev 1edc0ba8328a) that show:

Assertion failure: *callee.wasmJitEntry() != provisionalLazyJitEntryStub, at wasm/WasmInstance.cpp:3340

but that reduce to this MOZ_CRASH.

Julien, any updates on this issue?

Comment 11

[Julien Pages [:jpages]]

The regression was introduced in https://bugzilla.mozilla.org/show_bug.cgi?id=1910194, but this is a huge serie of patches, changing the whole compilation pipeline. I can't reproduce the issue in debug, without oomTest().

We are catching the issue but it should be fixed. I believe that the problem is a bad/misplaced error handling in an out of memory context.

The oom handling is there: https://searchfox.org/firefox-main/rev/c0447496b9e1f44ddc7836b87b8b312de49daeca/js/src/wasm/WasmInstance.cpp#3318

The error is in this function: https://searchfox.org/firefox-main/rev/c0447496b9e1f44ddc7836b87b8b312de49daeca/js/src/wasm/WasmCode.cpp#416
The problem is: we crash before getting to ReportOutOfMemory().

I'm working on a patch.

Comment 12

[Ryan Hunt [:rhunt]]

Can you describe exactly the sequence of things that go wrong?

It's hard to tell if this is guaranteed to crash or not. It sounds like it is, but there's not enough details in your comment to conclude that.

Comment 13

[Daniel Veditz [:dveditz]]

This appears to be hitting a Release assert so it's not clear the current symptoms are a vulnerability. Please clear the keyword for retriage if there's better evidence

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Daniel Veditz [:dveditz] from comment #13)

This appears to be hitting a Release assert so it's not clear the current symptoms are a vulnerability. Please clear the keyword for retriage if there's better evidence

I'm not sure though, as I mentioned in comment 10, I have had unreduced hard-to-reproduce testcases from recent revisions (e.g. as of gh rev 1edc0ba8328a) that show Assertion failure: *callee.wasmJitEntry() != provisionalLazyJitEntryStub, at wasm/WasmInstance.cpp:3340 (which then reduce to the MOZ_CRASH) meaning it is not only a Release assert.

We'd need to wait for confirmation from Julien.

Comment 15

[Julien Pages [:jpages]]

The issue is an out of memory happening at the wrong place. The associated testcase is a oomTest().

It starts in GetInterpEntryAndEnsureStubs

This function then calls js::wasm::Code::getOrCreateInterpEntry

At the end, we are out of memory inside js::wasm::Code::createManyLazyEntryStubs (in addCodeBlock).

The issue is, we are left in a weird state with half-created data structures in our lazy stubs mechanism. We then try to access an array at a specific offset, but we crash on a release assert because we are out of memory (ElementAt(aIndex = 2, aLength = 2)")). Nothing was added to this vector and we try to access it anyways.

I only have a release assert with this testcase but I can get other crash signatures (like the one you mentionned Gary) if I change the error-handling code in these functions. The core issue remains the same: we still need to detect sooner that we are out of memory.

If you change the testcase and the out of memory happens at a different place, I think that may be possible to get other crash signatures. I didn't try to change the testcase though because I can always reproduce the crash with this one.

The solution seems to be: try to reserve all the memory we need before entering this sequence of calls getOrCreateInterpEntry -> createOneLazyEntryStub -> createManyLazyEntryStubs -> addCodeBlock.
It's a bit tricky because we depend on the offsets, or the length of vectors at various places. Reserving memory before entering these calls is not trivial because of that, it messes with the rest of the code.

Comment 16

[Julien Pages [:jpages]]

Attachment: Bug 1998895 - wasm: Avoid an out of memory situation when creating lazy stubs.

Comment 17

[BugBot [:suhaib / :marco]]

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

Comment 18

[Ryan Hunt [:rhunt]]

It looks like exact sequence of events are:

1. Create a function and store it in a table, don't export it directly. This means it'll have a lazy entry stub created on-demand.
2. Get the function from JS and call it
3. This calls GetInterpEntryAndEnsureStubs
4. Which calls Code::getOrCreateInterpEntry
5. Which calls Code::createOneLazyEntryStub
6. The tail of this function mutates the members of Code, but an OOM in the middle of it returns false and state is not rolled back [1].
   • An entry to guard->lazyFuncExports is added that points at guard->blocks.length()
   • Actually appending the code block to guard->blocks fails.
   • This is the only vulnerable combination I can think of.
7. Goto step 2 again and retry.
8. Code::getOrCreateInterpEntry
9. Code::lookupLazyInterpEntry [2]
10. We see the code block added in (step 6), but it points at guard->blocks.length(), which is OOBs because we failed to append to the vector.
11. This crashes due to the release asserts in Vector that we have now.

If we didn't have a release assert here, it looks like the OOB read of the vector [2] would be used to construct a code pointer that we'd jump into here [3]. But I don't see a way to trigger that an avoid the release assert we have now.

So all together, I don't believe there is a way to trigger this OOM into something exploitable.

(In reply to Daniel Veditz [:dveditz] from comment #13)

This appears to be hitting a Release assert so it's not clear the current symptoms are a vulnerability. Please clear the keyword for retriage if there's better evidence

I'm not sure though, as I mentioned in comment 10, I have had unreduced hard-to-reproduce testcases from recent revisions (e.g. as of gh rev 1edc0ba8328a) that show Assertion failure: *callee.wasmJitEntry() != provisionalLazyJitEntryStub, at wasm/WasmInstance.cpp:3340 (which then reduce to the MOZ_CRASH) meaning it is not only a Release assert.

We'd need to wait for confirmation from Julien.

The Assertion failure: *callee.wasmJitEntry() != provisionalLazyJitEntryStub, at wasm/WasmInstance.cpp:3340 is a debug-only consistency check that is not security critical. It basically means that the wasm function is still pointing at the provisionalLazyJitEntryStub which is safe. We intend to have created the JIT entry which is faster, but the fact it hasn't is okay.

[1] https://searchfox.org/firefox-main/rev/438a3ce10eb77fb50d968463b7741117aec5bb4a/js/src/wasm/WasmCode.cpp#486-524
[2] https://searchfox.org/firefox-main/rev/438a3ce10eb77fb50d968463b7741117aec5bb4a/js/src/wasm/WasmCode.cpp#846
[3] https://searchfox.org/firefox-main/rev/438a3ce10eb77fb50d968463b7741117aec5bb4a/js/src/wasm/WasmInstance.cpp#3932

Comment 19

[Julien Pages [:jpages]]

I agree with Ryan's comment. When I was working on this patch I would see this assertion Assertion failure: *callee.wasmJitEntry() != provisionalLazyJitEntryStub, at wasm/WasmInstance.cpp:3340 and the one in your bug mozilla::detail::InvalidArrayIndex_CRAS, depending on where you're catching the out of memory situation.

Comment 20

[Ryan Hunt [:rhunt]]

De-restricting this due to the security analysis in comment 18.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/74a5eb2166f0
https://hg.mozilla.org/integration/autoland/rev/a30bf3792dd3
wasm: Avoid an out of memory situation when creating lazy stubs. r=rhunt

Comment 22

[Sandor Molnar[:smolnar]]

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

Comment 23

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:jpages, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox147 to wontfix.

For more information, please visit BugBot documentation.

Comment 24

[Julien Pages [:jpages]]

As mentioned in comment #18 and #19, we don't think this issue is exploitable: we are catching the problem with multiple release asserts. I don't think it's necessary to uplift this to beta.

I'll update the flags.