bytecodealliance / wasm-tools

CLI and Rust libraries for low-level manipulation of WebAssembly modules
Apache License 2.0
1.29k stars 233 forks source link

`wasmparser`: Valid Wasm is not accepted if `bulk-memory` is enabled and `reference-types` is disabled #889

Open Robbepop opened 1 year ago

Robbepop commented 1 year ago

I stumbled upon this when implementing bulk-memory Wasm proposal support in wasmi when integrating the new Wasm spec testsuite test cases for the bulk-memory Wasm proposal. Note: wasmi does not yet have support for the reference-types Wasm proposal and I guess this is important for this issue.

Namely, when parsing and validating the test cases via wasmparser (as always) it errors out with this message among others:

---- spec::bulk_memory::wasm_table_copy stdout ----
thread 'spec::bulk_memory::wasm_table_copy' panicked at 'tests/spec/testsuite/proposals/bulk-memory-operations/table_copy.wast: failed to execute `.wast` directive: reference types support is not enabled (at offset 0x74)', crates\wasmi\tests\spec\run.rs:44:9

---- spec::bulk_memory::wasm_table_init stdout ----
thread 'spec::bulk_memory::wasm_table_init' panicked at 'tests/spec/testsuite/proposals/bulk-memory-operations/table_init.wast: failed to execute `.wast` directive: reference types support is not enabled (at offset 0x74)', crates\wasmi\tests\spec\run.rs:44:9

The wasmparser error message is: reference types support is not enabled (at offset 0x74) Querying the wasmparser codebase got me the following code segments:


https://github.com/bytecodealliance/wasm-tools/blob/cacda73ccb6f9f8cc42e8391c77480187c99b48d/crates/wasmparser/src/validator.rs#L256

Which filters out ValType::FuncRef | ValType::ExternRef types, however, the bulk-memory Wasm proposal explicitly allows ValType::FuncRef in so-called elemexpr which can only occur in Wasm element const experessions. I think this is the problematic code. (Not tested though.)

elemexpr = refnull | funcref <func_idx>

Source: bulk-memory Wasm Proposal Overview elemexpr


https://github.com/bytecodealliance/wasm-tools/blob/cacda73ccb6f9f8cc42e8391c77480187c99b48d/crates/wasmparser/src/validator/operators.rs#L934

Unfortunately I am on wasmparser 0.91 so this issue might have already been resolved in a newer version. Here is a link to the WIP wasmi PR in case this helps. (It is very WIP ...)

alexcrichton commented 1 year ago

The bulk memory and reference types proposals were heavily intertwined and very little care was given at the time when implementing them in wasmparser as to which precise feature went where, so I'm sure mistakes were made or the spec has drifted since. It's fine to recategorize features under different proposals retroactively.

Robbepop commented 1 year ago

It's fine to recategorize features under different proposals retroactively.

What does this mean with respect to this issue?

The elemexpr was introduced in bulk-memory alongside table.init because without them it is impossible to initialize a table to null via table.init from what I understood.

Fixing this issue in wasmparser is a bit tricky since ref.null and ref.func instructions are only accepted in certain context under the bulk-memroy Wasm proposal.

alexcrichton commented 1 year ago

I'm not entirely sure unfortunately, I haven't put any effort into thinking about how to rationalize the bulk memory proposal without reference types or vice versa.