Stream: git-wasmtime

Topic: wasmtime / issue #5899 cranelift-interpreter: Trap on una...

Wasmtime GitHub notifications bot (Mar 01 2023 at 10:53):

afonso360 opened issue #5899:

:wave: Hey,

Feature

After #5893 we now have access to MemFlags when loading and storing to memory. We should check the aligned flag, and always trap if the access is unaligned.

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

By default, Cranelift memory instructions work with any unaligned effective address. If the aligned flag is set, the instruction is permitted to trap or return a wrong result if the effective address is misaligned.

But, trapping in these cases seems like the best course of action because the CLIF is invalid/undefined.

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_{load,store} check if the memory flags have the aligned flag set.
• If they do and the alignment of the address is smaller than the alignment of the type. We should return a MemoryError
  • The interpreter uses a virtual addressing scheme, so we don't have proper "alignment", but we can use the offset field in the address which should be good enough!
• 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

Fuzzgen never sets this flag wrongly (I hope), so this shouldn't affect the fuzzer. And the current implementation is legal so we can always just keep it.

Wasmtime GitHub notifications bot (Mar 01 2023 at 10:54):

afonso360 labeled issue #5899:

:wave: Hey,

Feature

After #5893 we now have access to MemFlags when loading and storing to memory. We should check the aligned flag, and always trap if the access is unaligned.

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

By default, Cranelift memory instructions work with any unaligned effective address. If the aligned flag is set, the instruction is permitted to trap or return a wrong result if the effective address is misaligned.

But, trapping in these cases seems like the best course of action because the CLIF is invalid/undefined.

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_{load,store} check if the memory flags have the aligned flag set.
• If they do and the alignment of the address is smaller than the alignment of the type. We should return a MemoryError
  • The interpreter uses a virtual addressing scheme, so we don't have proper "alignment", but we can use the offset field in the address which should be good enough!
• 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

Fuzzgen never sets this flag wrongly (I hope), so this shouldn't affect the fuzzer. And the current implementation is legal so we can always just keep it.

Wasmtime GitHub notifications bot (Mar 01 2023 at 10:54):

afonso360 labeled issue #5899:

:wave: Hey,

Feature

After #5893 we now have access to MemFlags when loading and storing to memory. We should check the aligned flag, and always trap if the access is unaligned.

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

By default, Cranelift memory instructions work with any unaligned effective address. If the aligned flag is set, the instruction is permitted to trap or return a wrong result if the effective address is misaligned.

But, trapping in these cases seems like the best course of action because the CLIF is invalid/undefined.

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_{load,store} check if the memory flags have the aligned flag set.
• If they do and the alignment of the address is smaller than the alignment of the type. We should return a MemoryError
  • The interpreter uses a virtual addressing scheme, so we don't have proper "alignment", but we can use the offset field in the address which should be good enough!
• 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

Fuzzgen never sets this flag wrongly (I hope), so this shouldn't affect the fuzzer. And the current implementation is legal so we can always just keep it.

Wasmtime GitHub notifications bot (Mar 01 2023 at 10:54):

afonso360 labeled issue #5899:

:wave: Hey,

Feature

After #5893 we now have access to MemFlags when loading and storing to memory. We should check the aligned flag, and always trap if the access is unaligned.

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

By default, Cranelift memory instructions work with any unaligned effective address. If the aligned flag is set, the instruction is permitted to trap or return a wrong result if the effective address is misaligned.

But, trapping in these cases seems like the best course of action because the CLIF is invalid/undefined.

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_{load,store} check if the memory flags have the aligned flag set.
• If they do and the alignment of the address is smaller than the alignment of the type. We should return a MemoryError
  • The interpreter uses a virtual addressing scheme, so we don't have proper "alignment", but we can use the offset field in the address which should be good enough!
• 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

Fuzzgen never sets this flag wrongly (I hope), so this shouldn't affect the fuzzer. And the current implementation is legal so we can always just keep it.

Wasmtime GitHub notifications bot (Mar 26 2023 at 12:20):

meithecatte commented on issue #5899:

Shouldn't this be closed, considering #5921 got merged?

Wasmtime GitHub notifications bot (Mar 26 2023 at 14:20):

afonso360 commented on issue #5899:

That's right, thanks for the reminder!

Wasmtime GitHub notifications bot (Mar 26 2023 at 14:20):

afonso360 closed issue #5899:

:wave: Hey,

Feature

After #5893 we now have access to MemFlags when loading and storing to memory. We should check the aligned flag, and always trap if the access is unaligned.

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

By default, Cranelift memory instructions work with any unaligned effective address. If the aligned flag is set, the instruction is permitted to trap or return a wrong result if the effective address is misaligned.

But, trapping in these cases seems like the best course of action because the CLIF is invalid/undefined.

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_{load,store} check if the memory flags have the aligned flag set.
• If they do and the alignment of the address is smaller than the alignment of the type. We should return a MemoryError
  • The interpreter uses a virtual addressing scheme, so we don't have proper "alignment", but we can use the offset field in the address which should be good enough!
• 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

Fuzzgen never sets this flag wrongly (I hope), so this shouldn't affect the fuzzer. And the current implementation is legal so we can always just keep it.

Last updated: Nov 22 2024 at 16:03 UTC