Address cr comments and some refactor (#4439)

TianlongLiang · web-flow · commit 969072567a15 · 2025-07-02T20:19:42.000+08:00
diff --git a/core/iwasm/aot/aot_reloc.h b/core/iwasm/aot/aot_reloc.h
@@ -187,7 +187,7 @@ typedef struct {
 
 #if WASM_ENABLE_SHARED_HEAP != 0
 #define REG_SHARED_HEAP_SYM()                 \
-    REG_SYM(wasm_runtime_update_last_used_shared_heap),
+    REG_SYM(wasm_runtime_check_and_update_last_used_shared_heap),
 #else
 #define REG_SHARED_HEAP_SYM()
 #endif
diff --git a/core/iwasm/common/wasm_memory.c b/core/iwasm/common/wasm_memory.c
@@ -178,16 +178,16 @@ wasm_runtime_create_shared_heap(SharedHeapInitArgs *init_args)
     }
 
     size = align_uint(size, os_getpagesize());
-    heap->size = size;
-    heap->start_off_mem64 = UINT64_MAX - heap->size + 1;
-    heap->start_off_mem32 = UINT32_MAX - heap->size + 1;
-    heap->attached_count = 0;
-
     if (size > APP_HEAP_SIZE_MAX || size < APP_HEAP_SIZE_MIN) {
         LOG_WARNING("Invalid size of shared heap");
         goto fail2;
     }
 
+    heap->size = size;
+    heap->start_off_mem64 = UINT64_MAX - heap->size + 1;
+    heap->start_off_mem32 = UINT32_MAX - heap->size + 1;
+    heap->attached_count = 0;
+
     if (init_args->pre_allocated_addr != NULL) {
         /* Create shared heap from a pre allocated buffer, its size need to
          * align with system page */
@@ -275,6 +275,13 @@ wasm_runtime_chain_shared_heaps(WASMSharedHeap *head, WASMSharedHeap *body)
             os_mutex_unlock(&shared_heap_list_lock);
             return NULL;
         }
+        if (cur == head && cur->chain_next) {
+            LOG_WARNING(
+                "To create shared heap chain, the 'head' shared heap can't "
+                "already be the 'head' in another a chain");
+            os_mutex_unlock(&shared_heap_list_lock);
+            return NULL;
+        }
     }
     for (cur = body; cur; cur = cur->chain_next) {
         if (cur->heap_handle && heap_handle_exist) {
@@ -519,6 +526,10 @@ wasm_runtime_attach_shared_heap(WASMModuleInstanceCommon *module_inst,
 void
 wasm_runtime_detach_shared_heap_internal(WASMModuleInstanceCommon *module_inst)
 {
+    /* Reset shared_heap_end_off = UINT64/32_MAX - 1 to handling a corner case,
+      app_offset >= shared_heap_start && app_offset <= shared_heap_end-bytes+1
+      when bytes=1 and both e->shared_heap_start_off and e->shared_heap_end_off
+      is 0xffffffff */
 #if WASM_ENABLE_INTERP != 0
     if (module_inst->module_type == Wasm_Module_Bytecode) {
         WASMModuleInstanceExtra *e =
@@ -614,19 +625,17 @@ is_app_addr_in_shared_heap(WASMModuleInstanceCommon *module_inst,
     shared_heap_start =
         (uint64)get_last_used_shared_heap_start_offset(module_inst);
     shared_heap_end = (uint64)get_last_used_shared_heap_end_offset(module_inst);
-    if (app_offset >= shared_heap_start
-        && app_offset <= shared_heap_end - bytes + 1
-        && bytes - 1 <= shared_heap_end) {
+    if (bytes - 1 <= shared_heap_end && app_offset >= shared_heap_start
+        && app_offset <= shared_heap_end - bytes + 1) {
         return true;
     }
 
     /* Early stop for app start address not in the shared heap(chain) at all */
     shared_heap_start =
         is_memory64 ? heap->start_off_mem64 : heap->start_off_mem32;
     shared_heap_end = is_memory64 ? UINT64_MAX : UINT32_MAX;
-    if (app_offset < shared_heap_start
-        || app_offset > shared_heap_end - bytes + 1
-        || bytes - 1 > shared_heap_end) {
+    if (bytes - 1 > shared_heap_end || app_offset < shared_heap_start
+        || app_offset > shared_heap_end - bytes + 1) {
         goto fail;
     }
 
@@ -636,9 +645,8 @@ is_app_addr_in_shared_heap(WASMModuleInstanceCommon *module_inst,
         shared_heap_start =
             is_memory64 ? cur->start_off_mem64 : cur->start_off_mem32;
         shared_heap_end = shared_heap_start - 1 + cur->size;
-        if (app_offset >= shared_heap_start
-            && app_offset <= shared_heap_end - bytes + 1
-            && bytes - 1 <= shared_heap_end) {
+        if (bytes - 1 <= shared_heap_end && app_offset >= shared_heap_start
+            && app_offset <= shared_heap_end - bytes + 1) {
             update_last_used_shared_heap(module_inst, cur, is_memory64);
             return true;
         }
@@ -1075,7 +1083,7 @@ wasm_runtime_validate_app_str_addr(WASMModuleInstanceCommon *module_inst_comm,
         shared_heap_base_addr_adj =
             (char *)get_last_used_shared_heap_base_addr_adj(module_inst_comm);
         str = shared_heap_base_addr_adj + app_str_offset;
-        str_end = shared_heap_base_addr_adj + shared_heap_end_off;
+        str_end = shared_heap_base_addr_adj + shared_heap_end_off + 1;
     }
     else
 #endif
@@ -1358,7 +1366,8 @@ wasm_check_app_addr_and_convert(WASMModuleInstance *module_inst, bool is_str,
 
         /* The whole string must be in the shared heap */
         str = (const char *)native_addr;
-        str_end = (const char *)shared_heap_base_addr_adj + shared_heap_end_off;
+        str_end =
+            (const char *)shared_heap_base_addr_adj + shared_heap_end_off + 1;
         while (str < str_end && *str != '\0')
             str++;
         if (str == str_end) {
diff --git a/core/iwasm/common/wasm_runtime_common.c b/core/iwasm/common/wasm_runtime_common.c
@@ -7886,12 +7886,10 @@ wasm_runtime_is_underlying_binary_freeable(WASMModuleCommon *const module)
 
 #if WASM_ENABLE_SHARED_HEAP != 0
 bool
-wasm_runtime_update_last_used_shared_heap(WASMModuleInstanceCommon *module_inst,
-                                          uintptr_t app_offset, size_t bytes,
-                                          uintptr_t *shared_heap_start_off_p,
-                                          uintptr_t *shared_heap_end_off_p,
-                                          uint8 **shared_heap_base_addr_adj_p,
-                                          bool is_memory64)
+wasm_runtime_check_and_update_last_used_shared_heap(
+    WASMModuleInstanceCommon *module_inst, uintptr_t app_offset, size_t bytes,
+    uintptr_t *shared_heap_start_off_p, uintptr_t *shared_heap_end_off_p,
+    uint8 **shared_heap_base_addr_adj_p, bool is_memory64)
 {
     WASMSharedHeap *heap = wasm_runtime_get_shared_heap(module_inst), *cur;
     uint64 shared_heap_start, shared_heap_end;
@@ -7906,9 +7904,8 @@ wasm_runtime_update_last_used_shared_heap(WASMModuleInstanceCommon *module_inst,
         shared_heap_start =
             is_memory64 ? cur->start_off_mem64 : cur->start_off_mem32;
         shared_heap_end = shared_heap_start - 1 + cur->size;
-        if (app_offset >= shared_heap_start
-            && app_offset <= shared_heap_end - bytes + 1
-            && bytes - 1 <= shared_heap_end) {
+        if (bytes - 1 <= shared_heap_end && app_offset >= shared_heap_start
+            && app_offset <= shared_heap_end - bytes + 1) {
             *shared_heap_start_off_p = (uintptr_t)shared_heap_start;
             *shared_heap_end_off_p = (uintptr_t)shared_heap_end;
             *shared_heap_base_addr_adj_p =
diff --git a/core/iwasm/common/wasm_runtime_common.h b/core/iwasm/common/wasm_runtime_common.h
@@ -1338,12 +1338,10 @@ wasm_runtime_set_linux_perf(bool flag);
 
 #if WASM_ENABLE_SHARED_HEAP != 0
 bool
-wasm_runtime_update_last_used_shared_heap(WASMModuleInstanceCommon *module_inst,
-                                          uintptr_t app_offset, size_t bytes,
-                                          uintptr_t *shared_heap_start_off_p,
-                                          uintptr_t *shared_heap_end_off_p,
-                                          uint8 **shared_heap_base_addr_adj_p,
-                                          bool is_memory64);
+wasm_runtime_check_and_update_last_used_shared_heap(
+    WASMModuleInstanceCommon *module_inst, uintptr_t app_offset, size_t bytes,
+    uintptr_t *shared_heap_start_off_p, uintptr_t *shared_heap_end_off_p,
+    uint8 **shared_heap_base_addr_adj_p, bool is_memory64);
 #endif
 
 #ifdef __cplusplus
diff --git a/core/iwasm/compilation/aot_emit_memory.c b/core/iwasm/compilation/aot_emit_memory.c
@@ -162,10 +162,11 @@ get_module_inst_extra_offset(AOTCompContext *comp_ctx);
  * 1. shared_heap_start_off 2. shared_heap_end_off 3. shared_heap_base_addr_adj
  */
 bool
-aot_check_shared_heap_chain(AOTCompContext *comp_ctx, AOTFuncContext *func_ctx,
-                            LLVMBasicBlockRef check_succ,
-                            LLVMValueRef start_offset, LLVMValueRef bytes,
-                            bool is_memory64)
+aot_check_shared_heap_chain_and_update(AOTCompContext *comp_ctx,
+                                       AOTFuncContext *func_ctx,
+                                       LLVMBasicBlockRef check_succ,
+                                       LLVMValueRef start_offset,
+                                       LLVMValueRef bytes, bool is_memory64)
 {
     LLVMValueRef param_values[7], ret_value, func, value, cmp;
     LLVMTypeRef param_types[7], ret_type, func_type, func_ptr_type;
@@ -179,7 +180,7 @@ aot_check_shared_heap_chain(AOTCompContext *comp_ctx, AOTFuncContext *func_ctx,
     param_types[6] = INT8_TYPE;
     ret_type = INT8_TYPE;
 
-    GET_AOT_FUNCTION(wasm_runtime_update_last_used_shared_heap, 7);
+    GET_AOT_FUNCTION(wasm_runtime_check_and_update_last_used_shared_heap, 7);
 
     /* Call function */
     param_values[0] = func_ctx->aot_inst;
@@ -285,7 +286,7 @@ build_check_app_addr_in_shared_heap_chain(
     /* Use start_offset > func_ctx->shared_heap_head_start_off to test
      * start_off falls in shared heap chain memory region. The shared heap
      * chain oob will be detected in app_addr_in_shared_heap block or
-     * aot_check_shared_heap_chain function
+     * aot_check_shared_heap_chain_and_update function
      */
     BUILD_ICMP(LLVMIntUGT, start_offset, func_ctx->shared_heap_head_start_off,
                is_in_shared_heap, "shared_heap_lb_cmp");
@@ -327,9 +328,9 @@ build_shared_heap_conditional_branching(
         BUILD_COND_BR(cmp, app_addr_in_cache_shared_heap,
                       check_shared_heap_chain);
         SET_BUILD_POS(check_shared_heap_chain);
-        if (!aot_check_shared_heap_chain(comp_ctx, func_ctx,
-                                         app_addr_in_cache_shared_heap,
-                                         start_offset, bytes, is_memory64))
+        if (!aot_check_shared_heap_chain_and_update(
+                comp_ctx, func_ctx, app_addr_in_cache_shared_heap, start_offset,
+                bytes, is_memory64))
             goto fail;
     }
     return true;
diff --git a/core/iwasm/compilation/aot_llvm.c b/core/iwasm/compilation/aot_llvm.c
@@ -1648,9 +1648,10 @@ create_shared_heap_info(AOTCompContext *comp_ctx, AOTFuncContext *func_ctx)
 
     /* if there is attached shared heap(s), the value will be valid start_off-1,
      * otherwise it will be UINT32_MAX/UINT64_MAX, so during the bounds checks,
-     * when has attached shared heap: offset > start_off - 1 => offset >= offset
-     * when no attached shared heap: offset > UINT32_MAX/UINT64_MAX is always
-     * false
+     * when has attached shared heap:
+     *   offset > start_off - 1 => offset >= start_off
+     * when no attached shared heap:
+     *   offset > UINT32_MAX/UINT64_MAX is always false
      * */
     if (!(func_ctx->shared_heap_head_start_off = LLVMBuildSelect(
               comp_ctx->builder, cmp, shared_heap_head_start_off_minus_one,
diff --git a/samples/shared-heap/src/shared_heap_chain.c b/samples/shared-heap/src/shared_heap_chain.c
@@ -24,7 +24,7 @@ produce_data(wasm_module_inst_t module_inst, wasm_exec_env_t exec_env,
                wasm_runtime_get_exception(module_inst));
         return false;
     }
-    if (free_on_fail && argv[0] == 0) {
+    if (argv[0] == 0) {
         printf("Failed to allocate memory from shared heap\n");
         return false;
     }
diff --git a/tests/unit/shared-heap/shared_heap_test.cc b/tests/unit/shared-heap/shared_heap_test.cc
@@ -35,7 +35,7 @@ static bool
 load_wasm(char *wasm_file_tested, unsigned int app_heap_size,
           ret_env &ret_module_env)
 {
-    const char *wasm_file = strdup(wasm_file_tested);
+    char *wasm_file = strdup(wasm_file_tested);
     unsigned int wasm_file_size = 0;
     unsigned int stack_size = 16 * 1024, heap_size = app_heap_size;
     char error_buf[128] = { 0 };
@@ -68,8 +68,10 @@ load_wasm(char *wasm_file_tested, unsigned int app_heap_size,
         goto fail;
     }
 
+    free(wasm_file);
     return true;
 fail:
+    free(wasm_file);
     destroy_module_env(ret_module_env);
     return false;
 }
diff --git a/tests/unit/shared-heap/wasm-apps/test_addr_conv.c b/tests/unit/shared-heap/wasm-apps/test_addr_conv.c
@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
  */
 
-#define NULL 0
+#include <stdio.h>
 
 extern void *
 shared_heap_malloc(int size);

Original file line number	Diff line number	Diff line change
`@@ -24,7 +24,7 @@ produce_data(wasm_module_inst_t module_inst, wasm_exec_env_t exec_env,`
`24`	`24`	`wasm_runtime_get_exception(module_inst));`
`25`	`25`	`return false;`
`26`	`26`	`}`
`27`		`- if (free_on_fail && argv[0] == 0) {`
	`27`	`+ if (argv[0] == 0) {`
`28`	`28`	`printf("Failed to allocate memory from shared heap\n");`
`29`	`29`	`return false;`
`30`	`30`	`}`
Original file line number	Diff line number	Diff line change
`@@ -35,7 +35,7 @@ static bool`
`35`	`35`	`load_wasm(char *wasm_file_tested, unsigned int app_heap_size,`
`36`	`36`	`ret_env &ret_module_env)`
`37`	`37`	`{`
`38`		`- const char *wasm_file = strdup(wasm_file_tested);`
	`38`	`+ char *wasm_file = strdup(wasm_file_tested);`
`39`	`39`	`unsigned int wasm_file_size = 0;`
`40`	`40`	`unsigned int stack_size = 16 * 1024, heap_size = app_heap_size;`
`41`	`41`	`char error_buf[128] = { 0 };`
`@@ -68,8 +68,10 @@ load_wasm(char *wasm_file_tested, unsigned int app_heap_size,`
`68`	`68`	`goto fail;`
`69`	`69`	`}`
`70`	`70`
	`71`	`+ free(wasm_file);`
`71`	`72`	`return true;`
`72`	`73`	`fail:`
	`74`	`+ free(wasm_file);`
`73`	`75`	`destroy_module_env(ret_module_env);`
`74`	`76`	`return false;`
`75`	`77`	`}`