-
Notifications
You must be signed in to change notification settings - Fork 703
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.
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
base: main
Are you sure you want to change the base?
Conversation
PR bytecodealliance#4300 introduced the rationale for validating heap_type. This patch moves the validation before the computation of type1 to prevent potential overflow.
Further improved the heap type check logic:
|
heap_type
validation when ref.null
} | ||
} | ||
else { | ||
if (!wasm_is_valid_heap_type(heap_type)) { |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
const uint8 *p_copy = p; | ||
int32 heap_type; | ||
read_leb_int32(p_copy, p_end, heap_type); | ||
#endif |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
type_idx
has already been defined as auint32
at the beginning of this function.- Based on the surrounding lines of code in function
wasm_loader_prepare_bytecode
whenWASM_OP_REF_NULL
, I think it's clearer and more consistent to keep the nameheap_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; }
CHECK_BUF(p, p_end, 1); | ||
type1 = read_uint8(p); | ||
|
||
#if WASM_ENABLE_GC == 0 |
There was a problem hiding this comment.
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
.
goto fail; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
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()
.
const uint8 *p_copy = p; | ||
int32 heap_type; | ||
read_leb_int32(p_copy, p_end, heap_type); | ||
#endif |
There was a problem hiding this comment.
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.
PR #4300 introduced the rationale for validating heap_type.
This patch moves the validation before the computation of type1 to prevent potential overflow.