afonso360 edited PR #5167 from interp-heap-sat-sub
to main
:
:wave: Hey,
I don't know if we should merge this or not!
This PR started on https://github.com/bytecodealliance/wasmtime/pull/5155#discussion_r1009872167 where @jameysharp pointed out that it was a bit weird that the interpreter used a
saturating_sub
when calculatingheap_addr
sizes.I added that because we do have some 0 sized
heap_addr
's in runtests. However when fixing that, I added a verifier rule, and it turns out we have a lot of 0 sizedheap_addr
's in our test suite. Are these legal?The instruction docs state the following:
Verify that the offset range
p .. p + Size - 1
is in bounds for the heap H, and generate an absolute address that is safe to dereference.So it sounds like
Size
must always be larger than 1?But it also says:
If
p + Size
is not greater than the heap boundWhich would allow 0 as a
Size
.So, lets make a decision about this! I don't have any preference for either, I'm just opening this as a PR because when I started fixing the interpreter I thought it would just be that one runtest that was wrong!
Also, there are some regressions in the x64 codegen that probably shouldn't be there.
afonso360 updated PR #5167 from interp-heap-sat-sub
to main
.
afonso360 has marked PR #5167 as ready for review.
jameysharp submitted PR review.
jameysharp created PR review comment:
I'd lean toward explicitly listing the remaining opcodes, and having an
_ => unreachable!()
at the end. Someone who adds a new instruction that uses one of theLoad
orStore
instruction formats may not realize they should update thismatch
in the middle of the verifier.
jameysharp created PR review comment:
This is algebraically equivalent to what you wrote, but makes more sense to me:
let needed_size = offset + access_size; let oob_bytes = needed_size - valid_size;
jameysharp submitted PR review.
jameysharp created PR review comment:
In principle I suppose we could verify arithmetic where one operand is a
heap_addr
and the rest are all constants. I'm not suggesting that should be in this PR (unless you really want to), but maybe note it in this comment?
afonso360 closed without merge PR #5167.
Last updated: Dec 23 2024 at 12:05 UTC