From 5cf7572ad408edc951b94baca40a10486cc1f7ec Mon Sep 17 00:00:00 2001
From: d1r1 <me@d1r1.me>
Date: Sat, 8 Aug 2026 15:19:58 +0400
Subject: [PATCH] fix(vm): trap on stack oob at every executor exit

- 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
---
 src/vm/executor.rs          | 15 ++++++-
 tests/value-stack-bounds.rs | 78 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 89 insertions(+), 4 deletions(-)

diff --git a/src/vm/executor.rs b/src/vm/executor.rs
index 3a540004..0ae7c86d 100644
--- a/src/vm/executor.rs
+++ b/src/vm/executor.rs
@@ -205,13 +205,19 @@ impl<'a, T> RwasmExecutor<'a, T> {
     /// With [`TrapCode::StackOverflow`] if `instr` addressed a cell outside the value stack. The
     /// offending access itself was already suppressed by [`ValueStackPtr`], this only stops the
     /// execution from carrying on with a corrupted stack.
+    ///
+    /// An out-of-bounds access outranks whatever `instr` reported on its own: that trap is an
+    /// artifact of the values the suppressed access substituted. The check therefore runs before
+    /// the instruction's own error is propagated — otherwise every exit that carries an error out
+    /// of [`RwasmExecutor::run`] would drop the flag, and [`TrapCode::ExecutionHalted`] returned by
+    /// a host function would even turn the run into a success.
     #[inline(always)]
     pub fn step(&mut self, instr: Opcode) -> Result<bool, TrapCode> {
-        let return_reached = self.execute(instr)?;
+        let result = self.execute(instr);
         if self.sp.is_out_of_bounds() {
             return Err(TrapCode::StackOverflow);
         }
-        Ok(return_reached)
+        result
     }
 
     #[inline(always)]
@@ -455,6 +461,11 @@ impl<'a, T> RwasmExecutor<'a, T> {
         for (i, x) in params.iter().enumerate() {
             buffer[params.len() - i - 1] = self.sp.pop_value(*x);
         }
+        // A parameter popped from outside the value stack is a fabricated zero. The host must not
+        // observe it: its side effects would happen before `step` gets to see the flag.
+        if self.sp.is_out_of_bounds() {
+            return Err(TrapCode::StackOverflow);
+        }
         for (i, x) in result.iter().enumerate() {
             buffer[params.len() + i] = Value::default(*x);
         }
diff --git a/tests/value-stack-bounds.rs b/tests/value-stack-bounds.rs
index 3168846b..b14d1746 100644
--- a/tests/value-stack-bounds.rs
+++ b/tests/value-stack-bounds.rs
@@ -9,10 +9,12 @@
 //! code; those cases are caught by the runtime bounds checks alone.
 
 use rwasm::{
-    instruction_set, ExecutionEngine, ImportLinker, InstructionSet, RwasmModule,
-    RwasmModuleBuilder, RwasmStore, TrapCode,
+    instruction_set, ExecutionEngine, ImportLinker, ImportName, InstructionSet, RwasmModule,
+    RwasmModuleBuilder, RwasmStore, StoreTr, TrapCode, TypedCaller, Value,
 };
+use rwasm_fuel_policy::SyscallFuelParams;
 use std::sync::Arc;
+use wasmparser::ValType;
 
 fn execute(code_section: InstructionSet) -> Result<(), TrapCode> {
     let module = RwasmModuleBuilder::new(code_section).build();
@@ -100,6 +102,78 @@ fn popping_below_the_stack_base_traps() {
     assert_eq!(execute(code), Err(TrapCode::StackOverflow));
 }
 
+/// A syscall whose parameters underflow the value stack must trap before the host observes them.
+///
+/// Verification accepts this module — the emulated stack height is `Unknown` after any call — so
+/// the runtime is the only thing standing between a malformed module and the host. Popping a
+/// parameter that is not there yields a fabricated zero, and the handler returning
+/// `ExecutionHalted` would otherwise carry the whole run out as a success.
+#[test]
+fn syscall_with_underflowing_params_traps_before_reaching_the_host() {
+    const EXIT_IDX: u32 = 75;
+
+    fn halting_handler(
+        caller: &mut TypedCaller<'_, Vec<i32>>,
+        _idx: u32,
+        params: &[Value],
+        _result: &mut [Value],
+    ) -> Result<(), TrapCode> {
+        caller
+            .data_mut()
+            .extend(params.iter().map(|p| p.i32().unwrap_or(i32::MIN)));
+        Err(TrapCode::ExecutionHalted)
+    }
+
+    let mut import_linker = ImportLinker::default();
+    import_linker.insert_function(
+        ImportName::new("fluentbase_v1preview", "_exit"),
+        EXIT_IDX,
+        SyscallFuelParams::default(),
+        &[ValType::I32],
+        &[],
+    );
+
+    let code = instruction_set! {
+        StackCheck(16)
+        Call(EXIT_IDX)
+        Return
+    };
+    assert!(
+        verify(code.clone()).is_ok(),
+        "verification cannot catch this one"
+    );
+
+    let module = RwasmModuleBuilder::new(code).build();
+    let mut store = RwasmStore::new(
+        Arc::new(import_linker),
+        Vec::<i32>::new(),
+        halting_handler,
+        Some(1_000_000),
+        None,
+    );
+    let result = ExecutionEngine::new().execute(&mut store, &module, &[], &mut []);
+
+    assert_eq!(result, Err(TrapCode::StackOverflow));
+    assert!(
+        store.data().is_empty(),
+        "the host must not have been called: {:?}",
+        store.data()
+    );
+}
+
+/// An instruction that both underflows the stack and traps on its own terms reports the underflow:
+/// the trap it raised is an artifact of the zero the suppressed pop substituted.
+#[test]
+fn a_trap_caused_by_an_underflow_is_reported_as_the_underflow() {
+    let code = instruction_set! {
+        StackCheck(16)
+        I32Const(0)
+        I32DivU
+        Return
+    };
+    assert_eq!(execute(code), Err(TrapCode::StackOverflow));
+}
+
 #[test]
 fn pushing_past_the_reserved_stack_window_traps() {
     // `StackCheck` reserves a single cell, so the pushes run out of stack long before the last