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

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

3 files changed, 24 insertions, 3 deletions

diff --git a/src/interp/interp.cc b/src/interp/interp.cc
index 74f33eb8..cb5623d8 100644
--- a/src/interp/interp.cc
+++ b/src/interp/interp.cc
@@ -1989,7 +1989,7 @@ RunResult Thread::DoCall(const Func::Ptr& func, Trap::Ptr* out_trap) {
     PushValues(func_type.results, results);
   } else {
     if (PushCall(*cast<DefinedFunc>(func.get()), out_trap) == RunResult::Trap) {
-      return RunResult::Ok;
+      return RunResult::Trap;
     }
   }
   return RunResult::Ok;
diff --git a/src/tools/spectest-interp.cc b/src/tools/spectest-interp.cc
index 130edd66..9e7e32e3 100644
--- a/src/tools/spectest-interp.cc
+++ b/src/tools/spectest-interp.cc
@@ -1406,8 +1406,9 @@ ActionResult CommandRunner::RunAction(int line_number,
   switch (action->type) {
     case ActionType::Invoke: {
       auto* func = cast<interp::Func>(extern_.get());
-      func->Call(store_, action->args, result.values, &result.trap,
-                 s_trace_stream);
+      auto ok = func->Call(store_, action->args, result.values, &result.trap,
+                           s_trace_stream);
+      assert((ok == Result::Ok) == (!result.trap));
       result.types = func->type().results;
       if (verbose == RunVerbosity::Verbose) {
         WriteCall(s_stdout_stream.get(), action->field_name, func->type(),
diff --git a/test/regress/recursion-issue.txt b/test/regress/recursion-issue.txt
new file mode 100644
index 00000000..a5fa660b
--- /dev/null
+++ b/test/regress/recursion-issue.txt
@@ -0,0 +1,20 @@
+;;; TOOL: run-interp-spec
+;;; NOTE: ref: issue-2398
+(module
+  (type $out-i32 (func (result i32)))
+
+  (table funcref
+    (elem
+      $runaway-with-return
+    )
+  )
+
+  (func $runaway-with-return (export "runaway-with-return") (type $out-i32)
+    (call_indirect (type $out-i32) (i32.const 0))
+  )
+)
+
+(assert_exhaustion (invoke "runaway-with-return") "call stack exhausted")
+(;; STDOUT ;;;
+2/2 tests passed.
+;;; STDOUT ;;)