[wasm-interp] Fix memory corruption with recursive call_indirect #2464
Conversation
0ad7539 to
16f41de
Compare
|
(we were trying to figure out a way to trigger the bug consistently, without triggering the memory corruption. it was hard, but we're glad we figured something out.) |
|
Could you add some details to the PR description saying what the issue is exactly and how this change fixes it? |
| (elem | ||
| $runaway-with-return | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I think this table declaration can fit all on one line maybe?
There was a problem hiding this comment.
Also, can this be reproduced even without a table? I.e. just with a direct call?
There was a problem hiding this comment.
as far as we could tell, no - the codepath for call is different from the codepath for call_indirect...
There was a problem hiding this comment.
we chose to write the table declaration like this for consistency with the code style of upstream tests
There was a problem hiding this comment.
as far as we could tell, no - the codepath for
callis different from the codepath forcall_indirect...
The reason I ask is that the changes on the interpreter side all like they are part of DoCall.. which doesn't look specific to indirect calls. But I've not looked at the interpreter code in a long time so I maybe be misunderstanding.
There was a problem hiding this comment.
no, for some reason O::Call has its own copy of part of DoCall... (and doesn't have the bug)
could it use DoCall? maybe. but it currently doesn't.
done |
sbc100
left a comment
There was a problem hiding this comment.
Thanks! I think I understand the issue better now. LGTM.
|
Can you update the PR title with |
|
done(?) |
The interpreter could overflow the stack without trapping properly in
call_indirectsituations. While it would set theout_trapto the trap reason, it would returnRunResult::Okand the interpreter code would only checkRunResult::Okto decide whether or not to keep running. In other words, while the stack overflow meant the interpreter wouldn't push a frame onto the call stack, the interpreter loop would continue advancing instructions, resulting in instructions after the runawaycall_indirectrunning.If the offending
call_indirectdidn't have return values, it would be as if the call returned normally. If it did have return values, nothing would be pushed onto the value stack, yet the return types would be pushed onto the type stack. With careful manipulation of the following instructions, this could be used to cause all sorts of memory corruption.As it turns out, the function exit code, as well as a handful of other instructions, do check the state of the value and type stacks and can safely reproduce the bug without the memory corruption, so that's what we made the test do.
The obvious fix was to make
call_indirectpropagateRunResult::Trapproperly. Additionally, we made it soassert_exhaustionchecks both theRunResultand theout_trap, and asserts if they don't match. This should help catch similar bugs in the future.Closes #2462
Fixes #2398