fix(vm): trap on stack oob at every executor exit - #178
Open
d1r1 wants to merge 1 commit into
Open
Conversation
- invalid module no longer exits as a successful halt - host syscall never runs on values from a bad pop - out-of-bounds now outranks the trap it caused
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Criterion results (vs baseline)Heads-up: runner perf is noisy; treat deltas as a smoke check. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #171, on top of its branch.
Problem
RwasmExecutor::stepchecks the out-of-bounds flag only afterexecutereturnsOk:Every exit that carries an error out of the instruction loop therefore drops the flag.
runthen callsvalue_stack.reset(), which clears it, and forTrapCode::ExecutionHaltedreturnsOk(())— the one place in the interpreter that converts anErrinto a success.run_with_stack_checkand theInterruptionCalledbranch lose it the same way.That is reachable from bytecode
new_verifiedaccepts:Verification accepts the module — the emulated stack height turns
Unknownafter any call, a documented limitation of the pass added in #171. At runtime the syscall pops its parameter off an empty stack, the pop is suppressed and substitutes a zero, the host handler runs with that fabricated argument, returnsExecutionHalted, andexecutehands the callerOk(()). An invalid module finishes as a successful halt, after the host has already committed a side effect on values the module never pushed.Fix
stepconverts the flag before the instruction's own error is propagated. Binding the result first covers every exit at once — both loops, theExecutionHaltedbranch and theInterruptionCalledbranch — sorunandrun_with_stack_checkneed no change:invoke_syscalltraps after popping its parameters, before the handler runs. This one cannot be folded intostep:stepsees the flag only once the instruction returns, and by then the host has already observed the fabricated zeros and acted on them.The existing check after the result-collection pops in
runstays — those pops happen outsidestep.Trap-code precedence
An out-of-bounds access now outranks the trap the instruction reported on its own.
I32DivUon an underflowed stack reportsIntegerDivisionByZeroonly because the suppressed pop fed it a zero divisor; the underflow is the cause, the division is the symptom.The blast radius was measured rather than assumed: with
ValueStackPtr::mark_out_of_boundspatched topanic!, 120 tests across the lib,compiler,fuel,locals,memory,stack-overflow,snippetsandintrinsicpass without a single hit. NothingRwasmModule::compileemits ever raises the flag, so the rule is observable only throughRwasmModule::new_verified. The differential fuzzer is unaffected as well: it treats any trap on both sides as equivalent, and every module it generates goes through the compiler.Tests
Two tests in
tests/value-stack-bounds.rs, each pinning one of the two gates:syscall_with_underflowing_params_traps_before_reaching_the_host— the repro above. AssertsErr(TrapCode::StackOverflow)and that the handler recorded nothing, and documents thatnew_verifiedaccepts the module. On the parent branch it fails withleft: Ok(()). With only the syscall gate removed it fails on the second assertion, with the handler having seen[0].a_trap_caused_by_an_underflow_is_reported_as_the_underflow—StackCheck(16); I32Const(0); I32DivU; Return. On the parent branch it fails withleft: Err(IntegerDivisionByZero). This is the only bytecode-level way to exercise the precedence rule, and it is the test that fails if thestepchange alone is reverted.InterruptionCalledgets no dedicated test on purpose: after this change it is not a separate code path, and once the syscall gate is in place there is no way to raise the flag inside an instruction that goes on to return it, since handlers are the only producers of that trap code.Verification
cargo test— 179 passed, 4 ignored, includinginterruption,wasmtime,fluentbaseandfuzz.cargo test --no-default-features --features stdover the targets that build without the wasmtime feature — 130 passed.cargo clippy --all-targetsclean.cargo +nightly-2025-09-20 fmt --checkreports no diff in either touched file.