From fd5cfe42e1f7786480b17aa4c9fe8d08ff3a8f16 Mon Sep 17 00:00:00 2001 From: Jia Liu Date: Tue, 17 Jun 2025 11:05:04 +0800 Subject: [PATCH 01/10] Follow-up to PR #4300: prevent potential overflow PR #4300 introduced the rationale for validating heap_type. This patch moves the validation before the computation of type1 to prevent potential overflow. --- core/iwasm/interpreter/wasm_loader.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/core/iwasm/interpreter/wasm_loader.c b/core/iwasm/interpreter/wasm_loader.c index fe045c6ea9..788a12cfb3 100644 --- a/core/iwasm/interpreter/wasm_loader.c +++ b/core/iwasm/interpreter/wasm_loader.c @@ -843,12 +843,28 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, #else int32 heap_type; read_leb_int32(p, p_end, heap_type); + + /* Validate heap_type before computing type1 to prevent + * potential overflow. */ + if (heap_type >= 0) { + if (!check_type_index(module, module->type_count, heap_type, + error_buf, error_buf_size)) { + goto fail; + } + } + else { + if (!wasm_is_valid_heap_type(heap_type)) { + set_error_buf(error_buf, error_buf_size, + "unknown type"); + goto fail; + } + } + type1 = (uint8)((int32)0x80 + heap_type); cur_value.gc_obj = NULL_REF; if (!is_byte_a_type(type1) - || !wasm_is_valid_heap_type(heap_type) || wasm_is_type_multi_byte_type(type1)) { p--; read_leb_uint32(p, p_end, type_idx); From d0aac00d7c6ad6a6900a204a12215194c097cc24 Mon Sep 17 00:00:00 2001 From: Jia Liu Date: Tue, 17 Jun 2025 11:35:10 +0800 Subject: [PATCH 02/10] improve error message --- core/iwasm/interpreter/wasm_loader.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/iwasm/interpreter/wasm_loader.c b/core/iwasm/interpreter/wasm_loader.c index 4472d68986..9b302b74f2 100644 --- a/core/iwasm/interpreter/wasm_loader.c +++ b/core/iwasm/interpreter/wasm_loader.c @@ -854,8 +854,8 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, } else { if (!wasm_is_valid_heap_type(heap_type)) { - set_error_buf(error_buf, error_buf_size, - "unknown type"); + set_error_buf_v(error_buf, error_buf_size, + "unknown type %d", heap_type); goto fail; } } From 58aae86fbcb32d60faa4cf74d27602c8bc2d7a25 Mon Sep 17 00:00:00 2001 From: Jia Liu Date: Tue, 17 Jun 2025 17:45:12 +0800 Subject: [PATCH 03/10] Refine heap_type validation and corresponding type1 calculation --- core/iwasm/interpreter/wasm_loader.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/iwasm/interpreter/wasm_loader.c b/core/iwasm/interpreter/wasm_loader.c index 9b302b74f2..27a7dd9d09 100644 --- a/core/iwasm/interpreter/wasm_loader.c +++ b/core/iwasm/interpreter/wasm_loader.c @@ -844,13 +844,14 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, int32 heap_type; read_leb_int32(p, p_end, heap_type); - /* Validate heap_type before computing type1 to prevent - * potential overflow. */ if (heap_type >= 0) { if (!check_type_index(module, module->type_count, heap_type, error_buf, error_buf_size)) { goto fail; } + wasm_set_refheaptype_typeidx(&cur_ref_type.ref_ht_typeidx, + true, heap_type); + type1 = cur_ref_type.ref_type; } else { if (!wasm_is_valid_heap_type(heap_type)) { @@ -858,10 +859,9 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, "unknown type %d", heap_type); goto fail; } + type1 = (uint8)((int32)0x80 + heap_type); } - type1 = (uint8)((int32)0x80 + heap_type); - cur_value.gc_obj = NULL_REF; if (!is_byte_a_type(type1) From 038e2fce51b628eed5d37a60f682a1bb61c611d0 Mon Sep 17 00:00:00 2001 From: Jia Liu Date: Wed, 18 Jun 2025 17:52:41 +0800 Subject: [PATCH 04/10] Removed some redundant logic --- core/iwasm/interpreter/wasm_loader.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/core/iwasm/interpreter/wasm_loader.c b/core/iwasm/interpreter/wasm_loader.c index 27a7dd9d09..c70f0711a7 100644 --- a/core/iwasm/interpreter/wasm_loader.c +++ b/core/iwasm/interpreter/wasm_loader.c @@ -866,14 +866,6 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, if (!is_byte_a_type(type1) || wasm_is_type_multi_byte_type(type1)) { - p--; - read_leb_uint32(p, p_end, type_idx); - if (!check_type_index(module, module->type_count, type_idx, - error_buf, error_buf_size)) - goto fail; - - wasm_set_refheaptype_typeidx(&cur_ref_type.ref_ht_typeidx, - true, type_idx); if (!push_const_expr_stack(&const_expr_ctx, flag, cur_ref_type.ref_type, &cur_ref_type, 0, &cur_value, From 0020653d36303fd2f02be5db7548946ca24b6c2f Mon Sep 17 00:00:00 2001 From: Jia Liu Date: Fri, 20 Jun 2025 11:24:10 +0800 Subject: [PATCH 05/10] Merged the if-else conditions --- core/iwasm/interpreter/wasm_loader.c | 37 +++++++++++++++++++--------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/core/iwasm/interpreter/wasm_loader.c b/core/iwasm/interpreter/wasm_loader.c index c70f0711a7..8e698ee757 100644 --- a/core/iwasm/interpreter/wasm_loader.c +++ b/core/iwasm/interpreter/wasm_loader.c @@ -843,6 +843,7 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, #else int32 heap_type; read_leb_int32(p, p_end, heap_type); + cur_value.gc_obj = NULL_REF; if (heap_type >= 0) { if (!check_type_index(module, module->type_count, heap_type, @@ -852,6 +853,21 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, wasm_set_refheaptype_typeidx(&cur_ref_type.ref_ht_typeidx, true, heap_type); type1 = cur_ref_type.ref_type; + + if (!is_byte_a_type(type1) + || wasm_is_type_multi_byte_type(type1)) { + if (!push_const_expr_stack(&const_expr_ctx, flag, + cur_ref_type.ref_type, + &cur_ref_type, 0, &cur_value, + error_buf, error_buf_size)) + goto fail; + } + else { + if (!push_const_expr_stack(&const_expr_ctx, flag, type1, + NULL, 0, &cur_value, + error_buf, error_buf_size)) + goto fail; + } } else { if (!wasm_is_valid_heap_type(heap_type)) { @@ -859,20 +875,17 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, "unknown type %d", heap_type); goto fail; } + /* + * When heap_type < 0 and wasm_is_valid_heap_type(heap_type) + * is true, under the current implementation, the condition + * (!is_byte_a_type(type1) || + * wasm_is_type_multi_byte_type(type1)) will always be + * false, so there is no need to check_type_index here. If + * the implementation changes in the future, this check may + * be needed. + */ type1 = (uint8)((int32)0x80 + heap_type); - } - - cur_value.gc_obj = NULL_REF; - if (!is_byte_a_type(type1) - || wasm_is_type_multi_byte_type(type1)) { - if (!push_const_expr_stack(&const_expr_ctx, flag, - cur_ref_type.ref_type, - &cur_ref_type, 0, &cur_value, - error_buf, error_buf_size)) - goto fail; - } - else { if (!push_const_expr_stack(&const_expr_ctx, flag, type1, NULL, 0, &cur_value, error_buf, error_buf_size)) From 17ac8c179991091a987e93e16ef5d1940bf03a14 Mon Sep 17 00:00:00 2001 From: Jia Liu Date: Wed, 25 Jun 2025 15:31:35 +0800 Subject: [PATCH 06/10] improve logic and comments --- core/iwasm/interpreter/wasm_loader.c | 37 +++++++++++++--------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/core/iwasm/interpreter/wasm_loader.c b/core/iwasm/interpreter/wasm_loader.c index 8e698ee757..52309ce9bf 100644 --- a/core/iwasm/interpreter/wasm_loader.c +++ b/core/iwasm/interpreter/wasm_loader.c @@ -854,20 +854,17 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, true, heap_type); type1 = cur_ref_type.ref_type; - if (!is_byte_a_type(type1) - || wasm_is_type_multi_byte_type(type1)) { - if (!push_const_expr_stack(&const_expr_ctx, flag, - cur_ref_type.ref_type, - &cur_ref_type, 0, &cur_value, - error_buf, error_buf_size)) - goto fail; - } - else { - if (!push_const_expr_stack(&const_expr_ctx, flag, type1, - NULL, 0, &cur_value, - error_buf, error_buf_size)) - goto fail; - } + /* + * Since wasm_set_refheaptype_typeidx(...) always sets type1 + * to REF_TYPE_HT_NULLABLE, the condition (!is_byte_a_type + * || wasm_is_type_multi_byte_byte()) is always true. Thus, + * this validation is no longer necessary and has been + * removed. + */ + if (!push_const_expr_stack(&const_expr_ctx, flag, type1, + &cur_ref_type, 0, &cur_value, + error_buf, error_buf_size)) + goto fail; } else { if (!wasm_is_valid_heap_type(heap_type)) { @@ -876,13 +873,13 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, goto fail; } /* - * When heap_type < 0 and wasm_is_valid_heap_type(heap_type) - * is true, under the current implementation, the condition + * When heap_type < 0, there is no need to call + * check_type_index, and the condition * (!is_byte_a_type(type1) || - * wasm_is_type_multi_byte_type(type1)) will always be - * false, so there is no need to check_type_index here. If - * the implementation changes in the future, this check may - * be needed. + * wasm_is_type_multi_byte_type(type1)) is always false. + * Therefore, for both reasons, check_type_index is + * unnecessary here. If the implementation changes in the + * future, this check may be needed. */ type1 = (uint8)((int32)0x80 + heap_type); From f297e0821ea1adcd515f9482d69d39c66a3af2f0 Mon Sep 17 00:00:00 2001 From: Jia Liu Date: Fri, 27 Jun 2025 11:14:52 +0800 Subject: [PATCH 07/10] revert changes, read heap_type by p_copy --- core/iwasm/interpreter/wasm_loader.c | 52 ++++++++++++---------------- 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/core/iwasm/interpreter/wasm_loader.c b/core/iwasm/interpreter/wasm_loader.c index 52309ce9bf..dd5881aea3 100644 --- a/core/iwasm/interpreter/wasm_loader.c +++ b/core/iwasm/interpreter/wasm_loader.c @@ -830,19 +830,21 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, case INIT_EXPR_TYPE_REFNULL_CONST: { uint8 type1; - -#if WASM_ENABLE_GC == 0 +#if WASM_ENABLE_GC != 0 + const uint8 *p_copy = p; + int32 heap_type; + read_leb_int32(p_copy, p_end, heap_type); +#endif CHECK_BUF(p, p_end, 1); type1 = read_uint8(p); +#if WASM_ENABLE_GC == 0 cur_value.ref_index = NULL_REF; if (!push_const_expr_stack(&const_expr_ctx, flag, type1, &cur_value, error_buf, error_buf_size)) goto fail; #else - int32 heap_type; - read_leb_int32(p, p_end, heap_type); cur_value.gc_obj = NULL_REF; if (heap_type >= 0) { @@ -850,21 +852,6 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, error_buf, error_buf_size)) { goto fail; } - wasm_set_refheaptype_typeidx(&cur_ref_type.ref_ht_typeidx, - true, heap_type); - type1 = cur_ref_type.ref_type; - - /* - * Since wasm_set_refheaptype_typeidx(...) always sets type1 - * to REF_TYPE_HT_NULLABLE, the condition (!is_byte_a_type - * || wasm_is_type_multi_byte_byte()) is always true. Thus, - * this validation is no longer necessary and has been - * removed. - */ - if (!push_const_expr_stack(&const_expr_ctx, flag, type1, - &cur_ref_type, 0, &cur_value, - error_buf, error_buf_size)) - goto fail; } else { if (!wasm_is_valid_heap_type(heap_type)) { @@ -872,17 +859,24 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, "unknown type %d", heap_type); goto fail; } - /* - * When heap_type < 0, there is no need to call - * check_type_index, and the condition - * (!is_byte_a_type(type1) || - * wasm_is_type_multi_byte_type(type1)) is always false. - * Therefore, for both reasons, check_type_index is - * unnecessary here. If the implementation changes in the - * future, this check may be needed. - */ - type1 = (uint8)((int32)0x80 + heap_type); + } + if (!is_byte_a_type(type1) + || wasm_is_type_multi_byte_type(type1)) { + p--; + read_leb_uint32(p, p_end, type_idx); + if (!check_type_index(module, module->type_count, type_idx, + error_buf, error_buf_size)) + goto fail; + wasm_set_refheaptype_typeidx(&cur_ref_type.ref_ht_typeidx, + true, type_idx); + if (!push_const_expr_stack(&const_expr_ctx, flag, + cur_ref_type.ref_type, + &cur_ref_type, 0, &cur_value, + error_buf, error_buf_size)) + goto fail; + } + else { if (!push_const_expr_stack(&const_expr_ctx, flag, type1, NULL, 0, &cur_value, error_buf, error_buf_size)) From 54bed4700c9d5f15e7bc54920c8727a7083da395 Mon Sep 17 00:00:00 2001 From: Jia Liu Date: Fri, 27 Jun 2025 11:32:39 +0800 Subject: [PATCH 08/10] add comments --- core/iwasm/interpreter/wasm_loader.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/core/iwasm/interpreter/wasm_loader.c b/core/iwasm/interpreter/wasm_loader.c index dd5881aea3..39ea523ccc 100644 --- a/core/iwasm/interpreter/wasm_loader.c +++ b/core/iwasm/interpreter/wasm_loader.c @@ -847,6 +847,11 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, #else cur_value.gc_obj = NULL_REF; + /* + * According to the current GC SPEC rules, the heap_type must be + * validated when ref.null is used. It can be an absheaptype, + * or the type C.types[typeidx] must be defined in the context. + */ if (heap_type >= 0) { if (!check_type_index(module, module->type_count, heap_type, error_buf, error_buf_size)) { From 5c7a469f65dcddaf3683c67f9fda5313ce65ff9b Mon Sep 17 00:00:00 2001 From: Jia Liu Date: Wed, 2 Jul 2025 16:28:16 +0800 Subject: [PATCH 09/10] revert code and add comments --- core/iwasm/interpreter/wasm_loader.c | 59 ++++++++++++++-------------- 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/core/iwasm/interpreter/wasm_loader.c b/core/iwasm/interpreter/wasm_loader.c index 39ea523ccc..4ecb92c9a9 100644 --- a/core/iwasm/interpreter/wasm_loader.c +++ b/core/iwasm/interpreter/wasm_loader.c @@ -829,52 +829,44 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, /* ref.null */ case INIT_EXPR_TYPE_REFNULL_CONST: { +#if WASM_ENABLE_GC == 0 uint8 type1; -#if WASM_ENABLE_GC != 0 - const uint8 *p_copy = p; - int32 heap_type; - read_leb_int32(p_copy, p_end, heap_type); -#endif CHECK_BUF(p, p_end, 1); type1 = read_uint8(p); - -#if WASM_ENABLE_GC == 0 cur_value.ref_index = NULL_REF; if (!push_const_expr_stack(&const_expr_ctx, flag, type1, &cur_value, error_buf, error_buf_size)) goto fail; #else - cur_value.gc_obj = NULL_REF; - /* * According to the current GC SPEC rules, the heap_type must be * validated when ref.null is used. It can be an absheaptype, - * or the type C.types[typeidx] must be defined in the context. + * or the type C.types[type_idx] must be defined in the context. + */ + int32 heap_type; + read_leb_int32(p, p_end, heap_type); + cur_value.gc_obj = NULL_REF; + + /* + * The current check of heap_type can deterministically infer + * the result of the previous condition + * `(!is_byte_a_type(type1) || + * wasm_is_type_multi_byte_type(type1))`. Therefore, the + * original condition is redundant and has been removed. + * + * This logic is consistent with the implementation of the + * `WASM_OP_REF_NULL` case in the `wasm_loader_prepare_bytecode` + * function. */ + if (heap_type >= 0) { if (!check_type_index(module, module->type_count, heap_type, error_buf, error_buf_size)) { goto fail; } - } - else { - if (!wasm_is_valid_heap_type(heap_type)) { - set_error_buf_v(error_buf, error_buf_size, - "unknown type %d", heap_type); - goto fail; - } - } - - if (!is_byte_a_type(type1) - || wasm_is_type_multi_byte_type(type1)) { - p--; - read_leb_uint32(p, p_end, type_idx); - if (!check_type_index(module, module->type_count, type_idx, - error_buf, error_buf_size)) - goto fail; wasm_set_refheaptype_typeidx(&cur_ref_type.ref_ht_typeidx, - true, type_idx); + true, heap_type); if (!push_const_expr_stack(&const_expr_ctx, flag, cur_ref_type.ref_type, &cur_ref_type, 0, &cur_value, @@ -882,9 +874,16 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, goto fail; } else { - if (!push_const_expr_stack(&const_expr_ctx, flag, type1, - NULL, 0, &cur_value, error_buf, - error_buf_size)) + if (!wasm_is_valid_heap_type(heap_type)) { + set_error_buf_v(error_buf, error_buf_size, + "unknown type %d", heap_type); + goto fail; + } + cur_ref_type.ref_ht_common.ref_type = + (uint8)((int32)0x80 + heap_type); + if (!push_const_expr_stack( + &const_expr_ctx, flag, cur_ref_type.ref_type, NULL, + 0, &cur_value, error_buf, error_buf_size)) goto fail; } #endif From 4055c1ff6bfe7e1531e0d4ff984914fcb0c61d55 Mon Sep 17 00:00:00 2001 From: Jia Liu Date: Wed, 9 Jul 2025 17:06:55 +0800 Subject: [PATCH 10/10] merge --- core/iwasm/interpreter/wasm_loader.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/core/iwasm/interpreter/wasm_loader.c b/core/iwasm/interpreter/wasm_loader.c index ed42f6bf11..ff4ac44336 100644 --- a/core/iwasm/interpreter/wasm_loader.c +++ b/core/iwasm/interpreter/wasm_loader.c @@ -1057,11 +1057,9 @@ load_init_expr(WASMModule *module, const uint8 **p_buf, const uint8 *buf_end, } cur_ref_type.ref_ht_common.ref_type = (uint8)((int32)0x80 + heap_type); - if (!push_const_expr_stack( - &const_expr_ctx, flag, cur_ref_type.ref_type, NULL, - 0, &cur_value, error_buf, error_buf_size)) - if (!push_const_expr_stack(&const_expr_ctx, flag, type1, - NULL, 0, &cur_value, + if (!push_const_expr_stack(&const_expr_ctx, flag, + cur_ref_type.ref_type, NULL, 0, + &cur_value, #if WASM_ENABLE_EXTENDED_CONST_EXPR != 0 NULL, #endif