Use uint32_t for value passed to memmove and memset

[nchamzn]

trafficstars

On a 64 bit host the top bits of n here can be garbage (as the value is 32bit in the wasm memory).

I have promised @loganek I'll write some tests for this in tests/wamr-compiler but so far I haven't made a minimal reproduction, will update the PR when I've done that.

[wenyongh]

@nchamzn thanks for the PR to fix the issue, it is a good catch!

I think we had better fix it by changing the aot compiler since in wasm memory64 spec proposal, when a memory is 64bit, the length for the opcode memory.fill and memory.copy is u64 but not u32, so here changing the argument to uint32 may cause integer truncation and make the memset/memmov failed if the length is larger than UINT32_MAX. Refer to: https://github.com/WebAssembly/memory64/blob/main/proposals/memory64/Overview.md#overview https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/interpreter/wasm_interp_classic.c#L5651

I uploaded a patch for a reference, could you help check whether it is better (please feel free to change if issue is found):

diff --git a/core/iwasm/compilation/aot_emit_memory.c b/core/iwasm/compilation/aot_emit_memory.c
index eedc5420..caccaef9 100644
--- a/core/iwasm/compilation/aot_emit_memory.c
+++ b/core/iwasm/compilation/aot_emit_memory.c
@@ -1090,6 +1090,14 @@ aot_compile_op_memory_copy(AOTCompContext *comp_ctx, AOTFuncContext *func_ctx)
     if (!(dst_addr = check_bulk_memory_overflow(comp_ctx, func_ctx, dst, len)))
         return false;

+    if (comp_ctx->pointer_size == sizeof(uint64)) {
+        len = LLVMBuildZExt(comp_ctx->builder, len, I64_TYPE, "len64");
+        if (!len) {
+            aot_set_last_error("llvm build bitcast failed.");
+            return false;
+        }
+    }
+
     call_aot_memmove = comp_ctx->is_indirect_mode || comp_ctx->is_jit_mode;
     if (call_aot_memmove) {
         LLVMTypeRef param_types[3], ret_type, func_type, func_ptr_type;
@@ -1097,7 +1105,7 @@ aot_compile_op_memory_copy(AOTCompContext *comp_ctx, AOTFuncContext *func_ctx)

         param_types[0] = INT8_PTR_TYPE;
         param_types[1] = INT8_PTR_TYPE;
-        param_types[2] = I32_TYPE;
+        param_types[2] = INTPTR_T_TYPE;
         ret_type = INT8_PTR_TYPE;

         if (!(func_type = LLVMFunctionType(ret_type, param_types, 3, false))) {
@@ -1172,9 +1180,17 @@ aot_compile_op_memory_fill(AOTCompContext *comp_ctx, AOTFuncContext *func_ctx)
     if (!(dst_addr = check_bulk_memory_overflow(comp_ctx, func_ctx, dst, len)))
         return false;

+    if (comp_ctx->pointer_size == sizeof(uint64)) {
+        len = LLVMBuildZExt(comp_ctx->builder, len, I64_TYPE, "len64");
+        if (!len) {
+            aot_set_last_error("llvm build bitcast failed.");
+            return false;
+        }
+    }
+
     param_types[0] = INT8_PTR_TYPE;
     param_types[1] = I32_TYPE;
-    param_types[2] = I32_TYPE;
+    param_types[2] = INTPTR_T_TYPE;
     ret_type = INT8_PTR_TYPE;

     if (!(func_type = LLVMFunctionType(ret_type, param_types, 3, false))) {

[nchamzn]

Hi, thanks for looking - yes your suggestion looks good to me

[wenyongh]

Hi, thanks for looking - yes your suggestion looks good to me

Welcome, I submitted PR #3378 to fix it with more comments added. Could you help check again? I think maybe we can merge it first if it is good and don't block the AOT memory64 feature (#3362) that @loganek is enabling, and then you can also update the tests in tests/wamr-compiler.

[wenyongh]

Close this PR since the issue was fixed with https://github.com/bytecodealliance/wasm-micro-runtime/pull/3378.