diff --git a/internal/vm/alloc_bounds.go b/internal/vm/alloc_bounds.go new file mode 100644 index 00000000..650e928c --- /dev/null +++ b/internal/vm/alloc_bounds.go @@ -0,0 +1,168 @@ +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 +} + +// 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 new file mode 100644 index 00000000..43bd6b41 --- /dev/null +++ b/internal/vm/alloc_bounds_internal_test.go @@ -0,0 +1,417 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm + +import ( + "bytes" + "fmt" + "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" +) + +// 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: +// +// 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"}, + + // --- 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) { + 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) + } + } +} + +// 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 +// 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. +// +// 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" + + 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 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 + func() { + defer func() { escaped = recover() }() + _, _ = machine.Run(iseq) + }() + 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, + // 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 := runKeepingError(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/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/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/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") }} 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`) +} diff --git a/internal/vm/vm.go b/internal/vm/vm.go index 651d70bf..20327ccb 100644 --- a/internal/vm/vm.go +++ b/internal/vm/vm.go @@ -1450,7 +1450,44 @@ 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, 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 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: + 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 // error — so a real CLI that ends by calling exit (e.g. Puppet's