Cranelift: properly handle unknown ref types instead of crashing

RESOLVED FIXED in Firefox 68

Status

()

enhancement
RESOLVED FIXED
2 months ago
Last month

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

(Blocks 1 bug, Regressed 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

2 months ago

Once this is done, I think we can re-enable fuzzing for Cranelift, because it will properly error out instead of panicking when it sees an unknown ref type. It's also a better UX than crashing Firefox, so I'll classify this as an enhancement.

Assignee

Comment 1

2 months ago

This requires https://github.com/CraneStation/cranelift/pull/754 to be merged and Spidermonkey to be bumped before we can merge this.

Attachment #9061330 - Attachment description: Bug 1547682: Cranelift: properly handle unknown type errors → Bug 1547682: Cranelift: properly handle unknown type errors; r?lth
Attachment #9061342 - Attachment description: Bug 1547682: Reenable wasm fuzzing for cranelift; → Bug 1547682: Reenable wasm fuzzing for cranelift; r?decoder
Assignee

Updated

2 months ago
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED

Comment 5

2 months ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd0c4622f00
Bump Cranelift to be8a83132df0a277da8fa3e6a9c5d03c4a05d57e; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bfcf5af011d
Cranelift: properly handle unknown type errors; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/815455c1634e
Reenable wasm fuzzing for cranelift; r=decoder
Assignee

Comment 7

2 months ago

That's unfortunate: these new failures result from the fact that the failure crate is now built with the std feature, as recently enabled by the cranelift-wasm crate. The failure crate with std support references an old backtraces crate that doesn't compile on CI; it is probably the reason why std support for this crate has been disabled by other uses of the failure crate in tree.

We can:

  • disable failure/std support in cranelift-wasm, which would imply the WasmError::User() field to not get a failure::Error (only defined with failure/std support). It can probably be replaced with a Box<std::error::Error>, or some simple error wrapper, and it wouldn't be too bad.
  • go the long way: upgrade backtraces in failure, take care of the other link errors etc.

I'll try the former first.

Attachment #9061330 - Attachment description: Bug 1547682: Cranelift: properly handle unknown type errors; r?lth → Bug 1547682: Cranelift: properly handle unknown type errors; r=lth
Attachment #9061342 - Attachment description: Bug 1547682: Reenable wasm fuzzing for cranelift; r?decoder → Bug 1547682: Reenable wasm fuzzing for cranelift; r=decoder
Assignee

Comment 8

2 months ago

I've updated the patches 2 and 3 to take into account the Cranelift PR that would need to be accepted first. If somebody wanted to land this before I come back from PTO, then they'd need to bump the Cranelift commit hash too and re-run mach vendor rust to regenerate the first patch. I'll keep the needinfo, in case it's not done when I get back.

Attachment #9061620 - Attachment description: Bug 1547682: Bump Cranelift to be8a83132df0a277da8fa3e6a9c5d03c4a05d57e; r?lth → Bug 1547682: Bump Cranelift to cc216b46b35a797d03c0f3e8b16a2096f1c6db61; r=lth

Comment 9

Last month
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc152a886c75
Bump Cranelift to cc216b46b35a797d03c0f3e8b16a2096f1c6db61; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3e245822b9f
Cranelift: properly handle unknown type errors; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/037e2534c8bd
Reenable wasm fuzzing for cranelift; r=decoder
Assignee

Updated

Last month
Flags: needinfo?(bbouvier)
You need to log in before you can comment on or make changes to this bug.