1865044 - Crash [@ js::jit::JSJitFrameIter::machineState() const] or Assertion failure: framePtr->hasCachedSavedFrame() || hasGoodExcuse, at vm/SavedStacks.cpp:1483 with Debugger

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20231115-571c293cedd2 (opt build, run with --fuzzing-safe --ion-offthread-compile=off --more-compartments):

a = newGlobal();
a.b = this;
a.eval("(" + function () {
    Debugger(b).onExceptionUnwind = function () {}
} + ")()");
function c(d) {
  binary = wasmTextToBinary(d)
  e = new WebAssembly.Module(binary)
  return new WebAssembly.Instance(e)
}
f = c(`
(type $g 
  (func (param i64 i64 funcref) (result i64))
)          
(func (export "vis") (param i64 i64 funcref) (result i64) 
  local.get 0 
  local.get 1
  local.get 2
  local.get 2
  ref.cast (ref $g)
  return_call_ref $g
)
`);
h = f.exports["vis"];
i = new WebAssembly.Function({
  parameters: [ "i64", "i64", "funcref" ], results: ["i64"]   
}, function () {});
h(3n, 1n, i);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555556e3f2ce in js::jit::JSJitFrameIter::machineState() const ()
#1  0x0000555556f38bd9 in js::jit::InlineFrameIterator::resetOn(js::jit::JSJitFrameIter const*) ()
#2  0x0000555556f37469 in js::FrameIter::settleOnActivation() ()
#3  0x00005555575a1639 in js::Debugger::fireExceptionUnwind(JSContext*, JS::Handle<JS::Value>, js::ResumeMode&, JS::MutableHandle<JS::Value>) ()
#4  0x00005555575a13cb in js::DebugAPI::slowPathOnExceptionUnwind(JSContext*, js::AbstractFramePtr) ()
#5  0x000055555777f09b in js::wasm::HandleThrow(JSContext*, js::wasm::WasmFrameIter&, js::jit::ResumeFromException*) ()
#6  0x000055555777fb23 in WasmHandleThrow(js::jit::ResumeFromException*) ()
#7  0x000026372362a49e in ?? ()
[...]
#11 0x0000000000000000 in ?? ()
rax	0x7fffffffc080	140737488339072
rbx	0x7fffffffb6b0	140737488336560
rcx	0x0	0
rdx	0x2637235fa289	42018258526857
rsi	0xccccccff6b1efbe9	-3689348597337359383
rdi	0x7fffffffb6b0	140737488336560
rbp	0x7fffffffb6a0	140737488336544
rsp	0x7fffffffb680	140737488336512
r8	0x2637235fa289	42018258526857
r9	0x0	0
r10	0x1	1
r11	0x55555669b720	93825010349856
r12	0x0	0
r13	0x10b	267
r14	0x7fffffffb920	140737488337184
r15	0x7fffffffb768	140737488336744
rip	0x555556e3f2ce <js::jit::JSJitFrameIter::machineState() const+94>
=> 0x555556e3f2ce <_ZNK2js3jit14JSJitFrameIter12machineStateEv+94>:	mov    0x30(%rsi),%rdi
   0x555556e3f2d2 <_ZNK2js3jit14JSJitFrameIter12machineStateEv+98>:	mov    (%rdi),%rsi

Marking s-s due to crash address but likely sec-moderate at most due to debugger involvement.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Ryan Hunt [:rhunt]]

Looks tail call related, Yury can you take a look?

Comment 4

[Yury Delendik (:yury)]

Yeah, looks very similar to bug 1862089 . I'll try to reproduce locally.

Comment 5

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1865044 using build mozilla-central 20231115163402-571c293cedd2. 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)]

(In reply to Yury Delendik (:yury) from comment #4)

Yeah, looks very similar to bug 1862089 . I'll try to reproduce locally.

Still fails with patch from bug 1862089.

I can see that DebugFrame::clearReturnJSValue() corrupts stack data because it is confused about frame being debug-able. So the defect will be present only in Wasm debug mode during exception/trap handling. Will investigate further for tail calls involvement.

Comment 7

[Yury Delendik (:yury)]

Attachment: Bug 1865044 - [wasm] Skip debugger results cleanup for return stubs. r?rhunt

Comment 8

[Yury Delendik (:yury)]

Comment on attachment 9364600 [details]
Bug 1865044 - [wasm] Skip debugger results cleanup for return stubs. r?rhunt

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch does not points to the problem right away, but test is. The issue is stack corruption with certain value -- creating meaningful exploit will be difficult
• 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?: 121
• 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?:
• How likely is this patch to cause regressions; how much testing does it need?: Low risk, and it will cause the issue only with devtools opened.
• Is Android affected?: Unknown

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 9364600 [details]
Bug 1865044 - [wasm] Skip debugger results cleanup for return stubs. r?rhunt

I'll leave the assignment off, but since this is looking like sec-moderate because it requires the debugger, we can land it with the test.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3022af75b98
[wasm] Skip debugger results cleanup for return stubs. r=rhunt

Comment 11

[Donal Meehan [:dmeehan]]

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

Comment 12

[Yury Delendik (:yury)]

Attachment: beta uplift: bug1865044+bug1866136.diff

Approval Request Comment
[Feature/Bug causing the regression]: wasm tail calls
[User impact if declined]: browsers crash when debugger is opened
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: low
[Why is the change risky/not risky?]: present when debugging wasm (with tail calls)
[String changes made/needed]:

Comment 13

[Yury Delendik (:yury)]

status-firefox-esr115: --- → affected

Tail calls were disabled for ff120 and below, no need to uplift the bug for older than ff121 versions.

Comment 14

[Yury Delendik (:yury)]

Attachment: beta uplift: bug1865044+1866136+1866252.diff

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. Also, don't forget to request an uplift for the patches in the regression caused by this fix.
• If no, please set status-firefox121 to wontfix.

For more information, please visit BugBot documentation.

Comment 16

[Yury Delendik (:yury)]

Comment on attachment 9365203 [details] [diff] [review]
beta uplift: bug1865044+1866136+1866252.diff

Beta/Release Uplift Approval Request

• User impact if declined: browsers crash when debugger is opened
• 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): present when debugging wasm (with tail calls)
• String changes made/needed:
• Is Android affected?: Unknown

Comment 17

[Donal Meehan [:dmeehan]]

Comment on attachment 9365203 [details] [diff] [review]
beta uplift: bug1865044+1866136+1866252.diff

Approved for 121.0b4

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/425ec2d6921e