1871951 - Crash [@ ??] or Assertion failure: highestByteVisitedInPrevFrame + 1 == scanStart, at wasm/WasmInstance.cpp:2746

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20231225-fdd85a789550 (debug build, run with --fuzzing-safe --ion-offthread-compile=off --wasm-compiler=baseline):

gczeal(18)
function a(str, imports) {
    b = wasmTextToBinary(str)
    try {
        c = new WebAssembly.Module(b)
    } catch {}
    return new WebAssembly.Instance(c, imports)
}
gczeal(10)
base = a(`(module
    (global (export "rngState")
      (mut i32) i32.const 1
    )
    (type $d (array (mut i32)))  (type $e (array (mut (ref null $d)))) (func $f 
        (param $arr (ref $d))
        (result (ref $d))
        local.get $arr
    )
    (func $createSecondaryArray 
                                (result (ref $d))
      (return_call $f
        (array.new $d
          i32.const 0(i32.const 1))
      )
    )
    (func $g (export "createPrimaryArrayLoop")
        (param  i32) (param $arrarr (ref $e)) 
        (result (ref $e))
            (local.get $arrarr
                                       call $createSecondaryArray)
            (return_call $g
                (i32.const 1)
                local.get $arrarr)
    )
)`)
t58 = `(module
    (type $d      (array (mut i32)))  (type $e (array (mut (ref null $d)))) (import "" "rngState" (global $h (mut i32)))
    (import "" "createPrimaryArrayLoop" 
      (func $g
        (param i32(ref $e)) 
        (result (ref $e))))
    (func $createPrimaryArray (result (ref $e))
      (return_call $g
        i32.const 0
        (array.new $e ref.null $d i32.const 500))
    )
   (func (export "churn")  (result i32)
     (local $arrarr (ref $e))
     (local.set $arrarr call $createPrimaryArray)
     (global.get $h)
   )
)`
j = a(t58, {"": base.exports})
fns = j.exports;
fns.churn()

Backtrace:

received signal SIGILL, Illegal instruction.
#0  0x317d8023 in ?? ()
#1  0x317d8157 in ?? ()
#2  0x318081c5 in ?? ()
#3  0x3180825f in ?? ()
#4  0x31808363 in ?? ()
#5  0x593726af in js::wasm::Instance::callExport(JSContext*, unsigned int, JS::CallArgs, js::wasm::CoercionLevel) ()
#6  0x59389706 in WasmCall(JSContext*, unsigned int, JS::Value*) ()
#7  0x57f989a4 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
[...]
#20 0x57dc6f17 in main ()
eax	0xffffbf48	-16568
ebx	0x0	0
ecx	0x0	0
edx	0xffffbf88	-16504
esi	0xf33fe040	-213917632
edi	0xf3288800	-215447552
ebp	0xffffbf58	4294950744
esp	0xffffbf58	4294950744
eip	0x317d8023	830308387
=> 0x317d8023:	ud2    
   0x317d8025:	sub    $0x10,%esp

The test here only reproduces on 32-bit but I have seen the assert on x64 as well. Marking s-s until triaged.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Julian Seward [:jseward]]

Could this possibly be related to recent in-line allocation stuff? Maybe unlikely .. I thought what has landed was for structs only, and this seems to do with arrays. But asking anyways ..

Comment 4

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1871951 using build mozilla-central 20231225214719-fdd85a789550. 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 5

[Ryan Hunt [:rhunt]]

Ben's gonna look at this sometime this week. Marking it with best guess of priority/severity for now.

Comment 6

[Yury Delendik (:yury)]

Findings and notes:

• The function $g (in the test) is an infinite loop that calls $createSecondaryArray, which calls $d via a return call. On timeout GC happens.
• For return calls, the right alignment of call arguments is important (that's probably a reason we observing issue on x86-32)
• The assert at https://searchfox.org/mozilla-central/source/js/src/wasm/WasmInstance.cpp#2745 triggered, because highestByteVisitedInPrevFrame + 1 == scanStart is not satisfied (I observe values 0xffef9e5b and 0xffef9e58 -- some overlap in frames)

My guess, we are breaking some assumptions described at https://searchfox.org/mozilla-central/source/js/src/wasm/WasmBCFrame.cpp#222 . The return calls require different assumption about arguments alignment padding, e.g. https://searchfox.org/mozilla-central/source/js/src/wasm/WasmStubs.cpp#738. The alignment padding size (between args and locals) may change during return call operations.

Comment 7

[Ryan Hunt [:rhunt]]

Okay, so:

1. Baseline is generating stack maps that cover the padding bytes to get outgoing stack args aligned
2. The outgoing stack arg size and padding can change by a function tail calling to another function that takes more/less stack args

So the stack map generated in (1) for a caller (a) is correct unless the callee function (b) tail calls to a different function (c) that needs more/less stack args, then the stack map for (a) may overlap with (c) on the stack arg padding area.

We should switch to a model where the padding region is owned by the outgoing/callee function, not the caller function.

Is this security sensitive? I don't think so, because at worst we will have a stack map for (a) that claims some padding bytes that are also claimed by a stack map (c)'s arguments. Our tracing logic will visit both, and so the arguments will still get traced.

However to be conservative, let's leave hidden until we have a confirmed fix.

Comment 8

[Ryan Hunt [:rhunt]]

Fixing this will probably involve:

1. Removing this part from the calc (https://searchfox.org/mozilla-central/rev/b1a029fadaaabb333d8139f9ec3924a20c0c941f/js/src/wasm/WasmBaselineCompile.cpp#1402-1403) which will remove outgoing stack arg padding from the baseline stack maps
2. Ensuring function entry trap stack maps include padding for stack args
   - https://searchfox.org/mozilla-central/rev/b1a029fadaaabb333d8139f9ec3924a20c0c941f/js/src/jit/CodeGenerator.cpp#14111
   - https://searchfox.org/mozilla-central/rev/b1a029fadaaabb333d8139f9ec3924a20c0c941f/js/src/wasm/WasmBaselineCompile.cpp#382-384
3. Ensuring function body stack maps include padding for stack args
   - Ion looks like it just needs to tweak nInboundStackArgs in CodeGenerator::generateWasm
   - Baseline looks like it just needs beginFunction inboundStackArgBytes modified, as in the function entry trap stack maps case

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Ryan Hunt [:rhunt] from comment #7)

Is this security sensitive? I don't think so, because at worst we will have a stack map for (a) that claims some padding bytes that are also claimed by a stack map (c)'s arguments. Our tracing logic will visit both, and so the arguments will still get traced.

However to be conservative, let's leave hidden until we have a confirmed fix.

I'll mark this sec-other then.

Comment 10

[Ben Visness [:bvisness]]

Attachment: Bug 1871951: Assign stack arg padding to callee's stackmap. r=jseward

Comment 11

[Ben Visness [:bvisness]]

Attachment: Bug 1871951: Add regression test for stackmap issue. r=jseward

Depends on D198856

Comment 12

[Julian Seward [:jseward]]

(In reply to Ryan Hunt [:rhunt] from comment #7)

Is this security sensitive? I don't think so, [..]

+1 for declassification.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb055a280040
Assign stack arg padding to callee's stackmap. r=jseward
https://hg.mozilla.org/integration/autoland/rev/2cac2f68bfdc
Add regression test for stackmap issue. r=jseward

Comment 14

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/fb055a280040
https://hg.mozilla.org/mozilla-central/rev/2cac2f68bfdc

Comment 15

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

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

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox123 to wontfix.

For more information, please visit BugBot documentation.

Comment 16

[Ben Visness [:bvisness]]

No uplift since this is just a debug assert and tracing still works fine.