1655745 - (fuzzbug) Encoding of prefixed subopcodes is unclear by spec

Status: RESOLVED INVALID

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

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Found with decoder's differential fuzzing tools; it's likely to be an error downstream in cranelift-wasm or even wasmparser, but we need one place to start tracking it.

let buffer = [0,97,115,109,1,0,0,0,1,8,1,96,3,127,127,127,1,127,3,2,1,0,7,8,1,4,109,97,105,110,0,0,10,10,1,8,0,0,252,128,0,0,0,11,0,14,4,110,97,109,101,1,7,1,0,4,109,97,105,110];
buffer = new Uint8Array(buffer).buffer;

let { exports } = new WebAssembly.Instance(
    new WebAssembly.Module(buffer)
);

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

It is finally a spec bug! It's unclear whether a prefixed subopcode should be encoded as a fixed byte (u8) or a variable-length u32 as we do in Spidermonkey (and as does wabt/binaryen). v8 does the former iiuc. Opened a spec issue.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1655745: use fixed bytes encodings for non-simd prefixed wasm opcodes; r?lth

Tentative patch, to be landed if
https://github.com/WebAssembly/spec/issues/1228 is agreed upon.

The validator did incorrectly accept prefixed opcodes encoded as variable u32,
while the spec doesn't explicitly allow for this, and use fixed opcodes for all
of these subopcodes.

The alternative implementation was to use a union for the b1 subopcode, that
would be either an u32 for simd and a u8 for non-simd. It didn't seem to seem
worth it, as it infected many places in the compilers and validation logic.

Comment 3

[Lars T Hansen [:lth]]

Yeah, nice job. IMO we should change the spec to follow Firefox here, and I've commented to that effect, but it'll likely take a vote by the CG to affirm that that's what we've already decided, spec interpreter notwithstanding. (And then I suspect a bit of pushback from V8 because I think this means they have to do some significant engineering work.)

Comment 4

[Lars T Hansen [:lth]]

There's been a CG vote and we're in the clear on the SpiderMonkey side - the spec behavior is now what we already do. Not sure if CL has its own decoder and whether that needs to be fixed.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Yeah, wasmparser needs to be fixed. Closing here.