Skip to content

improve logic of heap_type validation when ref.null #4372

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+25 −8
Open

improve logic of heap_type validation when ref.null #4372

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fd5cfe4
Follow-up to PR #4300: prevent potential overflow
Septa2112 Jun 17, 2025
66cd7f6
Merge branch 'main' into fix/overflow
Septa2112 Jun 17, 2025
d0aac00
improve error message
Septa2112 Jun 17, 2025
58aae86
Refine heap_type validation and corresponding type1 calculation
Septa2112 Jun 17, 2025
038e2fc
Removed some redundant logic
Septa2112 Jun 18, 2025
0020653
Merged the if-else conditions
Septa2112 Jun 20, 2025
17ac8c1
improve logic and comments
Septa2112 Jun 25, 2025
f297e08
revert changes, read heap_type by p_copy
Septa2112 Jun 27, 2025
54bed47
add comments
Septa2112 Jun 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions core/iwasm/interpreter/wasm_loader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,32 +830,49 @@ 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If following the spec, s33 should be a type index. might rename it as type_index better.

Copy link
Contributor Author

@Septa2112 Septa2112 Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might not be appropriate to rename heap_type to type_idx here:

  1. type_idx has already been defined as a uint32 at the beginning of this function.
  2. Based on the surrounding lines of code in function wasm_loader_prepare_bytecode when WASM_OP_REF_NULL, I think it's clearer and more consistent to keep the name heap_type.

    wasm-micro-runtime/core/iwasm/interpreter/wasm_loader.c

    Lines 13154 to 13169 in 18d4227

    pb_read_leb_int32(p, p_end, heap_type);
    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(&wasm_ref_type.ref_ht_typeidx,
    true, heap_type);
    ref_type = wasm_ref_type.ref_type;
    }
    else {
    if (!wasm_is_valid_heap_type(heap_type)) {
    set_error_buf(error_buf, error_buf_size,
    "unknown type");
    goto fail;
    }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, there is also a variable named type1, which is a bad name. Keeping two variables with unclear meanings will make the code difficult to understand.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing, L872 will read it again. Seems we can remove either one.

CHECK_BUF(p, p_end, 1);
type1 = read_uint8(p);

#if WASM_ENABLE_GC == 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like above, in L839, rename type1 to heap_type or abs_heaptype.

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);
type1 = (uint8)((int32)0x80 + heap_type);

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)) {
goto fail;
}
}
else {
if (!wasm_is_valid_heap_type(heap_type)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess heap_type should be type1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, considering the same code context in function wasm_loader_prepare_bytecode when WASM_OP_REF_NULL , it might be better to keep the check for int32 heap_type here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in this case, not reusing exactly the same code. Maybe it's possible by just copying the content from wasm_loader_prepare_bytecode(). However, don't use mixed logic here.

set_error_buf_v(error_buf, error_buf_size,
"unknown type %d", heap_type);
goto fail;
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after wasm_is_valid_heap_type(), not sure we still need is_byte_a_type() and wasm_is_type_multi_byte_type().

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);
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,
Expand Down
Loading