wasm2c: bounds-check active data loads #1829
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The bulk-memory proposal changed OOB access (when loading an active data segment) from a validation failure to an initialization-time trap, but wasm2c wasn't checking OOB access in LOAD_DATA(). When WASM_RT_MEMCHECK_SIGNAL_HANDLER is unset, this could produce memory corruption. When WASM_RT_MEMCHECK_SIGNAL_HANDLER is set, this led to failure in the new "data" tests that verify that modules with zero-length active data segments are uninstantiable if offset > mem.size. Add an unconditional bounds-check and restore the `test/wasm2c/spec/data.txt` test (one of the failing tests tracked at WebAssembly#1737).
bccb964 to
084d200
Compare
sbc100
approved these changes
Feb 17, 2022
Member
|
Thanks for working on this. Good to see some of those tests being re-enabled. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The bulk-memory proposal changed OOB access (when loading an active data segment) from a validation failure to an initialization-time trap, but wasm2c wasn't checking OOB access in
LOAD_DATA().When
WASM_RT_MEMCHECK_SIGNAL_HANDLERis unset, this could produce memory corruption. WhenWASM_RT_MEMCHECK_SIGNAL_HANDLERis set, this led to failure in the new “data” tests that verify that modules with zero-length active data segments are uninstantiable ifoffset > mem.size.This commit adds an unconditional bounds-check and restores the
test/wasm2c/spec/data.txttest (one of the failing tests tracked at #1737).(It's possible to avoid the unconditional bounds-check if we made
CWriter::WriteDataInitializersmore complicated to special-case the situation of a zero-length data segment. Then non-zero-length data segments could be loaded normally withMEMCHECK(which is a nop on platforms usingWASM_RT_MEMCHECK_SIGNAL_HANDLER), and only zero-length segments would require the unconditional bounds-check (RANGE_CHECK). But given that we're talking about something that happens only once per segment on module instantiation, that extra complexity didn't seem worth it.)