Stream: git-wasmtime

Topic: wasmtime / issue #5900 cranelift-interpreter: Trap on sto...

Wasmtime GitHub notifications bot (Mar 01 2023 at 11:05):

afonso360 opened issue #5900:

:wave: Hey,

Feature

After #5893 we now have access to MemFlags when loading and storing to memory. We should check the readonly flag, and always trap if there is a write to it.

Technically the current behaviour of ignoring it and loading correctly anyway is allowed.

Loads with this flag have no memory dependencies. This results in undefined behavior if the dereferenced memory is mutated at any time between when the function is called and when it is exited.

There is a world where we keep track of these flags (in both loads and stores) and can trap at any time a store to a previously readonly'd memory location. But this is a good start.

Benefit

This gets us a better interpretation of CLIF semantics and allows whoever is using the interpreter to catch these cases sooner.

Implementation

• In State::checked_store check if the memory flags have the readonly flag set and return a MemoryError if so.

• We can't build the usual tests for this.

  • Our runtests don't support checking for traps (cc #4781).
  • What we can do instead is build a CLIF program and invoke the interpreter using the library API. See this example for a test case testing traps on srem, we can do something similar for load/store.

Alternatives

The current implementation is fine, and we should technically never see a store with a readonly flag.

Wasmtime GitHub notifications bot (Mar 01 2023 at 11:05):

afonso360 labeled issue #5900:

:wave: Hey,

Feature

After #5893 we now have access to MemFlags when loading and storing to memory. We should check the readonly flag, and always trap if there is a write to it.

Technically the current behaviour of ignoring it and loading correctly anyway is allowed.

Loads with this flag have no memory dependencies. This results in undefined behavior if the dereferenced memory is mutated at any time between when the function is called and when it is exited.

There is a world where we keep track of these flags (in both loads and stores) and can trap at any time a store to a previously readonly'd memory location. But this is a good start.

Benefit

This gets us a better interpretation of CLIF semantics and allows whoever is using the interpreter to catch these cases sooner.

Implementation

• In State::checked_store check if the memory flags have the readonly flag set and return a MemoryError if so.

• We can't build the usual tests for this.

  • Our runtests don't support checking for traps (cc #4781).
  • What we can do instead is build a CLIF program and invoke the interpreter using the library API. See this example for a test case testing traps on srem, we can do something similar for load/store.

Alternatives

The current implementation is fine, and we should technically never see a store with a readonly flag.

Wasmtime GitHub notifications bot (Mar 01 2023 at 11:05):

afonso360 labeled issue #5900:

:wave: Hey,

Feature

After #5893 we now have access to MemFlags when loading and storing to memory. We should check the readonly flag, and always trap if there is a write to it.

Technically the current behaviour of ignoring it and loading correctly anyway is allowed.

Loads with this flag have no memory dependencies. This results in undefined behavior if the dereferenced memory is mutated at any time between when the function is called and when it is exited.

There is a world where we keep track of these flags (in both loads and stores) and can trap at any time a store to a previously readonly'd memory location. But this is a good start.

Benefit

This gets us a better interpretation of CLIF semantics and allows whoever is using the interpreter to catch these cases sooner.

Implementation

• In State::checked_store check if the memory flags have the readonly flag set and return a MemoryError if so.

• We can't build the usual tests for this.

  • Our runtests don't support checking for traps (cc #4781).
  • What we can do instead is build a CLIF program and invoke the interpreter using the library API. See this example for a test case testing traps on srem, we can do something similar for load/store.

Alternatives

The current implementation is fine, and we should technically never see a store with a readonly flag.

Wasmtime GitHub notifications bot (Mar 01 2023 at 11:05):

afonso360 labeled issue #5900:

:wave: Hey,

Feature

After #5893 we now have access to MemFlags when loading and storing to memory. We should check the readonly flag, and always trap if there is a write to it.

Technically the current behaviour of ignoring it and loading correctly anyway is allowed.

Loads with this flag have no memory dependencies. This results in undefined behavior if the dereferenced memory is mutated at any time between when the function is called and when it is exited.

There is a world where we keep track of these flags (in both loads and stores) and can trap at any time a store to a previously readonly'd memory location. But this is a good start.

Benefit

This gets us a better interpretation of CLIF semantics and allows whoever is using the interpreter to catch these cases sooner.

Implementation

• In State::checked_store check if the memory flags have the readonly flag set and return a MemoryError if so.

• We can't build the usual tests for this.

  • Our runtests don't support checking for traps (cc #4781).
  • What we can do instead is build a CLIF program and invoke the interpreter using the library API. See this example for a test case testing traps on srem, we can do something similar for load/store.

Alternatives

The current implementation is fine, and we should technically never see a store with a readonly flag.

Wasmtime GitHub notifications bot (Mar 01 2023 at 11:42):

bjorn3 commented on issue #5900:

Maybe disallow the readonly flag on stores in the clif ir verifier instead of the interpreter? Checking at runtime that no stores to readonly memory happen can't be done in the verifier, but checking that the readonly flag isn't set for stores can easily be checked in the verifier. I can't imagine any case why you would intentionally want the readonly flag on a store. It did unconditionally be UB, so you might as well insert the trap instruction instead.

Wasmtime GitHub notifications bot (Mar 01 2023 at 12:17):

afonso360 commented on issue #5900:

I think we had a similar situation here and I think a similar reasoning might apply? We can optimize other loads/stores such that we now statically know this memory region is readonly and thus we are now in the same situation where we get invalid CLIF by optimizing valid CLIF.

Wasmtime GitHub notifications bot (Mar 01 2023 at 12:24):

afonso360 edited a comment on issue #5900:

I think we had a similar situation here and I think a similar reasoning might apply? We can optimize other loads/stores such that we now statically know this memory region is readonly and thus we are now in the same situation where we get invalid CLIF by optimizing valid CLIF.

Edit: Although replacing with a trap is somewhat different, since it keeps it as legal IR. I'm not sure about this one.

Wasmtime GitHub notifications bot (Mar 01 2023 at 13:11):

bjorn3 commented on issue #5900:

The readonly flag applies to a specific instruction. If an optimization propagates readonly to other memory accesses, it has to explicitly set it for store operations. I don't think the same reasoning as #5167 applies as an access being in bounds of a memory region is a dynamic property where optimizations may cause more knowledge being gained, and only being known in the general case at runtime, while a store being marked as readonly is a static property that is always known at compile time. While the accesses memory region being readonly is a dynamic property, the actual flag on the store is a static property.

Wasmtime GitHub notifications bot (Jun 16 2023 at 18:10):

littlebenlittle commented on issue #5900:

Just opened a draft PR for this. The implementation is straightforward. To write a test case I need a little more info.

In the cranelift IR, what are the semantics of a read only store? Is it just "error", as in a conformant compiler should never do this?

The next issue is how to invoke a read only store using CLIF. How are MemFlags set? Is that the responsibility of the compiler or the programmer? If its on the programmer, i.e. read only stores are supported by CLIF, then this should be easy. If it's on the compiler, then we might need to write a non-conformant compiler and that's quite a bit more complex, I would assume.

Wasmtime GitHub notifications bot (Jun 16 2023 at 18:27):

jameysharp commented on issue #5900:

I like @bjorn3's suggestion in this thread that we should reject read-only stores in the verifier (cranelift/codegen/src/verifier/mod.rs), rather than in the interpreter. The MemFlags are a static property of the store instruction so we don't need to wait until we're running the program to check this. The aligned flag (#5899) is different because although the flag is static, the address is not.

I have to think some about @afonso360's comments, about whether optimizations could discover that a store is to a location which should be read-only. I'm not sure what optimization we might want to implement that would do that, and I think it's more likely that if there's a store after a read-only load we need to conclude that the load was wrong. But in any case this would still be an error that we should detect during or immediately after optimization, not at runtime.

As for how to test this: MemFlags are specified by whoever constructed the CLIF. That can be a frontend like Wasmtime or cg-clif, or it can be in the text representation; see cranelift/filetests/filetests/egraph/load-hoist.clif for an example with several readonly loads.

Wasmtime GitHub notifications bot (Jun 16 2023 at 18:39):

littlebenlittle commented on issue #5900:

Is this already implemented in the verifier on the main branch? [1]

There doesn't appear to be a test case, however.

[1] https://github.com/bytecodealliance/wasmtime/blob/62019b2e9bd931a73f8007c111d10ac3ac7d6c3b/cranelift/codegen/src/verifier/mod.rs#L1601

Wasmtime GitHub notifications bot (Jun 16 2023 at 18:40):

littlebenlittle edited a comment on issue #5900:

Is this already implemented in the verifier on the main branch?

https://github.com/bytecodealliance/wasmtime/blob/62019b2e9bd931a73f8007c111d10ac3ac7d6c3b/cranelift/codegen/src/verifier/mod.rs#L1601

There doesn't appear to be a test case, however.

Wasmtime GitHub notifications bot (Jun 16 2023 at 18:41):

jameysharp commented on issue #5900:

Hah, you're right!

Want to open a PR with a test case confirming that it works? There are other verifier tests in cranelift/filetests/filetests/verifier/.

Wasmtime GitHub notifications bot (Jun 16 2023 at 23:23):

jameysharp commented on issue #5900:

Closing this as @littlebenlittle just provided a test in #6593 showing that this already works the way we've decided it should. Thank you!

Wasmtime GitHub notifications bot (Jun 16 2023 at 23:23):

jameysharp closed issue #5900:

:wave: Hey,

Feature

After #5893 we now have access to MemFlags when loading and storing to memory. We should check the readonly flag, and always trap if there is a write to it.

Technically the current behaviour of ignoring it and loading correctly anyway is allowed.

Loads with this flag have no memory dependencies. This results in undefined behavior if the dereferenced memory is mutated at any time between when the function is called and when it is exited.

There is a world where we keep track of these flags (in both loads and stores) and can trap at any time a store to a previously readonly'd memory location. But this is a good start.

Benefit

This gets us a better interpretation of CLIF semantics and allows whoever is using the interpreter to catch these cases sooner.

Implementation

• In State::checked_store check if the memory flags have the readonly flag set and return a MemoryError if so.

• We can't build the usual tests for this.

  • Our runtests don't support checking for traps (cc #4781).
  • What we can do instead is build a CLIF program and invoke the interpreter using the library API. See this example for a test case testing traps on srem, we can do something similar for load/store.

Alternatives

The current implementation is fine, and we should technically never see a store with a readonly flag.

Last updated: Nov 22 2024 at 16:03 UTC