Wasmer's LLVM compiler generates code to pass 64-bit value to `memory_copy`

[wakabat]

Hey so I was testing the latest wasmer code on Rust 1.91.1 in RISC-V 64 bit architecture. And I noticed that wasmer would throw HeapAccessOutOfBounds error randomly. Upon further investigation, I noticed that the checked_add in wasmer_vm::vmcontext::memory_copy would overflow, but the passed values are small and should not really overflow.

Unfortunately my code is deeply coupled in a complex project so I don't have a reproducible test case, but I did managed to dump some logs with notes for you to check out: https://gist.github.com/wakabat/372009f1adf5f16eb5e121013016e485 And I'm happy to provide more information if needed.

I have to mention that I don't really have a need for Rust 1.91.1, it just happened that we have Rust 1.91.1 handy for our architecture when this quest started. I tested a series of Rust compilers, Rust 1.89.0 and Rust 1.90.0 are fine, but Rust 1.91.1 would trigger the bug. And as the note hints, I think this problem has existed in wasmer for some time, it's just Rust 1.91.1 finally exposes the issue to us.

I have to say that I'm not 100% sure that this is an issue with wasmer, it could be that I'm missing something in our code. Either case, we could use a little help here. Let us know what you think, many thanks!

FYI: I actually tried modifying code here, adding LLVM code pieces to truncate / mask len to a 32-bit value, but this does not really work. Apart from handling Operator::MemoryCopy, is there other part of the code that would invoke wasmer_vm_memory32_copy?

[marxin]

Thanks for the report! Are you talking about running the native rustc compliler on the riscv64gc platform? If so, can you try running the Wasmer's LLVM compiler tests with the mentioned revision?

It would be great if you could come up with a smaller reproducer.

[wakabat]

Okay so I still don't have a reproducible case(maybe I can have one when we finished cleaning up our WIP code and open source it properly), but I have a theory of what is going on here.

We compile a big chunk of golang code into WASM, and then use wasmer to build riscv64 code from the big WASM binary. And the error happens when runtime.call64, a golang internal function does MemoryCopy, which then calls
wasmer_vm_memory32_copy or wasmer_vm_imported_memory32_copy.

It's worth noting that functions like wasmer_vm_memory32_copy are at an FFI boundary: the caller code is generated via wasmer's LLVM compiler from WASM opcode, the callee function is compiled via rustc into the target code.

If we use --compiler-debug-dir option of wasmer to dump the caller side LLVM IR code, the following code sequence could be found:

  %local_0.2 = phi i32 [ %18, %end3 ], [ %51, %if_then.if_end_crit_edge ]
  %53 = sub i32 %local_0.2, 64

  // ...

  %72 = add nuw nsw i64 %.pre-phi84, 88
  %mem_value_ptr27 = getelementptr i8, ptr %7, i64 %72
  %73 = load volatile i64, ptr %mem_value_ptr27, align 1, !tbaa !16
  %74 = trunc i64 %73 to i32
  %75 = load volatile i64, ptr %mem_value_ptr21, align 1, !tbaa !16
  %76 = trunc i64 %75 to i32
  call void @wasmer_vm_memory32_copy(ptr %0, i32 0, i32 %53, i32 %74, i32 %76)

This code looks all right, the last 3 parameters(dst, src, and len) are all 32-bit LLVM values. src and len are truncated to 32 bits from 64-bit values via trunc.

However, if we dig into generated RISC-V assembly in .o file(I'm not showing this part since it could be long and tedious to read, but feel free to investigate this yourself, you can just follow any code sequence generated for MemoryCopy opcode), the trunc part is missing, LLVM decides that it will strip out the trunc IR code, assuming that the callee function would only use 32-bit values in the corresponding registers.

The funny thing here is, when rustc compiles wasmer_vm_memory32_copy, rustc also assumes that the caller code would make sure upper 32 bits of those parameter registers are all zeros, it then just work with 64-bit registers. And the bug will then be trigger: both sides assume the other part would take care of the problems.

And it's really hard debating on which side has a bug: we are calling from wasmer's LLVM generated code into rustc compiled code, it might be the case that both sides have their own assumptions and are correct, but when we use it across FFI boundaries in wasmer, errors happen.

This commit fixes the bug for us. We just assume that full 64-bit values are passed over to Rust side, and then do the truncating manually.

I can only share what we found at here, and what works for us. I will let you decide if wasmer should incorporate this change or not.

Let me know if you have more questions.

[wakabat]

It's worth noting that for the same wasmer_vm_memory32_copy or wasmer_vm_imported_memory32_copy function:

• LLVM compiler assumes the last 3 parameters are 32 bit values:

  wasmer/lib/compiler-llvm/src/translator/intrinsics.rs

  Lines 983 to 1010 in 3c25cdb

  	memory_copy: module.add_function(
  	"wasmer_vm_memory32_copy",
  	void_ty.fn_type(
  	&[
  	ctx_ptr_ty_basic_md,
  	i32_ty_basic_md,
  	i32_ty_basic_md,
  	i32_ty_basic_md,
  	i32_ty_basic_md,
  	],
  	false,
  	),
  	None,
  	),
  	imported_memory_copy: module.add_function(
  	"wasmer_vm_imported_memory32_copy",
  	void_ty.fn_type(
  	&[
  	ctx_ptr_ty_basic_md,
  	i32_ty_basic_md,
  	i32_ty_basic_md,
  	i32_ty_basic_md,
  	i32_ty_basic_md,
  	],
  	false,
  	),
  	None,
  	),

• Singlepass compiler seems to assume that the last 3 parameters are 64 bit values: https://github.com/marxin/wasmer/blob/245fbc9a26b351cf3cdaed1c5fa5b7f2b16b5a24/lib/compiler-singlepass/src/codegen.rs#L2758-L2760

Am I missing something? Does it make sense that across FFI boundaries we should only use 64-bit values to avoid confusions like the above mentioned one?