1851568 - Assertion failure: isSome(), at mozilla/Maybe.h:753 with wasm tail calls

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20230903-39747a728e31 (debug build, run with --fuzzing-safe --wasm-tail-calls --wasm-compiler=baseline test.js).

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x000055555821d331 in js::wasm::BaseStackFrame::loadIncomingStackResultAreaPtr(js::wasm::RegPtr) ()
#1  0x000055555823e7a5 in bool js::wasm::BaseCompiler::emitCallArgs<js::wasm::TailCallResults>(mozilla::Vector<js::wasm::PackedType<js::wasm::ValTypeTraits>, 16ul, js::SystemAllocPolicy> const&, js::wasm::TailCallResults, js::wasm::FunctionCall*, js::wasm::BaseCompiler::CalleeOnStack) ()
#2  0x000055555823ded1 in js::wasm::BaseCompiler::emitReturnCall() ()
#3  0x0000555558263a86 in js::wasm::BaseCompiler::emitBody() ()
#4  0x0000555558289e3a in js::wasm::BaselineCompileFunctions(js::wasm::ModuleEnvironment const&, js::wasm::CompilerEnvironment const&, js::LifoAlloc&, mozilla::Vector<js::wasm::FuncCompileInput, 8ul, js::SystemAllocPolicy> const&, js::wasm::CompiledCode*, mozilla::UniquePtr<char [], JS::FreePolicy>*) ()
#5  0x0000555558315991 in ExecuteCompileTask(js::wasm::CompileTask*, mozilla::UniquePtr<char [], JS::FreePolicy>*) ()
#6  0x0000555558316be7 in js::wasm::ModuleGenerator::finishFuncDefs() ()
#7  0x00005555582f5491 in bool DecodeCodeSection<js::wasm::Decoder>(js::wasm::ModuleEnvironment const&, js::wasm::Decoder&, js::wasm::ModuleGenerator&) ()
#8  0x00005555582f5081 in js::wasm::CompileBuffer(js::wasm::CompileArgs const&, js::wasm::ShareableBytes const&, mozilla::UniquePtr<char [], JS::FreePolicy>*, mozilla::Vector<mozilla::UniquePtr<char [], JS::FreePolicy>, 0ul, js::SystemAllocPolicy>*, JS::OptimizedEncodingListener*) ()
#9  0x000055555835ae58 in js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*) ()
#10 0x0000555557041d35 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
[...]
#22 0x0000555556e99301 in main ()
rax	0x555555906f58	93824996110168
rbx	0x7fffffffb0d0	140737488335056
rcx	0x5555588e43b8	93825046299576
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffa2d0	140737488331472
rsp	0x7fffffffa2c0	140737488331456
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f9a840	140737353721920
r10	0x2	2
r11	0x0	0
r12	0xffffffff	4294967295
r13	0x0	0
r14	0x7ffff2daaab8	140737267804856
r15	0x7fffffffa700	140737488332544
rip	0x55555821d331 <js::wasm::BaseStackFrame::loadIncomingStackResultAreaPtr(js::wasm::RegPtr)+177>
=> 0x55555821d331 <_ZN2js4wasm14BaseStackFrame30loadIncomingStackResultAreaPtrENS0_6RegPtrE+177>:	movl   $0x2f1,0x0
   0x55555821d33c <_ZN2js4wasm14BaseStackFrame30loadIncomingStackResultAreaPtrENS0_6RegPtrE+188>:	callq  0x555556f35f33 <abort>

Omitting the --wasm-compiler=baseline option turns the crash into a different assertion:

Assertion failure: producer_ != nullptr, at jit/MIR.h:248

This also still happens if I add setJitCompilerOption("wasm.baseline", 1); to the test itself. Is this difference expected? This might affect some fuzzers that use this option rather than the JS shell flag (e.g. for differential testing).

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Ryan Hunt [:rhunt]]

Yury, can you look at this?

Comment 4

[Yury Delendik (:yury)]

Reduced test case:

(module
  (func (result i32 f64)
    i32.const 1
    f64.const 2.0
  )
  (func (export "f") (result f64)
    return_call 0
  ))

It is validation issue -- we need "strictly" compare caller and callee results in OpIter.

Comment 5

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1851568 using build mozilla-central 20230903210251-39747a728e31. 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 6

[Yury Delendik (:yury)]

Attachment: Bug 1851568 - Improve validation of tail call result types. r=bvisness

Comment 7

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

:yury, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit BugBot documentation.

Comment 8

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

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

Comment 9

[Daniel Veditz [:dveditz]]

This looks like a release assertion and the variant a null deref. On the surface doesn't look exploitable, but maybe there's a way to get a different "wrong" result that gets around the release assert?

Comment 10

[Yury Delendik (:yury)]

This looks like a release assertion and the variant a null deref. On the surface doesn't look exploitable, but maybe there's a way to get a different "wrong" result that gets around the release assert?

I didn't really dive into possibility of different ways to exploit it. The wasm validation algorithm was incorrect, and the following compilation code does not expect input of wrong length. So, eventually, it would crash at some point at the compilation stage.

The tail calls feature is currently enabled only in nightly (see bug 1846789).

Comment 11

[Yury Delendik (:yury)]

Comment on attachment 9351679 [details]
Bug 1851568 - Improve validation of tail call result types. r=bvisness

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Easy
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?:
• If not all supported branches, which bug introduced the flaw?: Bug 1571998
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: the feature is not compiled for release/beta
• How likely is this patch to cause regressions; how much testing does it need?: passes spec tests + variations of crasher
• Is Android affected?: Unknown

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 9351679 [details]
Bug 1851568 - Improve validation of tail call result types. r=bvisness

sec-approval isn't needed for nightly-only features, so feel free to land.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19a4c0047d6e
Improve validation of tail call result types. r=bvisness

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/19a4c0047d6e

Comment 15

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

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

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

For more information, please visit BugBot documentation.

Comment 16

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter