Stream: git-wasmtime

Topic: wasmtime / PR #5155 cranelift: Correctly calculate heap a...


view this post on Zulip 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_addr instruction 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2022 at 16:10):

afonso360 updated PR #5155 from interp-fix-heapaddr to main.

view this post on Zulip 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_addr instruction 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.

view this post on Zulip 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_addr instruction 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.

view this post on Zulip 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 a load_size would be 1 at minimum. Since offset is 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 to offset would 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 31 2022 at 22:04):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 31 2022 at 22:04):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 31 2022 at 22:07):

jameysharp merged PR #5155.

view this post on Zulip 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_addr in 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 sized heap_addr's are legal there.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 16:02):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2022 at 16:06):

afonso360 edited PR review comment.


Last updated: Dec 23 2024 at 12:05 UTC