Stream: git-wasmtime

Topic: wasmtime / PR #5167 cranelift: Static check out of bounds...

Wasmtime GitHub notifications bot (Nov 02 2022 at 15:39):

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 calculating heap_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 sized heap_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 bound

Which 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.

Wasmtime GitHub notifications bot (Nov 02 2022 at 15:40):

afonso360 updated PR #5167 from interp-heap-sat-sub to main.

Wasmtime GitHub notifications bot (Nov 02 2022 at 15:50):

afonso360 has marked PR #5167 as ready for review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 16:57):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 16:57):

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 the Load or Store instruction formats may not realize they should update this match in the middle of the verifier.

Wasmtime GitHub notifications bot (Nov 02 2022 at 16:57):

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;

Wasmtime GitHub notifications bot (Nov 02 2022 at 16:57):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 16:57):

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?

Wasmtime GitHub notifications bot (Nov 24 2022 at 15:47):

afonso360 closed without merge PR #5167.

Last updated: Nov 22 2024 at 16:03 UTC