1943707 - Assertion failure: exn.isObject() && exn.toObject().is<ErrorObject>(), at js/src/wasm/WasmInstance.cpp:4067 or Crash [@ MarkPendingExceptionAsTrap]

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20250124-50b5bccbceda (build with debug, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --more-compartments).

Backtrace:

    Program terminated with signal SIGSEGV, Segmentation fault.
    #0  0x000056481b248564 in js::wasm::MarkPendingExceptionAsTrap(JSContext*) ()
    #1  0x000056481a18669f in js::WasmArrayObject* js::WasmArrayObject::createArray<true>(JSContext*, js::wasm::TypeDefInstanceData*, js::gc::Heap, unsigned int) ()
    #2  0x00001df3edb67a73 in ?? ()
    #3  0x00007fc7aa0a5090 in ?? ()
    #4  0x00001df3edbb6154 in ?? ()
    #5  0x00007fc7aa0a5090 in ?? ()
    #6  0x000056481ac774d4 in CanEnterBaselineJIT(JSContext*, JS::Handle<JSScript*>, js::AbstractFramePtr) ()
    Backtrace stopped: previous frame inner to this frame (corrupt stack?)
    rax	0x5648186ed9ff	94867647551999
    rbx	0x7fc7aa0a4fb0	140495528021936
    rcx	0x56481b988e90	94867700616848
    rdx	0x1	1
    rsi	0x0	0
    rdi	0x7fc7accc27d0	140495574280144
    rbp	0x7fc7aa0a4fe0	140495528021984
    rsp	0x7fc7aa0a4fa0	140495528021920
    r8	0x0	0
    r9	0x3	3
    r10	0x0	0
    r11	0x0	0
    r12	0x0	0
    r13	0x4	4
    r14	0x7fc7aa0a4fa0	140495528021920
    r15	0x7fc7a870e078	140495501189240
    rip	0x56481b248564 <js::wasm::MarkPendingExceptionAsTrap(JSContext*)+420>
    => 0x56481b248564 <_ZN2js4wasm26MarkPendingExceptionAsTrapEP9JSContext+420>:	movl   $0xfe3,0x0
       0x56481b24856f <_ZN2js4wasm26MarkPendingExceptionAsTrapEP9JSContext+431>:	callq  0x564819e45ad0 <abort>

Assertion indicates potential type confusion, marking s-s until cleared.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Daniel Veditz [:dveditz]]

This assertion is a release assert which should be safe -- if you get there. The alternative symptom shows we don't always get there before bad stuff happens.

Comment 4

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:jpages, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 5

[Julien Pages [:jpages]]

I think we can see the problem even with a simplified version of this test, without the loop:

try { 
  function wasmEvalBinary(binary, imports, compileOptions) {
    try {
        m = new WebAssembly.Module(binary, compileOptions);
    } catch(e) {}
    return new WebAssembly.Instance(m, imports);
  }
  function wasmEvalText(str, imports, compileOptions) {
    return wasmEvalBinary(wasmTextToBinary(str), imports, compileOptions);
  }
  let { createDefault } = wasmEvalText(`
    (module (type $a (array (mut i32)))
      (func (export "createDefault") (param i32) (result eqref)
        local.get 0
        array.new_default $a
      )
    )
  `).exports;
  
  try { createDefault(-1) } catch(exc) {}
} catch(exc) {}

Current understanding:

• The parameter for array_new default is invalid, which causes a trap (wasm::MarkPendingExceptionAsTrap) to be returned and this is later captured by the catch
• We should probably catch and handle this invalid parameter before for array.new_default (-1 in this case)
• I didn't find any recent patch causing this issue

Pernosco session with the simple example: https://pernos.co/debug/std0LndbE4UdwX9R4WQenQ/index.html

Comment 6

[Jan de Mooij [:jandem]]

@jpages: the code in the caller (WasmArrayObject::createArray) looks like this:

    js::ReportOversizedAllocation(cx, JSMSG_WASM_ARRAY_IMP_LIMIT);
    wasm::MarkPendingExceptionAsTrap(cx);

So we first throw an exception (set a pending exception on the JSContext), but then right after that in MarkPendingExceptionAsTrap we fail the assertion because we don't have an error object after all. Maybe you can check what kind of value exn is instead in MarkPendingExceptionAsTrap. One possibility is that it's the "out of memory" string if ReportOversizedAllocation failed to allocate the error object due to OOM.

Comment 7

[Julien Pages [:jpages]]

Attachment: Bug 1943707 - Modifying wasm::MarkPendingExceptionAsTrap(). r=rhunt?

Comment 8

[Julien Pages [:jpages]]

Ah, you might be right Jan, thanks!

I have a patch that seems to fix the assert on my machine. I'm not 100% sure it's the best fix though.

Comment 9

[Ryan Hunt [:rhunt]]

This assertion is a release assert which should be safe -- if you get there. The alternative symptom shows we don't always get there before bad stuff happens.

I think that the release assert should save us here? It should always happen before we try to access the object using the wrong type. What do you mean by the alternative symptom?

Comment 10

[Ryan Hunt [:rhunt]]

From talking with jpages, there is only one location where we dereference a pending exception (which is any JS value) to set the 'isWasmTrap' flag and it is this function [1]. The function always has a release assert to type check it before setting it. So this should always be a safe crash before we add this patch.

What alternative symptom were you referring to in your earlier comment? Just want to double check before I unflag this bug.

[1] https://searchfox.org/mozilla-central/rev/46c5f46a83146b311828b9cdd64b975a08066278/js/src/wasm/WasmInstance.cpp#4066-4067

Comment 11

[Daniel Veditz [:dveditz]]

What alternative symptom were you referring to in your earlier comment? Just want to double check before I unflag this bug.

I interpreted Decoder's summary "assertion ... or crash" as being two different outcomes, for instance if we satisfied the assertion but then crashed on the next line anyway due to some other corruption. If the "crash" is the normal effect of the release-assert wouldn't it be described as "assertion... AND crash" or some such? The initial description is pretty sparse so I default to assuming the worst case. Maybe he meant "Assertion (if you're in a debugger) ... or crash (if you're not)" and they are the same?

Comment 12

[Christian Holler (:decoder)]

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

What alternative symptom were you referring to in your earlier comment? Just want to double check before I unflag this bug.

I interpreted Decoder's summary "assertion ... or crash" as being two different outcomes

We always file by the data we see in fuzzing CI. For non-debug builds, that shows up as a crash and it is by signature usually not recognizable if that is a release assert or some other issue because non-debug builds don't emit the assertion message. If the assertion is a release assert, then it's very likely that this is the reason for the crash.

Comment 13

[Ryan Hunt [:rhunt]]

Ah, that all makes sense. Thank you! Yes, the test case here should always safely crash due to the release assert.

Comment 14

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20250207045558-f53d61ffefac.
The bug appears to have been introduced in the following build range:

Start: 0498598458d8c4322c8471ce6654e61063792ee5 (20250116180959)
End: 1ca180f4a04fe6c2522eb19e3e65fd5769cd777d (20250116194941)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0498598458d8c4322c8471ce6654e61063792ee5&tochange=1ca180f4a04fe6c2522eb19e3e65fd5769cd777d

Comment 15

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

Based on comment #14, this bug contains a bisection range found by bugmon. However, the Regressed by field is still not filled.

:jpages, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit BugBot documentation.

Comment 16

[Donal Meehan [:dmeehan]]

:jpages next week is the final week of beta for Fx136.
I see there is a patch attached and reviewed.
Checking if it just needs to ride the trains whenever it lands? i.e. there's nothing a user will hit and it won't need an uplift?

Comment 17

[Julien Pages [:jpages]]

Yes, I think this patch just needs to ride the train and it won't need any uplift.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/920c7384d7a9
Modifying wasm::MarkPendingExceptionAsTrap(). r=rhunt?

Comment 19

[amarc]

https://hg.mozilla.org/mozilla-central/rev/920c7384d7a9

Comment 20

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20250212093207-11a45cb6835c.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.