-
Notifications
You must be signed in to change notification settings - Fork 679
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
Fix several issues found by fuzzing #1931
Conversation
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.
Could you update the title so be a little more specific. Perhaps "Fix several issues found by fuzzing", and the you can do "Fixes #xxx, #yyy" in the description so have github automatically close those issue.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if these checks should be in the validator instead?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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