Fix several issues found by fuzzing #1931
Conversation
Mem2019
changed the title
|
@Mem2019 Thank you for fixing these! Do you have cycles to make the changes requested in the review and get this merged? Would be great to have this in... |
The returncallindirect regression is identical to WebAssembly#1414, but in the interpreter
There was a problem hiding this comment.
@sbc100 In an effort to get these issues closed, I made the requested changes and confirmed that the four regression tests fail before the patch is applied (and pass afterwards). Will plan to merge early next week unless you want to take another look.
src/interp/binary-reader-interp.cc
Outdated
| @@ -1169,6 +1172,9 @@ Result BinaryReaderInterp::OnReturnCallExpr(Index func_index) { | |||
|
|
|||
| Result BinaryReaderInterp::OnReturnCallIndirectExpr(Index sig_index, | |||
| Index table_index) { | |||
| if (sig_index >= module_.func_types.size()) { | |||
| return Result::Error; | |||
| } | |||
There was a problem hiding this comment.
I wonder if these checks should be in the validator instead?
There was a problem hiding this comment.
Actually it looks like we already have this code above, can we call this instead?
CHECK_RESULT(
validator_.OnCall(GetLocation(), Var(func_index, GetLocation())));
(edit: actually I guess not because that is validating func_index and not sig_index)
There was a problem hiding this comment.
Ah, good point. I changed them to use validator_.OnReturnCall and validator_.OnReturnCallIndirect which looks like what it should have been doing all along.
There was a problem hiding this comment.
I wonder if that fact that we were missing those validation checks means there are some tests that are missing from the spec tests? i.e. why didn't the spec tests find these? I'm they are missing from the spec tests we should followup by filing bugs upstream and eventually remove these downstream tests.
I looked into this. In this case, the tail-call proposal tests would have caught these binary-reader-interp bugs (causing a segfault or asan failure). But we're not running the tail-call proposal tests because (I guess?) the interpreter doesn't support tail calls yet. :-( |
Fixes WebAssembly#1922, fixes WebAssembly#1924, fixes WebAssembly#1929 Co-authored-by: Keith Winstein <[email protected]>
Fixes #1922
Fixes #1924
Fixes #1929