From c560c94e2b597700a850b010675951614601d973 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Mon, 5 Oct 2026 15:59:18 +0200 Subject: [PATCH 1/4] fix(vm): guard Ruby-sized allocations against the bound that fails (#777) `Array#*` had no overflow guard at all and `String#*` had one that tested arithmetic overflow of the product, which is not the bound `make` enforces. At a one-byte receiver `MaxInt/len(src)` is the largest int64, so the guard was vacuous and every count reached `make`. Forming the product first admits three regimes, and before this change each failed differently and none failed correctly: wrapped NEGATIVE [1,2] * (2**63-1) 2*(2**63-1) is -2 over what make takes [1] * (2**62) positive, above maxAlloc wrapped to ZERO [1,2,3,4] * (2**62) 4*2**62 is EXACTLY 0 The last was silent: `make` succeeded with capacity 0, so there was no panic and no exception, and the fill loop then appended 2**62 slices. Measured on the pre-fix binary it passed 1.1 GB resident in 60 s, still climbing. MRI's guards DIVIDE a ceiling by one factor and compare the quotient with the other, so the product is never formed. Transcribing that shape refuses all three regimes with one test. The two ceilings differ, and that difference is the whole asymmetry between the classes MRI raises: array.c v4.0.5:5235 ARY_MAX_SIZE/len < RARRAY_LEN(ary) -> ArgumentError string.c v4.0.5:2593 LONG_MAX/len < RSTRING_LEN(str) -> ArgumentError Array#* divides ARY_MAX_SIZE (LONG_MAX/sizeof(VALUE), array.c v4.0.5:73), a real allocation ceiling; String#* divides bare LONG_MAX, a pure overflow test. So `[1] * (2**62)` is an ArgumentError while `"x" * (2**62)` is a NoMemoryError (the allocation is what fails), and `"ab" * (2**62)` is an ArgumentError again. That asymmetry is reproduced rather than normalised. Also in this change: - Array#* routes through the VM (arrayTimes) for MRI's NUM2LONG coercion, so an out-of-range Bignum count is RangeError and not TypeError, a Float truncates, and an empty receiver short-circuits -- `[] * (2**62)` used to loop 2**62 times appending nothing. - Array.new checks ARY_MAX_SIZE itself (array.c v4.0.5:730, "array size too big") instead of letting the capacity reach `make`, whose panic callNative reported as `ArgumentError: new: runtime error: makeslice: cap out of range`. - Array#fill's cap is derived from ARY_MAX_SIZE rather than an unjustified 1<<40, and reports rb_ary_fill's own message (array.c v4.0.5:5087). - callNative classifies an allocation fault as NoMemoryError with MRI's message (gc.c v4.0.5:5545) instead of an ArgumentError carrying Go's internal text. This is the backstop for every unguarded `make` whose size comes from a Ruby value: ljust/rjust/center, Range#to_a and Integer#<< were all leaking `makeslice: len out of range` into a Ruby message. Second, independent defect: Run's deferred handler converted the recovered value with an unchecked `r.(RubyError)`, so any other panic value made the assertion panic INSIDE the handler. That is where `[recovered, repanicked]` came from, and it cost the cause (the assertion's value replaced it) and every line of cleanup below, leaving a host that recovers on a VM with a half-unwound frame stack. It also made Run return a nil error for a program that had not completed, because the final assignment was never reached. A type switch with a default arm wrapping the value keeps the cause legible for any future non-RubyError panic, and resolves the nil-error reading of #773 for this trigger without touching the API. Expectations measured byte-for-byte against the installed ruby 4.0.5. Co-Authored-By: Claude Opus 5 --- internal/vm/alloc_bounds.go | 147 +++++++++++ internal/vm/alloc_bounds_internal_test.go | 304 ++++++++++++++++++++++ internal/vm/arith.go | 83 ++++-- internal/vm/builtins.go | 27 +- internal/vm/object_model.go | 15 +- internal/vm/vm.go | 23 +- 6 files changed, 567 insertions(+), 32 deletions(-) create mode 100644 internal/vm/alloc_bounds.go create mode 100644 internal/vm/alloc_bounds_internal_test.go diff --git a/internal/vm/alloc_bounds.go b/internal/vm/alloc_bounds.go new file mode 100644 index 00000000..da199af3 --- /dev/null +++ b/internal/vm/alloc_bounds.go @@ -0,0 +1,147 @@ +package vm + +import ( + "math" + "runtime" + "strings" + "unsafe" + + "github.com/go-embedded-ruby/ruby/internal/object" +) + +// Bounds for an allocation whose size comes from a Ruby value. +// +// MRI guards such an allocation in two places, and the two give DIFFERENT +// exception classes. Reproducing that asymmetry is the point of this file; +// normalising it would be wrong. +// +// 1. A multiplying operator DIVIDES a ceiling by one factor and compares the +// quotient with the other, so the product is never formed: +// +// array.c v4.0.5:5235 if (ARY_MAX_SIZE/len < RARRAY_LEN(ary)) +// rb_raise(rb_eArgError, "argument too big"); +// string.c v4.0.5:2593 if (len && LONG_MAX/len < RSTRING_LEN(str)) +// rb_raise(rb_eArgError, "argument too big"); +// +// The two ceilings are not the same, and that is the whole asymmetry. +// Array#* divides ARY_MAX_SIZE, which is LONG_MAX/sizeof(VALUE) +// (array.c v4.0.5:73) — a real allocation ceiling. String#* divides bare +// LONG_MAX — a pure arithmetic-overflow test, because a String's unit is one +// byte and no product of a byte count can exceed a long without overflowing +// it. So, measured against MRI 4.0.5: +// +// [1] * (2**62) => ArgumentError: argument too big +// "x" * (2**62) => NoMemoryError: failed to allocate memory +// "ab" * (2**62) => ArgumentError: argument too big +// +// ARY_MAX_SIZE/2**62 is 0, which IS below RARRAY_LEN([1]) == 1, so Array#* +// refuses. LONG_MAX/2**62 is 1, which is NOT below RSTRING_LEN("x") == 1, so +// String#* proceeds to allocate and the ALLOCATION is what fails. Widen the +// receiver to two bytes and LONG_MAX/2**62 == 1 < 2, so String#* refuses too +// — which is why "ab" is an ArgumentError and "x" is not. +// +// 2. Whatever survives (1) is allocated, and a refused allocation is +// NoMemoryError. +// +// rbgo formed the product FIRST — len(a.Elems)*int(n) — and tested the result, +// which is three defects in one (issue #777). The product can wrap NEGATIVE +// (make panics "cap out of range"), wrap to exactly ZERO (make SUCCEEDS with cap +// 0 and the fill loop then grows without bound: no panic, no exception, just +// memory), or stay positive and still land above what make accepts. Dividing +// rather than multiplying makes all three unrepresentable, which is why every +// guard below divides and none multiplies before it has checked. +const ( + // aryMaxSize is ARY_MAX_SIZE (array.c v4.0.5:73, LONG_MAX/(int)sizeof(VALUE)): + // the largest element count whose byte size still fits a machine int. + // unsafe.Sizeof derives the element width from object.Value's own + // representation rather than hard-coding 16, so the bound follows the target's + // word size — 32-bit wasm included — and a change to Value cannot silently + // leave the bound behind. + aryMaxSize = int64(math.MaxInt) / int64(unsafe.Sizeof(object.Value(nil))) + + // strMaxSize is the bare LONG_MAX of string.c v4.0.5:2593. A String's element + // is one byte, so its sizeof is 1 and the ceiling IS the machine int maximum. + strMaxSize = int64(math.MaxInt) +) + +// repeatFitsArray reports whether repeating elems elements n times stays within +// ARY_MAX_SIZE, by MRI's division test (array.c v4.0.5:5235). n must not be +// negative; a zero n always fits (MRI returns an empty array before testing). +func repeatFitsArray(elems int, n int64) bool { + return n == 0 || aryMaxSize/n >= int64(elems) +} + +// repeatFitsString reports whether repeating srcLen bytes n times stays within a +// machine int, by MRI's division test (string.c v4.0.5:2593). n must not be +// negative; a zero n always fits. +func repeatFitsString(srcLen int, n int64) bool { + return n == 0 || strMaxSize/n >= int64(srcLen) +} + +// fitsArrayAlloc reports whether n elements stay within ARY_MAX_SIZE. It is the +// single-factor form of the test, for a size that is taken from a Ruby value +// without being multiplied (Array.new's capacity, Array#fill's new length). +func fitsArrayAlloc(n int64) bool { return n <= aryMaxSize } + +// allocValues returns an empty []object.Value with capacity n, raising +// NoMemoryError if the runtime refuses the capacity. The caller must already +// have checked n against aryMaxSize; this converts the remaining regime, where a +// capacity below ARY_MAX_SIZE is still more than the Go runtime will hand out. +func allocValues(n int64) []object.Value { + defer convertAllocPanic() + return make([]object.Value, 0, n) +} + +// allocBytes returns an empty []byte with capacity n, raising NoMemoryError if +// the runtime refuses the capacity. This is the regime MRI reaches for +// `"x" * (2**62)`: its own guard passes and malloc is what fails. +func allocBytes(n int64) []byte { + defer convertAllocPanic() + return make([]byte, 0, n) +} + +// convertAllocPanic turns a Go allocation fault into NoMemoryError, which is +// MRI's class and message for a refused allocation. The message is MRI's own +// literal (gc.c v4.0.5:5545, rb_vm_register_special_exception(... rb_eNoMemError, +// "failed to allocate memory")), and it is what MRI 4.0.5 prints for +// `"x" * (2**62)` as measured. +// +// It must be deferred DIRECTLY — `defer convertAllocPanic()`, never wrapped in a +// closure — because recover() only takes effect when called by the deferred +// function itself. +// +// Only an allocation fault converts. Go names those in the panic text, and the +// prefix is the only handle a program has on them: "makeslice:", "makemap:" and +// "growslice:" are allocation refusals, while "index out of range" and "slice +// bounds out of range" are ordinary faults that must keep their existing +// meaning. Anything else — a RubyError, a control-flow signal, a broken VM +// invariant — is re-panicked untouched so real defects stay loud. If a future Go +// release renames these, the classification degrades to the pre-existing +// behaviour (callNative's ArgumentError) rather than breaking, and +// TestAllocPanicTextStillNamesMakeslice fails to say so. +// +// Go's own maxAlloc is not reachable from a program, so it cannot be tested for +// up front; recovering the panic it raises is the only way to name that bound. +// One regime stays out of reach: a capacity BELOW maxAlloc that the host cannot +// actually satisfy makes the Go runtime abort with a fatal "out of memory" that +// no recover can catch, where MRI would raise NoMemoryError. That residual is a +// property of the runtime, not of this guard. +func convertAllocPanic() { + if r := recover(); r != nil { + if re, ok := r.(runtime.Error); ok && isAllocFault(re.Error()) { + raise("NoMemoryError", "failed to allocate memory") + } + panic(r) + } +} + +// isAllocFault reports whether a Go runtime panic text names an allocation +// refusal rather than an ordinary bounds fault. See convertAllocPanic. +func isAllocFault(msg string) bool { + for _, p := range [...]string{"makeslice:", "makemap:", "growslice:"} { + if strings.Contains(msg, p) { + return true + } + } + return false +} diff --git a/internal/vm/alloc_bounds_internal_test.go b/internal/vm/alloc_bounds_internal_test.go new file mode 100644 index 00000000..dfe33b3e --- /dev/null +++ b/internal/vm/alloc_bounds_internal_test.go @@ -0,0 +1,304 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm + +import ( + "bytes" + "math" + "strings" + "testing" + + "github.com/go-embedded-ruby/ruby/internal/compiler" + "github.com/go-embedded-ruby/ruby/internal/object" + "github.com/go-ruby-parser/parser" +) + +// TestRepeatBoundRegimes is the witness for issue #777. A size taken from a Ruby +// value and multiplied reaches `make` in three distinct regimes, and before the +// fix each failed differently and none failed correctly: +// +// wrapped NEGATIVE [1,2] * (2**63-1) 2*(2**63-1) is -2 as an int +// over what make takes [1] * (2**62) positive, above maxAlloc +// wrapped to ZERO [1,2,3,4] * (2**62) 4*2**62 is EXACTLY 0 +// +// The last is the dangerous one: `make` SUCCEEDED with capacity 0, so there was +// no panic and no exception, and the fill loop then appended 2**62 slices of 4. +// Measured on the pre-fix binary it passed 1.1 GB of resident memory in 60 s and +// was still climbing when the harness killed it. +// +// Every guard divides rather than multiplies (MRI's own shape, array.c +// v4.0.5:5235), so all three regimes are refused by the same test and the +// product is never formed. Expectations measured byte-for-byte against the +// installed ruby 4.0.5. +func TestRepeatBoundRegimes(t *testing.T) { + cases := []struct{ name, src, want string }{ + // --- Array#*: MRI divides ARY_MAX_SIZE, so all of these are ArgumentError. + {"ary_over_maxalloc", `begin; [1] * (2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: argument too big\n"}, + {"ary_two_elem", `begin; [1,2] * (2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: argument too big\n"}, + // The silent one: 4 * 2**62 == 2**64 == 0 in a machine int. + {"ary_wrap_to_zero", `begin; [1,2,3,4] * (2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: argument too big\n"}, + // 2 * (2**63-1) wraps to -2. + {"ary_wrap_negative", `begin; [1,2] * (2**63-1); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: argument too big\n"}, + {"ary_wrap_negative_odd", `begin; [1,2,3] * (2**63-1); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: argument too big\n"}, + + // --- String#*: MRI divides bare LONG_MAX, so the class depends on the + // receiver's LENGTH. This asymmetry is MRI's and is reproduced, not + // normalised -- see alloc_bounds.go. + {"str_one_byte_is_nomemory", `begin; "x" * (2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "NoMemoryError: failed to allocate memory\n"}, + {"str_two_byte_is_argerror", `begin; "ab" * (2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: argument too big\n"}, + {"str_three_byte_is_argerror", `begin; "xyz" * (2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: argument too big\n"}, + + // --- Empty receiver: repeats to empty for ANY in-range count, without + // looping. `[] * (2**62)` used to loop 2**62 times appending nothing. + {"empty_ary_huge", `p([] * (2**62))`, "[]\n"}, + {"empty_str_huge", `p("" * (2**62))`, "\"\"\n"}, + // ...but a negative count still raises, because MRI tests the sign before + // the empty case (array.c v4.0.5:5228-5233). + {"empty_ary_negative", `begin; [] * -1; rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: negative argument\n"}, + {"empty_str_negative", `begin; "" * -1; rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: negative argument\n"}, + + // --- Count coercion now goes through NUM2LONG for Array#* too, so an + // out-of-range Bignum is RangeError (MRI) and not TypeError (rbgo before). + {"ary_bignum_count", `begin; [1] * (10**40); rescue Exception=>e; puts e.class; end`, + "RangeError\n"}, + {"str_bignum_count", `begin; "x" * (10**40); rescue Exception=>e; puts e.class; end`, + "RangeError\n"}, + // A Float count truncates toward zero, as NUM2LONG does. + {"ary_float_count", `p([1,2] * 2.9)`, "[1, 2, 1, 2]\n"}, + // A #to_int object converts; this used to be a separate inline coercion. + {"ary_to_int_count", `o = Object.new; def o.to_int; 3; end; p([7] * o)`, "[7, 7, 7]\n"}, + // Nothing integer-ish at all is still a TypeError. + {"ary_no_coercion", `begin; [1] * :sym; rescue Exception=>e; puts e.class; end`, "TypeError\n"}, + + // --- Zero and ordinary counts keep working (the fast path). + {"ary_zero", `p([1,2] * 0)`, "[]\n"}, + {"ary_small", `p([1,2] * 3)`, "[1, 2, 1, 2, 1, 2]\n"}, + {"str_small", `p("ab" * 3)`, "\"ababab\"\n"}, + + // --- Array.new no longer leaks the Go text. Before: `ArgumentError: new: + // runtime error: makeslice: cap out of range`. + {"array_new_over_ceiling", `begin; Array.new(2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: array size too big\n"}, + {"array_new_with_default", `begin; Array.new(2**62, 0); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: array size too big\n"}, + + // --- Array#fill reports rb_ary_fill's own message (array.c v4.0.5:5087), + // which is not ary_new's. + {"fill_too_big", `begin; [1,2,3].fill(0, 0, 2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: argument too big\n"}, + + // --- The callNative backstop: a size that reaches a `make` with no guard of + // its own is now NoMemoryError with MRI's message, not an ArgumentError + // carrying Go's internal text. + {"ljust_backstop", `begin; "x".ljust(2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "NoMemoryError: failed to allocate memory\n"}, + {"rjust_backstop", `begin; "x".rjust(2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "NoMemoryError: failed to allocate memory\n"}, + {"center_backstop", `begin; "x".center(2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "NoMemoryError: failed to allocate memory\n"}, + {"range_to_a_backstop", `begin; (1..2**62).to_a; rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "NoMemoryError: failed to allocate memory\n"}, + {"integer_shift_backstop", `begin; 1 << (2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "NoMemoryError: failed to allocate memory\n"}, + // An ordinary bounds fault must NOT be reclassified: only an allocation + // fault becomes NoMemoryError, so this stays the ArgumentError it was. + {"index_fault_stays_argerror", `p("x".ljust(3))`, "\"x \"\n"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := eval(t, c.src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + }) + } +} + +// TestAllocPanicTextStillNamesMakeslice pins the one thing isAllocFault cannot +// control: the text the Go runtime puts in a makeslice panic. The classification +// in convertAllocPanic reads that text because a program has no other handle on +// maxAlloc, so if a Go release renames it this test says so rather than letting +// every refused allocation quietly go back to being an ArgumentError with Go's +// internals in the message. +// +// This is the known positive the classifier is validated on. +func TestAllocPanicTextStillNamesMakeslice(t *testing.T) { + msg := func() (s string) { + defer func() { + r := recover() + s = r.(error).Error() + }() + n := int64(math.MaxInt64 / 4) // above maxAlloc on every supported platform + _ = make([]byte, 0, n) + return "" + }() + if !strings.Contains(msg, "makeslice:") { + t.Fatalf("Go's makeslice panic text no longer contains %q: %q\n"+ + "isAllocFault classifies on this prefix; update it (and this test) together.", "makeslice:", msg) + } + if !isAllocFault(msg) { + t.Errorf("isAllocFault(%q) = false, want true", msg) + } + // The negative side: an ordinary bounds fault must not be classified as an + // allocation refusal, or callNative would report every one of them as + // NoMemoryError. + for _, notAlloc := range []string{ + "runtime error: index out of range [5] with length 3", + "runtime error: slice bounds out of range [:9] with capacity 4", + "runtime error: invalid memory address or nil pointer dereference", + } { + if isAllocFault(notAlloc) { + t.Errorf("isAllocFault(%q) = true, want false", notAlloc) + } + } +} + +// TestRepeatBoundsAreDerivedNotGuessed checks the two ceilings against the +// definitions they are transcribed from, so a change to object.Value's width (or +// a 32-bit target) cannot leave them behind. ARY_MAX_SIZE is LONG_MAX divided by +// the element width (array.c v4.0.5:73); strMaxSize is bare LONG_MAX, because a +// String's element is one byte (string.c v4.0.5:2593). +func TestRepeatBoundsAreDerivedNotGuessed(t *testing.T) { + if strMaxSize != int64(math.MaxInt) { + t.Errorf("strMaxSize = %d, want math.MaxInt = %d", strMaxSize, int64(math.MaxInt)) + } + if aryMaxSize >= strMaxSize { + t.Errorf("aryMaxSize (%d) must be strictly below strMaxSize (%d): an element is wider than a byte", + aryMaxSize, strMaxSize) + } + if aryMaxSize <= 0 { + t.Fatalf("aryMaxSize = %d, want positive", aryMaxSize) + } + // The division guards must agree with the ceilings at the boundary, and must + // never form the product. + if !repeatFitsArray(1, aryMaxSize) { + t.Errorf("repeatFitsArray(1, aryMaxSize) = false, want true at the boundary") + } + if repeatFitsArray(2, aryMaxSize) { + t.Errorf("repeatFitsArray(2, aryMaxSize) = true, want false just past the boundary") + } + if !repeatFitsString(1, strMaxSize) { + t.Errorf("repeatFitsString(1, strMaxSize) = false, want true at the boundary") + } + if repeatFitsString(2, strMaxSize) { + t.Errorf("repeatFitsString(2, strMaxSize) = true, want false just past the boundary") + } + // A zero count always fits, whatever the other factor: MRI returns empty + // before testing, and dividing by zero must never be reached. + if !repeatFitsArray(math.MaxInt32, 0) || !repeatFitsString(math.MaxInt32, 0) { + t.Error("a zero count must fit for any receiver length") + } + // fitsArrayAlloc is the single-factor form used by Array.new / Array#fill. + if !fitsArrayAlloc(aryMaxSize) { + t.Error("fitsArrayAlloc(aryMaxSize) = false, want true at the boundary") + } + if fitsArrayAlloc(aryMaxSize + 1) { + t.Error("fitsArrayAlloc(aryMaxSize+1) = true, want false") + } +} + +// TestNonRubyErrorPanicKeepsItsCause is the witness for the second defect in +// #777, independent of the first. Run's deferred handler converted the recovered +// value with an UNCHECKED `r.(RubyError)` assertion, so any other panic value +// made the assertion panic INSIDE the handler. The host then saw +// +// returned err : +// escaped panic : interface conversion: interface {} is runtime.errorString, +// not vm.RubyError +// +// — the cause gone, and none of the handler's cleanup run, so a host that +// recovers in a wrapper carried on with a VM holding a half-unwound frame stack. +// +// Fixing the guards removed #777's trigger; this test uses a native method that +// panics with a plain value so the handler's default arm is exercised directly, +// and would stay meaningful for any future non-RubyError panic. +func TestNonRubyErrorPanicKeepsItsCause(t *testing.T) { + const cause = "a panic value that is not a RubyError" + + prog, err := parser.Parse(`def deep; boom; end +deep`) + 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 + machine := New(&buf) + // callNative re-panics anything that is not a runtime.Error untouched, so a + // plain string reaches Run's handler exactly as a broken VM invariant would. + machine.cObject.defineArgc("boom", 0, func(_ *VM, _ object.Value, _ []object.Value, _ *Proc) object.Value { + panic(cause) + }) + + // The whole point: Run must RETURN, not let a panic escape. Anything escaping + // here is the original defect. + var escaped any + runErr := func() (e error) { + defer func() { escaped = recover() }() + _, e = machine.Run(iseq) + return e + }() + if escaped != nil { + t.Fatalf("a panic escaped Run: %v (the handler re-panicked, which is the defect)", escaped) + } + + // (c): Run must not report success for a program that did not complete. + if runErr == nil { + t.Fatal("Run returned a nil error for a program that panicked (issue #773 from this side)") + } + // The cause must survive. Before the fix the only text naming it was gone. + if !strings.Contains(runErr.Error(), cause) { + t.Errorf("the cause did not survive: err = %q, want it to contain %q", runErr.Error(), cause) + } + + // The handler's cleanup must have run: these are the frame stacks it resets, + // and skipping them is what left a surviving host on an undefined VM. + if n := len(machine.frameNames); n != 0 { + t.Errorf("frameNames not reset: len = %d, want 0 (handler cleanup did not run)", n) + } + if n := len(machine.frameFiles); n != 0 { + t.Errorf("frameFiles not reset: len = %d, want 0", n) + } + if n := len(machine.frameCrefs); n != 0 { + t.Errorf("frameCrefs not reset: len = %d, want 0", n) + } + if n := len(machine.frameMethods); n != 0 { + t.Errorf("frameMethods not reset: len = %d, want 0", n) + } + if n := len(machine.fileStack); n != 0 { + t.Errorf("fileStack not reset: len = %d, want 0", n) + } +} + +// TestRunReportsAnErrorForARefusedAllocation closes the loop between the two +// defects on #777's own trigger: the expression that used to escape as +// `[recovered, repanicked]` with a nil error now comes back as an ordinary Ruby +// exception through Run's error return, with the frame stacks reset. +func TestRunReportsAnErrorForARefusedAllocation(t *testing.T) { + machine, out, runErr := runSrcErr(t, `[1] * (2**62)`) + if runErr == nil { + t.Fatal("Run returned nil for a program that raised") + } + if got, want := runErr.Error(), "ArgumentError: argument too big"; got != want { + t.Errorf("err = %q, want %q", got, want) + } + if out != "" { + t.Errorf("stdout = %q, want empty", out) + } + if n := len(machine.frameNames); n != 0 { + t.Errorf("frameNames not reset: len = %d, want 0", n) + } +} diff --git a/internal/vm/arith.go b/internal/vm/arith.go index 476cec1f..7c6ba0b0 100644 --- a/internal/vm/arith.go +++ b/internal/vm/arith.go @@ -362,12 +362,13 @@ func (vm *VM) binaryOpBuiltin(op bytecode.Op, a, b object.Value) object.Value { if _, isStr := b.(*object.String); isStr || vm.respondsToDynamic(b, "to_str") { return vm.arrayJoin(aa, vm.joinSeparator(b), map[*object.Array]bool{}) } - // A non-String argument is coerced to an Integer via #to_int for the - // repeat case; the coerced value falls through to arrayOp below. - if vm.respondsToDynamic(b, "to_int") { - b = vm.send(b, "to_int", nil, nil) - } } + // The Integer repeat case also needs a live VM, so it no longer falls + // through to the VM-less arrayOp: arrayTimes applies MRI's NUM2LONG + // coercion (repeatLong, which subsumes the #to_int conversion this used to + // do inline, and raises RangeError rather than TypeError for an + // out-of-range Bignum) and MRI's ARY_MAX_SIZE bound. See issue #777. + return vm.arrayTimes(aa, b) } // String#* (repeat) needs a live VM: the count is coerced with MRI's // NUM2LONG semantics (Float truncates, a Bignum raises RangeError, any other @@ -739,6 +740,13 @@ func (vm *VM) stringPlus(a *object.String, b object.Value) object.Value { // raises ArgumentError rather than allocating (so an empty receiver returns "" // for any in-range count without looping). The result is always a base String // in the receiver's encoding. +// +// The overflow test is MRI's own (string.c v4.0.5:2593), which divides bare +// LONG_MAX and so deliberately lets a one-byte receiver through for any count: +// `"x" * (2**62)` is NOT an ArgumentError on MRI, it is the ALLOCATION that +// fails, as NoMemoryError. allocBytes reproduces that rather than normalising it +// — see alloc_bounds.go for the asymmetry against Array#*, and issue #777 for the +// `make` panic this used to be. func (vm *VM) stringTimes(a *object.String, b object.Value) object.Value { n := vm.repeatLong(b) if n < 0 { @@ -748,16 +756,55 @@ func (vm *VM) stringTimes(a *object.String, b object.Value) object.Value { if len(src) == 0 || n == 0 { return object.NewStringBytesEnc(nil, a.Enc) // empty result, no allocation/looping } - if n > int64(math.MaxInt/len(src)) { + if !repeatFitsString(len(src), n) { raise("ArgumentError", "argument too big") } - out := make([]byte, 0, len(src)*int(n)) + out := allocBytes(int64(len(src)) * n) for i := int64(0); i < n; i++ { out = append(out, src...) } return object.NewStringBytesEnc(out, a.Enc) } +// arrayTimes implements Array#* with an Integer-ish count (rb_ary_times, +// array.c v4.0.5:5216). It needs a live VM for two reasons: the count is +// converted like NUM2LONG (repeatLong, so a Float truncates, an out-of-range +// Bignum raises RangeError and a #to_int object converts), and the bound it +// enforces is MRI's — ARY_MAX_SIZE divided by the count, never the product. +// +// Before #777 this lived in the VM-less arrayOp with no bound at all: it formed +// len(a.Elems)*int(n) as an int and handed the result to make, so `[1] * (2**62)` +// panicked on a wrapped-negative capacity and `[1,2,3,4] * (2**62)` wrapped to +// exactly 0, allocated nothing, and then grew without bound in the fill loop. +func (vm *VM) arrayTimes(a *object.Array, b object.Value) object.Value { + n := vm.repeatLong(b) + // MRI tests len == 0 BEFORE the sign (array.c v4.0.5:5228-5233), so a zero + // count returns an empty array and any other negative count raises. The order + // matters for an EMPTY receiver too: `[] * -1` raises on MRI, so the + // empty-receiver short-circuit below must come after this, not before it. + if n == 0 { + return object.NewArrayFromSlice(nil) + } + if n < 0 { + raise("ArgumentError", "negative argument") + } + if len(a.Elems) == 0 { + // An empty receiver repeats to empty for any in-range count: MRI's guard + // passes (ARY_MAX_SIZE/n is never below 0), the length multiplies to 0 and + // ary_new(0) returns []. Short-circuiting here is what keeps `[] * (2**62)` + // from looping 2**62 times appending nothing, which it used to do. + return object.NewArrayFromSlice(nil) + } + if !repeatFitsArray(len(a.Elems), n) { + raise("ArgumentError", "argument too big") + } + out := allocValues(int64(len(a.Elems)) * n) + for i := int64(0); i < n; i++ { + out = append(out, a.Elems...) + } + return object.NewArrayFromSlice(out) +} + func stringOp(op bytecode.Op, a *object.String, b object.Value) object.Value { switch op { // String#+ (OpAdd), #* (OpMul) and % (OpMod) are all intercepted in binaryOp @@ -797,22 +844,12 @@ func arrayOp(op bytecode.Op, a *object.Array, b object.Value) object.Value { out := make([]object.Value, 0, len(a.Elems)+len(bb.Elems)) out = append(append(out, a.Elems...), bb.Elems...) return object.NewArrayFromSlice(out) - case bytecode.OpMul: - // Array * String (join with a separator) is intercepted in binaryOp and - // routed through Array#join for full coercion/encoding handling, so only the - // Integer repeat case reaches here. - n, ok := b.(object.Integer) - if !ok { - raise("TypeError", "no implicit conversion of %s into Integer", b.Inspect()) - } - if n < 0 { - raise("ArgumentError", "negative argument") - } - out := make([]object.Value, 0, len(a.Elems)*int(n)) - for i := int64(0); i < int64(n); i++ { - out = append(out, a.Elems...) - } - return object.NewArrayFromSlice(out) + // Array#* is intercepted in binaryOp for BOTH of its meanings and routed + // through the VM-aware arrayJoin (String separator) or arrayTimes (Integer + // repeat), so neither reaches this VM-less path. The repeat case moved there in + // #777: it needs the VM for MRI's NUM2LONG coercion, and computing the result + // length here as len(a.Elems)*int(n) was an unchecked int multiply that could + // wrap negative, wrap to zero, or exceed what make accepts. } return raise("NoMethodError", "undefined method '%s' for an Array", op) } diff --git a/internal/vm/builtins.go b/internal/vm/builtins.go index bff56d77..8928301e 100644 --- a/internal/vm/builtins.go +++ b/internal/vm/builtins.go @@ -3218,12 +3218,20 @@ func (vm *VM) bootstrap() { if n < 0 { raise("ArgumentError", "negative array size") } + // ary_new refuses a capacity above ARY_MAX_SIZE with its own message + // (array.c v4.0.5:730, "array size too big"). Without this test the + // capacity reached make, whose panic callNative then reported as + // `ArgumentError: new: runtime error: makeslice: cap out of range` — the Go + // text leaking into a Ruby message (issue #777). + if !fitsArrayAlloc(n) { + raise("ArgumentError", "array size too big") + } // Build the array incrementally so that if the block calls break, the array // is left holding the elements produced before the break, as MRI does. if blk != nil && len(args) == 2 { vm.rbWarn("block supersedes default value argument") } - arr.Elems = make([]object.Value, 0, n) + arr.Elems = allocValues(n) for i := int64(0); i < n; i++ { switch { case blk != nil: @@ -9249,10 +9257,12 @@ func (vm *VM) flattenDepthIn(elems []object.Value, depth int, path map[*object.A return out, changed } -// maxFillSize caps how large Array#fill may grow an array. MRI raises when the -// requested size is unreasonable; rejecting here keeps a pathological length -// (e.g. fill(x, 1, fixnum_max)) from attempting a doomed allocation. -const maxFillSize = 1 << 40 +// maxFillSize caps how large Array#fill may grow an array. It is ARY_MAX_SIZE +// (see alloc_bounds.go), the same ceiling ary_new enforces, rather than the +// 1<<40 that stood here before #777 -- that number was below MRI's real limit +// and nothing justified it, so a length between 1<<40 and ARY_MAX_SIZE was +// refused here and accepted by MRI. +const maxFillSize = aryMaxSize // arrayFill implements Array#fill in every MRI form: fill(obj), fill(obj, start), // fill(obj, start, length), fill(obj, range) and the block variants @@ -9423,8 +9433,11 @@ func (vm *VM) fillRangeBounds(r *object.Range, alen int) (beg, end int) { // inverted interval is a no-op. It returns a. func (vm *VM) arrayFillRange(a *object.Array, beg, end int, item object.Value, blk *Proc) object.Value { if end > len(a.Elems) { - if end > maxFillSize { - raise("ArgumentError", "array size too big") + if int64(end) > maxFillSize { + // rb_ary_fill's own test (array.c v4.0.5:5087) raises with this message, + // which is NOT ary_new's "array size too big" -- measured on MRI 4.0.5, + // [1,2,3].fill(0, 0, 2**62) gives "argument too big". + raise("ArgumentError", "argument too big") } grow := make([]object.Value, end-len(a.Elems)) for i := range grow { diff --git a/internal/vm/object_model.go b/internal/vm/object_model.go index 8352b8eb..e486495f 100644 --- a/internal/vm/object_model.go +++ b/internal/vm/object_model.go @@ -2292,7 +2292,20 @@ func (vm *VM) callNative(m *Method, self object.Value, args []object.Value, blk } defer func() { if r := recover(); r != nil { - if _, ok := r.(runtime.Error); ok { + if re, ok := r.(runtime.Error); ok { + // An ALLOCATION fault is a refused allocation, not a bad argument, and + // MRI calls it NoMemoryError. Reporting it as ArgumentError also leaked + // the Go text verbatim into a Ruby message, which is how + // `"x".ljust(2**62)` came to raise + // `ArgumentError: ljust: runtime error: makeslice: len out of range` + // where MRI 4.0.5 raises `NoMemoryError: failed to allocate memory` + // (issue #777). This is the backstop for every size that reaches a + // `make` from a Ruby value without a guard of its own; the named + // operators guard themselves, because MRI refuses some of those with + // ArgumentError BEFORE allocating and only a guard can tell. + if isAllocFault(re.Error()) { + panic(RubyError{Class: "NoMemoryError", Message: "failed to allocate memory"}) + } panic(RubyError{Class: "ArgumentError", Message: fmt.Sprintf("%s: %v", m.name, r)}) } panic(r) diff --git a/internal/vm/vm.go b/internal/vm/vm.go index 651d70bf..5de85fef 100644 --- a/internal/vm/vm.go +++ b/internal/vm/vm.go @@ -1450,7 +1450,28 @@ func (vm *VM) Run(iseq *bytecode.ISeq) (result object.Value, err error) { result, err = object.NilV, nil return } - rerr := r.(RubyError) + // Every panic value that is not a RubyError used to CRASH HERE. This was + // an unchecked `r.(RubyError)` assertion, so a Go runtime fault — the + // makeslice panic of #777, say — made the assertion itself panic INSIDE + // this handler. That is where `[recovered, repanicked]` came from, and it + // cost two things: the assertion's own panic value replaced the original, + // so the only text naming the real cause was gone, and every line of + // cleanup below was skipped, leaving a host that recovers in a wrapper + // holding a VM with a half-unwound frame stack. It also made Run return a + // nil error for a program that had not completed, because the assignment + // at the end of this handler was never reached (issue #773 from the other + // side). + // + // Guarding the conversion fixes the whole class, not just #777's trigger: + // any future non-RubyError panic keeps its cause, the cleanup runs, and + // Run reports a non-nil error. + var rerr RubyError + switch v := r.(type) { + case RubyError: + rerr = v + default: + rerr = RubyError{Class: "fatal", Message: fmt.Sprintf("%v", v)} + } // A SystemExit (Kernel#exit/abort) is a clean program termination, not a // crash: unwind to the top, run at_exit handlers, and return without an // error — so a real CLI that ends by calling exit (e.g. Puppet's From f94b64e6862a1041a4e3470a2bf96b4fbf6a8158 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Mon, 5 Oct 2026 16:08:55 +0200 Subject: [PATCH 2/4] test(vm): a witness per allocation-bound regime, and the re-panic (#777) TestRepeatBoundRegimes covers the three ways a Ruby-sized product used to reach `make`: wrapped negative, positive but above what make accepts, and wrapped to EXACTLY zero -- the silent one, where make succeeded with capacity 0 and the fill loop grew without bound. It also pins the String/Array class asymmetry per receiver length, the empty-receiver short-circuit (and that a negative count still raises through it), the NUM2LONG count coercion, and the NoMemoryError backstop for ljust/rjust/center/Range#to_a/Integer#<<. TestAllocPanicTextStillNamesMakeslice is the known positive the fault classifier is validated on: it provokes a real makeslice panic and asserts the text still carries the prefix isAllocFault reads, so a Go rename is reported rather than silently sending every refused allocation back to being an ArgumentError with Go internals in its message. Its negative side asserts that an ordinary index/slice/nil fault is NOT reclassified. TestRepeatBoundsAreDerivedNotGuessed checks both ceilings against the definitions they transcribe, at the boundary and one past it, so the bound cannot drift from object.Value's width or the target word size. TestNonRubyErrorPanicKeepsItsCause exercises Run's handler through a native method that panics with a plain string, which callNative passes through untouched. It asserts all three things the unchecked assertion cost: that no panic escapes Run, that the cause survives in the returned error, and that the handler's frame-stack cleanup ran. runKeepingError duplicates runSrcErr deliberately: that one sits behind `//go:build !windows && !wasm`, and CI vets the test binary for GOOS=wasip1 and GOOS=js. The benchmarks measure what the guards cost on the hot path. Interleaved before/after rounds (minimum of 3, load average ~13 on a shared machine): Array#* small 150.5us -> 145.8us, wide 175.9us -> 179.1us, zero-count 114.0us -> 103.6us (the short-circuit), String#* small 128.9us -> 130.2us, wide 66.9us -> 66.1us. Identical allocs/op throughout: the deferred recover adds no allocation. Co-Authored-By: Claude Opus 5 --- internal/vm/alloc_bounds_internal_test.go | 23 +++++++++++++- internal/vm/repeat_bench_test.go | 38 +++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 internal/vm/repeat_bench_test.go diff --git a/internal/vm/alloc_bounds_internal_test.go b/internal/vm/alloc_bounds_internal_test.go index dfe33b3e..9c1cd48f 100644 --- a/internal/vm/alloc_bounds_internal_test.go +++ b/internal/vm/alloc_bounds_internal_test.go @@ -15,6 +15,27 @@ import ( "github.com/go-ruby-parser/parser" ) +// runKeepingError runs a program and returns the VM, its stdout and whatever +// came out of Run, so a test can assert on a program that ends by raising AND on +// the state the VM was left in. runSrcErr does the same but lives behind +// `//go:build !windows && !wasm`, and these tests must compile on every lane — +// CI vets the test binary for GOOS=wasip1 and GOOS=js. +func runKeepingError(t *testing.T, src string) (*VM, string, error) { + 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 + machine := New(&buf) + _, runErr := machine.Run(iseq) + return machine, buf.String(), runErr +} + // TestRepeatBoundRegimes is the witness for issue #777. A size taken from a Ruby // value and multiplied reaches `make` in three distinct regimes, and before the // fix each failed differently and none failed correctly: @@ -288,7 +309,7 @@ deep`) // `[recovered, repanicked]` with a nil error now comes back as an ordinary Ruby // exception through Run's error return, with the frame stacks reset. func TestRunReportsAnErrorForARefusedAllocation(t *testing.T) { - machine, out, runErr := runSrcErr(t, `[1] * (2**62)`) + machine, out, runErr := runKeepingError(t, `[1] * (2**62)`) if runErr == nil { t.Fatal("Run returned nil for a program that raised") } diff --git a/internal/vm/repeat_bench_test.go b/internal/vm/repeat_bench_test.go new file mode 100644 index 00000000..0a394af3 --- /dev/null +++ b/internal/vm/repeat_bench_test.go @@ -0,0 +1,38 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm_test + +import "testing" + +// Array#* and String#* are hot, and #777's guards sit directly on their fast +// path: one division and one extra call (allocValues / allocBytes, which carry a +// deferred recover so a refused allocation becomes NoMemoryError). These measure +// what that costs at the sizes real code uses, so the bound is paid for +// knowingly rather than assumed free. +// +// Array#* also moved from the VM-less arrayOp to vm.arrayTimes, which replaces an +// inline `b.(object.Integer)` with repeatLong -- that is the other thing these +// watch. + +func BenchmarkArrayTimesSmall(b *testing.B) { + benchProgram(b, `a = [1, 2, 3]; i = 0; while i < 1000; a * 4; i += 1; end`) +} + +func BenchmarkArrayTimesWide(b *testing.B) { + benchProgram(b, `a = (1..64).to_a; i = 0; while i < 200; a * 8; i += 1; end`) +} + +func BenchmarkArrayTimesZero(b *testing.B) { + // The short-circuit path: no allocation, no loop. + benchProgram(b, `a = [1, 2, 3]; i = 0; while i < 1000; a * 0; i += 1; end`) +} + +func BenchmarkStringTimesSmall(b *testing.B) { + benchProgram(b, `s = "ab"; i = 0; while i < 1000; s * 8; i += 1; end`) +} + +func BenchmarkStringTimesWide(b *testing.B) { + benchProgram(b, `s = "abcdefghijklmnopqrstuvwxyz"; i = 0; while i < 200; s * 64; i += 1; end`) +} From a33fe0a38055d6e299b31798522bbfa9fd60f5e6 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Mon, 5 Oct 2026 16:26:07 +0200 Subject: [PATCH 3/4] fix(vm): check the SIGN at the entropy sites, not in the backstop (#777) Found by the make( census, and it is a regression the backstop in the previous commit introduced rather than a pre-existing one. Go's makeslice raises the SAME panic text -- "makeslice: len out of range" -- for a negative length and for one above maxAlloc. callNative cannot tell those apart from the text, so classifying every allocation fault as NoMemoryError reported MRI's ArgumentError case as NoMemoryError: before after this commit Random.new.bytes(-1) ArgumentError: bytes: ArgumentError: negative runtime error: makeslice: string size (or size too len out of range big) MRI 4.0.5 raises `ArgumentError: negative string size (or size too big)`, so the class was accidentally right before the backstop and wrong after it. The message was wrong either way. Three sites checked neither sign nor magnitude: Random#bytes, Random.bytes and OpenSSL::Random.random_bytes. They now go through randomByteCount, which settles the sign at the site with MRI's exact message. That fixes the class and makes a fault that DOES reach the backstop from these methods genuinely an oversized allocation -- `Random.new.bytes(2**62)` still gives MRI's NoMemoryError. Every other candidate the census found already checks its sign (the seven socket recv/read sites, Array#insert, Range#take, IO#truncate, IO#pwrite, IO::Buffer's bufSizeArg), so this is the whole of the ambiguity. Also corrects an existing Array#fill expectation that was asserting the cap rather than MRI. maxFillSize was an unjustified 1<<40, so every length between 1<<40 and ARY_MAX_SIZE was refused by rbgo and NOT by MRI. Measured on 4.0.5: `[1,2,3].fill(10, 1, 1 << 50)` raises NoMemoryError, while 1<<40 and 1<<35 are genuinely materialised (both passed 2 GB resident before the probe's cap stopped them) -- the old bound was wrong in both directions. Co-Authored-By: Claude Opus 5 --- internal/vm/alloc_bounds.go | 21 ++++++ internal/vm/alloc_bounds_internal_test.go | 81 +++++++++++++++++++++++ internal/vm/array_fill_test.go | 17 ++++- internal/vm/openssl.go | 2 +- internal/vm/random.go | 4 +- 5 files changed, 120 insertions(+), 5 deletions(-) diff --git a/internal/vm/alloc_bounds.go b/internal/vm/alloc_bounds.go index da199af3..650e928c 100644 --- a/internal/vm/alloc_bounds.go +++ b/internal/vm/alloc_bounds.go @@ -145,3 +145,24 @@ func isAllocFault(msg string) bool { } return false } + +// randomByteCount converts a Ruby byte count for the entropy methods +// (Random#bytes, Random.bytes, OpenSSL::Random.random_bytes), refusing a +// negative one with MRI's message (measured on ruby 4.0.5: +// `Random.new.bytes(-1)` raises +// `ArgumentError: negative string size (or size too big)`). +// +// The sign MUST be checked here rather than left to the callNative backstop, +// because Go's makeslice raises the SAME panic text — "makeslice: len out of +// range" — for a negative length and for one above maxAlloc. The backstop +// cannot tell those apart, so without this check a negative count would be +// reported as NoMemoryError, which is MRI's answer for the oversized case only. +// With the sign settled here, a fault that reaches the backstop from these +// methods really is an oversized allocation. +func randomByteCount(v object.Value) int { + n := intArg(v) + if n < 0 { + raise("ArgumentError", "negative string size (or size too big)") + } + return int(n) +} diff --git a/internal/vm/alloc_bounds_internal_test.go b/internal/vm/alloc_bounds_internal_test.go index 9c1cd48f..a964ed98 100644 --- a/internal/vm/alloc_bounds_internal_test.go +++ b/internal/vm/alloc_bounds_internal_test.go @@ -136,6 +136,26 @@ func TestRepeatBoundRegimes(t *testing.T) { // An ordinary bounds fault must NOT be reclassified: only an allocation // fault becomes NoMemoryError, so this stays the ArgumentError it was. {"index_fault_stays_argerror", `p("x".ljust(3))`, "\"x \"\n"}, + + // --- The entropy methods check the SIGN themselves, because Go's makeslice + // uses the same panic text for a negative length and an oversized one, so + // the backstop cannot tell them apart. Without the check a negative count + // came back as NoMemoryError where MRI 4.0.5 raises ArgumentError. + {"random_bytes_negative", `begin; Random.new.bytes(-1); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: negative string size (or size too big)\n"}, + {"random_class_bytes_negative", `begin; Random.bytes(-1); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: negative string size (or size too big)\n"}, + {"openssl_random_bytes_negative", `require "openssl" +begin; OpenSSL::Random.random_bytes(-1); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "ArgumentError: negative string size (or size too big)\n"}, + // ...and with the sign settled, a fault that DOES reach the backstop from + // these methods really is an oversized allocation. + {"random_bytes_too_big", `begin; Random.new.bytes(2**62); rescue Exception=>e; puts "#{e.class}: #{e.message}"; end`, + "NoMemoryError: failed to allocate memory\n"}, + // A valid count still works. + {"random_bytes_ok", `p Random.new(7).bytes(4).bytesize`, "4\n"}, + {"openssl_random_bytes_ok", `require "openssl" +p OpenSSL::Random.random_bytes(8).bytesize`, "8\n"}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -185,6 +205,67 @@ func TestAllocPanicTextStillNamesMakeslice(t *testing.T) { } } +// TestConvertAllocPanicOnlyConvertsAnAllocationFault pins convertAllocPanic's +// contract: it converts an allocation fault and passes everything else through +// UNCHANGED. +// +// Its re-panic arm cannot fire from either of its two production callers -- +// inside `make([]T, 0, n)` nothing but makeslice can panic -- so this is a +// contract test, not a reachability claim about allocValues/allocBytes. It earns +// its place because the arm is what keeps the helper safe to defer around a +// wider body, and because a helper that silently swallowed an unrelated panic +// (returning a nil slice to its caller) would be a worse defect than the one +// #777 fixed. +func TestConvertAllocPanicOnlyConvertsAnAllocationFault(t *testing.T) { + // A non-runtime.Error value passes through untouched. + got := func() (r any) { + defer func() { r = recover() }() + defer convertAllocPanic() + panic("not an allocation fault") + }() + if got != "not an allocation fault" { + t.Errorf("a plain panic value was not passed through: got %v", got) + } + + // A RubyError passes through as itself, so a raise from inside a wrapped body + // keeps its class. + got = func() (r any) { + defer func() { r = recover() }() + defer convertAllocPanic() + panic(RubyError{Class: "TypeError", Message: "unrelated"}) + }() + if re, ok := got.(RubyError); !ok || re.Class != "TypeError" { + t.Errorf("a RubyError was not passed through: got %#v", got) + } + + // A runtime.Error that is NOT an allocation fault also passes through: only + // isAllocFault's prefixes convert. + got = func() (r any) { + defer func() { r = recover() }() + defer convertAllocPanic() + s := []int{1, 2, 3} + i := 5 + _ = s[i] // index out of range: a runtime.Error, not an allocation fault + return nil + }() + re, ok := got.(error) + if !ok || !strings.Contains(re.Error(), "index out of range") { + t.Errorf("an index fault was not passed through: got %#v", got) + } + + // And an allocation fault DOES convert, to MRI's class and message. + got = func() (r any) { + defer func() { r = recover() }() + defer convertAllocPanic() + _ = allocBytes(math.MaxInt64 / 4) + return nil + }() + rerr, ok := got.(RubyError) + if !ok || rerr.Class != "NoMemoryError" || rerr.Message != "failed to allocate memory" { + t.Errorf("an allocation fault did not convert: got %#v", got) + } +} + // TestRepeatBoundsAreDerivedNotGuessed checks the two ceilings against the // definitions they are transcribed from, so a change to object.Value's width (or // a 32-bit target) cannot leave them behind. ARY_MAX_SIZE is LONG_MAX divided by diff --git a/internal/vm/array_fill_test.go b/internal/vm/array_fill_test.go index 35ea7518..246221b3 100644 --- a/internal/vm/array_fill_test.go +++ b/internal/vm/array_fill_test.go @@ -70,9 +70,22 @@ func TestArrayFillErrors(t *testing.T) { // Non-numeric start / length -> TypeError. {`begin; [].fill("a", true); rescue TypeError; puts "TE"; end`, "TE\n"}, {`begin; [1, 2, 3].fill("x", 1, "foo"); rescue TypeError; puts "TE"; end`, "TE\n"}, - // A Bignum length -> RangeError; a fixnum-huge length -> "array size too big". + // A Bignum length -> RangeError. {`begin; [1, 2, 3].fill(10, 1, 2 ** 64); rescue RangeError; puts "RE"; end`, "RE\n"}, - {`begin; [1, 2, 3].fill(10, 1, 1 << 50); rescue ArgumentError; puts "AE"; end`, "AE\n"}, + // A length above ARY_MAX_SIZE -> ArgumentError, with rb_ary_fill's own + // message (array.c v4.0.5:5087). + {`begin; [1, 2, 3].fill(10, 1, 2 ** 62); rescue ArgumentError => e; puts e.message; end`, + "argument too big\n"}, + // A length BELOW ARY_MAX_SIZE that still cannot be allocated -> NoMemoryError. + // This case asserted ArgumentError until #777, which was asserting the cap + // itself rather than MRI: maxFillSize was an unjustified 1<<40, so every + // length between 1<<40 and ARY_MAX_SIZE was refused here and NOT refused by + // MRI. Measured on ruby 4.0.5, `[1,2,3].fill(10, 1, 1 << 50)` raises + // `NoMemoryError: failed to allocate memory`, and 1<<40 and 1<<35 are + // genuinely materialised (both passed 2 GB resident before the probe's cap + // stopped them), so the old bound was wrong in both directions. + {`begin; [1, 2, 3].fill(10, 1, 1 << 50); rescue NoMemoryError => e; puts e.message; end`, + "failed to allocate memory\n"}, // A range whose begin is before the start of the array -> RangeError. {`begin; [1, 2, 3].fill("x", -5..-3); rescue RangeError; puts "RE"; end`, "RE\n"}, // A block raising mid-fill leaves the already-filled elements in place. diff --git a/internal/vm/openssl.go b/internal/vm/openssl.go index 26c79ad9..c34ccfc9 100644 --- a/internal/vm/openssl.go +++ b/internal/vm/openssl.go @@ -320,7 +320,7 @@ func (vm *VM) registerOpenSSLRandom(mod *RClass) { mod.consts["Random"] = r r.smethods["random_bytes"] = &Method{name: "random_bytes", owner: r, native: func(_ *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { - return object.NewStringBytesEnc(secureBytes(int(intArg(args[0]))), "ASCII-8BIT") + return object.NewStringBytesEnc(secureBytes(randomByteCount(args[0])), "ASCII-8BIT") }} } diff --git a/internal/vm/random.go b/internal/vm/random.go index 2dd0a0d9..19f66839 100644 --- a/internal/vm/random.go +++ b/internal/vm/random.go @@ -411,7 +411,7 @@ func (vm *VM) registerRandom() { }) cRandom.define("bytes", func(_ *VM, self object.Value, args []object.Value, _ *Proc) object.Value { r := self.(*RandomObj) - b := make([]byte, int(intArg(args[0]))) + b := make([]byte, randomByteCount(args[0])) r.fillBytes(b) return object.NewStringBytes(b) }) @@ -456,7 +456,7 @@ func (vm *VM) registerRandom() { return vm.randValue(vm.defaultRandom, args) }} cRandom.smethods["bytes"] = &Method{name: "bytes", owner: cRandom, native: func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { - b := make([]byte, int(intArg(args[0]))) + b := make([]byte, randomByteCount(args[0])) vm.defaultRandom.fillBytes(b) return object.NewStringBytesEnc(b, "ASCII-8BIT") }} From 72af4649fbb3aad85dc4ce4ddc847d846688ad2b Mon Sep 17 00:00:00 2001 From: tannevaled Date: Mon, 5 Oct 2026 16:40:52 +0200 Subject: [PATCH 4/4] fix(vm): preserve the cause and the cleanup, but keep propagating (#777) CI caught this and it is a correction to the previous commit, not a new defect. `TestRunPropagatesInternalPanic` (whitebox_test.go:155) went red on both arch lanes: // A non-RubyError panic inside exec (here an out-of-range local) must // propagate out of Run rather than be swallowed. Wrapping every non-RubyError panic into a returned RubyError, as the previous commit did, contradicted a deliberate and documented policy this VM already draws in one place: callNative converts a fault caused by Ruby INPUT into a rescuable exception so an embedding host can never be crashed by a program it ran, while a broken VM INVARIANT stays loud so a real defect is not laundered into an error a host would log and move past. An out-of-range local is the second kind, and turning it into a Ruby error would hide exactly the bugs that assertion exists to surface. What #777 actually broke was the CAUSE and the CLEANUP, not the propagation. Both are fixed without touching the propagation. Measured, host-shaped, on both kinds of panic: before after [1]*(2**62) err= err=ArgumentError: argument too big (a Ruby-input fault) escaped=interface conversion: ... escaped= runtime.errorString frameNames=1 frameNames=0 panic("a broken VM err= err= invariant") escaped=interface conversion: ... escaped=a broken VM invariant is string frameNames=2 frameNames=0 So the Ruby-input fault becomes a legible returned exception (because the allocation guards make it an ordinary RubyError), and the invariant break still propagates -- but carrying its OWN value instead of the failed assertion's, and leaving a clean VM instead of a half-unwound frame stack. Consequence for the #773 reading, stated precisely: this defect's fix does not by itself make Run return non-nil. The allocation guards do, for #777's trigger. TestNonRubyErrorPanicKeepsItsCause now asserts what is actually guaranteed -- the panic propagates, its cause is the original value, and all five frame stacks are reset -- rather than the wrapping the previous commit assumed. Co-Authored-By: Claude Opus 5 --- internal/vm/alloc_bounds_internal_test.go | 43 ++++++++++++++--------- internal/vm/vm.go | 36 +++++++++++++------ 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/internal/vm/alloc_bounds_internal_test.go b/internal/vm/alloc_bounds_internal_test.go index a964ed98..43bd6b41 100644 --- a/internal/vm/alloc_bounds_internal_test.go +++ b/internal/vm/alloc_bounds_internal_test.go @@ -6,6 +6,7 @@ package vm import ( "bytes" + "fmt" "math" "strings" "testing" @@ -325,6 +326,13 @@ func TestRepeatBoundsAreDerivedNotGuessed(t *testing.T) { // Fixing the guards removed #777's trigger; this test uses a native method that // panics with a plain value so the handler's default arm is exercised directly, // and would stay meaningful for any future non-RubyError panic. +// +// Note what is NOT asserted: that Run returns an error. The panic propagates by +// design (see the handler, and TestRunPropagatesInternalPanic) because this VM +// converts a fault caused by Ruby input but lets a broken VM invariant stay +// loud. So this defect's fix does not by itself resolve the nil-error reading of +// #773 -- the allocation guards do, for #777's trigger, by making it an ordinary +// RubyError. See TestRunReportsAnErrorForARefusedAllocation. func TestNonRubyErrorPanicKeepsItsCause(t *testing.T) { const cause = "a panic value that is not a RubyError" @@ -345,25 +353,28 @@ deep`) panic(cause) }) - // The whole point: Run must RETURN, not let a panic escape. Anything escaping - // here is the original defect. + // The panic still PROPAGATES -- that is deliberate and TestRunPropagatesInternalPanic + // pins it, because a broken VM invariant must not be laundered into a Ruby + // error a host would log and move past. What #777 broke was the CAUSE and the + // CLEANUP, and those are what this asserts. var escaped any - runErr := func() (e error) { + func() { defer func() { escaped = recover() }() - _, e = machine.Run(iseq) - return e + _, _ = machine.Run(iseq) }() - if escaped != nil { - t.Fatalf("a panic escaped Run: %v (the handler re-panicked, which is the defect)", escaped) - } - - // (c): Run must not report success for a program that did not complete. - if runErr == nil { - t.Fatal("Run returned a nil error for a program that panicked (issue #773 from this side)") - } - // The cause must survive. Before the fix the only text naming it was gone. - if !strings.Contains(runErr.Error(), cause) { - t.Errorf("the cause did not survive: err = %q, want it to contain %q", runErr.Error(), cause) + if escaped == nil { + t.Fatal("the internal panic did not propagate out of Run") + } + // The cause must be the ORIGINAL value. Before the fix it was replaced by the + // failed assertion's own panic ("interface conversion: interface {} is + // runtime.errorString, not vm.RubyError"), so the text naming the real cause + // was gone. + s, ok := escaped.(string) + if !ok || s != cause { + t.Errorf("the cause did not survive: escaped = %#v, want the original %q", escaped, cause) + } + if strings.Contains(fmt.Sprintf("%v", escaped), "interface conversion") { + t.Errorf("the handler's own assertion panicked again: %v", escaped) } // The handler's cleanup must have run: these are the frame stacks it resets, diff --git a/internal/vm/vm.go b/internal/vm/vm.go index 5de85fef..20327ccb 100644 --- a/internal/vm/vm.go +++ b/internal/vm/vm.go @@ -1454,23 +1454,39 @@ func (vm *VM) Run(iseq *bytecode.ISeq) (result object.Value, err error) { // an unchecked `r.(RubyError)` assertion, so a Go runtime fault — the // makeslice panic of #777, say — made the assertion itself panic INSIDE // this handler. That is where `[recovered, repanicked]` came from, and it - // cost two things: the assertion's own panic value replaced the original, - // so the only text naming the real cause was gone, and every line of - // cleanup below was skipped, leaving a host that recovers in a wrapper - // holding a VM with a half-unwound frame stack. It also made Run return a - // nil error for a program that had not completed, because the assignment - // at the end of this handler was never reached (issue #773 from the other - // side). + // cost two things, both measured on the pre-fix tree: + // + // escaped panic : interface conversion: interface {} is + // runtime.errorString, not vm.RubyError + // frameNames : 1 + // + // The assertion's own value REPLACED the original, so `makeslice: cap out + // of range` — the only text naming the real cause — was gone; and the + // cleanup below was skipped, so a host that recovers in a wrapper carried + // on with a VM holding a half-unwound frame stack. // // Guarding the conversion fixes the whole class, not just #777's trigger: - // any future non-RubyError panic keeps its cause, the cleanup runs, and - // Run reports a non-nil error. + // any non-RubyError panic now keeps its own cause and leaves a clean VM. + // + // It PROPAGATES rather than being wrapped into a returned error, because + // this VM draws that line deliberately and in one place already: + // callNative converts a fault caused by Ruby INPUT into a rescuable Ruby + // exception so an embedding host can never be crashed by a program it + // ran, while a broken VM INVARIANT stays loud so a real defect is not + // laundered into an error a host would log and move past. An out-of-range + // local is the second kind. TestRunPropagatesInternalPanic pins it, and + // what #777 broke was the cause and the cleanup — not the propagation. var rerr RubyError switch v := r.(type) { case RubyError: rerr = v default: - rerr = RubyError{Class: "fatal", Message: fmt.Sprintf("%v", v)} + vm.frameNames = vm.frameNames[:0] + vm.frameFiles = vm.frameFiles[:0] + vm.frameCrefs = vm.frameCrefs[:0] + vm.frameMethods = vm.frameMethods[:0] + vm.fileStack = vm.fileStack[:0] + panic(v) } // A SystemExit (Kernel#exit/abort) is a clean program termination, not a // crash: unwind to the top, run at_exit handlers, and return without an