Fix sign extension of i32 addresses in interpreter memory access#8348
Fix sign extension of i32 addresses in interpreter memory access#8348sumleo wants to merge 1 commit intoWebAssembly:mainfrom
Conversation
src/wasm-interpreter.h
Outdated
| size_t indexVal = index.getSingleValue().getUnsigned(); | ||
| if (indexVal >= data->values.size()) { | ||
| trap("array oob"); | ||
| } |
There was a problem hiding this comment.
This looks like a correct fix, but the title and description of the PR look unrelated?
| if (indexVal >= data->values.size()) { | ||
| trap("array oob"); | ||
| } | ||
| auto& field = data->values[indexVal]; |
There was a problem hiding this comment.
@tlively is it possible the spec tests do not have bounds checks for this instruction, and ArrayCmpXChg?
There was a problem hiding this comment.
We don't have real spec tests for GC atomics yet and it looks like we don't have our own interpreter tests or fuzzer support for them, either.
sumleo
force-pushed
the
fix-interpreter-address-signext
branch
from
73c73bb to
1aaf4f9
Compare
|
Thanks for catching that! The branch previously contained the wrong change (an array bounds check that was already covered by #8351). I've force-pushed with the actual sign extension fix for |
sumleo
force-pushed
the
fix-interpreter-address-signext
branch
from
1aaf4f9 to
88aba0f
Compare
ptr.geti32() returns int32_t, which gets sign-extended to int64_t via C++ ternary promotion rules before being stored as uint64_t. For i32 addresses >= 0x80000000, this produces incorrect 64-bit addresses (e.g., 0xFFFFFFFF80000000 instead of 0x80000000), causing spurious out-of-bounds traps. Fix by casting through uint32_t first to zero-extend instead of sign-extend.
sumleo
force-pushed
the
fix-interpreter-address-signext
branch
from
88aba0f to
624d4c7
Compare
3 participants
Summary
getFinalAddressandgetFinalAddressWithoutOffsetin the interpreterptr.geti32()returnsint32_t, which gets sign-extended toint64_tvia C++ ternary promotion rules before being stored asuint64_t0x80000000, this produces incorrect 64-bit values (e.g.,0xFFFFFFFF80000000instead of0x80000000), causing spurious out-of-bounds trapsuint32_tfirst to zero-extend instead of sign-extendTest plan
binaryen-unittests)