pulley: Ungate memory64 feature #9780
Conversation
alexcrichton
requested review from
cfallin and
fitzgen
and removed request for
a team
|
note this is currently stacked on #9779 |
github-actions
bot
added
cranelift
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "cranelift", "pulley", "wasmtime:api", "wasmtime:config"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
To modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
| let high_bits = pos.ins().ushr(index, c32); | ||
| let high_bits = pos.ins().ireduce(pointer_ty, high_bits); | ||
| pos.ins() | ||
| .trapnz(high_bits, ir::TrapCode::HEAP_OUT_OF_BOUNDS); |
There was a problem hiding this comment.
I think we should make a note re: Spectre here -- trapnz internally uses a branch and so one might expect there to be some exposure here. I think we're actually safe because the worst that happens is that the guest accesses an OOB address like 0x1_0000_1000 where 0x1000 is in-bounds, and gets its own in-bounds data; in other words, this check is layered on top of the actual bounds-check on the lower 32 bits, so there is still not any visibility outside the sandbox in the misspeculated path. But it's... worth noting, if only because the guest might in turn rely on OOBs not to speculatively read valid data (niche but possible).
| // Sanity-check what should already be true from wasm module validation. | ||
| // Note that for 32-bit targets the absolute maximum is `1<<32` during | ||
| // compilation, not one-page-less-than-u32::MAX, so need to handle that | ||
| // specially here. | ||
| let absolute_max64 = if cfg!(target_pointer_width = "32") { | ||
| 1 << 32 | ||
| } else { | ||
| u64::try_from(absolute_max).unwrap() | ||
| }; | ||
| if let Ok(size) = ty.minimum_byte_size() { | ||
| assert!(size <= absolute_max64); | ||
| } | ||
| if let Ok(max) = ty.maximum_byte_size() { | ||
| assert!(max <= absolute_max64); | ||
| } |
There was a problem hiding this comment.
Why were these asserts removed?
There was a problem hiding this comment.
They were basically a pain to maintain on 32-bit because I already had to modify the minimum check to use 1<<32 rather than absolute_max (due to absolute_max being a usize and not being able to represent 1<<32) and then this PR uncovered an assertion in the next one where a 64-bit linear memory's maximum size far exceeds 1<<32 as well. Given that these are just debug asserts that are already basically checked in many other places it seemed best to just remove them instead of trying to contort ourselves to keep them in all portable conditions.
This commit is similar to bytecodealliance#9779 in that it's removing a proposal from the "known list of panicking features" for Pulley to allow more tests to run on Pulley. This then fills out a few miscellaneous instructions to get a full suite of tests passing in Pulley related to memory64 and other instructions. prtest:full
alexcrichton
force-pushed
the
pulley-ungate-memory64
branch
from
47a9046 to
3d22b3d
Compare
This commit is similar to #9779 in that it's removing a proposal from
the "known list of panicking features" for Pulley to allow more tests to
run on Pulley. This then fills out a few miscellaneous instructions to
get a full suite of tests passing in Pulley related to memory64 and
other instructions.
cc #9783