1917807 - Assertion failure: jitCaller->footer()->type() == expected, at js/src/wasm/WasmFrameIter.cpp:190

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20240906-9df162c80958 (build with debug, run with --fuzzing-safe --cpu-count=2 --wasm-compiler=optimized --no-native-regexp --ion-check-range-analysis --baseline-warmup-threshold=1 --fast-warmup --ion-offthread-compile=off --ion-extra-checks test.js).

Backtrace:

    received signal SIGSEGV, Segmentation fault.
    #0  0x00005555582796b5 in js::wasm::WasmFrameIter::popFrame() ()
    #1  0x00005555572153d9 in js::JitFrameIter::operator++() ()
    #2  0x0000555557216599 in js::FrameIter::popJitFrame() ()
    #3  0x000055555721606d in js::FrameIter::operator++() ()
    #4  0x00005555573e121d in js::SavedStacks::insertFrames(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) ()
    #5  0x00005555573e05e9 in js::SavedStacks::saveCurrentStack(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) ()
    #6  0x000055555763e978 in JS::CaptureCurrentStack(JSContext*, JS::MutableHandle<JSObject*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) ()
    #7  0x00005555576494bf in js::ErrorToException(JSContext*, JSErrorReport*, JSErrorFormatString const* (*)(void*, unsigned int), void*) ()
    #8  0x0000555557211b1b in js::ReportErrorNumberVA(JSContext*, js::IsWarning, JSErrorFormatString const* (*)(void*, unsigned int), void*, unsigned int, js::ErrorArgumentsType, __va_list_tag*) ()
    #9  0x0000555557621abe in JS_ReportErrorNumberUTF8(JSContext*, JSErrorFormatString const* (*)(void*, unsigned int), void*, unsigned int, ...) ()
    #10 0x0000555558296781 in js::wasm::ReportTrapError(JSContext*, unsigned int) ()
    #11 0x0000555558253c81 in WasmHandleTrap() ()
    #12 0x000009933a6cf0b8 in ?? ()
    [...]
    #31 0x0000000000000000 in ?? ()
    rax	0x5555557dbcbe	93824994884798
    rbx	0x7fffffffadd0	140737488334288
    rcx	0x555558899550	93825045992784
    rdx	0x1	1
    rsi	0x0	0
    rdi	0x7ffff7be67d0	140737349838800
    rbp	0x7fffffffab20	140737488333600
    rsp	0x7fffffffaaf0	140737488333552
    r8	0x0	0
    r9	0x3	3
    r10	0x0	0
    r11	0x0	0
    r12	0x7fffffffadd8	140737488334296
    r13	0x7fffffffbbe0	140737488337888
    r14	0x7fffffffade0	140737488334304
    r15	0x9933a6efa26	10527945194022
    rip	0x5555582796b5 <js::wasm::WasmFrameIter::popFrame()+997>
    => 0x5555582796b5 <_ZN2js4wasm13WasmFrameIter8popFrameEv+997>:	movl   $0xbe,0x0
       0x5555582796c0 <_ZN2js4wasm13WasmFrameIter8popFrameEv+1008>:	callq  0x555556f17ec0 <abort>

This is from a new wasm fuzzer I built. I was not able to separate the test parts from some of the harness parts, so the attachment needs to be kept confidential once the bug is opened.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Jan de Mooij [:jandem]]

This is related to tail calls. The JitEntry stub for JIT => Wasm calls initializes an ExitFrameType on the stack here but later that field gets clobbered with a different value and this confuses frame iteration when we trap after that. The code that clobbers the frame type looks like what we emit in CollapseWasmFrameFast.

Comment 4

[Yury Delendik (:yury)]

Attachment: smaller test case

Reproducible on aarch64 with --wasm-compiler=optimized --baseline-warmup-threshold=1

Comment 5

[Yury Delendik (:yury)]

Pernosco debugging session https://pernos.co/debug/Q2IhuarxkpAFbgF3zHbr2w/index.html

Comment 6

[Jan de Mooij [:jandem]]

More reduced test case. Fails with --wasm-compiler=ion --blinterp-eager

function processWAST(source) {
    let modBuf = wasmTextToBinary(source);
    let module = new WebAssembly.Module(modBuf);
    let instance = new WebAssembly.Instance(module);
    for (let i = 0; i < 10; ++i)  {
        try {
            instance.exports.odd();
        } catch (e) {} 
    }
}
processWAST(`(module
(table 2 2 funcref)
(elem (i32.const 0) $odd $odd)
(type $t (func (param) (result i32)))
(func $odd (export "odd") (param i32 i32 i32 i32 i32 i32 i32) (result i32)
  (return_call_indirect (type $t) (i32.const 1) (local.get 1))
))`); 

Comment 7

[Yury Delendik (:yury)]

Attachment: Bug 1917807 - Fix GenerateJitEntry call parameters alignment. r?jandem

Comment 8

[Yury Delendik (:yury)]

Wasm tails calls expect particular alignment of call arguments: parameters area, which is padded to WasmStackAlignment, shall not contain any other fields (e.g. ExitFooterFrame).

The expectation is the same for GenerateDirectCallFromJit, but I didn't come up with failing test case. The initial frame is aligned by 16, fake frame size (56 @x64 or 44 @x86) and footer is (8 or 4).

Comment 9

[Yury Delendik (:yury)]

Attachment: Bug 1917807 - Add test and assert for jit exit and args alignments. r?jandem

Comment 10

[Yury Delendik (:yury)]

The misalignment is caused by bug 1910880, first bad commit is https://hg.mozilla.org/mozilla-central/rev/cda8bed193914e4e6bedf875aecd9abbcc74606e . Limiting affected range to FF130.

Comment 11

[Yury Delendik (:yury)]

Comment on attachment 9424261 [details]
Bug 1917807 - Fix GenerateJitEntry call parameters alignment. r?jandem

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Hard. An exploit will rely on complex behavior of JS JIT engine tiering and wasm tail calls. Different on every platform too.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?:
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Applies to beta branch without problem
• How likely is this patch to cause regressions; how much testing does it need?: Low-mid. The fix applied to somewhat freshly updated code. This bug related to wasm tail calls (which is rarely used), rest of the code is well tested.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 9424261 [details]
Bug 1917807 - Fix GenerateJitEntry call parameters alignment. r?jandem

Approved to land and uplift

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61b6c7aca39d
Fix GenerateJitEntry call parameters alignment. r=jandem

Comment 15

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

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Yury Delendik (:yury)]

Comment on attachment 9424261 [details]
Bug 1917807 - Fix GenerateJitEntry call parameters alignment. r?jandem

Beta/Release Uplift Approval Request

• User impact if declined: Crashes in wasm programs that use tail calls.
• 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): Affects only some wasm programs
• String changes made/needed:
• Is Android affected?: Yes

Comment 18

[Dianna Smith [:diannaS]]

Comment on attachment 9424261 [details]
Bug 1917807 - Fix GenerateJitEntry call parameters alignment. r?jandem

Approved for 131.0b8

Comment 19

[Pulsebot]

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

Comment 20

[BugBot [:suhaib / :marco]]

a month ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-test 2024-11-12] .

yury, please refer to the original comment to better understand the reason for the reminder.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e04549125889
Add test and assert for jit exit and args alignments. r=jandem

Comment 22

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

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

Comment 23

[Bugmon [:jkratzer for issues]]

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