Wasmtime GitHub notifications bot (Oct 29 2022 at 16:10): Wasmtime GitHub notifications bot (Oct 29 2022 at 16:10):afonso360 opened PR #5155 from interp-fix-heapaddr to main:
:wave: Hey,
This PR fixes an issue with the
heap_addrinstruction in the interpreter. We were accidentally including the size as part of the offset when computing heap addresses.This failed when one tries to load the top n bytes of the heap, the interpreter incorrectly reported a HeapOutOfBounds error.
Wasmtime GitHub notifications bot (Oct 29 2022 at 16:10):afonso360 updated PR #5155 from interp-fix-heapaddr to main.
Wasmtime GitHub notifications bot (Oct 29 2022 at 16:11):afonso360 edited PR #5155 from interp-fix-heapaddr to main:
:wave: Hey,
This PR fixes an issue with the
heap_addrinstruction in the interpreter. We were accidentally including the size as part of the offset when computing heap addresses.This failed when trying to load the top n bytes of the heap, the interpreter incorrectly reported a HeapOutOfBounds error.
Wasmtime GitHub notifications bot (Oct 29 2022 at 17:06):afonso360 edited PR #5155 from interp-fix-heapaddr to main:
:wave: Hey,
This PR fixes an issue with the
heap_addrinstruction in the interpreter. We were accidentally including the size as part of the offset when computing heap addresses.This failed when trying to load the top n bytes of the heap, the interpreter incorrectly reported a HeapOutOfBounds error.
Wasmtime GitHub notifications bot (Oct 31 2022 at 22:04):jameysharp created PR review comment:
Why is this a
saturating_sub? I haven't checked the instruction definition but I'd have expected that aload_sizewould be 1 at minimum. Sinceoffsetis unsigned and we're apparently assuming the addition can't overflow, that would mean the subtraction can't underflow.Related: subtracting 1 from
load_size_before_ adding it tooffsetwould avoid overflow if we're accessing valid heap addresses near 2^64. But I assume the interpreter would run out of memory long before growing a heap big enough for that to matter.
Wasmtime GitHub notifications bot (Oct 31 2022 at 22:04):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Oct 31 2022 at 22:04):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Oct 31 2022 at 22:07):jameysharp merged PR #5155.
Wasmtime GitHub notifications bot (Nov 01 2022 at 16:02):afonso360 created PR review comment:
Why is this a saturating_sub? I haven't checked the instruction definition but I'd have expected that a load_size would be 1 at minimum.
I originally did this because we have a zero sized
heap_addrin the runtest test suite, which was panicking!I then tried to fix it by making that properly sized. And then I had the idea, "Hey lets put a verifier rule for this!". And it turns out our test suite is full of zero sized
heap_addr's, I've opened #5167 with the exact changes, but we should probably discuss if zero sizedheap_addr's are legal there.
Wasmtime GitHub notifications bot (Nov 01 2022 at 16:02):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Nov 01 2022 at 16:06):afonso360 edited PR review comment.
Last updated: Nov 22 2024 at 17:03 UTC