Stream: git-wasmtime

Topic: wasmtime / issue #5167 cranelift: Forbid zero sized `heap...

Wasmtime GitHub notifications bot (Nov 01 2022 at 18:02):

cfallin commented on issue #5167:

Really interesting question here!

My first impulse is actually slightly different: in general I don't like edge cases or discontinuities, and for the same reason that I like, e.g., zero-sized slices to be possible, I think that a heap_addr with a size of 0 should be legal. It should execute without a trap if the base address given is less than or equal to the length of the heap.

In other words, the base + size - 1 computation finds the address of the last byte of the thing being accessed, and this implicitly assumes there is a last byte (i.e. it's like a vec.last().unwrap()). Instead of base + size - 1 < heap.base + heap.len, I might suggest base + size <= heap.base + heap.len. That naturally handles the zero-sized case, and feels like the right answer for "limit of behavior as size approaches zero". (In general I'm also slightly allergic to - 1 and + 1 in offset computations; they feel like a sign that we haven't found the most natural expression of the computation. For the same reasons that 0-based array indexing makes more sense etc etc)

FWIW, the instruction docs just below the quoted part do seem consistent with this: "If p + Size is greater than the heap bound, generate a trap." (Inverting "greater than" we get "less than or equal", and "heap bound" is heap.base + heap.len.)

Of course it's invalid to actually use the computed pointer to do any access of size greater than zero (that is, any access at all); but that's perfectly fine. That does mean that the tests that you're correcting here are illegal anyway, as they do v + 8 and v + 128 on the resulting pointers. So separately, we should update those tests to use appropriate Sizes (though it doesn't actually matter for the properties we're testing).

Thoughts?

Wasmtime GitHub notifications bot (Nov 02 2022 at 11:59):

afonso360 commented on issue #5167:

That sounds reasonable!

And I think we can enforce some of that via the verifier. If we can compute all of the offsets, and that a load/store is directly referencing a heap_addr, we can probably check if the access is legal. This still misses a bunch of cases, but I think it might catch the most common ones.

I'm going to give this a try.

Wasmtime GitHub notifications bot (Nov 02 2022 at 12:24):

afonso360 edited a comment on issue #5167:

That sounds reasonable!

And I think we can enforce some of that via the verifier. If we can compute all of the offsets, and that a load/store is directly referencing a heap_addr, we can probably check if the access is legal. This still misses a bunch of cases, but I think it might catch the most common ones.

I'm going to give that a try.

Last updated: Oct 23 2024 at 20:03 UTC