WebAssembly / wabt

The WebAssembly Binary Toolkit
Apache License 2.0
6.91k stars 702 forks source link

[wasm-interp] Fix memory corruption with recursive call_indirect #2464

Closed SoniEx2 closed 2 months ago

SoniEx2 commented 2 months ago

The interpreter could overflow the stack without trapping properly in call_indirect situations. While it would set the out_trap to the trap reason, it would return RunResult::Ok and the interpreter code would only check RunResult::Ok to 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 runaway call_indirect running.

If the offending call_indirect didn'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_indirect propagate RunResult::Trap properly. Additionally, we made it so assert_exhaustion checks both the RunResult and the out_trap, and asserts if they don't match. This should help catch similar bugs in the future.

Closes #2462 Fixes #2398

SoniEx2 commented 2 months ago

@keithw @sbc100

SoniEx2 commented 2 months ago

(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.)

sbc100 commented 2 months ago

Could you add some details to the PR description saying what the issue is exactly and how this change fixes it?

SoniEx2 commented 2 months ago

Could you add some details to the PR description saying what the issue is exactly and how this change fixes it?

done

sbc100 commented 2 months ago

Can you update the PR title with [wasm-interp] and include call_indirect in the title?

SoniEx2 commented 2 months ago

done(?)