Skip to content

Conversation

@Robbepop
Copy link
Collaborator

@Robbepop Robbepop commented May 5, 2025

I tried to update Wasmi to wasmparser v0.229 and encountered the following bug in the existing Wasm spec testsuite:

thread 'wasm_binary_leb128' panicked at crates/wast/tests/mod.rs:11:9:
failed directive on testsuite/binary-leb128:217:1: module succeeded to compile and validate but should have failed with: integer representation too long

tl;dr: integer representation too long

After some debugging I found that this PR recently changed the logic to parse var_u32 or var_u64 depending on memory64 flags for memories.rs and for tables.rs.

It looked strange to me since memory64 and table64 flags are first extracted from some flags bitfields and then the new PR uses reader.memory64() and reader.table64() instead for querying those features.

Before the mentioned PR, it uses read_var_u64 only for memories and tables which explicitly are flagged as memory64 whereas now, it uses the reader's memory64 proposal flag instead. To me this looks incorrect.

With this PR I successfully fixed the bugs I found on Wasmi by simply reverting the logic to before the mentioned PR.

However, the Wasm spec testsuite of wasm-tools is unhappy now.

edit: Hmm, I just noticed that the Wasm 3.0 spec demands to always parse limits as u64 values, so indeed the current behavior of wasmparser might be correct.

image

@alexcrichton @keithw

@Robbepop
Copy link
Collaborator Author

Robbepop commented May 5, 2025

Never mind, I just noticed that Wasmi never uses Parser::set_features and so far that has never been an issue. But probably with this PR the issue just got uncovered.

Closed.

@Robbepop Robbepop closed this May 5, 2025
@Robbepop Robbepop deleted the rf-fix-parsing-overlong-ints branch May 5, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants