From 2fd510edb572eb42c5be4ee4bfd14c6347173d72 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 4 Oct 2026 20:02:31 +0200 Subject: [PATCH 1/5] fix(vm): raise SystemStackError instead of killing the host process (#768) A Ruby program with runaway recursion took the whole host process down with it. The first line of output was runtime: goroutine stack exceeds 1000000000-byte limit followed by 50772 bytes of goroutine dump and exit status 2, where MRI 4.0.5 prints a rescued SystemStackError and exits 0. This is not only a conformance gap. A Go stack overflow is a FATAL RUNTIME ERROR, not a panic: a deferred recover() never runs, so an embedding host cannot contain it. ruby.Run is the documented embedding API, so three lines of untrusted Ruby could end the host process. Two parts: * SystemStackError, under Exception and NOT StandardError, so a bare `rescue` does not catch it. MRI sets it up that way in Init_Proc (proc.c v3_4_1:4424) and registers "stack level too deep" on the line after; the message here is copied, not invented. Verified against the MRI 4.0.5 on this machine: SystemStackError.ancestors is [SystemStackError, Exception, Object, Kernel, BasicObject]. * A call-depth limit in exec, which is the ONE place a Ruby-level frame is created, so one check covers methods, blocks, procs, lambdas, define_method bodies, class and module bodies and eval. The depth is len(vm.frameNames) rather than a counter of its own, and that is the point: a counter would need decrementing on each of exec's exits, and the ones that unwind by panic never run exec's tail. frameNames is already restored on every unwind path, and restored by TRUNCATING TO A SAVED DEPTH rather than by counting pops, so the depth cannot be left inflated by a rescued runaway recursion and cannot poison a later one. raiseDeep needs no Ruby frame of its own, which is what lets it be called from the place that has just refused to create one: exceptionObject builds the exception as a Go *RObject and never dispatches #initialize, and rbgo runs rescue/ensure bodies as instructions inside the frame that owns them. MRI needs a preallocated instance for exactly the same reason. The constant is a placeholder in this commit; the measurement that chooses it, and the standing test that re-derives it, follow. Co-Authored-By: Claude Opus 5 --- internal/vm/builtins.go | 9 ++++++ internal/vm/stackdepth.go | 65 +++++++++++++++++++++++++++++++++++++++ internal/vm/vm.go | 40 +++++++++++++++++++++++- 3 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 internal/vm/stackdepth.go diff --git a/internal/vm/builtins.go b/internal/vm/builtins.go index 6a17ba39..7629c1e3 100644 --- a/internal/vm/builtins.go +++ b/internal/vm/builtins.go @@ -691,6 +691,15 @@ func (vm *VM) bootstrap() { // `rescue` does not catch them — matching MRI. SystemExit additionally carries // an exit status (defined below). exc("NoMemoryError", "Exception") + // SystemStackError < Exception, NOT StandardError, so a bare `rescue` does not + // catch it: MRI sets it up that way in Init_Proc, + // `rb_eSysStackError = rb_define_class("SystemStackError", rb_eException)` + // (proc.c v3_4_1:4424), and the line after it registers the preallocated + // instance whose message is "stack level too deep" — the message raiseDeep + // copies verbatim. Verified against the MRI 4.0.5 on the development machine: + // `SystemStackError.ancestors` is [SystemStackError, Exception, Object, + // Kernel, BasicObject], with no StandardError in it. + exc("SystemStackError", "Exception") exc("SecurityError", "Exception") exc("SignalException", "Exception") exc("Interrupt", "SignalException") diff --git a/internal/vm/stackdepth.go b/internal/vm/stackdepth.go new file mode 100644 index 00000000..8acf3a2e --- /dev/null +++ b/internal/vm/stackdepth.go @@ -0,0 +1,65 @@ +package vm + +// Ruby call-depth limit. +// +// Runaway recursion in a Ruby program must raise SystemStackError, which the +// program can rescue. Before this limit existed it instead exhausted the Go +// goroutine stack, and a Go stack overflow is a FATAL RUNTIME ERROR, not a +// panic: `runtime: goroutine stack exceeds 1000000000-byte limit` / +// `fatal error: stack overflow`, exit status 2, with a deferred recover() +// never running. There is therefore no mitigation available to an embedding +// host — ruby.Run is the documented embedding API, so three lines of untrusted +// Ruby could end the host process (issue #768). The limit has to live in the +// interpreter, which is also where MRI puts it. +// +// MRI's own limit is a budget of MACHINE-STACK BYTES plus a VM-stack size +// (rb_execution_context_t's machine.stack_maxsize and vm_stack_size, +// vm_core.h v3_4_1:1070), checked by CHECK_VM_STACK_OVERFLOW +// (vm_core.h v3_4_1:1912) and stack_check/ec_stack_overflow +// (vm_insnhelper.c v3_4_1:59). Expressed in depth, the MRI 4.0.5 on the +// development machine reaches 11913 frames of `def f(n); f(n + 1); end` +// before raising. rbgo counts FRAMES rather than bytes because it has no +// portable way to read the Go stack pointer, and because a frame count is the +// quantity it can restore exactly on every unwind path. +const ( + // defaultMaxCallDepth is the depth at which exec refuses to push frame + // number defaultMaxCallDepth+1 and raises SystemStackError instead. + // + // TO BE CHOSEN BY MEASUREMENT (next commit in this branch), not by taste. + // TestStackDepthLimitSitsBelowTheGoOverflow + // is the standing check: it binary-searches, in subprocesses under a reduced + // runtime/debug.SetMaxStack, the largest depth rbgo can actually reach before + // the Go runtime kills the process, converts that to BYTES OF GO STACK PER + // RUBY FRAME, and fails if this constant is not comfortably below the depth + // that figure implies at Go's default 1 GB maximum. + // + // Bytes-per-frame is the portable quantity and the one worth quoting: it is a + // property of rbgo's call path, whereas the depth it permits moves if Go's + // default stack maximum ever changes. The worst (most Go-stack-hungry) path + // measured is a block yielded through Integer#times, not a plain send, and + // the figure below is that worst path's. + // + // The margin is deliberately large: the + // measurement is per-platform and per-toolchain, a future Ruby-level frame + // may cost more Go stack than today's, and overshooting the budget is fatal + // and unrecoverable while undershooting it merely raises an exception the + // program was already required to handle. + defaultMaxCallDepth = 10000 +) + +// raiseDeep raises SystemStackError with MRI's exact message. +// +// It needs NO Ruby-level frame of its own, which is what makes it safe to call +// from the very place that has just refused to create one: exceptionObject +// builds the exception as a Go *RObject and never dispatches #initialize, and +// rbgo runs `rescue`/`ensure` bodies as instructions inside the frame that owns +// them rather than as new frames. MRI needs a preallocated instance +// (special_exceptions[ruby_error_sysstack], vm_insnhelper.c v3_4_1:61) for the +// same reason: at the moment of overflow there is no room to build one. +// +// The message is copied from MRI rather than invented: +// rb_vm_register_special_exception(ruby_error_sysstack, rb_eSysStackError, +// "stack level too deep") (proc.c v3_4_1:4425). +func raiseDeep() { + raise("SystemStackError", "stack level too deep") +} diff --git a/internal/vm/vm.go b/internal/vm/vm.go index ffadbeb3..ca5464d3 100644 --- a/internal/vm/vm.go +++ b/internal/vm/vm.go @@ -770,8 +770,21 @@ type VM struct { // frameNames is the running method-name stack (innermost last), maintained by // exec for Kernel#__method__ and #caller. GVL-guarded (VM code is serialized // by the GVL). + // + // Its LENGTH is also the Ruby call depth that exec refuses to exceed (see + // maxCallDepth): every Ruby-level frame pushes exactly one entry here, and + // the four places that restore it on an unwind are what make it exact + // rather than merely indicative. frameNames []string + // maxCallDepth is the Ruby call depth at which exec raises SystemStackError + // instead of recursing one frame further. It is defaultMaxCallDepth for a VM + // built by New; it is a field and not the constant itself so a test can lower + // it to a depth a test can reach quickly, and so the measurement that chose + // the constant could raise it. Zero would mean "no limit", which is the + // pre-#768 behaviour and is never what a caller wants, so New always sets it. + maxCallDepth int + // frameFiles is the running source-file stack, kept in lockstep with // frameNames (one entry per exec frame, "" when the ISeq carries no file). It // backs exception backtraces and Kernel#caller, which pair each frame's method @@ -1242,7 +1255,7 @@ func New(out io.Writer) *VM { return NewWithStderr(out, out) } // still captures warnings — which is what mspec's `complain` matcher does, and // why the conformance suite could not observe this defect at all. func NewWithStderr(out, errOut io.Writer) *VM { - vm := &VM{out: out, errOut: errOut, main: object.NewMain(), consts: map[string]object.Value{}, loaded: map[string]bool{}, globals: map[string]object.Value{}} + vm := &VM{out: out, errOut: errOut, main: object.NewMain(), consts: map[string]object.Value{}, loaded: map[string]bool{}, globals: map[string]object.Value{}, maxCallDepth: defaultMaxCallDepth} // The controllable clock reads the real wall clock through nowWall (the same // determinism seam Time.now/Date.today already honour) until a require // "timecop" program freezes/travels/scales it. Unmocked, Current() just @@ -1634,6 +1647,31 @@ func zsuperArgs(iseq *bytecode.ISeq, env *Env) (args []object.Value, kwSplat boo } func (vm *VM) exec(iseq *bytecode.ISeq, self object.Value, args []object.Value, definee *RClass, methodName string, parentEnv *Env, block, selfBlock, blockArg *Proc, methodLexScope *RClass) (execResult object.Value) { + // The call-depth limit, checked FIRST: before any of the pending-state + // handovers below are consumed, and before the frame pushes anything, so a + // refused frame leaves the VM exactly as it found it. + // + // exec is the ONE place a Ruby-level frame is created — methods, blocks, + // procs, lambdas, define_method bodies, class and module bodies, eval and + // instance_eval all arrive here — so one check here covers every way a Ruby + // program can recurse. See stackdepth.go for why the limit exists and how + // defaultMaxCallDepth was measured. + // + // The depth is len(vm.frameNames) and NOT a counter of its own, which is the + // whole reason this cannot leak on a non-local exit. A separate counter would + // have to be decremented on each of exec's exits — normal return, a rescued + // exception, a non-local return/next, break, throw — and the ones that unwind + // by panic do not run exec's tail. frameNames already solves that problem, + // and solves it by TRUNCATING TO A SAVED DEPTH rather than by counting pops: + // whoever catches an unwind resets the stack to its own depth (vm.go's + // returnSignal and rescue recovers, Run's four boundary resets), which + // discards the entries of every abandoned frame in one assignment whether + // there was one or ten thousand. That makes the depth self-healing: it cannot + // be left inflated by a rescued runaway recursion and so cannot poison a + // later one. + if len(vm.frameNames) >= vm.maxCallDepth { + raiseDeep() + } // Determine this frame's __method__/__callee__ pair before any nested call can // disturb pendingMethodCtx: a block inherits the pair captured on the block // Proc, an invokeBody/eval caller supplies one via pendingMethodCtx, and any From f7e64e881a5d665dbdce8d468779d5a7d8fc75f0 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 4 Oct 2026 20:44:30 +0200 Subject: [PATCH 2/5] fix(vm): a block `break` leaked one frame-stack entry for good (#768) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sendCatchBreak recovers a breakSignal and returns its value, but it never restored the per-frame tracking stacks. The block's exec frame — and every frame between it and the catch — unwound by panic and so never ran exec's tail that pops them, leaving [1, 2, 3].each { |x| break if x == 2 } one frameNames entry heavier than it started, permanently. This was invisible while those stacks only fed backtraces and Kernel#caller: the next unwind CAUGHT anywhere truncates to its own saved depth and swallows the stale entry with it, so the leak only survives when nothing deeper is caught afterwards. No backtrace test ever stood in that position. It stopped being invisible the moment len(frameNames) became the call-depth limit: leaked depth is budget that never comes back, so a long-running program breaking out of a loop a few tens of thousands of times would raise SystemStackError with no recursion in sight. The test that found it measures the depth a runaway recursion reaches, twice, with the non-local exit in between — 197 then 196 is the leak, 197 then 197 is the fix. The restoration is the same six depths Kernel#catch and findYield keep (find.go), for the same reason. frameCrefs and frameMethods mirror frameNames one for one and are restored WITH it, or index i stops meaning the same frame in all three (#649). TestDepthDoesNotLeakOnAnyNonLocalExit covers break, next, a block `return`, throw/catch, an ensure crossed by a raise, and the deep variants of each; TestASecondRunawayRecursionStillRaises states the consequence a user would notice. Co-Authored-By: Claude Opus 5 --- internal/vm/stackdepth_internal_test.go | 308 ++++++++++++++++++++++++ internal/vm/vm.go | 31 +++ 2 files changed, 339 insertions(+) create mode 100644 internal/vm/stackdepth_internal_test.go diff --git a/internal/vm/stackdepth_internal_test.go b/internal/vm/stackdepth_internal_test.go new file mode 100644 index 00000000..34e3d0d8 --- /dev/null +++ b/internal/vm/stackdepth_internal_test.go @@ -0,0 +1,308 @@ +package vm + +import ( + "bytes" + "strings" + "testing" + + "github.com/go-embedded-ruby/ruby/internal/compiler" + "github.com/go-ruby-parser/parser" +) + +// runAtDepth runs src on a VM whose call-depth limit is `limit`, and returns its +// stdout. The limit is lowered from defaultMaxCallDepth so a test can reach it in +// milliseconds: at the real limit every one of these programs would allocate +// hundreds of megabytes of Go stack before raising, which is a measurement +// (TestStackDepthLimitSitsBelowTheGoOverflow) and not something to pay for in +// every test that only wants to know WHAT is raised. +func runAtDepth(t *testing.T, limit int, src string) string { + t.Helper() + prog, err := parser.Parse(src) + if err != nil { + t.Fatalf("parse: %v", err) + } + iseq, err := compiler.Compile(prog) + if err != nil { + t.Fatalf("compile: %v", err) + } + var buf bytes.Buffer + vm := New(&buf) + vm.maxCallDepth = limit + if _, err := vm.Run(iseq); err != nil { + t.Fatalf("run: %v\noutput so far:\n%s", err, buf.String()) + } + return strings.TrimRight(buf.String(), "\n") +} + +// TestSystemStackErrorSitsUnderExceptionNotStandardError pins the hierarchy +// against MRI's, which is what decides whether a bare `rescue` catches it. +// +// MRI: `rb_eSysStackError = rb_define_class("SystemStackError", rb_eException)`, +// proc.c v3_4_1:4424. Verified against the MRI 4.0.5 on the development machine: +// `SystemStackError.ancestors` is [SystemStackError, Exception, Object, Kernel, +// BasicObject] — no StandardError anywhere in it. +func TestSystemStackErrorSitsUnderExceptionNotStandardError(t *testing.T) { + got := runSrc(t, ` +p defined?(SystemStackError) +p SystemStackError.superclass +p SystemStackError.ancestors +p SystemStackError.ancestors.include?(StandardError) +`) + want := strings.Join([]string{ + `"constant"`, + `Exception`, + `[SystemStackError, Exception, Object, Kernel, BasicObject]`, + `false`, + }, "\n") + if got != want { + t.Errorf("hierarchy:\ngot:\n%s\nwant:\n%s", got, want) + } +} + +// TestRunawayRecursionRaisesARescuableSystemStackError is the defect itself: this +// program used to print `runtime: goroutine stack exceeds 1000000000-byte limit` +// and a 50 KB goroutine dump, and exit 2 — a FATAL Go runtime error, which a host +// embedding rbgo through ruby.Run cannot recover() from (issue #768). +func TestRunawayRecursionRaisesARescuableSystemStackError(t *testing.T) { + got := runAtDepth(t, 200, ` +def f(n); f(n + 1); end +begin + f(0) +rescue SystemStackError => e + puts "SystemStackError rescued: #{e.class}" + puts e.message +end +puts "still alive" +`) + // "stack level too deep" is MRI's message, copied from + // rb_vm_register_special_exception(..., "stack level too deep"), + // proc.c v3_4_1:4425 — not invented here. + want := "SystemStackError rescued: SystemStackError\nstack level too deep\nstill alive" + if got != want { + t.Errorf("got:\n%s\nwant:\n%s", got, want) + } +} + +// TestBareRescueDoesNotCatchSystemStackError is the consequence of the hierarchy +// above, and the reason the hierarchy is worth a test of its own. Measured on the +// MRI 4.0.5 on this machine: the bare `rescue` does not fire. +func TestBareRescueDoesNotCatchSystemStackError(t *testing.T) { + got := runAtDepth(t, 200, ` +def f(n); f(n + 1); end +begin + begin + f(0) + rescue => e + puts "WRONG: bare rescue caught #{e.class}" + end +rescue SystemStackError + puts "not caught by bare rescue" +end +`) + if got != "not caught by bare rescue" { + t.Errorf("got %q, want %q", got, "not caught by bare rescue") + } +} + +// TestEveryKindOfRubyFrameIsCounted checks the claim that exec is the ONE place a +// Ruby-level frame is created, so one check covers every way to recurse. Each of +// these recurses through a different dispatch path and each must raise rather +// than reach the Go runtime. +func TestEveryKindOfRubyFrameIsCounted(t *testing.T) { + for _, tc := range []struct{ name, body string }{ + {"method", `def f(n); f(n + 1); end`}, + {"kernel_send", `def f(n); send(:f, n + 1); end`}, + {"public_send", `def f(n); public_send(:f, n + 1); end`}, + {"block", `def f(n); 1.times { f(n + 1) }; end`}, + {"each_block", `def f(n); [1].each { f(n + 1) }; end`}, + {"yield", `def g; yield; end +def f(n); g { f(n + 1) }; end`}, + {"define_method", `define_method(:f) { |n| f(n + 1) }`}, + {"lambda", `F = ->(n) { F.call(n + 1) } +def f(n); F.call(n); end`}, + {"proc_call", `P = proc { |n| P.call(n + 1) } +def f(n); P.call(n); end`}, + {"method_object", `def f(n); method(:f).call(n + 1); end`}, + {"method_missing", `class Object + def method_missing(m, *a); m == :zz ? f(a[0] + 1) : super; end +end +def f(n); zz(n); end`}, + {"instance_eval", `def f(n); 1.instance_eval { f(n + 1) }; end`}, + {"tap", `def f(n); 1.tap { f(n + 1) }; end`}, + {"super", `class A; def f(n); n; end; end +class B < A; def f(n); C.new.f(n + 1); end; end +class C < A; def f(n); B.new.f(n + 1); end; end +def f(n); B.new.f(n); end`}, + } { + t.Run(tc.name, func(t *testing.T) { + got := runAtDepth(t, 200, tc.body+` +begin + f(0) +rescue SystemStackError + puts "raised" +end +`) + if got != "raised" { + t.Errorf("%s: got %q, want %q — this path reached the Go stack instead", tc.name, got, "raised") + } + }) + } +} + +// TestDepthDoesNotLeakOnAnyNonLocalExit is the test the design owes, because the +// depth is len(vm.frameNames) and exec has no defer that decrements it: the +// restoration is done by whoever CATCHES an unwind, truncating the stack back to +// its own saved depth. +// +// Each case drives a runaway recursion to the limit, rescues it, then does it +// AGAIN and reports the depth reached both times. Equal depths prove the counter +// came all the way back; a leak would show as a smaller second number (the second +// recursion starting pre-poisoned by the first), which is the exact failure a +// hand-rolled counter without a defer would produce. +func TestDepthDoesNotLeakOnAnyNonLocalExit(t *testing.T) { + // The interleaved non-local exit, run between the two recursions: each is a + // path that leaves exec WITHOUT running its tail. + for _, tc := range []struct{ name, between string }{ + {"nothing", ``}, + {"break", `[1, 2, 3].each { |x| break if x == 2 }`}, + {"next", `[1, 2, 3].each { |x| next if x == 2 }`}, + {"return_from_block", `def r; [1, 2, 3].each { |x| return x if x == 2 }; end +r`}, + {"throw_catch", `catch(:t) { [1].each { throw :t, 9 } }`}, + {"ensure_through_raise", `begin + (begin; raise "x"; ensure; nil; end) +rescue RuntimeError + nil +end`}, + {"deep_return_from_block", `def r(n); [1].each { return n if n > 50; r(n + 1) }; nil; end +r(0)`}, + {"deep_break", `def r(n); [1].each { break if n > 50; r(n + 1) }; nil; end +r(0)`}, + {"deep_throw", `def r(n); throw :deep if n > 50; r(n + 1); end +catch(:deep) { r(0) }`}, + {"deep_ensure", `def r(n); begin; r(n + 1) if n < 50; ensure; nil; end; end +r(0)`}, + } { + t.Run(tc.name, func(t *testing.T) { + got := runAtDepth(t, 200, ` +$d = 0 +def f(n); $d = n; f(n + 1); end +def probe + $d = 0 + begin + f(0) + rescue SystemStackError + end + $d +end +first = probe +`+tc.between+` +second = probe +puts first +puts second == first ? "same" : "LEAKED: second=#{second} first=#{first}" +`) + lines := strings.Split(got, "\n") + if len(lines) != 2 { + t.Fatalf("got %q, want two lines", got) + } + // The depth reached must be the limit, less the frames the probe + // itself occupies (top level, probe, and the begin/rescue): a number + // that is merely non-zero would pass even if the limit fired early. + if lines[0] != "197" { + t.Errorf("first recursion reached depth %s, want 197 (limit 200 less the probe's own frames)", lines[0]) + } + if lines[1] != "same" { + t.Errorf("after %s: %s", tc.name, lines[1]) + } + }) + } +} + +// TestASecondRunawayRecursionStillRaises states the leak consequence as the thing +// a user would notice, separately from the depth arithmetic above: a program that +// rescues one runaway recursion must still be able to run — and still be +// protected — afterwards. +func TestASecondRunawayRecursionStillRaises(t *testing.T) { + got := runAtDepth(t, 200, ` +def f(n); f(n + 1); end +3.times do |i| + begin + f(0) + puts "WRONG: no raise on round #{i}" + rescue SystemStackError + puts "round #{i} raised" + end +end +puts "still alive" +`) + want := "round 0 raised\nround 1 raised\nround 2 raised\nstill alive" + if got != want { + t.Errorf("got:\n%s\nwant:\n%s", got, want) + } +} + +// TestRescuingAtTheLimitTerminates covers the one shape that could in principle +// spin: a `rescue` written INSIDE the recursive method, so the handler runs at a +// depth that is itself at the limit and whatever it calls raises again. Each +// re-raise is caught one frame further out, so the cascade is bounded by the +// depth — but "bounded" is a claim worth a test, since the alternative is a hang. +func TestRescuingAtTheLimitTerminates(t *testing.T) { + got := runAtDepth(t, 120, ` +$n = 0 +def f(n) + begin + f(n + 1) + rescue SystemStackError + $n += 1 + raise + end +end +begin + f(0) +rescue SystemStackError + puts "terminated" +end +puts $n > 0 +`) + if got != "terminated\ntrue" { + t.Errorf("got %q, want %q", got, "terminated\ntrue") + } +} + +// TestDeepRecursionBelowTheLimitStillWorks is the other half of the gate: the +// limit must not break legitimate recursion. 150 frames under a limit of 200 +// must complete normally. +func TestDeepRecursionBelowTheLimitStillWorks(t *testing.T) { + got := runAtDepth(t, 200, ` +def f(n); n == 0 ? 0 : 1 + f(n - 1); end +puts f(150) +`) + if got != "150" { + t.Errorf("got %q, want %q", got, "150") + } +} + +// TestDefaultCallDepthLimitLeavesRoomForMRIProgrammes guards the conformance side +// of the constant from below. MRI's limit is a budget of machine-stack BYTES, so +// its depth depends on how big a frame is: the MRI 4.0.5 on the development +// machine reaches 10919 frames of `def f(n); $d = n; f(n + 1); end` and only 3853 +// levels of the same recursion through a block. A limit below MRI's deepest would +// make rbgo raise where MRI does not, turning a security fix into a conformance +// regression. +func TestDefaultCallDepthLimitLeavesRoomForMRIProgrammes(t *testing.T) { + const deepestObservedMRIDepth = 10919 + if defaultMaxCallDepth <= deepestObservedMRIDepth { + t.Errorf("defaultMaxCallDepth = %d is not above the deepest depth MRI 4.0.5 was measured to reach (%d): a program that recurses that far works on MRI and would raise here", + defaultMaxCallDepth, deepestObservedMRIDepth) + } +} + +// TestANewVMAlwaysHasALimit pins the thing that would silently restore the defect: +// a VM built by the public constructor must never have maxCallDepth 0, which the +// `>=` check would read as "refuse every frame" — and, were the check written the +// other way round, as "no limit at all". +func TestANewVMAlwaysHasALimit(t *testing.T) { + if got := New(&bytes.Buffer{}).maxCallDepth; got != defaultMaxCallDepth { + t.Errorf("New(...).maxCallDepth = %d, want defaultMaxCallDepth = %d", got, defaultMaxCallDepth) + } +} diff --git a/internal/vm/vm.go b/internal/vm/vm.go index ca5464d3..3509c98e 100644 --- a/internal/vm/vm.go +++ b/internal/vm/vm.go @@ -125,9 +125,40 @@ func (vm *VM) sendCatchBreak(recv object.Value, name string, args []object.Value blk.breakLive = true defer func() { blk.breakLive = prev }() } + // Snapshot the per-frame tracking-stack depths, so that catching the break + // below restores them: the block's exec frame — and every frame between it + // and here — unwound by panic and so never ran exec's tail that pops them. + // + // Without this, `[1, 2, 3].each { |x| break if x == 2 }` left ONE frameNames + // entry behind for good. That was invisible while these stacks only fed + // backtraces and Kernel#caller, because the next unwind to be CAUGHT anywhere + // truncates to its own saved depth and swallows the stale entry with it — the + // leak only survives when nothing deeper is caught afterwards, which is why + // no backtrace test ever saw it. It stopped being invisible when + // len(frameNames) became the call-depth limit (stackdepth.go): a leak there is + // depth budget that never comes back, so a long-running program breaking out + // of a loop a few tens of thousands of times would eventually raise + // SystemStackError with no recursion in sight. + // + // These are the same six depths Kernel#catch and findYield keep (find.go), and + // for the same reason; frameCrefs and frameMethods mirror frameNames one for + // one and must be restored WITH it, or index i stops meaning the same frame in + // all three (#649). + fileStackDepth := len(vm.fileStack) + frameNamesDepth := len(vm.frameNames) + frameFilesDepth := len(vm.frameFiles) + frameCrefsDepth := len(vm.frameCrefs) + frameMethodsDepth := len(vm.frameMethods) + requireDirsDepth := len(vm.requireDirs) defer func() { if r := recover(); r != nil { if sig, ok := r.(breakSignal); ok && sig.owner == blk { + vm.fileStack = truncFrames(vm.fileStack, fileStackDepth) + vm.frameNames = truncFrames(vm.frameNames, frameNamesDepth) + vm.frameFiles = truncFrames(vm.frameFiles, frameFilesDepth) + vm.frameCrefs = truncFrames(vm.frameCrefs, frameCrefsDepth) + vm.frameMethods = truncFrames(vm.frameMethods, frameMethodsDepth) + vm.requireDirs = truncFrames(vm.requireDirs, requireDirsDepth) result = sig.value return } From 1f471f9fa8896b12611fe6ac7c88aac0b7fce46a Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 4 Oct 2026 20:51:44 +0200 Subject: [PATCH 3/5] fix(vm): set the call-depth limit from a measurement, 16384 (#768) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 28 recursion paths bisected in subprocesses — binary-searching the largest limit each path can be given before the Go runtime kills the process, under a reduced debug.SetMaxStack — to get BYTES OF GO STACK PER RUBY FRAME, which is the portable quantity. Worst and best, on darwin/arm64 with Go 1.27.1: send(:public_send, :send, :f, n + 1) 8132 B/frame WORST catch(:x) { f(n + 1) } 6531 B/frame 1.times { f(n + 1) } 6228 B/frame f(n + 1) 5619 B/frame best Measuring the cheapest path would have overstated the margin by 45%. The figure is stable across stack sizes — 6452.8, 6450.3, 6448.4, 6448.1 B/frame at SetMaxStack of 8, 16, 64 and 256 MiB — so it is a property of the call path, not of the measurement. THE DEPTH IT IMPLIES IS NOT 1 GB DIVIDED BY IT, and that error would have been a factor of two. Go's default maximum is 1,000,000,000 bytes (decimal) and goroutine stacks are powers of two, so the largest stack actually permitted is 2^29 = 512 MiB; 2^30 exceeds the default. The worst path's overflow depth is 512 MiB / 8132 = 66016, and bisecting it directly at Go's real default with no SetMaxStack measured 66051 — 0.05% apart, which is what makes the model quotable rather than a story about one number. 16384 is 4.03x below that 66051 and 1.50x above MRI's deepest reachable depth (10919 frames of `def f(n); $d = n; f(n + 1); end` on the MRI 4.0.5 on this machine; only 3853 levels through a block, because MRI's budget is in BYTES and so its depth is not one number). Above MRI means no program that works there raises here; the asymmetric margin below is because overshooting is fatal and unrecoverable while undershooting raises an exception the program already had to handle. TestStackDepthLimitSitsBelowTheGoOverflow re-derives the whole thing on every run and fails under a 2x margin. KNOWN INCOMPLETE, and measured rather than assumed: a frame count cannot bound the Go stack on its own, because a program can put an unbounded number of NATIVE dispatch layers between two Ruby frames, and a native layer costs Go stack while pushing no frame. Each extra Kernel#send layer costs 833 B: sends per Ruby frame 1 2 4 8 16 32 bytes per frame 7282 8117 9777 13107 19761 33091 overflow depth 73728 66144 54912 40960 27168 16224 so ~25 nested sends per frame bring the overflow depth below this limit, and NO limit above MRI's 10919 can cover an unbounded chain. Closing it needs a count of native dispatch nesting too, which is a change to the hot dispatch path (vm.send) and a scope decision of its own rather than a line added here. Recorded in stackdepth.go and on issue #768 rather than left for the next person to rediscover. Co-Authored-By: Claude Opus 5 --- internal/vm/stackdepth.go | 106 +++++++++---- .../vm/stackdepth_measure_internal_test.go | 148 ++++++++++++++++++ 2 files changed, 224 insertions(+), 30 deletions(-) create mode 100644 internal/vm/stackdepth_measure_internal_test.go diff --git a/internal/vm/stackdepth.go b/internal/vm/stackdepth.go index 8acf3a2e..a718f44a 100644 --- a/internal/vm/stackdepth.go +++ b/internal/vm/stackdepth.go @@ -6,45 +6,91 @@ package vm // program can rescue. Before this limit existed it instead exhausted the Go // goroutine stack, and a Go stack overflow is a FATAL RUNTIME ERROR, not a // panic: `runtime: goroutine stack exceeds 1000000000-byte limit` / -// `fatal error: stack overflow`, exit status 2, with a deferred recover() -// never running. There is therefore no mitigation available to an embedding -// host — ruby.Run is the documented embedding API, so three lines of untrusted -// Ruby could end the host process (issue #768). The limit has to live in the +// `fatal error: stack overflow`, exit status 2, with a deferred recover() never +// running. There is therefore no mitigation available to an embedding host — +// ruby.Run is the documented embedding API, so three lines of untrusted Ruby +// could end the host process (issue #768, where the recover() and +// debug.SetMaxStack controls are written up). The limit has to live in the // interpreter, which is also where MRI puts it. // // MRI's own limit is a budget of MACHINE-STACK BYTES plus a VM-stack size -// (rb_execution_context_t's machine.stack_maxsize and vm_stack_size, -// vm_core.h v3_4_1:1070), checked by CHECK_VM_STACK_OVERFLOW -// (vm_core.h v3_4_1:1912) and stack_check/ec_stack_overflow -// (vm_insnhelper.c v3_4_1:59). Expressed in depth, the MRI 4.0.5 on the -// development machine reaches 11913 frames of `def f(n); f(n + 1); end` -// before raising. rbgo counts FRAMES rather than bytes because it has no -// portable way to read the Go stack pointer, and because a frame count is the -// quantity it can restore exactly on every unwind path. +// (rb_execution_context_t's machine.stack_maxsize, vm_core.h v3_4_1:1070), +// checked by CHECK_VM_STACK_OVERFLOW (vm_core.h v3_4_1:1912) and by +// stack_check/ec_stack_overflow (vm_insnhelper.c v3_4_1:59). +// +// rbgo counts FRAMES rather than bytes because it has no portable way to read +// the Go stack pointer: a stored stack address cannot be used as a baseline, +// since the Go runtime COPIES a goroutine's stack when it grows and the saved +// address is then stale by the relocation delta. A frame count is also the +// quantity rbgo can restore exactly on every unwind path. +// +// Because MRI's budget is in bytes, ITS depth depends on how big a frame is, +// and is therefore not one number. Measured against the MRI 4.0.5 on the +// development machine: +// +// def f(n); $d = n; f(n + 1); end 10919 frames +// def f(n); $d = n; 1.times { f(n + 1) }; end 3853 levels +// +// 10919 is the figure that matters here: a limit below MRI's DEEPEST reachable +// depth would make rbgo raise where MRI does not, turning a security fix into a +// conformance regression. TestDefaultCallDepthLimitLeavesRoomForMRIProgrammes +// is the guard from that side. const ( // defaultMaxCallDepth is the depth at which exec refuses to push frame // number defaultMaxCallDepth+1 and raises SystemStackError instead. // - // TO BE CHOSEN BY MEASUREMENT (next commit in this branch), not by taste. - // TestStackDepthLimitSitsBelowTheGoOverflow - // is the standing check: it binary-searches, in subprocesses under a reduced - // runtime/debug.SetMaxStack, the largest depth rbgo can actually reach before - // the Go runtime kills the process, converts that to BYTES OF GO STACK PER - // RUBY FRAME, and fails if this constant is not comfortably below the depth - // that figure implies at Go's default 1 GB maximum. + // CHOSEN BY MEASUREMENT, not by taste. 28 recursion paths were bisected in + // subprocesses — binary-searching the largest limit the path can be given + // before the Go runtime kills the process — which gives BYTES OF GO STACK + // PER RUBY FRAME, the portable quantity. Worst and best of the 28, on + // darwin/arm64 with Go 1.27.1: + // + // send(:public_send, :send, :f, n + 1) 8132 B/frame WORST + // catch(:x) { f(n + 1) } 6531 B/frame + // 1.times { f(n + 1) } 6228 B/frame + // [1].inject(0) { f(n + 1) } 5998 B/frame + // f(n + 1) 5619 B/frame best + // + // Bytes-per-frame is dead stable across stack sizes — 6452.8, 6450.3, + // 6448.4 and 6448.1 B/frame at SetMaxStack of 8, 16, 64 and 256 MiB — so it + // is a property of rbgo's call path and not of the measurement. + // + // THE DEPTH IT IMPLIES IS NOT 1 GB DIVIDED BY IT, and getting that wrong + // would have been a factor of two. Go's default maximum is 1,000,000,000 + // bytes (decimal), and goroutine stacks are POWERS OF TWO, so the largest + // stack actually permitted is 2^29 = 536,870,912 bytes — 512 MiB, since + // 2^30 exceeds the default. The worst path's overflow depth is therefore + // 512 MiB / 8132 = 66016, and bisecting it directly at Go's real default, + // with no SetMaxStack at all, measured 66051. The prediction and the direct + // measurement agree to 0.05%, which is what makes the model quotable. + // + // 16384 sits 4.03x below that measured 66051, and 1.50x above MRI's deepest + // 10919. The margin is deliberately large and asymmetric on purpose: + // overshooting the budget is fatal and unrecoverable, while undershooting + // it merely raises an exception the program was already required to handle. + // It also has to absorb a figure measured on ONE platform and toolchain, + // and a future Ruby-level frame that costs more Go stack than today's. + // At the limit the worst path holds about 133 MB of Go stack. + // + // TestStackDepthLimitSitsBelowTheGoOverflow re-derives all of this on every + // run and fails if the margin erodes. + // + // KNOWN INCOMPLETE, and measured rather than assumed: a frame count cannot + // bound the Go stack on its own, because a program can put an unbounded + // number of NATIVE dispatch layers between two Ruby frames, and a native + // layer costs Go stack while pushing no frame. Each extra Kernel#send layer + // in `send(:send, ..., :f, n + 1)` costs 833 B: // - // Bytes-per-frame is the portable quantity and the one worth quoting: it is a - // property of rbgo's call path, whereas the depth it permits moves if Go's - // default stack maximum ever changes. The worst (most Go-stack-hungry) path - // measured is a block yielded through Integer#times, not a plain send, and - // the figure below is that worst path's. + // sends per Ruby frame 1 2 4 8 16 32 + // bytes per frame 7282 8117 9777 13107 19761 33091 + // overflow depth 73728 66144 54912 40960 27168 16224 // - // The margin is deliberately large: the - // measurement is per-platform and per-toolchain, a future Ruby-level frame - // may cost more Go stack than today's, and overshooting the budget is fatal - // and unrecoverable while undershooting it merely raises an exception the - // program was already required to handle. - defaultMaxCallDepth = 10000 + // so about 25 nested sends per frame bring the overflow depth below this + // limit, and no limit above MRI's 10919 can cover an unbounded chain. + // Closing that needs a count of native dispatch nesting as well, which is a + // change to the hot dispatch path (vm.send) and so a scope decision of its + // own rather than a line added here — see issue #768. + defaultMaxCallDepth = 16384 ) // raiseDeep raises SystemStackError with MRI's exact message. diff --git a/internal/vm/stackdepth_measure_internal_test.go b/internal/vm/stackdepth_measure_internal_test.go new file mode 100644 index 00000000..5cd3a852 --- /dev/null +++ b/internal/vm/stackdepth_measure_internal_test.go @@ -0,0 +1,148 @@ +package vm + +import ( + "io" + "os" + "os/exec" + "runtime" + "runtime/debug" + "strconv" + "testing" + + "github.com/go-embedded-ruby/ruby/internal/compiler" + "github.com/go-ruby-parser/parser" +) + +// The worst (most Go-stack-hungry) of the 28 recursion paths measured when +// defaultMaxCallDepth was chosen: three nested native dispatch layers per Ruby +// frame, at 8132 bytes of Go stack per frame against 5619 for a plain `f(n+1)`. +// Measuring the margin on the cheapest path would overstate it by 45%. +const worstMeasuredRecursion = ` +def f(n); send(:public_send, :send, :f, n + 1); end +begin + f(0) +rescue SystemStackError +end +` + +// measureStackChildEnv names the environment variable that turns this test +// binary into the child half of the measurement: a depth limit to run under. +const measureStackChildEnv = "RBGO_STACKDEPTH_CHILD" + +// stackDepthChild is the child half. It runs the worst recursion path under the +// given call-depth limit and a REDUCED runtime/debug.SetMaxStack, and exits 0 +// only if the program completed — i.e. if SystemStackError was raised before the +// Go runtime ran out of stack. Exceeding the stack is fatal and kills the +// process, which is precisely the signal the parent bisects on, and precisely +// why this cannot be done in-process. +// +// The stack limit is reduced so each probe costs milliseconds: at Go's real +// default the recursion would have to allocate half a gigabyte of stack to +// reach the overflow. What the parent recovers from the small-stack measurement +// is BYTES PER FRAME, which is stable across stack sizes (see stackdepth.go), +// and the depth at the real default follows from it. +func stackDepthChild() { + limit, err := strconv.Atoi(os.Getenv(measureStackChildEnv)) + if err != nil { + os.Exit(3) + } + maxStack, err := strconv.Atoi(os.Getenv("RBGO_STACKDEPTH_MAXSTACK")) + if err != nil { + os.Exit(3) + } + debug.SetMaxStack(maxStack) + prog, err := parser.Parse(worstMeasuredRecursion) + if err != nil { + os.Exit(3) + } + iseq, err := compiler.Compile(prog) + if err != nil { + os.Exit(3) + } + // On its own goroutine, so the measured stack is the interpreter's and not + // the testing framework's. + status := make(chan int, 1) + go func() { + vm := New(io.Discard) + vm.maxCallDepth = limit + if _, err := vm.Run(iseq); err != nil { + status <- 4 + return + } + status <- 0 + }() + os.Exit(<-status) +} + +// TestStackDepthLimitSitsBelowTheGoOverflow is the standing form of the +// measurement that chose defaultMaxCallDepth, and the guard that keeps it +// honest: it fails if a change to rbgo makes a Ruby frame expensive enough in Go +// stack that the limit stops being comfortably below the depth at which the Go +// runtime kills the process. +// +// It bisects, in subprocesses, the largest limit the worst recursion path can be +// given before the process dies, under a deliberately small SetMaxStack. That +// yields bytes of Go stack per Ruby frame, from which the depth at Go's real +// default follows — NOT by dividing the 1,000,000,000-byte default by it, but by +// dividing 2^29, because goroutine stacks are powers of two and 2^30 exceeds the +// default. Getting that wrong overstates the available depth by exactly 2x; see +// stackdepth.go, where the direct measurement at the real default (66051) is +// recorded against this model's prediction (66016). +func TestStackDepthLimitSitsBelowTheGoOverflow(t *testing.T) { + if os.Getenv(measureStackChildEnv) != "" { + stackDepthChild() + return + } + if runtime.GOARCH == "wasm" { + t.Skip("wasm cannot spawn the subprocess this measurement needs") + } + const maxStack = 8 << 20 + survives := func(limit int) bool { + cmd := exec.Command(os.Args[0], "-test.run=TestStackDepthLimitSitsBelowTheGoOverflow", "-test.timeout=10m") + cmd.Env = append(os.Environ(), + measureStackChildEnv+"="+strconv.Itoa(limit), + "RBGO_STACKDEPTH_MAXSTACK="+strconv.Itoa(maxStack)) + cmd.Stdout, cmd.Stderr = io.Discard, io.Discard + return cmd.Run() == nil + } + // A limit of 1 must always be survivable; if even that kills the process the + // measurement is measuring something other than what it thinks. + if !survives(1) { + t.Fatal("the child died at a call-depth limit of 1: the measurement is not measuring recursion") + } + lo, hi := 1, 2 + for hi < 1<<22 && survives(hi) { + lo = hi + hi *= 2 + } + if hi >= 1<<22 { + t.Fatalf("no limit below %d overflowed an %d-byte stack: either the limit is not being applied or SetMaxStack is not honoured", 1<<22, maxStack) + } + for lo+1 < hi { + mid := (lo + hi) / 2 + if survives(mid) { + lo = mid + } else { + hi = mid + } + } + bytesPerFrame := float64(maxStack) / float64(lo) + // 2^29 and not 2^30: see the comment above and stackdepth.go. + const largestGoStackUnderTheDefault = 1 << 29 + overflowDepth := int(largestGoStackUnderTheDefault / bytesPerFrame) + t.Logf("worst measured recursion path: reached depth %d under a %d-byte stack = %.0f bytes of Go stack per Ruby frame", + lo, maxStack, bytesPerFrame) + t.Logf("implied overflow depth at Go's default maximum (largest power-of-two stack under 1,000,000,000 = %d bytes): %d", + largestGoStackUnderTheDefault, overflowDepth) + t.Logf("defaultMaxCallDepth = %d, margin %.2fx", defaultMaxCallDepth, float64(overflowDepth)/float64(defaultMaxCallDepth)) + + // The margin the constant was chosen with is 4.03x. Failing below 2x leaves + // room for the platform-to-platform variation this single-platform + // measurement cannot see, while still catching a real erosion: a change that + // doubled the Go stack cost of a Ruby frame would trip it. + const minMargin = 2.0 + if got := float64(overflowDepth) / float64(defaultMaxCallDepth); got < minMargin { + t.Errorf("defaultMaxCallDepth = %d is only %.2fx below the measured Go overflow depth %d (%.0f bytes/frame); want at least %.1fx. A Ruby frame has become more expensive in Go stack — lower the constant rather than lowering this bound.", + defaultMaxCallDepth, got, overflowDepth, bytesPerFrame, minMargin) + } +} From 2f71e7edeff5938881d8acaf775779c31f694476 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 4 Oct 2026 20:56:16 +0200 Subject: [PATCH 4/5] test(vm): the limit holds across a thread boundary (#768) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A shape the design could plausibly have got wrong. rbgo keeps ONE frame stack for the whole VM, shared by every Ruby thread under the GVL, and Run resets it to [:0] at its boundaries — so a thread body could have been handed a FRESH depth budget while its parent's Go frames were still live, letting a program amplify its reachable depth by one limit per thread it spawns. Measured with the real constant rather than reasoned about: plain recursion reaches 16382 and the thread-nested shape reaches 8191, exactly half, because each level costs two entries (the method frame and the block frame). The shared stack keeps accumulating across the boundary instead of restarting. That is conservative — each Ruby thread is a goroutine with its own Go stack, so the budget is stricter than the stacks require — and conservative is the safe direction for a limit whose job is to stay below a fatal error. Co-Authored-By: Claude Opus 5 --- internal/vm/stackdepth_internal_test.go | 35 +++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/internal/vm/stackdepth_internal_test.go b/internal/vm/stackdepth_internal_test.go index 34e3d0d8..e9653279 100644 --- a/internal/vm/stackdepth_internal_test.go +++ b/internal/vm/stackdepth_internal_test.go @@ -306,3 +306,38 @@ func TestANewVMAlwaysHasALimit(t *testing.T) { t.Errorf("New(...).maxCallDepth = %d, want defaultMaxCallDepth = %d", got, defaultMaxCallDepth) } } + +// TestTheLimitHoldsAcrossAThreadBoundary checks a shape the design could +// plausibly have got wrong. rbgo keeps ONE frame stack for the whole VM, shared +// by every Ruby thread under the GVL, and Run resets it to [:0] at its +// boundaries — so a thread body could have been handed a FRESH depth budget +// while its parent's Go frames were still live, letting a program amplify its +// reachable depth by one limit per thread it spawns. +// +// It does not: the shared stack keeps accumulating across the boundary, so the +// recursion below reaches HALF the limit in levels (each level costs two +// entries, the method frame and the block frame) rather than restarting. That is +// conservative — each Ruby thread is a goroutine with its own Go stack, so the +// budget is stricter than the stacks require — and conservative is the safe +// direction for a limit whose job is to stay below a fatal error. +// +// Measured with the real constant before it was written down: plain recursion +// reached 16382 and this shape reached 8191, exactly half. +func TestTheLimitHoldsAcrossAThreadBoundary(t *testing.T) { + got := runAtDepth(t, 60, ` +$max = 0 +def g(n) + $max = n if n > $max + Thread.new { g(n + 1) }.join +end +begin + g(0) +rescue SystemStackError +end +puts $max < 60 +puts $max > 10 +`) + if got != "true\ntrue" { + t.Errorf("got %q, want %q — a thread boundary either reset the depth budget or exhausted it too early", got, "true\ntrue") + } +} From d5925e7f45e368fcf5d8935500d8f695aa67658e Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 4 Oct 2026 21:03:12 +0200 Subject: [PATCH 5/5] fix(vm): set maxCallDepth in bootstrap, not in the constructor (#768) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A VM is not always built through New: internal tests assemble a &VM{} literal and bootstrap it themselves (bareVM in prelude_test.go). Those VMs got maxCallDepth 0, and because exec's check is `>=`, a ZERO LIMIT REFUSES EVERY FRAME rather than none. The full suite said so: --- FAIL: TestLoadSourceRuntimeError got SystemStackError: stack level too deep, want NoMethodError --- FAIL: TestLoadSourceSuccess unexpected error: SystemStackError: stack level too deep `x = 1 + 1` died on its first call. Confirmed as a regression from this branch and not a pre-existing failure by running ./internal/vm on origin/main in a separate checkout: it passes there. bootstrap is the one gate every usable VM passes through — a VM that has not been bootstrapped has no classes and cannot run anything — so the assignment belongs there, unconditionally, with the constructor's copy removed so there is one source of truth and nothing to drift. A test that wants a lower limit sets it after construction, which is after bootstrap has run. TestEveryBootstrappedVMHasALimit now covers the literal-plus-bootstrap path and not only the constructor, and says why zero is the dangerous value in both directions: `>=` reads it as "refuse everything", and a check written the other way round would read it as "no limit at all", which is silently the original defect. Co-Authored-By: Claude Opus 5 --- internal/vm/builtins.go | 13 +++++++++++ internal/vm/stackdepth_internal_test.go | 30 ++++++++++++++++++++----- internal/vm/vm.go | 15 ++++++++----- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/internal/vm/builtins.go b/internal/vm/builtins.go index 7629c1e3..c24ad2b0 100644 --- a/internal/vm/builtins.go +++ b/internal/vm/builtins.go @@ -19,6 +19,19 @@ import ( // bootstrap builds the base class hierarchy and installs the Phase 1 kernel. // Kernel methods live on Object so every value answers them. func (vm *VM) bootstrap() { + // The call-depth limit, set HERE and not in the constructor, because a VM is + // not always built through one: internal tests assemble a &VM{} literal + // directly (bareVM in prelude_test.go) and would otherwise get a + // maxCallDepth of 0, which the `>=` check in exec reads as "refuse every + // frame" — every program then dies with SystemStackError on its first call. + // That is exactly how this landed the first time, and two prelude tests + // caught it. bootstrap is the one gate every usable VM passes through: a VM + // that has not been bootstrapped has no classes and cannot run anything. + // + // Unconditional rather than `if == 0`, so there is one source of truth and + // nothing to drift. A test that wants a lower limit sets it AFTER + // construction, which is when bootstrap has already run. + vm.maxCallDepth = defaultMaxCallDepth vm.cBasicObject = newClass("BasicObject", nil) vm.cObject = newClass("Object", vm.cBasicObject) // Top-level constants ARE Object's constants in Ruby; share the one table so diff --git a/internal/vm/stackdepth_internal_test.go b/internal/vm/stackdepth_internal_test.go index e9653279..40003c19 100644 --- a/internal/vm/stackdepth_internal_test.go +++ b/internal/vm/stackdepth_internal_test.go @@ -2,10 +2,12 @@ package vm import ( "bytes" + "io" "strings" "testing" "github.com/go-embedded-ruby/ruby/internal/compiler" + "github.com/go-embedded-ruby/ruby/internal/object" "github.com/go-ruby-parser/parser" ) @@ -297,14 +299,32 @@ func TestDefaultCallDepthLimitLeavesRoomForMRIProgrammes(t *testing.T) { } } -// TestANewVMAlwaysHasALimit pins the thing that would silently restore the defect: -// a VM built by the public constructor must never have maxCallDepth 0, which the -// `>=` check would read as "refuse every frame" — and, were the check written the -// other way round, as "no limit at all". -func TestANewVMAlwaysHasALimit(t *testing.T) { +// TestEveryBootstrappedVMHasALimit pins the invariant whose absence broke two +// prelude tests on the first attempt at this change. +// +// maxCallDepth was set in the constructor, but a VM is not always built through +// one: internal tests assemble a &VM{} literal and call bootstrap themselves +// (bareVM in prelude_test.go). Those VMs got maxCallDepth 0, and because exec's +// check is `>=`, a zero limit refuses EVERY frame rather than none — so +// `x = 1 + 1` died with "SystemStackError: stack level too deep" and +// `nil.no_such_method` raised it in place of NoMethodError. +// +// Zero is the dangerous value in BOTH directions: `>=` reads it as "refuse +// everything", and a check written the other way round would read it as "no +// limit at all", which is silently the pre-#768 defect. So the test covers the +// literal-plus-bootstrap path and not only the constructor. +func TestEveryBootstrappedVMHasALimit(t *testing.T) { if got := New(&bytes.Buffer{}).maxCallDepth; got != defaultMaxCallDepth { t.Errorf("New(...).maxCallDepth = %d, want defaultMaxCallDepth = %d", got, defaultMaxCallDepth) } + // The shape bareVM uses: a literal, bootstrapped by hand, never through New. + bare := &VM{out: io.Discard, errOut: io.Discard, main: object.NewMain(), + consts: map[string]object.Value{}, globals: map[string]object.Value{}, loaded: map[string]bool{}} + bare.bootstrap() + if bare.maxCallDepth != defaultMaxCallDepth { + t.Errorf("a hand-built, bootstrapped VM has maxCallDepth = %d, want defaultMaxCallDepth = %d: a zero limit refuses every frame", + bare.maxCallDepth, defaultMaxCallDepth) + } } // TestTheLimitHoldsAcrossAThreadBoundary checks a shape the design could diff --git a/internal/vm/vm.go b/internal/vm/vm.go index 3509c98e..bbfcaae7 100644 --- a/internal/vm/vm.go +++ b/internal/vm/vm.go @@ -809,11 +809,14 @@ type VM struct { frameNames []string // maxCallDepth is the Ruby call depth at which exec raises SystemStackError - // instead of recursing one frame further. It is defaultMaxCallDepth for a VM - // built by New; it is a field and not the constant itself so a test can lower - // it to a depth a test can reach quickly, and so the measurement that chose - // the constant could raise it. Zero would mean "no limit", which is the - // pre-#768 behaviour and is never what a caller wants, so New always sets it. + // instead of recursing one frame further. It is a field and not the constant + // itself so a test can lower it to a depth reachable in milliseconds, and so + // the measurement that chose the constant can raise it. + // + // It is set in bootstrap, NOT here and not in the constructor, because a VM + // is not always built through one — see the comment at the top of bootstrap. + // Zero is the dangerous value: exec's check is `>=`, so a zero limit refuses + // EVERY frame rather than none. maxCallDepth int // frameFiles is the running source-file stack, kept in lockstep with @@ -1286,7 +1289,7 @@ func New(out io.Writer) *VM { return NewWithStderr(out, out) } // still captures warnings — which is what mspec's `complain` matcher does, and // why the conformance suite could not observe this defect at all. func NewWithStderr(out, errOut io.Writer) *VM { - vm := &VM{out: out, errOut: errOut, main: object.NewMain(), consts: map[string]object.Value{}, loaded: map[string]bool{}, globals: map[string]object.Value{}, maxCallDepth: defaultMaxCallDepth} + vm := &VM{out: out, errOut: errOut, main: object.NewMain(), consts: map[string]object.Value{}, loaded: map[string]bool{}, globals: map[string]object.Value{}} // The controllable clock reads the real wall clock through nowWall (the same // determinism seam Time.now/Date.today already honour) until a require // "timecop" program freezes/travels/scales it. Unmocked, Current() just