fix(vm): bound value stack accesses during module verification and execution - #171
fix(vm): bound value stack accesses during module verification and execution#171dmitry123 wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds static stack-height verification, bounds-checked value-stack operations, runtime ChangesStack safety
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/value-stack-bounds.rs (2)
110-149: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAdd execution coverage for the compiled fixtures.
This test proves that the new verifier does not reject bytecode this crate produces. It does not prove that the new runtime bounds checks leave those modules alone. A false positive in
ValueStackPtrwould turn a working module into aTrapCode::StackOverflow, and this test would still pass.The three fixtures are named
*-stack-ub.wasm, which suggests they previously exercised the out-of-bounds path. Run at least one of them throughExecutionEngine::executeand assert the result is notErr(TrapCode::StackOverflow).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/value-stack-bounds.rs` around lines 110 - 149, Extend compiled_modules_pass_verification to execute at least one compiled stack-UB fixture through ExecutionEngine::execute, using the existing compilation setup and appropriate entrypoint inputs. Assert execution does not return Err(TrapCode::StackOverflow), while preserving the current verification assertions for all fixtures.
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific verification error.
assert!(verify(code.clone()).is_err())passes for any verification failure. If the verifier later rejects these modules for an unrelated reason, for example a code-fallthrough regression, the test still passes and the local-depth coverage is silently lost.Match on
RwasmModuleVerificationError::LocalDepthOutOfBoundsfor the three local tests and onStackUnderflowforbulk_drop_below_the_stack_base_traps. The unit tests insrc/module/verification.rsalready use this stricter form.Also applies to: 50-50, 63-63, 84-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/value-stack-bounds.rs` at line 38, Replace the broad is_err assertions in the three local-depth tests and bulk_drop_below_the_stack_base_traps with specific matches against RwasmModuleVerificationError::LocalDepthOutOfBounds and StackUnderflow respectively, following the stricter pattern used in src/module/verification.rs.src/vm/executor.rs (1)
126-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the reset sequence into a helper.
Lines 108-111, lines 126-131, and lines 159-161 in
run_with_stack_checkrepeat the same three statements: reset the value stack, reset the call stack, clearstore.last_signature. A future addition to the dirty-state set has to be applied in three places.♻️ Proposed helper
+ /// Clears the execution state left behind by a halted or trapped run. + fn reset_execution_state(&mut self) { + self.value_stack.reset(); + self.call_stack.reset(); + self.store.last_signature = None; + }Then apply it at each site:
if self.sp.is_out_of_bounds() { - self.value_stack.reset(); - self.call_stack.reset(); - self.store.last_signature = None; + self.reset_execution_state(); return Err(TrapCode::StackOverflow); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vm/executor.rs` around lines 126 - 131, Extract the repeated value-stack reset, call-stack reset, and store.last_signature clearing from run_with_stack_check into a dedicated helper method on the executor. Replace the reset sequences at the sites around lines 108-111, 126-131, and 159-161 with calls to that helper, preserving each existing return or error path.src/module/verification.rs (1)
350-417: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftMake
stack_effectexhaustive overOpcode.The catch-all arm at Line 415 assigns
(1, 1)to every opcode that no earlier arm matches. A new opcode with a different stack effect then gets a silently wrong effect. That can reject valid bytecode or accept invalid bytecode without any compile-time signal.List the remaining opcodes explicitly and remove the catch-all, or keep a catch-all that is
unreachable!()only after every variant is covered. The_ if opcode.is_binary_instruction()guard can stay, because a guarded arm does not affect exhaustiveness checking of the listed variants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/module/verification.rs` around lines 350 - 417, Make stack_effect exhaustive by explicitly matching every remaining Opcode variant not covered by the existing arms, preserving the intended (1, 1) effect for the unary integer opcodes currently handled by the catch-all. Remove the catch-all, or replace it with unreachable!() only after all variants are explicitly covered; retain the guarded is_binary_instruction arm.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/value-stack-bounds.rs`:
- Around line 1-5: Correct the module-level documentation in the value-stack
bounds test file: remove the claim that every module is rejected by
RwasmModule::new_verified, and state that some cases pass verification but are
still executed to validate the interpreter’s runtime stack-safety checks.
Preserve the explanation that the tests cover out-of-bounds accesses through
unverified or runtime-only paths.
- Around line 100-108: Update the comment in
pushing_past_the_reserved_stack_window_traps to state that the final push
exceeds the reserved value-stack window, not the second push. Add a verification
assertion using the existing verification path to confirm the module is rejected
with StackOverflow, while preserving the current execute-time trap assertion.
---
Nitpick comments:
In `@src/module/verification.rs`:
- Around line 350-417: Make stack_effect exhaustive by explicitly matching every
remaining Opcode variant not covered by the existing arms, preserving the
intended (1, 1) effect for the unary integer opcodes currently handled by the
catch-all. Remove the catch-all, or replace it with unreachable!() only after
all variants are explicitly covered; retain the guarded is_binary_instruction
arm.
In `@src/vm/executor.rs`:
- Around line 126-131: Extract the repeated value-stack reset, call-stack reset,
and store.last_signature clearing from run_with_stack_check into a dedicated
helper method on the executor. Replace the reset sequences at the sites around
lines 108-111, 126-131, and 159-161 with calls to that helper, preserving each
existing return or error path.
In `@tests/value-stack-bounds.rs`:
- Around line 110-149: Extend compiled_modules_pass_verification to execute at
least one compiled stack-UB fixture through ExecutionEngine::execute, using the
existing compilation setup and appropriate entrypoint inputs. Assert execution
does not return Err(TrapCode::StackOverflow), while preserving the current
verification assertions for all fixtures.
- Line 38: Replace the broad is_err assertions in the three local-depth tests
and bulk_drop_below_the_stack_base_traps with specific matches against
RwasmModuleVerificationError::LocalDepthOutOfBounds and StackUnderflow
respectively, following the stricter pattern used in src/module/verification.rs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7113a309-e7ae-4e78-b367-0ad66015f223
📒 Files selected for processing (5)
src/module/mod.rssrc/module/verification.rssrc/vm/executor.rssrc/vm/value_stack.rstests/value-stack-bounds.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Fixes FLU-1094.
A module accepted by
RwasmModule::new_verifiedcould read and write outside the value stack: verification only rejected a local depth of0and never checked the pops ofDrop,BulkDrop,Select, or any arithmetic opcode. The interpreter had no release-mode guard either, becauseValueStackPtr::inc_by/dec_byandValueStack::get_release_unchecked_mutjustified theirunsafewith "all rwasm bytecode has been validated during translation" — true for bytecode this crate produces, false for thenew_verifiedpath, which exists precisely to accept foreign bytecode.LocalSet/LocalTeewrite through the same pointer, so this was an arbitrary write relative to aSmallVecwhose inline storage lives in the caller's native stack frame.Both halves of the suggested fix are implemented, with the runtime as the primary guarantee and verification as defense in depth.
Runtime bounds (
src/vm/value_stack.rs)ValueStackPtrnow carriesendalongsidesrcand bounds-checks every pointer move and every dereference in all build profiles. An out-of-range access performs no memory access at all: the pointer is parked on the stack base and a sticky flag is raised, whichRwasmExecutor::stepturns intoTrapCode::StackOverflowbefore the next instruction runs. The flag survives the executor re-derivingsp, because it travels intoValueStackthroughsync_stack_ptrand back out throughstack_ptr.This is what actually closes the hole, and it holds for any module regardless of how it was decoded.
ValueStack::drop,push,pop, andpeek_as_slice_mutwere hardened the same way, andget_release_unchecked_mutis gone.Rather than threading
Resultthrough ~150 call sites, the flag approach keeps the diff reviewable and costs one predictable compare-and-branch per stack op.Verification (
src/module/verification.rs)verify_modulenow runs an abstract interpretation over the whole code section. Every function entry —source_pc,pc 0, and everyCallInternal/ReturnCallInternal/RefFunc/elem_sectiontarget — seeds an emulated stack height of zero, which is propagated along all control flow edges over aUnvisited/Known/Unknownlattice, so each program counter is processed at most twice and the pass staysO(code_len).New rejections:
depth == 0Drop,BulkDrop,Select, arithmetic, …)BulkConst, constants, …)Two honest limitations, both documented in the code:
Unknownafter any call and only height-independent bounds apply from there. Threading the import linker intoverify()would be needed to do better.max(N_MAX_STACK_SIZE, largest StackCheck reservation in the module)rather than a tight per-function bound. A callee legitimately reads its parameters from below its own entry stack pointer, and rWasm does not record parameter counts, so a tighter bound is not derivable from the module alone.Neither limitation is load-bearing for memory safety — the runtime check is.
new_checked/new_checked_exactrustdoc now says explicitly that they validate the encoding and perform no structural validation.Testing
tests/value-stack-bounds.rs: the issue's repro (SIGSEGVoncargo test --releasebefore this change) plus theLocalSet/LocalTeewrite variants, underflow, and overflow — each asserted to be both rejected bynew_verifiedand trapped by the interpreter.tests/assetsand asserts they verify.verification.rs, including two that pin down what must not be rejected: locals addressed below the function entry, and bulk operands covered by aStackCheck.RwasmModule::compileassertverify()on its own output and ran the full suite: the 92 Wasm spec testsuites, snippets, fluentbase, and the release nitro-verifier test all pass. That caught one real false positive during development — the compiler legitimately emitsBulkConst(65534)for functions with many locals — which is why the window is module-derived rather than a fixed constant.Full
make testequivalent is green,cargo clippy --all-targets --all-featuresis clean.Benchmark
benches/bench.rs,Comparisons/bench_strategy_rwasm:develNo regression; the small improvement is within codegen noise.
Summary by CodeRabbit
Bug Fixes
Documentation