Fix missing IS_INVALID_TAGINDEX check in RETHROW handler#4837
Fix missing IS_INVALID_TAGINDEX check in RETHROW handler#4837sumleo wants to merge 3 commits intobytecodealliance:mainfrom
Conversation
sumleo
requested review from
TianlongLiang,
lum1n0us,
no1wudi and
yamt
as code owners
sumleo
force-pushed
the
fix/rethrow-tag-index-validation
branch
from
4de4f92 to
bb89991
Compare
| wasm_module_inst_t inst = wasm_runtime_instantiate( | ||
| module, 8192, 8192, error_buf, sizeof(error_buf)); | ||
|
|
||
| if (inst) { |
There was a problem hiding this comment.
should have an assertion about inst.
if(inst) should be removed since else branch are not expected.
There was a problem hiding this comment.
Done. Replaced if(inst) with ASSERT_NE(inst, nullptr) so the test fails immediately if instantiation doesn't succeed.
lum1n0us
left a comment
lum1n0us
left a comment
There was a problem hiding this comment.
The patch looks good to me.
However, the test cases might need additional work:
- Case name: Use "exception-handling" or something similar instead of "wasm-interp."
- CMakeLists.txt: I highly suggest following an existing case, such as the one in tests/unit/interpreter.
| wasm_function_inst_t func = | ||
| wasm_runtime_lookup_function(inst, "f"); | ||
| EXPECT_NE(func, nullptr) << "Function 'f' should be found"; | ||
|
|
There was a problem hiding this comment.
should be an assertion about func.
if(func) should be removed.
There was a problem hiding this comment.
Done. Removed the if(func) conditional and upgraded EXPECT_NE to ASSERT_NE so the test aborts if the function isn't found.
| wasm_runtime_create_exec_env(inst, 8192), func, 0, NULL); | ||
| /* The function may or may not succeed depending on the | ||
| * exact exception handling implementation details */ | ||
| (void)ok; |
There was a problem hiding this comment.
better have a return value for compare.
and use wasm_runtime_get_exception to check if necessary.
There was a problem hiding this comment.
Done. Now checking the return value of wasm_runtime_call_wasm with ASSERT_TRUE(ok) and including wasm_runtime_get_exception(inst) in the failure message. Also properly creating and destroying exec_env.
Add validation for exception_tag_index in the RETHROW opcode handler to prevent out-of-bounds access to module->module->tags[] when the tag index is INVALID_TAGINDEX (0xFFFFFFFF). This matches the existing check in the THROW handler. When CATCH_ALL catches a cross-module exception with an unknown tag, it pushes INVALID_TAGINDEX onto the stack. Without this check, a subsequent RETHROW would access tags[0xFFFFFFFF].
sumleo
force-pushed
the
fix/rethrow-tag-index-validation
branch
from
bb89991 to
88d5d3e
Compare
- CMakeLists.txt: Follow the interpreter test pattern by adding DRUN_ON_LINUX definition, consistent spacing for set() calls, and comment before add_executable - CMakeLists.txt (parent): Register exception-handling subdirectory so the test is included in the build - exception_handling_test.cc: Replace GTEST_SKIP on module load failure with ASSERT_NE to fail the test instead of skipping - exception_handling_test.cc: Replace if(!ok) conditional with ASSERT_TRUE on wasm_runtime_call_wasm return value, using wasm_runtime_get_exception for the failure message
The hand-crafted WASM binary in load_module_with_exception_handling had an off-by-one in the code section: body size was declared as 7 but the actual body (local count + try/catch_all/end/end) is only 6 bytes. This caused the WASM loader to fail with "unexpected end" when it tried to read past the available bytes. Fix the body size from 7 to 6 and the code section size from 9 to 8.
2 participants
The RETHROW opcode handler reads
exception_tag_indexfrom the stack and directly accessesmodule->module->tags[exception_tag_index]without checking forINVALID_TAGINDEX.When
CATCH_ALLcatches a cross-module exception with an unknown tag, it pushesINVALID_TAGINDEX(0xFFFFFFFF) onto the stack. If RETHROW then executes, it accessestags[0xFFFFFFFF]— a massive out-of-bounds read.The THROW handler correctly checks
IS_INVALID_TAGINDEXbefore the array access. This patch adds the same check to the RETHROW handler: when the tag index is invalid, skip thetags[]access and setcell_num_to_copyto 0, allowing the exception to propagate toCATCH_ALLhandlers.