Skip to content

Commit 33fa4f1

Browse files
committed
Fix frame-stack corruption returning a predicate-bearing named return type
retSlowPath held a raw pointer into ctx.vs.frames[fi] across the call to enforceFuncReturnTypes. For a function whose declared return type is a predicate-bearing named type, that call reenters the VM to run the predicate as a nested call, which reuses (and overwrites) the same frames[] slot once frame_top no longer counts it — so the outer code's later reads of frame.base/frame.ret_ip picked up the nested call's frame instead of its own, desyncing frame_top and aborting with a fatal VM integrity error on the following ret. Fixed by capturing frame.base/frame.ret_ip/frame.has_typed_returns into locals before frame_top is dropped, matching the pattern the adjacent multi-named-return spread path already used. Fixes #212
1 parent 55a3f7a commit 33fa4f1

2 files changed

Lines changed: 43 additions & 12 deletions

File tree

src/compiler_test.zig

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4198,15 +4198,12 @@ test "gbc: variant-type constants (shared fields, record arm, single-payload arm
41984198
}
41994199

42004200
test "gbc: a predicate-bearing named type still enforces its predicate after round-tripping" {
4201-
// Note: `f` returns `int`, not `Score`, deliberately — a function whose
4202-
// *declared, checked* return type is itself a predicate-bearing named
4203-
// type hits a separate, pre-existing, GBC-unrelated VM bug (confirmed
4204-
// via `git stash` to already reproduce on plain interpreted execution
4205-
// with zero GBC involvement — see issue #212): enforceFuncReturnTypes's
4206-
// named_t path re-invokes the predicate as a reentrant VM call while
4207-
// already mid-`ret`-opcode-handling, which corrupts the frame stack.
4208-
// Constructing `Score(n)` as a local (not a checked function return)
4209-
// exercises the predicate without going anywhere near that path.
4201+
// Note: `f` returns `int`, not `Score` — deliberately independent of
4202+
// issue #212 (now fixed, see the regression test below), which was a
4203+
// separate, GBC-unrelated VM bug specific to a *declared, checked*
4204+
// function return type that is itself a predicate-bearing named type.
4205+
// Constructing `Score(n)` as a local exercises the predicate without
4206+
// relying on that other, separately-tested path.
42104207
const src =
42114208
\\type Score int predicate func(x) { return x >= 0 and x <= 100 }
42124209
\\func f(n int) int {
@@ -4245,6 +4242,32 @@ test "gbc: a predicate-bearing named type still enforces its predicate after rou
42454242
try std.testing.expectError(error.PredicateFailed, vm.callGlobal(ctx, "f", &.{.{ .int = 200 }}));
42464243
}
42474244

4245+
// Returning a predicate-bearing named type as a function's *declared,
4246+
// checked* return type used to crash with a fatal VM integrity error
4247+
// (ImpossibleOpcodeState, frame corruption) the moment the function
4248+
// returned. Root cause: retSlowPath (vm.zig) held a raw pointer into
4249+
// ctx.vs.frames[fi] across the call to enforceFuncReturnTypes, which — for
4250+
// a predicate-bearing named_t return — makes a reentrant nested VM call to
4251+
// run the predicate function. That nested call reuses (and overwrites) the
4252+
// exact same frames[] slot once frame_top no longer counts it, so the outer
4253+
// retSlowPath's later reads of frame.base/frame.ret_ip picked up the nested
4254+
// call's frame data instead of its own. Fixed (issue #212) by capturing
4255+
// frame.base/frame.ret_ip/frame.has_typed_returns into locals before
4256+
// frame_top is dropped, matching the pattern the multi-named-return spread
4257+
// path already used a few lines above.
4258+
test "compiler: predicate-bearing named type as a function's declared return type (issue #212)" {
4259+
var rt = try setup();
4260+
defer rt.deinit();
4261+
try runSrc(&rt,
4262+
\\type Score int predicate func(x) { return x >= 0 and x <= 100 }
4263+
\\func make_score(n int) Score { return Score(n) }
4264+
);
4265+
const ok = try rt.callGlobal("make_score", &.{.{ .int = 50 }});
4266+
const inner = ok.namedInner() orelse ok;
4267+
try std.testing.expectEqual(@as(i64, 50), inner.int);
4268+
try std.testing.expectError(error.PredicateFailed, rt.callGlobal("make_score", &.{.{ .int = 500 }}));
4269+
}
4270+
42484271
test "gbc: interface-type constants round-trip and assert_interface still enforces conformance" {
42494272
// Exercises the exact concern that made interface support riskier than
42504273
// struct/named/variant: interfaceMethodMatches (vm_types.zig) compares

src/lang/vm.zig

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1536,12 +1536,20 @@ fn retSlowPath(ctx: VMContext, retval_in: Value) !bool {
15361536
}
15371537
}
15381538
ctx.vs.popTempRoot();
1539+
// Capture frame.base/ret_ip/has_typed_returns into locals before dropping
1540+
// frame_top: enforceFuncReturnTypes may run a predicate function as a
1541+
// reentrant nested VM call, which reuses (and overwrites) this exact
1542+
// ctx.vs.frames[fi] slot once it's no longer counted by frame_top — so
1543+
// `frame` must not be dereferenced again after that call.
1544+
const frame_base = frame.base;
1545+
const frame_ret_ip = frame.ret_ip;
1546+
const frame_has_typed_returns = frame.has_typed_returns;
15391547
ctx.vs.frame_top = fi;
1540-
if (frame.has_typed_returns) {
1548+
if (frame_has_typed_returns) {
15411549
if (fsig_ret) |fsig| try vmtyp.enforceFuncReturnTypes(ctx, fsig, retval);
15421550
}
1543-
ctx.vs.stack_top = if (frame.base > 0) frame.base - 1 else 0;
1544-
ctx.vs.ip = frame.ret_ip;
1551+
ctx.vs.stack_top = if (frame_base > 0) frame_base - 1 else 0;
1552+
ctx.vs.ip = frame_ret_ip;
15451553
try ctx.vs.vmPush(retval);
15461554
if (ctx.vs.frame_top == ctx.vs.call_depth_target) return true;
15471555
return false;

0 commit comments

Comments
 (0)