-
Notifications
You must be signed in to change notification settings - Fork 628
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
EH: fix validation of delegate opcode #3107
Conversation
core/iwasm/interpreter/wasm_loader.c
Outdated
uint32 depth; | ||
|
||
read_leb_uint32(p, p_end, depth); | ||
CHECK_BR(depth); |
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.
Not sure whether it should be CHECK_BR(depth + 1)
? Seems the target block to check is loader_ctx->frame_csp - depth - 2
.
In check_branch_block, the code is:
wasm-micro-runtime/core/iwasm/interpreter/wasm_loader.c
Lines 7120 to 7121 in 3111a86
CHECK_BR(depth); | |
frame_csp_tmp = loader_ctx->frame_csp - depth - 1; |
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.
good point. i will check.
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.
fixed.
* Note: the caller hasn't popped the try-delegate frame yet. | ||
*/ | ||
bh_assert(loader_ctx->csp_num > 0); | ||
if (loader_ctx->csp_num - 1 <= depth) { |
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.
Not sure whether the check is enough? The original code calls check_branch_block:
check_branch_block -> CHECK_BR -> wasm_loader_check_br
And in wasm_loader_check_br, there are more checks:
wasm-micro-runtime/core/iwasm/interpreter/wasm_loader.c
Lines 7058 to 7088 in 3111a86
cur_block = loader_ctx->frame_csp - 1; | |
target_block = loader_ctx->frame_csp - (depth + 1); | |
target_block_type = &target_block->block_type; | |
frame_ref = loader_ctx->frame_ref; | |
/* Note: loop's arity is different from if and block. loop's arity is | |
* its parameter count while if and block arity is result count. | |
*/ | |
if (target_block->label_type == LABEL_TYPE_LOOP) | |
arity = block_type_get_param_types(target_block_type, &types); | |
else | |
arity = block_type_get_result_types(target_block_type, &types); | |
/* If the stack is in polymorphic state, just clear the stack | |
* and then re-push the values to make the stack top values | |
* match block type. */ | |
if (cur_block->is_stack_polymorphic) { | |
for (i = (int32)arity - 1; i >= 0; i--) { | |
#if WASM_ENABLE_FAST_INTERP != 0 | |
POP_OFFSET_TYPE(types[i]); | |
#endif | |
POP_TYPE(types[i]); | |
} | |
for (i = 0; i < (int32)arity; i++) { | |
#if WASM_ENABLE_FAST_INTERP != 0 | |
bool disable_emit = true; | |
int16 operand_offset = 0; | |
PUSH_OFFSET_TYPE(types[i]); | |
#endif | |
PUSH_TYPE(types[i]); | |
} |
Is it better to use CHECK_BR(depth + 1)
?
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.
the most of checks in wasm_loader_check_br are not appropriate for delegate, where parameters are given via the exception, not stack.
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.
OK, got it, thanks!
cf. #1884 (comment)