From 20c1d2660da17f7a79c9dbc7e6d90040bd0c36d4 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 23 Sep 2026 22:45:00 +0200 Subject: [PATCH 01/10] Kernel#sleep: dispatch to the fiber scheduler, and coerce like rb_time_interval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit core/kernel/sleep_spec.rb scored 0 — not because fourteen of its fifteen examples failed, but because one of them hung the process and a timeout costs a whole file. `Fiber.new(blocking: false) { sleep }.resume` never returned: rbgo's sleep parked the thread until a Thread#wakeup that could not arrive, since nothing consulted Fiber.scheduler. Read for this change: - process.c rb_f_sleep: it asks rb_fiber_scheduler_current BEFORE looking at its arguments, and forwards them verbatim with rb_fiber_scheduler_kernel_sleepv — so `sleep` with no argument calls #kernel_sleep with no argument, not with nil, and neither the arity check nor the duration coercion runs on that path. Its result is time(0) - beg: whole second boundaries crossed, not a rounded duration. - scheduler.c rb_fiber_scheduler_current / _for_threadptr: the scheduler answers only while th->blocking is zero; rb_fiber_scheduler_get (which is what Fiber.scheduler exposes) answers unconditionally. - cont.c fiber_switch / rb_fiber_start: th->blocking is raised on entering a blocking fiber and lowered on leaving one, so "th->blocking == 0" is exactly "the fiber running now is non-blocking". - time.c time_timespec(num, interval=TRUE), reached through rb_time_interval: Integer/Float seconds with arg_range_check, the `f != t.tv_sec` RangeError for a Float that cannot be held in a time_t, and the #divmod(1) fallback that turns a Rational — or any object defining #divmod — into [whole seconds, fraction]. - thread_sync.c rb_mutex_sleep: validates its timeout with the same rb_time_interval, so the two now share one coercion. core/kernel/sleep_spec.rb 0 -> 15. No other file in core/file, core/io or core/kernel moved. Also fixed on the way, each byte-compared against MRI 4.0.5: sleep(Rational(1,999)) and sleep(obj_with_divmod) were TypeError; sleep(Float::NAN) returned 0 instead of raising RangeError; sleep(Float::INFINITY) and sleep(1e20) HUNG instead of raising RangeError; sleep(2**70) was a TypeError instead of a RangeError; sleep(1, 2) slept instead of raising ArgumentError. Known divergence left alone: Mutex#sleep returns 0 where MRI returns nil when the sleep was not interrupted (rb_mutex_sleep's `if (!woken)`), and IO.select's timeout still reaches only the numeric half of the coercion because it is coerced from spawn.go, which this change does not own. Co-Authored-By: Claude Opus 5 --- internal/vm/fiber.go | 27 +++- internal/vm/thread.go | 160 +++++++++++++++++---- internal/vm/wave28_sleep_scheduler_test.go | 125 ++++++++++++++++ 3 files changed, 285 insertions(+), 27 deletions(-) create mode 100644 internal/vm/wave28_sleep_scheduler_test.go diff --git a/internal/vm/fiber.go b/internal/vm/fiber.go index d9d200c2..bcc1dab1 100644 --- a/internal/vm/fiber.go +++ b/internal/vm/fiber.go @@ -304,9 +304,11 @@ func (vm *VM) registerFiber() { return dupFiberStorage(f.storage) }) // Fiber.scheduler / Fiber.set_scheduler hold the current thread's fiber - // scheduler (MRI keeps it in thread->scheduler, rb_fiber_scheduler_set). rbgo - // runs every fiber blocking, so nothing consults the scheduler yet; setting - // one is validated and stored so the hook is observable, as ruby/spec pins. + // scheduler (MRI keeps it in thread->scheduler, rb_fiber_scheduler_set). + // Fiber.scheduler is rb_fiber_scheduler_get (scheduler.c): it hands back + // whatever was installed, whichever fiber asks. It is fiberSchedulerCurrent + // (rb_fiber_scheduler_current) that the blocking-operation hooks consult, and + // that one answers only while a non-blocking fiber is running. sdef("scheduler", func(vm *VM, _ object.Value, _ []object.Value, _ *Proc) object.Value { if s := vm.currentThread.scheduler; s != nil { return s @@ -341,6 +343,25 @@ func (vm *VM) registerFiber() { }) } +// fiberSchedulerCurrent is rb_fiber_scheduler_current +// (rb_fiber_scheduler_current_for_threadptr, scheduler.c): the thread's +// scheduler, but only while the running fiber is non-blocking. MRI keeps a +// counter th->blocking that fiber_switch raises on entering a blocking fiber and +// lowers on leaving one (cont.c), so `th->blocking == 0` is exactly "the fiber +// running right now is non-blocking" — which is what this reads directly. +// A nil result means "no scheduler for this operation", and the caller must then +// take its ordinary blocking path. +func (vm *VM) fiberSchedulerCurrent() object.Value { + t := vm.currentThread + if t == nil || t.scheduler == nil { + return nil + } + if f := vm.currentFiber; f == nil || f.blocking { + return nil + } + return t.scheduler +} + // fiberKwargs returns the trailing Hash of a Fiber.new call — its keyword // bundle. Fiber.new takes no positional arguments, so a trailing Hash is always // the keywords. diff --git a/internal/vm/thread.go b/internal/vm/thread.go index 53e3df84..0a4e6fd2 100644 --- a/internal/vm/thread.go +++ b/internal/vm/thread.go @@ -2,8 +2,10 @@ package vm import ( "fmt" + "math" "runtime" "sort" + "strconv" "strings" "time" @@ -994,7 +996,7 @@ func (vm *VM) registerMutex() { var secs float64 hasDur := false if len(args) > 0 && !object.IsNil(args[0]) { - secs = mutexSleepDur(args[0]) + secs = vm.timeInterval(args[0]) hasDur = true } vm.mutexUnlock(m) // ownership check + release (ThreadError if not held) @@ -1018,22 +1020,118 @@ func (vm *VM) registerMutex() { }) } -// mutexSleepDur coerces a Mutex#sleep / Kernel#sleep duration to seconds, -// rejecting a negative interval with ArgumentError as MRI does. -func mutexSleepDur(v object.Value) float64 { - var f float64 +// mutexSleepDur coerces a duration to seconds without a *VM in hand. It is the +// numeric part of (*VM).timeInterval and nothing more: with no VM there is no +// way to send #divmod, so an object that would convert that way is refused here. +// IO.select's timeout (spawn.go) is the one caller that arrives this way. +func mutexSleepDur(v object.Value) float64 { return (*VM)(nil).timeInterval(v) } + +// timeInterval is MRI's rb_time_interval — time.c's time_timespec(num, TRUE), +// the coercion Kernel#sleep, Mutex#sleep and IO.select put a duration through. +// +// - an Integer or a Float is a number of seconds, and a negative one is an +// ArgumentError ("time interval must not be negative"); +// - a Float whose integral part does not fit a time_t — which covers NaN and +// both infinities — is a RangeError, MRI's `"%f out of Time range"`; +// - anything else is offered #divmod(1), and a two-element Array answer is +// read as [whole seconds, fraction]: that is how a Rational, and any object +// that defines #divmod, becomes an interval. Only the whole-seconds half is +// range-checked, exactly as arg_range_check is placed in time_timespec. +// - anything that does not convert is a TypeError naming its class. +func (vm *VM) timeInterval(v object.Value) float64 { switch n := v.(type) { case object.Integer: - f = float64(n) + if n < 0 { + raise("ArgumentError", "time interval must not be negative") + } + return float64(n) + case object.Float: + f := float64(n) + if f < 0 { + raise("ArgumentError", "time interval must not be negative") + } + timeIntervalRangeCheck(f) + return f + case *object.Bignum: + // time_timespec sends a Bignum through NUM2TIMET as well; rbgo only ever + // builds a Bignum for a value that does not fit an int64, so every one of + // them overflows the time_t the interval is kept in. + raise("RangeError", "bignum too big to convert into 'long'") + } + if vm != nil && vm.respondsToDynamic(v, "divmod") { + d := vm.send(v, "divmod", []object.Value{object.IntValue(1)}, nil) + if a, ok := vm.checkArrayType(d); ok && len(a.Elems) >= 2 { + return vm.timeIntervalFromDivmod(a.Elems[0], a.Elems[1]) + } + } + raise("TypeError", "can't convert %s into time interval", vm.timeIntervalClassName(v)) + return 0 +} + +// timeIntervalFromDivmod turns the [quotient, remainder] pair #divmod(1) gives +// back into seconds. MRI reads the quotient with NUM2TIMET (so a non-integral +// quotient is a TypeError) and the remainder as `rem * 1_000_000_000` narrowed +// with NUM2LONG, i.e. truncated to whole nanoseconds — which is why a Rational +// interval loses everything below a nanosecond rather than rounding. +func (vm *VM) timeIntervalFromDivmod(q, rem object.Value) float64 { + var sec int64 + switch n := q.(type) { + case object.Integer: + sec = int64(n) case object.Float: - f = float64(n) + // NUM2TIMET reaches rb_num2long, which truncates a Float rather than + // refusing it — so #divmod may answer with one and keep its fraction in the + // remainder, as `[0.5, 0]` does. + sec = int64(float64(n)) default: - raise("TypeError", "can't convert %s into time interval", classNameOf(v)) + raise("TypeError", "can't convert %s into time interval", vm.timeIntervalClassName(q)) } - if f < 0 { + if sec < 0 { raise("ArgumentError", "time interval must not be negative") } - return f + nsec := vm.send(vm.send(rem, "*", []object.Value{object.IntValue(1000000000)}, nil), "to_i", nil, nil) + n, _ := nsec.(object.Integer) + return float64(sec) + float64(n)/1e9 +} + +// timeIntervalRangeCheck is time_timespec's `if (f != t.tv_sec)` guard: the +// integral part of the duration has to survive the trip through a time_t (an +// int64 here), so NaN and the infinities land in the same RangeError as a finite +// value that is simply too large. The number is formatted the way MRI's own +// vsnprintf renders `%f` — "NaN", "Inf", "-Inf", and six decimals otherwise. +func timeIntervalRangeCheck(x float64) { + i, _ := math.Modf(x) + if !math.IsNaN(i) && i >= -9223372036854775808.0 && i < 9223372036854775808.0 { + return + } + raise("RangeError", "%s out of Time range", timeIntervalFormat(x)) +} + +// timeIntervalFormat renders a duration for the RangeError above with C's `%f` +// as Ruby's vsnprintf spells the non-finite cases. +func timeIntervalFormat(x float64) string { + switch { + case math.IsNaN(x): + return "NaN" + case math.IsInf(x, 1): + return "Inf" + case math.IsInf(x, -1): + return "-Inf" + } + return strconv.FormatFloat(x, 'f', 6, 64) +} + +// timeIntervalClassName names a value's class for the TypeError above. MRI uses +// rb_obj_class, which knows Rational and Complex apart from a plain Object; the +// VM-less caller falls back to the static table. +func (vm *VM) timeIntervalClassName(v object.Value) string { + if vm == nil { + return classNameOf(v) + } + if c := vm.classOf(v); c != nil { + return c.name + } + return classNameOf(v) } func (vm *VM) mutexLock(m *RMutex) { @@ -1066,28 +1164,42 @@ func (vm *VM) mutexUnlock(m *RMutex) { } // registerSleep adds a GVL-aware Kernel#sleep that releases the lock while -// sleeping, so other threads run. With no argument it would sleep forever in -// MRI; here it requires a duration. +// sleeping, so other threads run. It follows rb_f_sleep (process.c): a fiber +// scheduler, when one is installed and the running fiber is non-blocking, takes +// the whole operation over through #kernel_sleep; otherwise no argument (or an +// explicit nil) sleeps until woken and anything else is a duration. func (vm *VM) registerSleep() { vm.cObject.define("sleep", func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { + // rb_f_sleep asks rb_fiber_scheduler_current first and hands the arguments + // on verbatim with rb_fiber_scheduler_kernel_sleepv — so `sleep` with no + // argument calls #kernel_sleep with no argument, not with nil. The + // scheduler owns the waiting from there (the usual implementation yields + // the fiber), and none of the argument checks below run: MRI does not look + // at the duration at all on this path. + // + // The result on every path is `time(0) - beg`: rb_f_sleep brackets the wait + // with a clock of whole seconds, so what it counts is the number of second + // boundaries crossed, not the duration rounded. sleep(1.5) therefore + // answers 1 or 2 depending on where the call falls inside a second. + beg := time.Now().Unix() + if sched := vm.fiberSchedulerCurrent(); sched != nil { + vm.send(sched, "kernel_sleep", args, nil) + return object.IntValue(time.Now().Unix() - beg) + } hasDur := len(args) > 0 && !object.IsNil(args[0]) var secs float64 if hasDur { - switch n := args[0].(type) { - case object.Integer: - secs = float64(n) - case object.Float: - secs = float64(n) - default: - raise("TypeError", "can't convert %s into time interval", classNameOf(args[0])) - } - if secs < 0 { - raise("ArgumentError", "time interval must not be negative") + // rb_check_arity(argc, 0, 1) guards the duration branch only, so a + // second argument is refused here but tolerated by a scheduler above. + if len(args) > 1 { + raise("ArgumentError", "wrong number of arguments (given %d, expected 0..1)", len(args)) } + secs = vm.timeInterval(args[0]) + } else if len(args) > 1 { + raise("ArgumentError", "wrong number of arguments (given %d, expected 0..1)", len(args)) } t := vm.currentThread ch := t.parkWake() - start := time.Now() if hasDur { vm.threadBlock(func() { select { @@ -1101,6 +1213,6 @@ func (vm *VM) registerSleep() { } t.unpark() vm.serviceSafepoint(t) // a Thread#raise that woke the park fires here - return object.IntValue(int64(time.Since(start).Seconds() + 0.5)) + return object.IntValue(time.Now().Unix() - beg) }) } diff --git a/internal/vm/wave28_sleep_scheduler_test.go b/internal/vm/wave28_sleep_scheduler_test.go new file mode 100644 index 00000000..e779e10a --- /dev/null +++ b/internal/vm/wave28_sleep_scheduler_test.go @@ -0,0 +1,125 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm_test + +import "testing" + +// TestWave28KernelSleepScheduler pins rb_f_sleep (process.c) and the coercion it +// leans on, rb_time_interval (time.c time_timespec with interval=TRUE). +// +// rb_f_sleep asks rb_fiber_scheduler_current first — the thread's scheduler, but +// only while the running fiber is non-blocking (scheduler.c, +// rb_fiber_scheduler_current_for_threadptr reading th->blocking, which cont.c +// keeps at zero exactly for a non-blocking fiber) — and hands it the arguments +// verbatim through rb_fiber_scheduler_kernel_sleepv. So `sleep` with no argument +// reaches #kernel_sleep with no argument, and the duration is never inspected on +// that path. +// +// Every want string below is the raw stdout of the same source under MRI ruby +// 4.0.5, compared byte for byte. +func TestWave28KernelSleepScheduler(t *testing.T) { + // sched builds the ruby/spec LoggingScheduler in miniature: it records the + // hook it was called with and yields, which is what lets `resume` return. + const sched = ` +class S + attr_reader :events + def initialize; @events = []; end + def block(*a); @events << [:block, a]; Fiber.yield; end + def unblock(*a); @events << [:unblock, a]; Fiber.yield; end + def io_wait(*a); @events << [:io_wait, a]; Fiber.yield; end + def kernel_sleep(*a); @events << [:kernel_sleep, a]; Fiber.yield; end +end +Fiber.set_scheduler(S.new) +` + cases := []struct{ src, want string }{ + // A non-blocking fiber routes sleep to the scheduler; with no argument the + // hook is called with no argument (kernel_sleepv forwards argc verbatim), + // and with one it carries that one. + {sched + `f = Fiber.new(blocking: false) { sleep }; f.resume; p Fiber.scheduler.events`, "[[:kernel_sleep, []]]\n"}, + {sched + `f = Fiber.new(blocking: false) { sleep(0.01) }; f.resume; p Fiber.scheduler.events`, "[[:kernel_sleep, [0.01]]]\n"}, + {sched + `f = Fiber.new(blocking: false) { sleep(nil) }; f.resume; p Fiber.scheduler.events`, "[[:kernel_sleep, [nil]]]\n"}, + // A blocking fiber does not consult it: th->blocking is non-zero there. + {sched + `f = Fiber.new(blocking: true) { sleep(0.001) }; f.resume; p Fiber.scheduler.events`, "[]\n"}, + // Neither does the root fiber, which is blocking too. + {sched + `sleep(0.001); p Fiber.scheduler.events`, "[]\n"}, + // Fiber.scheduler is rb_fiber_scheduler_get: it answers from any fiber, + // blocking or not, which is what the checks above rely on. + {sched + `p Fiber.new(blocking: true) { Fiber.scheduler.class }.resume`, "S\n"}, + // The scheduler path skips rb_check_arity entirely — MRI never looks at + // the arguments it forwards. + {sched + `f = Fiber.new(blocking: false) { sleep(1, 2) }; f.resume; p Fiber.scheduler.events`, "[[:kernel_sleep, [1, 2]]]\n"}, + // ...and it never coerces them, so a String reaches the hook unrefused. + {sched + `f = Fiber.new(blocking: false) { sleep("2") }; f.resume; p Fiber.scheduler.events`, "[[:kernel_sleep, [\"2\"]]]\n"}, + // Uninstalling the scheduler puts the ordinary path back. + {sched + `Fiber.set_scheduler(nil); p Fiber.new(blocking: false) { sleep(0.001) }.resume`, "0\n"}, + + // rb_time_interval, numeric cases. + {`p sleep(0)`, "0\n"}, + {`p sleep(0.001)`, "0\n"}, + {`begin; sleep(-1); rescue => e; p [e.class, e.message]; end`, "[ArgumentError, \"time interval must not be negative\"]\n"}, + {`begin; sleep(-0.1); rescue => e; p [e.class, e.message]; end`, "[ArgumentError, \"time interval must not be negative\"]\n"}, + {`begin; sleep(-Float::INFINITY); rescue => e; p [e.class, e.message]; end`, "[ArgumentError, \"time interval must not be negative\"]\n"}, + // The `f != t.tv_sec` guard: a Float whose integral part cannot be held in + // a time_t. MRI's own vsnprintf renders the non-finite cases "NaN"/"Inf". + {`begin; sleep(Float::NAN); rescue => e; p [e.class, e.message]; end`, "[RangeError, \"NaN out of Time range\"]\n"}, + {`begin; sleep(Float::INFINITY); rescue => e; p [e.class, e.message]; end`, "[RangeError, \"Inf out of Time range\"]\n"}, + {`begin; sleep(1e20); rescue => e; p [e.class, e.message]; end`, "[RangeError, \"100000000000000000000.000000 out of Time range\"]\n"}, + // NUM2TIMET on a Bignum: rbgo only builds one for a value an int64 cannot + // hold, so every Bignum overflows the time_t. + {`begin; sleep(2**70); rescue => e; p [e.class, e.message]; end`, "[RangeError, \"bignum too big to convert into 'long'\"]\n"}, + {`begin; sleep(-(2**70)); rescue => e; p [e.class, e.message]; end`, "[RangeError, \"bignum too big to convert into 'long'\"]\n"}, + + // rb_time_interval, the #divmod(1) fallback: [whole seconds, fraction]. + {`p sleep(Rational(1, 999))`, "0\n"}, + {`o = Object.new; def o.divmod(*); [0, 0.001]; end; p sleep(o)`, "0\n"}, + // NUM2TIMET truncates a Float quotient rather than refusing it. + {`o = Object.new; def o.divmod(*); [0.5, 0]; end; p sleep(o)`, "0\n"}, + // arg_range_check applies to the whole-seconds half only. + {`begin; sleep(Rational(-1, 2)); rescue => e; p [e.class, e.message]; end`, "[ArgumentError, \"time interval must not be negative\"]\n"}, + // A #divmod that does not answer with an Array is no conversion at all. + {`o = Object.new; def o.divmod(*); 5; end; begin; sleep(o); rescue => e; p [e.class, e.message]; end`, "[TypeError, \"can't convert Object into time interval\"]\n"}, + // Nothing to send #divmod to: a String has none. + {`begin; sleep('2'); rescue => e; p [e.class, e.message]; end`, "[TypeError, \"can't convert String into time interval\"]\n"}, + {`begin; sleep(:s); rescue => e; p [e.class, e.message]; end`, "[TypeError, \"can't convert Symbol into time interval\"]\n"}, + // A #divmod reached only through method_missing still converts, as + // rb_check_funcall's respond_to_missing? probe allows. + {`o = Object.new; def o.respond_to_missing?(n, p = false); n == :divmod; end; def o.method_missing(n, *a); n == :divmod ? [0, 0.001] : super; end; p sleep(o)`, "0\n"}, + + // rb_check_arity(argc, 0, 1) guards the duration branch of rb_f_sleep. + {`begin; sleep(1, 2); rescue => e; p [e.class, e.message]; end`, "[ArgumentError, \"wrong number of arguments (given 2, expected 0..1)\"]\n"}, + {`begin; sleep(nil, nil); rescue => e; p [e.class, e.message]; end`, "[ArgumentError, \"wrong number of arguments (given 2, expected 0..1)\"]\n"}, + + // Mutex#sleep validates its timeout with the same rb_time_interval + // (thread_sync.c rb_mutex_sleep), so the Rational reaches it too. + {`m = Mutex.new; m.lock; m.sleep(Rational(1, 999)); p m.locked?`, "true\n"}, + {`m = Mutex.new; m.lock; begin; m.sleep('2'); rescue => e; p [e.class, e.message]; end`, "[TypeError, \"can't convert String into time interval\"]\n"}, + + // IO.select's timeout reaches the coercion without a VM to send #divmod + // with (spawn.go), so it stops at the numeric cases and refuses the rest. + {`begin; IO.select([], [], [], Object.new); rescue => e; p [e.class, e.message]; end`, "[TypeError, \"can't convert Object into time interval\"]\n"}, + {`begin; IO.select([], [], [], -1); rescue => e; p [e.class, e.message]; end`, "[ArgumentError, \"time interval must not be negative\"]\n"}, + {`p IO.select([], [], [], 0)`, "nil\n"}, + } + for _, c := range cases { + if got := eval(t, c.src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + } +} + +// TestWave28SleepReturnsWholeSeconds pins rb_f_sleep's result: it brackets the +// wait with time(0), a clock of whole seconds, so what comes back is the number +// of second boundaries crossed and never a rounding of the duration. A sub-second +// sleep can only answer 0 or 1, and a thread woken early reports far less than +// the duration it asked for. +func TestWave28SleepReturnsWholeSeconds(t *testing.T) { + if got := eval(t, `p [0, 1].include?(sleep(0.2))`); got != "true\n" { + t.Errorf("sub-second sleep result: got %q", got) + } + const woken = `r = nil; t = Thread.new { r = sleep(600) }; Thread.pass until t.status == "sleep"; t.wakeup; t.join; p r < 600` + if got := eval(t, woken); got != "true\n" { + t.Errorf("woken sleep result: got %q", got) + } +} From 4b797e882c6d64c29fed9e63db663ac835759362 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 23 Sep 2026 22:58:32 +0200 Subject: [PATCH 02/10] File.open: the mode/flags/perm contract, driven by open(2) flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit File.open reduced every mode to one of six fopen-style strings and opened from that, so the O_CREAT / O_TRUNC / O_EXCL half of a mode was lost: "wx" did not refuse an existing file, File::WRONLY on a missing path created it instead of raising ENOENT, the permission argument never reached the creation, and the :flags option was ignored outright. Read for this change: - io.c rb_io_modestr_fmode: 'r' is FMODE_READABLE, 'w' is WRITABLE|TRUNC|CREATE, 'a' is WRITABLE|APPEND|CREATE, and 'x' is FMODE_EXCL — but it checks modestr[0] itself, which is why "rx" and "ax" are "invalid access mode", not an exclusive open. - io.c rb_io_oflags_fmode and rb_io_fmode_oflags: the two directions between an fmode and an open(2) flag set. O_APPEND is NOT part of the access bits, so File::RDONLY|File::APPEND is a read-only stream and writing to it is an IOError. - io.c rb_io_extract_modeenc: rb_scan_args "12:", :mode ("mode specified twice"), :flags (OR-ed into the flags, with the fmode then re-derived and the encoding left standing), :perm ("perm specified twice"). - io.c validate_enc_binmode and rb_econv_prepare_options: "newline decorator with binary mode", and "unexpected value for newline option". - file.c rb_file_open_generic: what open(2) then does with those flags. ioModeSpec now carries the create/trunc/excl bits and an oflags() that is rb_io_fmode_oflags, so the integer form, the string form and the :flags option all meet in one place instead of three. core/file/open_spec.rb 55 -> 72, core/file/new_spec.rb 18 -> 21. Nothing else in core/file, core/io or core/kernel moved. Two smaller things the same read turned up, each byte-compared with MRI: - a stream's write permission is granted at OPEN, so File.new(p, "w", 0444) can be written through the handle it returns while a fresh File.open(p, "w") on it is EACCES. A buffer-backed stream defers the write to close, so ioFlush now replays that grant rather than losing it; - O_APPEND moves only WRITES to the end: File.open(p, "a+").read returned "" where MRI returns the file, and #pos on a fresh "a" stream read as the file's size instead of 0. Co-Authored-By: Claude Opus 5 --- internal/vm/io.go | 194 ++++++++++++++++++++-- internal/vm/io_class.go | 111 +++++++++++-- internal/vm/wave28_file_open_mode_test.go | 141 ++++++++++++++++ 3 files changed, 422 insertions(+), 24 deletions(-) create mode 100644 internal/vm/wave28_file_open_mode_test.go diff --git a/internal/vm/io.go b/internal/vm/io.go index c7921bc2..a1230e2b 100644 --- a/internal/vm/io.go +++ b/internal/vm/io.go @@ -1,10 +1,13 @@ package vm import ( + "errors" "io" + "io/fs" "os" "strconv" "strings" + "syscall" "unicode/utf8" "github.com/go-embedded-ruby/ruby/internal/object" @@ -740,23 +743,165 @@ func stringIOModeFlags(mode string) (read, write, trunc, appnd bool) { // it. A trailing Hash mode argument is thus no longer mistaken for a mode String. func (vm *VM) openFileArgs(cls *RClass, args []object.Value) *IOObj { pos, opts := splitIOOpts(args) - if len(pos) == 0 { - raise("ArgumentError", "wrong number of arguments (given 0, expected 1+)") + // rb_scan_args "12:" — path, an optional mode and an optional permission, + // and nothing else. A fourth positional argument is not an options Hash, so + // MRI counts it and refuses. + if len(pos) == 0 || len(pos) > 3 { + raise("ArgumentError", "wrong number of arguments (given %d, expected 1..3)", len(pos)) } - mode := "r" - if len(pos) > 1 && !object.IsNil(pos[1]) { - mode = vm.vmodeString(pos[1]) - } else if opts != nil { - if m, ok := opts.Get(object.Symbol("mode")); ok && !object.IsNil(m) { - mode = vm.vmodeString(m) + path := pathArg(vm, pos[0]) + ms := vm.ioResolveModeEnc(pos, opts) + // rb_io_extract_modeenc's *vperm_p: the third positional argument, which the + // :perm option may supply instead but never as well. + perm, permGiven := int64(0o666), false + if len(pos) > 2 && !object.IsNil(pos[2]) { + perm, permGiven = vm.repeatLong(pos[2]), true + } + if opts != nil { + if v, ok := opts.Get(object.Symbol("perm")); ok && !object.IsNil(v) { + if permGiven { + raise("ArgumentError", "perm specified twice") + } + perm, permGiven = vm.repeatLong(v), true } } - o := openFileIO(cls, pathArg(vm, pos[0]), mode) - ms := vm.ioResolveModeEnc(pos, opts) + o := openFileSpec(cls, path, &ms, perm, permGiven) o.extEnc, o.intEnc, o.binmode = ms.extEnc, ms.intEnc, ms.binmode return o } +// modeString renders the fopen-style mode a stream reports and #reopen replays. +// It is derived from the fmode rather than remembered, so an integer mode and +// the :flags option produce the same spelling as the equivalent mode string. +func (ms *ioModeSpec) modeString() string { + var base string + switch { + case ms.appendMode: + base = "a" + case ms.trunc && ms.writable: + base = "w" + case ms.writable && !ms.readable: + base = "w" + default: + base = "r" + } + if ms.readable && ms.writable { + base += "+" + } + if ms.binmode { + base += "b" + } + return base +} + +// openFileSpec is rb_file_open_generic (file.c) for this VM's buffer-backed +// streams: it applies the open(2) flag set ms describes to path, and builds the +// IOObj whose buffer stands in for the descriptor. +// +// The flags decide, in MRI's order, what open(2) itself would do: +// - O_CREAT|O_EXCL on an existing path is EEXIST — the whole of what the 'x' +// mode flag and File::EXCL mean; +// - no O_CREAT on a missing path is ENOENT; +// - O_CREAT creates the file with perm (0666 before the umask when none was +// given), so File.stat sees it before the first flush; +// - O_TRUNC empties it. +// +// The access bits then set the halves the stream refuses: a read-only stream +// raises "not opened for writing" on write and a write-only one "not opened for +// reading" on read, and O_APPEND alone does not make a stream writable. +func openFileSpec(cls *RClass, p string, ms *ioModeSpec, perm int64, permGiven bool) *IOObj { + o := &IOObj{cls: cls, isStr: true, path: p, openMode: ms.modeString()} + st, statErr := os.Stat(p) + exists := statErr == nil + switch { + case ms.create && ms.excl && exists: + raise("Errno::EEXIST", "File exists @ rb_sysopen - %s", p) + case !exists && !ms.create: + raiseOpenErrno(statErr, p) + } + if exists && st.IsDir() && ms.writable { + // open(2) refuses a directory for writing whatever the rest of the flags + // say; opening one for reading succeeds, which is what File.open(dir) is. + raise("Errno::EISDIR", "Is a directory @ rb_sysopen - %s", p) + } + if !permGiven { + perm = 0o666 + } + switch { + case !exists: + // O_CREAT: materialise the file now, as MRI's open does, so File.exist? + // and File.stat see it (with its permissions) before any write is flushed. + f, err := os.OpenFile(p, os.O_CREATE|os.O_WRONLY, os.FileMode(perm&0o7777)) + if err != nil { + raiseOpenErrno(err, p) + } + _ = f.Close() + case ms.trunc: + if err := os.Truncate(p, 0); err != nil { + raiseOpenErrno(err, p) + } + case notRegular(p): + // Nothing that is not a regular file can be read whole, because some of + // them do not end: File.open("/dev/zero") allocated 83 GB here and killed a + // CI runner before the open returned. A character device, a fifo, a socket + // or a directory opens with an empty buffer instead, so the position + // arithmetic works — which is all core/io/seek_spec.rb asks of /dev/zero — + // and a read sees end-of-file rather than the machine going away. + // + // That reads see nothing is a limit of a buffer-backed IO rather than a + // decision: see IOObj, whose whole model is the file's bytes in memory. + default: + b, err := os.ReadFile(p) + if err != nil { + raiseOpenErrno(err, p) + } + o.buf = b + } + // A write-only stream must still be able to reopen the path for writing; a + // read-only one must not, so a permission failure shows up at open, not at + // close. os.ReadFile above already reported an unreadable file. + if ms.writable && exists && !ms.trunc { + if err := writableCheck(p); err != nil { + raiseOpenErrno(err, p) + } + } + // The read cursor starts at 0 even in append mode: O_APPEND moves only WRITES + // to the end (writeBytes does that), so `File.open(p, "a+").read` returns the + // whole file, as it does in MRI, and `#pos` on a freshly opened "a" stream is + // 0 rather than the file's size. + o.writable = ms.writable + o.rdClosed = !ms.readable + o.wrClosed = !ms.writable + o.appendMode = ms.appendMode + return o +} + +// writableCheck reports the error opening p for writing would give, without +// disturbing its contents — the permission failure MRI's open(2) raises at +// File.open time but a buffer-backed stream would otherwise only meet at flush. +func writableCheck(p string) error { + f, err := os.OpenFile(p, os.O_WRONLY, 0) + if err != nil { + return err + } + return f.Close() +} + +// raiseOpenErrno turns a Go open/read failure into the Errno MRI's rb_sysopen +// reports, message and all. Anything unrecognised falls back to ENOENT, which is +// what this code raised for every failure before. +func raiseOpenErrno(err error, p string) { + switch { + case errors.Is(err, fs.ErrPermission): + raise("Errno::EACCES", "Permission denied @ rb_sysopen - %s", p) + case errors.Is(err, syscall.EISDIR): + raise("Errno::EISDIR", "Is a directory @ rb_sysopen - %s", p) + case errors.Is(err, fs.ErrExist): + raise("Errno::EEXIST", "File exists @ rb_sysopen - %s", p) + } + raise("Errno::ENOENT", "No such file or directory @ rb_sysopen - %s", p) +} + // vmodeString reduces a File.open mode argument to the fopen-style base mode // string openFileIO understands: an Integer (or #to_int object) is mapped through // flagsToMode; a String is taken as-is (its ":enc" suffix is tolerated and later @@ -861,12 +1006,35 @@ func notRegular(p string) bool { } // ioFlush writes a writable file stream's buffer back to disk. +// +// The write happens at flush rather than at open, which is where this model +// differs from a descriptor: in MRI the permission to write was granted when the +// file was opened, and a later chmod — even the `File.new(path, "w", 0444)` that +// creates a read-only file and then writes through the descriptor it returns — +// cannot take it back. Reopening the path would, so a flush that is refused for +// permission puts the owner-write bit back for the length of the write and +// restores the file's mode afterwards. That is the descriptor's grant, replayed; +// it never widens a file this stream was not already entitled to write. func ioFlush(o *IOObj) { - if o.writable && o.path != "" { - if err := os.WriteFile(o.path, o.buf, 0o644); err != nil { - raise("Errno::ENOENT", "No such file or directory @ rb_sysopen - %s", o.path) + if !o.writable || o.path == "" { + return + } + err := os.WriteFile(o.path, o.buf, 0o644) + if err == nil { + return + } + if errors.Is(err, fs.ErrPermission) { + if st, sterr := os.Stat(o.path); sterr == nil { + mode := st.Mode().Perm() + if chmodErr := os.Chmod(o.path, mode|0o200); chmodErr == nil { + err = os.WriteFile(o.path, o.buf, 0o644) + _ = os.Chmod(o.path, mode) + } } } + if err != nil { + raiseOpenErrno(err, o.path) + } } // ioFlushClose flushes then marks the stream closed (the File.open block exit). diff --git a/internal/vm/io_class.go b/internal/vm/io_class.go index b4b3ef1c..c5006fd5 100644 --- a/internal/vm/io_class.go +++ b/internal/vm/io_class.go @@ -658,6 +658,44 @@ type ioModeSpec struct { extEnc, intEnc string hasEnc bool // the mode STRING carried a ":enc" suffix explicit bool // a mode argument (or :mode option) was given + + // create, trunc and excl are the rest of MRI's fmode: FMODE_CREATE, + // FMODE_TRUNC and FMODE_EXCL. A wrapper around an existing descriptor has no + // use for them, but File.open's open(2) does — "w" is O_WRONLY|O_CREAT|O_TRUNC + // and "wx" adds O_EXCL, which is the whole of what the 'x' flag means. + create, trunc, excl bool + + // newline is the :newline option's decorator, "" when none was asked for. MRI + // turns it into an ECONV_*_NEWLINE_DECORATOR bit and then refuses the + // combination with binmode (validate_enc_binmode, io.c). + newline string +} + +// oflags is rb_io_fmode_oflags (io.c): the open(2) flag set this fmode asks for. +// It is the form File.open needs, and the form the :flags option merges into. +func (ms *ioModeSpec) oflags() int64 { + var f int64 + switch { + case ms.readable && ms.writable: + f = fO_RDWR + case ms.writable: + f = fO_WRONLY + default: + f = fO_RDONLY + } + if ms.appendMode { + f |= fO_APPEND + } + if ms.trunc { + f |= fO_TRUNC + } + if ms.create { + f |= fO_CREAT + } + if ms.excl { + f |= fO_EXCL + } + return f } // ioResolveModeEnc decodes the (fd, mode, **opts) arguments of IO.new / IO.open @@ -687,7 +725,18 @@ func (vm *VM) ioResolveModeEnc(pos []object.Value, opts *object.Hash) ioModeSpec ms.readable = true // no explicit mode ⇒ the wrapper inherits the fd's mode } if opts != nil { + // rb_io_extract_modeenc reads :flags right after the mode and re-derives + // the fmode from the merged open flags, so `File.open(p, "w", flags: + // File::EXCL)` is exactly `File.open(p, File::WRONLY|File::CREAT| + // File::TRUNC|File::EXCL)`. The encoding fields survive because + // flagsModeSpec does not touch them — MRI resolves the encoding before this + // point for the same reason. + if v, ok := opts.Get(object.Symbol("flags")); ok && !object.IsNil(v) { + flagsModeSpec(ms.oflags()|vm.repeatLong(v), &ms) + ms.explicit = true + } extractBinmode(opts, &ms) + extractNewline(opts, &ms) } // A binary stream defaults to ASCII-8BIT external encoding unless the mode // string already named one (a later :encoding option may still override it). @@ -727,20 +776,29 @@ func (vm *VM) resolveVmode(v object.Value, ms *ioModeSpec) { vm.parseModeString(s.Str(), ms) } -// flagsModeSpec fills the read/write/append intent of ms from an integer open-flag -// set (a bitwise OR of File::RDONLY/WRONLY/RDWR/APPEND). +// flagsModeSpec is rb_io_oflags_fmode (io.c): it REPLACES the access half of ms +// from an integer open-flag set (a bitwise OR of File::RDONLY/WRONLY/RDWR/APPEND/ +// TRUNC/CREAT/EXCL), leaving the encoding fields alone — which is what lets the +// :flags option re-derive the fmode after OR-ing into the flags without losing an +// encoding the mode string already named. +// +// O_APPEND does NOT imply writability here, any more than it does in MRI: the +// access bits alone decide, so File::RDONLY|File::APPEND is a READ-ONLY stream +// and writing to it is an IOError. func flagsModeSpec(flags int64, ms *ioModeSpec) { + ms.readable, ms.writable = false, false switch flags & 0x3 { - case fO_RDONLY: - ms.readable = true case fO_WRONLY: ms.writable = true - default: // RDWR + case fO_RDWR: ms.readable, ms.writable = true, true + default: // fO_RDONLY (and the invalid 0x3, which open(2) rejects) + ms.readable = true } - if flags&fO_APPEND != 0 { - ms.writable, ms.appendMode = true, true - } + ms.appendMode = flags&fO_APPEND != 0 + ms.trunc = flags&fO_TRUNC != 0 + ms.create = flags&fO_CREAT != 0 + ms.excl = flags&fO_EXCL != 0 } // parseModeString fills ms from an fopen-style mode string ("r"/"w"/"a" with an @@ -760,9 +818,12 @@ func (vm *VM) parseModeString(mode string, ms *ioModeSpec) { case 'r': ms.readable = true case 'w': - ms.writable = true + // rb_io_modestr_fmode: 'w' is FMODE_WRITABLE|FMODE_TRUNC|FMODE_CREATE and + // 'a' is FMODE_WRITABLE|FMODE_APPEND|FMODE_CREATE — the create and truncate + // halves are part of the letter, not something File.open adds later. + ms.writable, ms.trunc, ms.create = true, true, true case 'a': - ms.writable, ms.appendMode = true, true + ms.writable, ms.appendMode, ms.create = true, true, true default: raise("ArgumentError", "invalid access mode %s", mode) } @@ -780,7 +841,14 @@ func (vm *VM) parseModeString(mode string, ms *ioModeSpec) { raise("ArgumentError", "invalid access mode %s", mode) } ms.textmode = true - case 'x': // exclusive-create flag; no effect on the fmode intent + case 'x': + // FMODE_EXCL, and only on a 'w' base: rb_io_modestr_fmode checks + // modestr[0] itself, so "rx" and "ax" are an invalid access mode rather + // than an exclusive open. + if base[0] != 'w' { + raise("ArgumentError", "invalid access mode %s", mode) + } + ms.excl = true default: raise("ArgumentError", "invalid access mode %s", mode) } @@ -806,6 +874,27 @@ func (vm *VM) parseEncPart(enc string, ms *ioModeSpec) { } } +// extractNewline applies the :newline option, which names one of MRI's newline +// decorators. rb_econv_prepare_options refuses anything else by name, and +// validate_enc_binmode (io.c) then refuses any decorator at all on a binary +// stream — there is no newline to translate in bytes. +func extractNewline(opts *object.Hash, ms *ioModeSpec) { + v, ok := opts.Get(object.Symbol("newline")) + if !ok || object.IsNil(v) { + return + } + sym, isSym := v.(object.Symbol) + switch { + case isSym && (sym == "universal" || sym == "crlf" || sym == "cr" || sym == "lf"): + ms.newline = string(sym) + default: + raise("ArgumentError", "unexpected value for newline option: %s", v.ToS()) + } + if ms.binmode { + raise("ArgumentError", "newline decorator with binary mode") + } +} + // extractBinmode applies the :textmode / :binmode options to ms, raising the // ArgumentError conflicts MRI's extract_binmode does ("textmode specified twice", // "binmode specified twice", "both textmode and binmode specified"). diff --git a/internal/vm/wave28_file_open_mode_test.go b/internal/vm/wave28_file_open_mode_test.go new file mode 100644 index 00000000..9e441ab1 --- /dev/null +++ b/internal/vm/wave28_file_open_mode_test.go @@ -0,0 +1,141 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm_test + +import ( + "os" + "path/filepath" + "testing" +) + +// TestWave28FileOpenModeContract pins File.open's mode / flags / permission +// contract against the C it is ported from: +// +// - io.c rb_io_modestr_fmode — 'r' is FMODE_READABLE, 'w' is +// WRITABLE|TRUNC|CREATE, 'a' is WRITABLE|APPEND|CREATE, and 'x' is FMODE_EXCL +// but ONLY when modestr[0] is 'w', which is why "rx" and "ax" are an invalid +// access mode rather than an exclusive open; +// - io.c rb_io_oflags_fmode / rb_io_fmode_oflags — the two directions between +// an fmode and an open(2) flag set. O_APPEND does not imply writability, so +// File::RDONLY|File::APPEND is a read-only stream; +// - io.c rb_io_extract_modeenc — :mode ("mode specified twice"), :flags +// (OR-ed into the open flags, then the fmode re-derived), :perm ("perm +// specified twice") and the rb_scan_args "12:" arity; +// - io.c validate_enc_binmode — "newline decorator with binary mode"; +// - file.c rb_file_open_generic — what open(2) then does with those flags. +// +// Every want string is the raw stdout of the same source under MRI ruby 4.0.5, +// compared byte for byte in a scratch directory holding a file "f.txt" with the +// contents "hello file\n" and an empty directory "adir". +func TestWave28FileOpenModeContract(t *testing.T) { + cases := []struct{ src, want string }{ + // --- the 'x' flag: FMODE_EXCL, and only on a 'w' base. + {`File.open("f.txt", "wx")`, "[Errno::EEXIST, \"File exists @ rb_sysopen - f.txt\"]\n"}, + {`File.open("f.txt", "rx")`, "[ArgumentError, \"invalid access mode rx\"]\n"}, + {`File.open("f.txt", "ax")`, "[ArgumentError, \"invalid access mode ax\"]\n"}, + {`File.open("n1.txt", "wx") { |f| f.write "c" }; File.read("n1.txt")`, "\"c\"\n"}, + + // --- integer modes go through open(2)'s flags, not a mode string. + {`File.open("nope.txt", File::WRONLY)`, "[Errno::ENOENT, \"No such file or directory @ rb_sysopen - nope.txt\"]\n"}, + {`File.open("f.txt", File::CREAT | File::EXCL)`, "[Errno::EEXIST, \"File exists @ rb_sysopen - f.txt\"]\n"}, + {`f = File.open("n2.txt", File::CREAT); [File.exist?("n2.txt"), f.class]`, "[true, File]\n"}, + {`File.open("f.txt", File::TRUNC | File::WRONLY) {}; File.read("f.txt")`, "\"\"\n"}, + // O_APPEND alone leaves the stream read-only. + {`File.open("f.txt", File::RDONLY | File::APPEND) { |f| f.write "x" }`, "[IOError, \"not opened for writing\"]\n"}, + {`File.open("f.txt", File::RDONLY | File::APPEND) { |f| f.read }`, "\"hello file\\n\"\n"}, + + // --- the :flags option merges into the open flags and re-derives the fmode. + {`File.open("f.txt", "w", flags: File::EXCL) {}`, "[Errno::EEXIST, \"File exists @ rb_sysopen - f.txt\"]\n"}, + {`File.open("f.txt", mode: "w", flags: File::EXCL) {}`, "[Errno::EEXIST, \"File exists @ rb_sysopen - f.txt\"]\n"}, + {`File.open("f.txt", File::WRONLY | File::CREAT, flags: File::EXCL) {}`, "[Errno::EEXIST, \"File exists @ rb_sysopen - f.txt\"]\n"}, + // The encoding the mode string named survives the re-derivation, because + // MRI resolves it before reaching the :flags branch. + {`File.open("f.txt", "rb", flags: File::RDONLY) { |f| f.external_encoding.to_s }`, "\"ASCII-8BIT\"\n"}, + + // --- arity and the "specified twice" conflicts. + {`File.open("f.txt", File::CREAT, 0755, "test")`, "[ArgumentError, \"wrong number of arguments (given 4, expected 1..3)\"]\n"}, + {`File.open`, "[ArgumentError, \"wrong number of arguments (given 0, expected 1..3)\"]\n"}, + {`File.open("f.txt", "r", mode: "w")`, "[ArgumentError, \"mode specified twice\"]\n"}, + {`File.open("p3.txt", "w", 0744, perm: 0755) {}`, "[ArgumentError, \"perm specified twice\"]\n"}, + + // --- the permission argument reaches the creation, in both spellings. + {`File.open("p2.txt", File::CREAT | File::RDWR, 0744) { |f| f.write "z" }; File.stat("p2.txt").mode.to_s(8)`, "\"100744\"\n"}, + {`File.open("p4.txt", "w", perm: 0745) {}; File.stat("p4.txt").mode.to_s(8)`, "\"100745\"\n"}, + + // --- the :newline decorator. + {`File.open("f.txt", "rb", newline: :universal) {}`, "[ArgumentError, \"newline decorator with binary mode\"]\n"}, + {`File.open("f.txt", "r", newline: :bogus) {}`, "[ArgumentError, \"unexpected value for newline option: bogus\"]\n"}, + {`File.open("f.txt", "r", newline: nil) { |f| f.read }`, "\"hello file\\n\"\n"}, + {`File.open("f.txt", "r", newline: :universal) { |f| f.read }`, "\"hello file\\n\"\n"}, + + // --- a directory opens for reading and refuses to be written. + {`File.open("adir").class`, "File\n"}, + {`File.open("adir", "w")`, "[Errno::EISDIR, \"Is a directory @ rb_sysopen - adir\"]\n"}, + + // --- the access halves the fmode closes. + {`File.open("f.txt", "w") { |f| f.read }`, "[IOError, \"not opened for reading\"]\n"}, + {`File.open("f.txt", "r") { |f| f.write "q" }`, "[IOError, \"not opened for writing\"]\n"}, + + // --- O_APPEND moves WRITES to the end, and only writes: a freshly opened + // append stream reads from the beginning and reports position 0. + {`File.open("f.txt", "a") { |f| f.pos }`, "0\n"}, + {`File.open("f.txt", "a+") { |f| f.read }`, "\"hello file\\n\"\n"}, + {`File.open("f.txt", "a") { |f| f.write "CD" }; File.read("f.txt")`, "\"hello file\\nCD\"\n"}, + } + for _, c := range cases { + dir := openModeScratch(t) + src := "Dir.chdir(" + rubyString(dir) + ")\nbegin\n p(begin\n" + c.src + "\nend)\nrescue => e\n p [e.class, e.message]\nend\n" + if got := eval(t, src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + } +} + +// TestWave28FileOpenPermissionGrant covers the one place a buffer-backed stream +// has to work to look like a descriptor: the permission to write is granted when +// the file is OPENED, so a chmod afterwards — including the read-only file +// File.new(path, "w", 0444) itself creates — cannot take it back, while a fresh +// open of the same path is refused with EACCES. Both halves asserted against MRI +// 4.0.5. +func TestWave28FileOpenPermissionGrant(t *testing.T) { + dir := openModeScratch(t) + src := "Dir.chdir(" + rubyString(dir) + ")\n" + ` +fh = File.open("ro.txt", "w") +fh.chmod(0444) +begin + File.open("ro.txt", "w") + puts "reopened" +rescue => e + p [e.class, e.message] +end +fh.close +f = File.new("wro.txt", "w", 0444) +f.puts("test") +f.close +p File.read("wro.txt") +` + const want = "[Errno::EACCES, \"Permission denied @ rb_sysopen - ro.txt\"]\n\"test\\n\"\n" + if got := eval(t, src); got != want { + t.Errorf("got=%q\nwant=%q", got, want) + } + // Leave nothing unwritable behind for the test framework's cleanup. + for _, n := range []string{"ro.txt", "wro.txt"} { + _ = os.Chmod(filepath.Join(dir, n), 0o644) + } +} + +// openModeScratch builds the fixture the cases above assume: a fresh directory +// holding "f.txt" (contents "hello file\n") and an empty directory "adir". +func openModeScratch(t *testing.T) string { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "f.txt"), []byte("hello file\n"), 0o644); err != nil { + t.Fatalf("fixture: %v", err) + } + if err := os.Mkdir(filepath.Join(dir, "adir"), 0o755); err != nil { + t.Fatalf("fixture: %v", err) + } + return dir +} From 9e2d4b3d3795e8476e1d3ac0c1c931387defdea2 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 23 Sep 2026 23:04:15 +0200 Subject: [PATCH 03/10] File#flock, File#size and IO#truncate: reach the file, not just the buffer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit core/file/flock_spec.rb scored 0 because File#flock did not exist, and three more specs asked the buffer a question only the file could answer. Read for this change: - file.c rb_file_flock: it flushes a writable stream, then calls flock(2) in a LOOP — with LOCK_NB a refusal (EWOULDBLOCK/EAGAIN/EACCES) is `false`, and WITHOUT it MRI waits 0.1 s with rb_thread_wait_for and tries again. MRI polls here rather than blocking in the syscall, which is what keeps the wait interruptible, and is exactly what this VM's cooperative threads can reproduce. - thread.c rb_thread_wait_for / sleep_hrtime: the wait that poll uses, now a named threadWaitFor shared with Kernel#sleep, so the waiting thread reports "sleep" and a Thread#kill reaches it. - io.c rb_io_truncate: ftruncate(2) on the descriptor, so the file is that size AT ONCE — File.size sees it before any flush — and GetOpenFile runs before rb_io_check_writable, so a closed stream is "closed stream" whatever mode it had. - file.c rb_file_size: an fstat on the OPEN descriptor, so it reports a change another stream made and keeps answering after the path is gone. A lock cannot be emulated: flock(2) is a property of an open file description. These streams are buffer-backed and have no descriptor (see ioFd), so File#flock opens a real one on the path, holds it for as long as the lock is, and drops it on LOCK_UN or on close. lockFdFn / flockFn / closeFdFn / flockAgainErrs are the platform seam; the windows and wasm variants report ENOTSUP, and wasip1 has no EWOULDBLOCK at all, which is why the retry list is per platform rather than written into the shared code. core/file/flock_spec.rb 0 -> 3 (2 of the 5 need ruby_exe, skipped) core/file/truncate_spec.rb 18 -> 23 core/file/size_spec.rb 20 -> 22 Nothing else in core/file, core/io or core/kernel moved. Co-Authored-By: Claude Opus 5 --- internal/vm/filestat_unix.go | 20 +++ internal/vm/filestat_wasm.go | 13 ++ internal/vm/filestat_windows.go | 13 ++ internal/vm/io.go | 152 +++++++++++++++++++ internal/vm/thread.go | 19 +++ internal/vm/wave28_flock_internal_test.go | 177 ++++++++++++++++++++++ 6 files changed, 394 insertions(+) create mode 100644 internal/vm/wave28_flock_internal_test.go diff --git a/internal/vm/filestat_unix.go b/internal/vm/filestat_unix.go index 36ed8057..2ebfb6a2 100644 --- a/internal/vm/filestat_unix.go +++ b/internal/vm/filestat_unix.go @@ -63,3 +63,23 @@ func statSys(fi fs.FileInfo) statFields { hasSys: true, } } + +// lockFdFn, flockFn and closeFdFn are the POSIX seam behind File#flock. A +// buffer-backed stream has no descriptor of its own, but flock(2) is a property +// of an open file description, so one is opened on the file's path and held for +// as long as the lock is. They are function variables so a whitebox test can +// drive the retry and Errno mapping without taking a real lock. +// +// File::LOCK_SH/EX/NB/UN carry the same values on Linux and BSD as the constants +// rbgo defines, so the operation is passed through unchanged. +var ( + lockFdFn = func(path string) (int, error) { return unix.Open(path, unix.O_RDONLY, 0) } + flockFn = func(fd, op int) error { return unix.Flock(fd, op) } + closeFdFn = func(fd int) error { return unix.Close(fd) } +) + +// flockAgainErrs are the errno values flock(2) reports for "another open file +// description holds this lock", which rb_file_flock retries on rather than +// raising. EACCES is in the list because some systems report it in place of +// EWOULDBLOCK. +var flockAgainErrs = []error{syscall.EWOULDBLOCK, syscall.EAGAIN, syscall.EACCES} diff --git a/internal/vm/filestat_wasm.go b/internal/vm/filestat_wasm.go index fba3897c..6bf8e705 100644 --- a/internal/vm/filestat_wasm.go +++ b/internal/vm/filestat_wasm.go @@ -42,3 +42,16 @@ func statSys(fi fs.FileInfo) statFields { hasSys: true, } } + +// lockFdFn / flockFn / closeFdFn are unsupported on wasm: there is no flock(2), so +// File#flock reports the operation as unsupported. They exist only to satisfy the +// seam the shared io.go code references. +var ( + lockFdFn = func(string) (int, error) { return 0, errors.ErrUnsupported } + flockFn = func(int, int) error { return errors.ErrUnsupported } + closeFdFn = func(int) error { return nil } +) + +// flockAgainErrs is empty here: with no flock(2) there is no "would block" to +// retry on, and every call reports the operation as unsupported instead. +var flockAgainErrs []error diff --git a/internal/vm/filestat_windows.go b/internal/vm/filestat_windows.go index 032c7c65..b189d13b 100644 --- a/internal/vm/filestat_windows.go +++ b/internal/vm/filestat_windows.go @@ -46,3 +46,16 @@ func statSys(fi fs.FileInfo) statFields { func statOwned(*FileStat) bool { return true } func statGrpowned(*FileStat) bool { return false } func statPerm(fi fs.FileInfo) int64 { return int64(fi.Mode().Perm()) &^ 0o022 } + +// lockFdFn / flockFn / closeFdFn are unsupported on Windows: there is no flock(2), so +// File#flock reports the operation as unsupported. They exist only to satisfy the +// seam the shared io.go code references. +var ( + lockFdFn = func(string) (int, error) { return 0, errors.ErrUnsupported } + flockFn = func(int, int) error { return errors.ErrUnsupported } + closeFdFn = func(int) error { return nil } +) + +// flockAgainErrs is empty here: with no flock(2) there is no "would block" to +// retry on, and every call reports the operation as unsupported instead. +var flockAgainErrs []error diff --git a/internal/vm/io.go b/internal/vm/io.go index a1230e2b..c0834006 100644 --- a/internal/vm/io.go +++ b/internal/vm/io.go @@ -8,6 +8,7 @@ import ( "strconv" "strings" "syscall" + "time" "unicode/utf8" "github.com/go-embedded-ruby/ruby/internal/object" @@ -88,6 +89,14 @@ type IOObj struct { // only its clearing — a zero-value IOObj reports close-on-exec, as MRI does. closeOnExecOff bool + // lockFd is the real descriptor File#flock holds on path while a lock is + // taken. A buffer-backed stream has no descriptor of its own (see ioFd), but + // flock(2) is a property of an open file description and cannot be emulated, + // so one is opened on demand and closed when the lock is released or the + // stream is. hasLockFd distinguishes "not opened" from the valid fd 0. + lockFd int + hasLockFd bool + // strObj is the live String object backing a StringIO — MRI's StringIO holds // (and mutates in place) the very String passed to it, so #string returns that // same object (identity) and writes/truncation are visible through it. buf is @@ -583,6 +592,26 @@ func (vm *VM) registerIO() { } return object.IntValue(0) }) + // File#size is rb_file_size: an fstat on the OPEN descriptor, so it reports + // what the file is now — including a change another stream made since this one + // was opened — and keeps reporting the size the descriptor still holds after + // the path has been unlinked. The buffer is flushed first so this stream's own + // unwritten bytes are counted, and the buffer's length is the answer once the + // path is gone, which is the descriptor MRI still has. + cFile.define("size", func(_ *VM, self object.Value, _ []object.Value, _ *Proc) object.Value { + o := self.(*IOObj) + if o.closed { + raise("IOError", "closed stream") + } + ioFlush(o) + if st, err := os.Stat(o.path); err == nil { + return object.IntValue(st.Size()) + } + return object.IntValue(int64(len(o.buf))) + }) + cFile.define("flock", func(vm *VM, self object.Value, args []object.Value, _ *Proc) object.Value { + return vm.fileFlock(self.(*IOObj), int(vm.toIntCoerce(args[0]))) + }) // IO.read/binread/write/binwrite/foreach/readlines, installed identically on IO // and File (File.readlines/foreach included) now that both classes exist. vm.registerIOClassMethods(cIO, cFile) @@ -887,6 +916,115 @@ func writableCheck(p string) error { return f.Close() } +// flock operations, the values File::LOCK_SH and friends carry. BSD and Linux +// agree on all four, so they are also what flock(2) is handed unchanged. +const ( + flockSH = 0x1 + flockEX = 0x2 + flockNB = 0x4 + flockUN = 0x8 +) + +// fileFlock is rb_file_flock (file.c). MRI flushes a writable stream first, then +// calls flock(2) in a loop: with LOCK_NB a refusal is `false`, and WITHOUT it the +// call is retried every 0.1 s — MRI polls here rather than blocking in the +// syscall, which is what makes the wait interruptible, and is why this VM's +// cooperative threads can reproduce it exactly (the wait is rb_thread_wait_for, +// so the waiting thread reports "sleep" and a Thread#kill reaches it). +// +// The descriptor is opened on demand: this VM's streams are buffer-backed and +// have no descriptor of their own, but an flock belongs to an open file +// description and cannot be faked, so a real one is held for as long as the lock +// is. LOCK_UN, and closing the stream, let it go. +func (vm *VM) fileFlock(o *IOObj, op int) object.Value { + if o.closed { + raise("IOError", "closed stream") + } + if o.writable { + ioFlush(o) + } + if op&flockUN != 0 { + if o.hasLockFd { + if err := flockFn(o.lockFd, flockUN); err != nil { + raiseFlockErrno(err, o.path) + } + o.releaseLockFd() + } + return object.IntValue(0) + } + fd, err := o.lockDescriptor() + if err != nil { + raiseOpenErrno(err, o.path) + } + for { + err := flockFn(fd, op) + if err == nil { + return object.IntValue(0) + } + if !flockWouldBlock(err) { + raiseFlockErrno(err, o.path) + } + if op&flockNB != 0 { + return object.False + } + vm.threadWaitFor(100 * time.Millisecond) + if o.closed { + raise("IOError", "closed stream") + } + } +} + +// lockDescriptor opens (once) the real descriptor this stream's flocks are taken +// on, and hands it back on every later call so the lock is upgraded or downgraded +// in place rather than taken twice. +func (o *IOObj) lockDescriptor() (int, error) { + if o.hasLockFd { + return o.lockFd, nil + } + fd, err := lockFdFn(o.path) + if err != nil { + return 0, err + } + o.lockFd, o.hasLockFd = fd, true + return fd, nil +} + +// releaseLockFd closes the descriptor lockDescriptor opened, which is itself what +// drops any lock still held on it. +func (o *IOObj) releaseLockFd() { + if !o.hasLockFd { + return + } + _ = closeFdFn(o.lockFd) + o.lockFd, o.hasLockFd = 0, false +} + +// flockWouldBlock reports the "someone else holds it" errors rb_file_flock +// retries on: EWOULDBLOCK/EAGAIN, and the EACCES some systems report instead. +// The list itself is per platform (flockAgainErrs), because not every target +// names all three — wasip1 has no EWOULDBLOCK at all. +func flockWouldBlock(err error) bool { + for _, e := range flockAgainErrs { + if errors.Is(err, e) { + return true + } + } + return false +} + +// raiseFlockErrno reports a flock failure the way rb_syserr_fail_path does, naming +// the file. An unsupported platform reports ENOTSUP, which is what a system +// without flock(2) gives. +func raiseFlockErrno(err error, p string) { + switch { + case errors.Is(err, syscall.EBADF): + raise("Errno::EBADF", "Bad file descriptor @ rb_file_flock - %s", p) + case errors.Is(err, syscall.EINVAL): + raise("Errno::EINVAL", "Invalid argument @ rb_file_flock - %s", p) + } + raise("Errno::ENOTSUP", "Operation not supported @ rb_file_flock - %s", p) +} + // raiseOpenErrno turns a Go open/read failure into the Errno MRI's rb_sysopen // reports, message and all. Anything unrecognised falls back to ENOENT, which is // what this code raised for every failure before. @@ -1245,6 +1383,7 @@ func defIOWrite(cls *RClass) { o := self.(*IOObj) ioFlush(o) o.pipeEndClosed() + o.releaseLockFd() // close(2) drops any flock this stream still holds o.closed = true return object.NilV }) @@ -1564,6 +1703,11 @@ func defStringIORead(cls *RClass) { }) cls.define("truncate", func(vm *VM, self object.Value, args []object.Value, _ *Proc) object.Value { o := self.(*IOObj) + // rb_io_truncate goes through GetOpenFile then rb_io_check_writable, in + // that order: a closed stream is "closed stream" whichever mode it had. + if o.closed && !ioIsStringIO(o) { + raise("IOError", "closed stream") + } if o.wrClosed { // a read-only stream cannot be truncated raise("IOError", "not opened for writing") } @@ -1577,6 +1721,14 @@ func defStringIORead(cls *RClass) { o.buf = append(o.buf, make([]byte, n-len(o.buf))...) } o.syncStr() + // rb_io_truncate is ftruncate(2) on the descriptor: the file on disk is + // that size at once, not at the next flush, which is what File.size and a + // second stream opened on the same path see straight afterwards. + if o.path != "" && !ioIsStringIO(o) { + if err := os.Truncate(o.path, int64(n)); err != nil { + raiseOpenErrno(err, o.path) + } + } return object.IntValue(0) }) cls.define("read", func(vm *VM, self object.Value, args []object.Value, _ *Proc) object.Value { diff --git a/internal/vm/thread.go b/internal/vm/thread.go index 0a4e6fd2..4fb02e96 100644 --- a/internal/vm/thread.go +++ b/internal/vm/thread.go @@ -1163,6 +1163,25 @@ func (vm *VM) mutexUnlock(m *RMutex) { m.owner = nil } +// threadWaitFor is rb_thread_wait_for (thread.c): park the current thread for +// d, releasing the GVL so other threads run, and deliver whatever asynchronous +// event woke it. It is the wait a blocking operation polls on — File#flock's +// 0.1 s retry, which MRI spells the same way — so the thread reports "sleep" +// while it waits and a Thread#kill or Thread#raise reaches it at the safepoint +// rather than after the whole operation. +func (vm *VM) threadWaitFor(d time.Duration) { + t := vm.currentThread + ch := t.parkWake() + vm.threadBlock(func() { + select { + case <-ch: + case <-time.After(d): + } + }) + t.unpark() + vm.serviceSafepoint(t) +} + // registerSleep adds a GVL-aware Kernel#sleep that releases the lock while // sleeping, so other threads run. It follows rb_f_sleep (process.c): a fiber // scheduler, when one is installed and the running fiber is non-blocking, takes diff --git a/internal/vm/wave28_flock_internal_test.go b/internal/vm/wave28_flock_internal_test.go new file mode 100644 index 00000000..2ffcd3d8 --- /dev/null +++ b/internal/vm/wave28_flock_internal_test.go @@ -0,0 +1,177 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm + +import ( + "os" + "path/filepath" + "runtime" + "syscall" + "testing" +) + +// flockScratch writes a file for the cases below and returns its +// forward-slashed path. +func flockScratch(t *testing.T) string { + t.Helper() + p := filepath.Join(t.TempDir(), "locked.txt") + if err := os.WriteFile(p, []byte("1234567890"), 0o644); err != nil { + t.Fatalf("fixture: %v", err) + } + return slash(p) +} + +// TestWave28FlockReal drives File#flock against the real flock(2) on the +// platforms that have one. rb_file_flock (file.c) answers 0 for a lock taken and +// 0 for the unlock, and a second, independent open file description asking for an +// incompatible lock with LOCK_NB gets `false` rather than an exception — the one +// thing a shared lock does not produce, since two shared locks are compatible. +// Asserted against MRI ruby 4.0.5. +func TestWave28FlockReal(t *testing.T) { + if runtime.GOOS == "windows" || runtime.GOARCH == "wasm" { + t.Skip("no flock(2) on this platform; the seam reports ENOTSUP instead") + } + p := flockScratch(t) + cases := []struct{ src, want string }{ + {`f = File.open("` + p + `", "w+"); p f.flock(File::LOCK_EX); p f.flock(File::LOCK_UN); f.close`, "0\n0\n"}, + {`f = File.open("` + p + `", "w+"); p f.flock(File::LOCK_SH); p f.flock(File::LOCK_UN); f.close`, "0\n0\n"}, + // Two shared locks on the same file are compatible, so neither is refused. + {`a = File.open("` + p + `", "r"); b = File.open("` + p + `", "r"); a.flock(File::LOCK_SH); p b.flock(File::LOCK_SH | File::LOCK_NB); p b.flock(File::LOCK_UN); a.flock(File::LOCK_UN); a.close; b.close`, "0\n0\n"}, + // An exclusive lock refuses a second one; with LOCK_NB that is `false`. + {`a = File.open("` + p + `", "w+"); b = File.open("` + p + `", "w"); a.flock(File::LOCK_EX); p b.flock(File::LOCK_EX | File::LOCK_NB); a.flock(File::LOCK_UN); a.close; b.close`, "false\n"}, + // Unlocking a stream that never locked is still 0 — there is no descriptor + // to act on, and MRI's flock(LOCK_UN) on an unlocked fd succeeds too. + {`f = File.open("` + p + `", "r"); p f.flock(File::LOCK_UN); f.close`, "0\n"}, + // Closing the stream drops the lock, so the next taker is not refused. + {`a = File.open("` + p + `", "w+"); a.flock(File::LOCK_EX); a.close; b = File.open("` + p + `", "w"); p b.flock(File::LOCK_EX | File::LOCK_NB); b.close`, "0\n"}, + {`f = File.open("` + p + `", "r"); f.close; begin; f.flock(File::LOCK_EX); rescue => e; p [e.class, e.message]; end`, "[IOError, \"closed stream\"]\n"}, + } + for _, c := range cases { + if got := runFS(t, c.src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + } +} + +// TestWave28FlockRetriesWithoutLockNB covers the loop rb_file_flock spells out: +// WITHOUT LOCK_NB a refusal is not an answer, it is a wait — MRI sleeps 0.1 s and +// calls flock(2) again, rather than blocking inside the syscall, which is what +// keeps the wait interruptible. The seam counts the calls so the retry is +// observed rather than assumed. +func TestWave28FlockRetriesWithoutLockNB(t *testing.T) { + p := flockScratch(t) + calls := 0 + restore := swapFlockSeam(func(string) (int, error) { return 7, nil }, + func(fd, op int) error { + calls++ + if calls < 3 { + return syscall.EWOULDBLOCK + } + return nil + }, + func(int) error { return nil }) + defer restore() + + if got := runFS(t, `f = File.open("`+p+`", "r"); p f.flock(File::LOCK_EX); f.close`); got != "0\n" { + t.Fatalf("got %q, want %q", got, "0\n") + } + if calls != 3 { + t.Errorf("flock(2) called %d times, want 3 (two refusals then the grant)", calls) + } +} + +// TestWave28FlockErrnoMapping covers the failures rb_file_flock does NOT retry: +// each is rb_syserr_fail_path, naming the file. The unsupported platforms reach +// the last case through their own seam, which is why the stub returns exactly +// what they do. +func TestWave28FlockErrnoMapping(t *testing.T) { + p := flockScratch(t) + for _, c := range []struct { + err error + want string + }{ + {syscall.EBADF, "[Errno::EBADF, \"Bad file descriptor @ rb_file_flock - PATH\"]\n"}, + {syscall.EINVAL, "[Errno::EINVAL, \"Invalid argument @ rb_file_flock - PATH\"]\n"}, + {syscall.ENOTSUP, "[Errno::ENOTSUP, \"Operation not supported @ rb_file_flock - PATH\"]\n"}, + } { + err := c.err + restore := swapFlockSeam(func(string) (int, error) { return 7, nil }, + func(int, int) error { return err }, + func(int) error { return nil }) + src := `f = File.open("` + p + `", "r"); begin; f.flock(File::LOCK_EX); rescue => e; p [e.class, e.message]; end` + got := runFS(t, src) + restore() + want := replaceAllPath(c.want, p) + if got != want { + t.Errorf("errno %v: got %q, want %q", err, got, want) + } + } + // A LOCK_UN whose flock(2) fails reports the same way rather than silently + // dropping the descriptor. + restore := swapFlockSeam(func(string) (int, error) { return 7, nil }, + func(_, op int) error { + if op&flockUN != 0 { + return syscall.EBADF + } + return nil + }, + func(int) error { return nil }) + src := `f = File.open("` + p + `", "r"); f.flock(File::LOCK_SH); begin; f.flock(File::LOCK_UN); rescue => e; p [e.class, e.message]; end` + got := runFS(t, src) + restore() + want := replaceAllPath("[Errno::EBADF, \"Bad file descriptor @ rb_file_flock - PATH\"]\n", p) + if got != want { + t.Errorf("unlock failure: got %q, want %q", got, want) + } +} + +// TestWave28FlockOpenFailure covers the other half of lockDescriptor: the +// descriptor a lock needs cannot always be opened, and that failure is the +// ordinary rb_sysopen one, named for the path. +func TestWave28FlockOpenFailure(t *testing.T) { + p := flockScratch(t) + restore := swapFlockSeam(func(string) (int, error) { return 0, os.ErrPermission }, + func(int, int) error { return nil }, + func(int) error { return nil }) + defer restore() + src := `f = File.open("` + p + `", "r"); begin; f.flock(File::LOCK_EX); rescue => e; p [e.class, e.message]; end` + want := replaceAllPath("[Errno::EACCES, \"Permission denied @ rb_sysopen - PATH\"]\n", p) + if got := runFS(t, src); got != want { + t.Errorf("got %q, want %q", got, want) + } +} + +// swapFlockSeam installs stub flock seams and returns the restore function. +func swapFlockSeam(open func(string) (int, error), lock func(int, int) error, closeFd func(int) error) func() { + oOpen, oLock, oClose, oAgain := lockFdFn, flockFn, closeFdFn, flockAgainErrs + lockFdFn, flockFn, closeFdFn = open, lock, closeFd + // The retry list is per platform and empty where there is no flock(2); the + // stub speaks EWOULDBLOCK, so give every platform that one entry. + flockAgainErrs = []error{syscall.EWOULDBLOCK} + return func() { lockFdFn, flockFn, closeFdFn, flockAgainErrs = oOpen, oLock, oClose, oAgain } +} + +// replaceAllPath substitutes the scratch path into an expected message. +func replaceAllPath(want, p string) string { + out := "" + for { + i := indexOf(want, "PATH") + if i < 0 { + return out + want + } + out += want[:i] + p + want = want[i+4:] + } +} + +// indexOf is strings.Index, kept local so this file needs no extra import. +func indexOf(s, sub string) int { + for i := 0; i+len(sub) <= len(s); i++ { + if s[i:i+len(sub)] == sub { + return i + } + } + return -1 +} From 0eba4391a3b81a383fee4de9bc4eab74386d0e3c Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 23 Sep 2026 23:10:46 +0200 Subject: [PATCH 04/10] IO::Buffer.map, and the allocation flags IO::Buffer.new records MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit core/io/buffer/map_spec.rb scored 0 — IO::Buffer.map did not exist — and the state predicates around it answered a fixed false, so eleven further buffer spec files each lost an example or two to the same gap. Read for this change: - io_buffer.c io_buffer_map and rb_io_buffer_map: rb_check_arity(1, 4), and with no size the whole file, which is why a zero-length file has nothing to map ("Invalid negative or zero file size!"). - io_buffer.c io_buffer_extract_size / _offset / _flags: three numbers with three different refusals — "not an Integer" for a size, rb_num2long's "no implicit conversion from X" for an offset, and "Flags can't be negative!" before the mask that deliberately ignores unknown bits. - io_buffer.c io_buffer_map_file: MAP_SHARED with PROT_READ|PROT_WRITE unless PRIVATE or READONLY was asked for. That is what makes a shared writable mapping of a stream not opened for writing the EACCES the spec calls "a SystemCallError unless read-only". A non-private mapping is marked EXTERNAL and SHARED; a private one is neither. - io_buffer.c rb_io_buffer_initialize + io_buffer_initialize + io_flags_for_size: with no flags the SIZE chooses — below a page the buffer is internal, from a page upwards it is MAPPED. With flags, INTERNAL or MAPPED must be among them (they are the only two ways to get the memory: otherwise AllocationError), and every other bit is simply RECORDED, so INTERNAL|SHARED is an internal buffer that reports #shared?. A size of zero allocates nothing and the flags are not consulted at all, not even READONLY. - include/ruby/io/buffer.h enum rb_io_buffer_flags for the values. A mapping cannot be faked, but it does not have to be: this VM's file streams already ARE their bytes in memory (see IOObj), so a shared mapping is that byte slice — writes through the buffer are the stream's writes — while a private mapping takes the copy MAP_PRIVATE would. core/io/buffer/map_spec.rb 0 -> 14 (2 need fork, skipped) core/io/buffer/initialize_spec.rb 5 -> 13 core/io/buffer/shared_spec.rb 2 -> 4 core/io/buffer/internal_spec.rb 1 -> 3 core/io/buffer/null_spec.rb 10 -> 12 and +1 each in empty_spec, free_spec, mapped_spec, private_spec, resize_spec, transfer_spec and valid_spec. Nothing in core/file, core/io or core/kernel moved down. Co-Authored-By: Claude Opus 5 --- internal/vm/io_buffer.go | 188 ++++++++++++++++++++++- internal/vm/wave28_io_buffer_map_test.go | 151 ++++++++++++++++++ 2 files changed, 331 insertions(+), 8 deletions(-) create mode 100644 internal/vm/wave28_io_buffer_map_test.go diff --git a/internal/vm/io_buffer.go b/internal/vm/io_buffer.go index 5d26751a..4bbca401 100644 --- a/internal/vm/io_buffer.go +++ b/internal/vm/io_buffer.go @@ -8,6 +8,8 @@ import ( binpkg "encoding/binary" "math" "math/big" + "os" + "strings" "github.com/go-embedded-ruby/ruby/internal/object" ) @@ -28,6 +30,18 @@ type ioBuffer struct { parent *ioBuffer // for a slice, the buffer it was sliced from (nil for a non-slice) sliceOff int // this slice's offset within parent (only meaningful when parent != nil) sliceLen int // this slice's length within parent (only meaningful when parent != nil) + + // mapped, shared and priv are RB_IO_BUFFER_MAPPED / _SHARED / _PRIVATE + // (include/ruby/io/buffer.h). IO::Buffer.new takes a MAPPED buffer for + // anything from a page upwards, and IO::Buffer.map takes one over a file — + // shared with it by default, or a private copy that the file never sees. + mapped, shared, priv bool + + // null is a buffer with no memory at all, which is what a zero size gives: + // io_buffer_initialize leaves base NULL, and #null? is true while #free has + // never been called. It is distinct from freed, which is a buffer that HAD + // memory. + null bool } // sliceValid reports IO::Buffer#valid?. A non-slice is always valid (freeing it @@ -115,6 +129,60 @@ func bufInt(v object.Value) int64 { func bufUint(v object.Value) uint64 { return uint64(bufInt(v)) } +// The IO::Buffer allocation/state flags, as include/ruby/io/buffer.h numbers +// them. They are also the values the IO::Buffer:: constants carry. +const ( + bufFlagEXTERNAL = 1 + bufFlagINTERNAL = 2 + bufFlagMAPPED = 4 + bufFlagSHARED = 8 + bufFlagLOCKED = 32 + bufFlagPRIVATE = 64 + bufFlagREADONLY = 128 +) + +// bufDefaultFlags is io_flags_for_size (io_buffer.c): with no flags given, +// IO::Buffer.new allocates internally below a page and maps from a page upwards. +func bufDefaultFlags(size int64) int64 { + if size >= 16384 { + return bufFlagMAPPED + } + return bufFlagINTERNAL +} + +// bufFlagsArg is io_buffer_extract_flags: a negative flag set is an ArgumentError +// before anything else, a non-Integer is "not an Integer" as every other +// IO::Buffer number is, and unknown bits are deliberately ignored. +func bufFlagsArg(v object.Value) int64 { + n, ok := v.(object.Integer) + if !ok { + raise("TypeError", "not an Integer") + } + if int64(n) < 0 { + raise("ArgumentError", "Flags can't be negative!") + } + mask := int64(bufFlagEXTERNAL | bufFlagINTERNAL | bufFlagMAPPED | bufFlagSHARED | + bufFlagLOCKED | bufFlagPRIVATE | bufFlagREADONLY) + return int64(n) & mask +} + +// bufOffsetArg is io_buffer_extract_offset. Unlike a size it goes through +// NUM2OFFT, so a non-Integer is rb_num2long's "no implicit conversion from X" +// rather than "not an Integer". +func bufOffsetArg(v object.Value) int64 { + n, ok := v.(object.Integer) + if !ok { + if object.IsNil(v) { + raise("TypeError", "no implicit conversion from nil") + } + raise("TypeError", "no implicit conversion from %s", strings.ToLower(classNameOf(v))) + } + if int64(n) < 0 { + raise("ArgumentError", "Offset can't be negative!") + } + return int64(n) +} + // bufSizeArg validates an IO::Buffer size (.new / #resize): a non-Integer raises // TypeError "not an Integer" and a negative value raises ArgumentError "Size // can't be negative!" — the exact checks and messages from io_buffer.c @@ -222,16 +290,113 @@ func (vm *VM) registerIOBuffer() { sm := func(name string, fn NativeFn) { cBuf.smethods[name] = &Method{name: name, owner: cBuf, native: fn} } dm := func(name string, fn NativeFn) { cBuf.define(name, fn) } + // IO::Buffer.new(size = DEFAULT_SIZE, flags = io_flags_for_size(size)) — + // rb_io_buffer_initialize + io_buffer_initialize (io_buffer.c). Given no flags + // MRI chooses them by size: anything below a page is allocated internally, and + // a page or more is MAPPED. Given flags, one of INTERNAL or MAPPED has to be + // among them or there is no way to get the memory, which is the + // AllocationError. A zero size allocates nothing at all and the flags are not + // even consulted — the buffer is null. sm("new", func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { + if len(args) > 2 { + raise("ArgumentError", "wrong number of arguments (given %d, expected 0..2)", len(args)) + } size := int64(65536) if len(args) > 0 { size = bufSizeArg(args[0]) } - flags := int64(0) + flags := bufDefaultFlags(size) if len(args) > 1 { - flags = intArg(args[1]) + flags = bufFlagsArg(args[1]) } - return &ioBuffer{data: make([]byte, size), readonly: flags&128 != 0} + b := &ioBuffer{} + if size == 0 { + // io_buffer_initialize never looks at the flags on this path: with no + // memory to get there is nothing for them to describe, so even READONLY + // is dropped. + b.null = true + return b + } + // INTERNAL and MAPPED are the two ways to obtain the memory and one of them + // must be present; every other bit is simply recorded, so INTERNAL|SHARED is + // an internally allocated buffer that also reports #shared?. + switch { + case flags&bufFlagINTERNAL != 0: + // allocated internally: internal? is the absence of the other three + case flags&bufFlagMAPPED != 0: + b.mapped = true + default: + raise("IO::Buffer::AllocationError", "Could not allocate buffer!") + } + b.readonly = flags&bufFlagREADONLY != 0 + b.shared = flags&bufFlagSHARED != 0 + b.priv = flags&bufFlagPRIVATE != 0 + b.external = flags&bufFlagEXTERNAL != 0 + b.data = make([]byte, size) + return b + }) + // IO::Buffer.map(io, size = nil, offset = 0, flags = 0) — io_buffer_map + + // io_buffer_map_file (io_buffer.c). The mapping is MAP_SHARED unless PRIVATE + // was asked for, and PROT_READ|PROT_WRITE unless READONLY was: a shared + // writable mapping of a stream that was not opened for writing is the EACCES + // mmap(2) gives, which is the "SystemCallError unless read-only" the spec + // names. + // + // This VM's file streams are their bytes in memory (see IOObj), so a shared + // mapping IS that byte slice — writes through the buffer are the stream's + // writes, which is the sharing mmap provides — while a private mapping takes a + // copy the file can never see. + sm("map", func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { + if len(args) < 1 || len(args) > 4 { + raise("ArgumentError", "wrong number of arguments (given %d, expected 1..4)", len(args)) + } + o := ioGetIO(vm, args[0]) + if o.closed { + raise("IOError", "closed stream") + } + ioFlush(o) + fileSize := int64(len(o.buf)) + if st, err := os.Stat(o.path); err == nil { + fileSize = st.Size() + } + var size int64 + if len(args) >= 2 && !object.IsNil(args[1]) { + if size = bufSizeArg(args[1]); size == 0 { + raise("ArgumentError", "Size can't be zero!") + } + } else { + if fileSize <= 0 { + raise("ArgumentError", "Invalid negative or zero file size!") + } + size = fileSize + } + var offset int64 + if len(args) >= 3 { + offset = bufOffsetArg(args[2]) + } + if size > fileSize { + raise("ArgumentError", "Size can't be larger than file size!") + } + if offset+size > fileSize { + raise("ArgumentError", "Offset too large!") + } + flags := int64(0) + if len(args) >= 4 { + flags = bufFlagsArg(args[3]) + } + b := &ioBuffer{mapped: true, readonly: flags&bufFlagREADONLY != 0} + switch { + case flags&bufFlagPRIVATE != 0: + b.priv = true + b.data = append([]byte(nil), o.buf[offset:offset+size]...) + default: + if !b.readonly && !ioFmodeWritable(o) { + raise("Errno::EACCES", "Permission denied - io_buffer_map_file:mmap") + } + b.external, b.shared = true, true + b.data = o.buf[offset : offset+size : offset+size] + } + return b }) sm("for", func(vm *VM, _ object.Value, args []object.Value, blk *Proc) object.Value { // io_buffer.c io_buffer_for: without a block the buffer is always a @@ -276,14 +441,21 @@ func (vm *VM) registerIOBuffer() { return object.Bool(fn(self.(*ioBuffer))) }) } - pred("null?", func(b *ioBuffer) bool { return b.freed }) + pred("null?", func(b *ioBuffer) bool { return b.freed || b.null }) pred("empty?", func(b *ioBuffer) bool { return !b.freed && len(b.data) == 0 }) pred("valid?", func(b *ioBuffer) bool { return b.sliceValid() }) pred("external?", func(b *ioBuffer) bool { return !b.freed && b.external }) - pred("internal?", func(b *ioBuffer) bool { return !b.freed && !b.external && !b.borrowed }) - pred("mapped?", func(b *ioBuffer) bool { return false }) - pred("shared?", func(b *ioBuffer) bool { return false }) - pred("private?", func(b *ioBuffer) bool { return false }) + // The four allocation flags are exclusive in practice: a buffer is internal, + // external, mapped-and-shared or mapped-and-private, and io_buffer.c keeps them + // as separate bits rather than deriving one from the absence of the others — a + // private mapping is neither internal nor external, which is why internal? has + // to exclude the mapped forms explicitly. + pred("internal?", func(b *ioBuffer) bool { + return !b.freed && !b.null && !b.external && !b.borrowed && !b.mapped + }) + pred("mapped?", func(b *ioBuffer) bool { return !b.freed && !b.null && b.mapped }) + pred("shared?", func(b *ioBuffer) bool { return !b.freed && b.shared }) + pred("private?", func(b *ioBuffer) bool { return !b.freed && b.priv }) pred("locked?", func(b *ioBuffer) bool { return b.locked }) pred("readonly?", func(b *ioBuffer) bool { return b.readonly }) diff --git a/internal/vm/wave28_io_buffer_map_test.go b/internal/vm/wave28_io_buffer_map_test.go new file mode 100644 index 00000000..f4c033b3 --- /dev/null +++ b/internal/vm/wave28_io_buffer_map_test.go @@ -0,0 +1,151 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm_test + +import ( + "os" + "path/filepath" + "testing" +) + +// TestWave28IOBufferMap pins IO::Buffer.map against io_buffer.c: +// +// - io_buffer_map / rb_io_buffer_map: rb_check_arity(argc, 1, 4), and with no +// size the whole file — a file of zero length has no mapping to make; +// - io_buffer_extract_size / _offset / _flags: three different refusals for +// three different numbers ("not an Integer" for a size, "no implicit +// conversion from X" for an offset, "Flags can't be negative!" for flags); +// - io_buffer_map_file: MAP_SHARED with PROT_READ|PROT_WRITE unless PRIVATE or +// READONLY was asked for, so a shared writable mapping of a stream that was +// not opened for writing is the EACCES mmap(2) gives. A non-private mapping +// is marked external and shared; a private one is neither. +// +// Every want string is the raw stdout of the same source under MRI ruby 4.0.5, +// compared byte for byte against a fixture file holding "abc\xC3\xA2def\n". +func TestWave28IOBufferMap(t *testing.T) { + cases := []struct{ src, want string }{ + {`b = IO::Buffer.map(f); [b.size, b.get_string]`, "[9, \"abc\\xC3\\xA2def\\n\"]\n"}, + {`b = IO::Buffer.map(f); [b.internal?, b.mapped?, b.external?, b.empty?, b.null?, b.shared?, b.private?, b.readonly?, b.locked?, b.valid?]`, + "[false, true, true, false, false, true, false, false, false, true]\n"}, + {`b = IO::Buffer.map(f, 4); [b.size, b.get_string]`, "[4, \"abc\\xC3\"]\n"}, + {`IO::Buffer.map(f, nil).size`, "9\n"}, + {`b = IO::Buffer.map(f, 4, 0); [b.size, b.get_string]`, "[4, \"abc\\xC3\"]\n"}, + + // The three number refusals, each with its own wording. + {`IO::Buffer.map(f, 0)`, "[ArgumentError, \"Size can't be zero!\"]\n"}, + {`IO::Buffer.map(f, -1)`, "[ArgumentError, \"Size can't be negative!\"]\n"}, + {`IO::Buffer.map(f, "10")`, "[TypeError, \"not an Integer\"]\n"}, + {`IO::Buffer.map(f, 10.0)`, "[TypeError, \"not an Integer\"]\n"}, + {`IO::Buffer.map(f, 8192)`, "[ArgumentError, \"Size can't be larger than file size!\"]\n"}, + {`IO::Buffer.map(f, 4, "4096")`, "[TypeError, \"no implicit conversion from string\"]\n"}, + {`IO::Buffer.map(f, 4, nil)`, "[TypeError, \"no implicit conversion from nil\"]\n"}, + {`IO::Buffer.map(f, 4, -1)`, "[ArgumentError, \"Offset can't be negative!\"]\n"}, + {`IO::Buffer.map(f, 8, 4)`, "[ArgumentError, \"Offset too large!\"]\n"}, + {`IO::Buffer.map`, "[ArgumentError, \"wrong number of arguments (given 0, expected 1..4)\"]\n"}, + {`IO::Buffer.map(f, 1, 0, 0, 0)`, "[ArgumentError, \"wrong number of arguments (given 5, expected 1..4)\"]\n"}, + + // READONLY maps without asking for write access, and refuses writes. + {`b = IO::Buffer.map(f, nil, 0, IO::Buffer::READONLY); [b.readonly?, b.get_string]`, "[true, \"abc\\xC3\\xA2def\\n\"]\n"}, + {`b = IO::Buffer.map(f, nil, 0, IO::Buffer::READONLY); b.set_string("test")`, + "[IO::Buffer::AccessError, \"Buffer is not writable!\"]\n"}, + + // PRIVATE is a copy: neither external nor shared, and the file never sees + // the writes. + {`b = IO::Buffer.map(f, nil, 0, IO::Buffer::PRIVATE); [b.private?, b.shared?, b.external?, b.mapped?, b.internal?]`, + "[true, false, false, true, false]\n"}, + {`b = IO::Buffer.map(f, nil, 0, IO::Buffer::PRIVATE); b.set_string("test12345"); [b.get_string, f.read]`, + "[\"test12345\", \"abc\\xC3\\xA2def\\n\"]\n"}, + + // The mapping outlives the stream: mmap keeps the pages, close(2) does not + // take them back. + {`b = IO::Buffer.map(f); f.close; b.get_string`, "\"abc\\xC3\\xA2def\\n\"\n"}, + } + for _, c := range cases { + dir := bufferMapScratch(t) + src := "f = File.open(" + rubyString(filepath.Join(dir, "read_text.txt")) + ", \"rb+\")\n" + + "begin\n p(begin\n" + c.src + "\nend)\nrescue => e\n p [e.class, e.message]\nend\n" + if got := eval(t, src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + } +} + +// TestWave28IOBufferMapModeGuards covers the two file states that decide whether +// a mapping can be made at all: a stream not opened for writing cannot carry the +// default shared writable mapping (EACCES from mmap, which is the SystemCallError +// the spec names) but can carry a READONLY one; and a file of zero length has +// nothing to map. MRI 4.0.5. +func TestWave28IOBufferMapModeGuards(t *testing.T) { + dir := bufferMapScratch(t) + ro := rubyString(filepath.Join(dir, "read_text.txt")) + empty := rubyString(filepath.Join(dir, "empty.txt")) + cases := []struct{ src, want string }{ + {`f = File.open(` + ro + `, "rb"); IO::Buffer.map(f)`, + "[Errno::EACCES, \"Permission denied - io_buffer_map_file:mmap\"]\n"}, + {`f = File.open(` + ro + `, "rb"); b = IO::Buffer.map(f, nil, 0, IO::Buffer::READONLY); [b.readonly?, b.get_string]`, + "[true, \"abc\\xC3\\xA2def\\n\"]\n"}, + {`f = File.open(` + ro + `, "rb"); b = IO::Buffer.map(f, nil, 0, IO::Buffer::PRIVATE); b.set_string("test12345"); b.get_string`, + "\"test12345\"\n"}, + {`f = File.open(` + empty + `, "wb+"); IO::Buffer.map(f)`, + "[ArgumentError, \"Invalid negative or zero file size!\"]\n"}, + {`f = File.open(` + ro + `, "rb"); f.close; IO::Buffer.map(f)`, + "[IOError, \"closed stream\"]\n"}, + } + for _, c := range cases { + src := "begin\n p(begin\n" + c.src + "\nend)\nrescue => e\n p [e.class, e.message]\nend\n" + if got := eval(t, src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + } +} + +// TestWave28IOBufferNewFlags pins IO::Buffer.new's allocation flags — +// rb_io_buffer_initialize + io_buffer_initialize + io_flags_for_size +// (io_buffer.c). With no flags the SIZE chooses: below a page the buffer is +// allocated internally, from a page upwards it is mapped. With flags, INTERNAL or +// MAPPED has to be among them because they are the only two ways to obtain the +// memory, and every other bit is simply recorded — INTERNAL|SHARED is an +// internally allocated buffer that also reports #shared?. A size of zero +// allocates nothing and the flags are never consulted at all. MRI 4.0.5. +func TestWave28IOBufferNewFlags(t *testing.T) { + cases := []struct{ src, want string }{ + {`b = IO::Buffer.new(IO::Buffer::PAGE_SIZE); [b.internal?, b.mapped?, b.external?]`, "[false, true, false]\n"}, + {`b = IO::Buffer.new(8); [b.internal?, b.mapped?, b.external?]`, "[true, false, false]\n"}, + {`b = IO::Buffer.new; [b.size, b.internal?, b.mapped?]`, "[65536, false, true]\n"}, + {`b = IO::Buffer.new(0); [b.null?, b.internal?, b.mapped?, b.empty?, b.size]`, "[true, false, false, true, 0]\n"}, + {`b = IO::Buffer.new(0, 0xffff); [b.null?, b.empty?, b.internal?, b.mapped?, b.external?, b.shared?, b.private?, b.readonly?, b.locked?, b.valid?, b.size]`, + "[true, true, false, false, false, false, false, false, false, true, 0]\n"}, + {`b = IO::Buffer.new(8, IO::Buffer::MAPPED); [b.mapped?, b.internal?]`, "[true, false]\n"}, + {`b = IO::Buffer.new(10, IO::Buffer::INTERNAL | IO::Buffer::SHARED | IO::Buffer::READONLY); [b.internal?, b.shared?, b.readonly?, b.mapped?, b.external?, b.private?]`, + "[true, true, true, false, false, false]\n"}, + {`b = IO::Buffer.new(10, IO::Buffer::INTERNAL | IO::Buffer::PRIVATE); [b.internal?, b.private?]`, "[true, true]\n"}, + {`b = IO::Buffer.new(20000, IO::Buffer::MAPPED | IO::Buffer::EXTERNAL); [b.mapped?, b.external?, b.internal?]`, "[true, true, false]\n"}, + {`IO::Buffer.new(8, 0)`, "[IO::Buffer::AllocationError, \"Could not allocate buffer!\"]\n"}, + {`IO::Buffer.new(8, -1)`, "[ArgumentError, \"Flags can't be negative!\"]\n"}, + {`IO::Buffer.new(8, 0.0)`, "[TypeError, \"not an Integer\"]\n"}, + {`IO::Buffer.new(-1)`, "[ArgumentError, \"Size can't be negative!\"]\n"}, + {`IO::Buffer.new(1, 2, 3)`, "[ArgumentError, \"wrong number of arguments (given 3, expected 0..2)\"]\n"}, + } + for _, c := range cases { + src := "begin\n p(begin\n" + c.src + "\nend)\nrescue => e\n p [e.class, e.message]\nend\n" + if got := eval(t, src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + } +} + +// bufferMapScratch builds the fixture the map cases assume: "read_text.txt" with +// the nine bytes "abc\xC3\xA2def\n", and an empty "empty.txt". +func bufferMapScratch(t *testing.T) string { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "read_text.txt"), []byte("abc\xc3\xa2def\n"), 0o644); err != nil { + t.Fatalf("fixture: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "empty.txt"), nil, 0o644); err != nil { + t.Fatalf("fixture: %v", err) + } + return dir +} From b6fce487430a332ee511d9eeee4b8bea5a5b00f7 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 23 Sep 2026 23:28:04 +0200 Subject: [PATCH 05/10] File.open(fd), the block's close, and two assertions MRI disagrees with MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Read for this change: - io.c rb_file_initialize: with FEWER THAN THREE positional arguments, a first argument that converts through rb_check_to_int is a DESCRIPTOR and the call is io_initialize on it, not an open of a path. The permission argument is what rules that out, which is why File.new(fd, mode, perm) is the ordinary TypeError about the path. - io.c rb_io_s_open: the block form closes through the RUBY-level #close in an ensure, so a subclass override runs. - io.c rb_io_s_new: it warns when handed a block it will never call. - io.c io_initialize for the fd wrapper this VM already had for IO.new, now shared with File.open/File.new rather than duplicated. core/file/open_spec.rb 72 -> 78, core/file/new_spec.rb 21 -> 25. Two existing assertions were corrected against MRI 4.0.5 rather than worked around, because measuring said they were the thing that was wrong: - io_modeenc_internal_test.go asserted that when BOTH the block and #close raise, the BLOCK's exception escapes. MRI answers with the one from #close — for File.open, for IO.open, and for a bare begin/raise/ensure/raise alike, because an ensure's exception supersedes the one it was unwinding. The VM now does the same. - io_buffer_internal_test.go asserted IO::Buffer.new(4, READONLY) is a read-only buffer. READONLY alone names no way to obtain the memory, so MRI raises AllocationError; INTERNAL|READONLY is the buffer meant, and both are now asserted. Also hardened on the way, before anything could reach it: openFileSpec no longer truncates or probes for writability anything that is not a regular file. Probing a fifo means opening it for writing, which BLOCKS until a reader appears — an open that never returns, which is how an earlier wave's File.mkfifo cost this project 55 examples. Co-Authored-By: Claude Opus 5 --- internal/vm/io.go | 77 +++++++++++++----- internal/vm/io_buffer_internal_test.go | 9 ++- internal/vm/io_class.go | 86 ++++++++++++++------ internal/vm/io_modeenc_internal_test.go | 7 +- internal/vm/wave28_file_fd_open_test.go | 102 ++++++++++++++++++++++++ 5 files changed, 235 insertions(+), 46 deletions(-) create mode 100644 internal/vm/wave28_file_fd_open_test.go diff --git a/internal/vm/io.go b/internal/vm/io.go index c0834006..f9b4b0ad 100644 --- a/internal/vm/io.go +++ b/internal/vm/io.go @@ -549,22 +549,34 @@ func (vm *VM) registerIO() { // flushes and closes afterwards, returning the block's value. cFile := vm.consts["File"].(*RClass) cFile.super = cIO // File < IO, inheriting the read+write protocol; is_a?(IO) holds - cFile.smethods["open"] = &Method{name: "open", owner: cFile, native: func(vm *VM, _ object.Value, args []object.Value, blk *Proc) object.Value { - o := vm.openFileArgs(cFile, args) // openFileArgs rejects a missing path - if blk != nil { - defer ioFlushClose(o) - return vm.callBlock(blk, []object.Value{o}) + cFile.smethods["open"] = &Method{name: "open", owner: cFile, native: func(vm *VM, self object.Value, args []object.Value, blk *Proc) object.Value { + o := vm.openFileArgs(fileRecvClass(self, cFile), args) // openFileArgs rejects a missing path + if blk == nil { + return o } - return o + // rb_io_s_open closes through the Ruby-level #close, so a subclass that + // overrides it runs and an exception it raises propagates — except the + // IOError "closed stream" of a block that closed the file itself. When the + // block raises too, the block's exception is the one that escapes. + return vm.ioYieldAndClose(o, blk) }} // File.new opens a file-backed IO like File.open, but never takes a block (it // always returns the open stream). A missing path argument is an ArgumentError. - cFile.smethods["new"] = &Method{name: "new", owner: cFile, native: func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { + cFile.smethods["new"] = &Method{name: "new", owner: cFile, native: func(vm *VM, self object.Value, args []object.Value, blk *Proc) object.Value { if len(args) == 0 { - raise("ArgumentError", "wrong number of arguments (given 0, expected 1+)") + raise("ArgumentError", "wrong number of arguments (given 0, expected 1..3)") + } + // rb_io_s_new warns when handed a block it will not call: File.new always + // returns the open stream, and only File.open yields it. + if blk != nil { + vm.rbWarn("warning: File::new() does not take block; use File::open() instead") } - return vm.openFileArgs(cFile, args) + return vm.openFileArgs(fileRecvClass(self, cFile), args) }} + + // fileRecvClass is declared below; File.open and File.new both build an + // instance of the class they were CALLED on, so a File subclass yields its own + // instances (and its own #close override runs at the end of the block). // File instance metadata operations. Puppet's replace_file writes to a // Uniquefile (a DelegateClass(File)) and then chmod/chowns it before renaming // it into place, so the open File needs path/chmod/chown that act on its @@ -778,6 +790,16 @@ func (vm *VM) openFileArgs(cls *RClass, args []object.Value) *IOObj { if len(pos) == 0 || len(pos) > 3 { raise("ArgumentError", "wrong number of arguments (given %d, expected 1..3)", len(pos)) } + // rb_file_initialize (io.c): with fewer than three positional arguments, a + // first argument that converts through #to_int is a DESCRIPTOR rather than a + // path, and the call is IO#initialize on it. The permission argument is what + // rules that out — it is meaningless for an already-open descriptor, which is + // why File.new(fd, mode, perm) is a TypeError about the path instead. + if len(pos) < 3 { + if fd, ok := vm.ioFdArg(pos[0]); ok { + return vm.ioAdoptFd(cls, fd, pos, opts) + } + } path := pathArg(vm, pos[0]) ms := vm.ioResolveModeEnc(pos, opts) // rb_io_extract_modeenc's *vperm_p: the third positional argument, which the @@ -856,7 +878,23 @@ func openFileSpec(cls *RClass, p string, ms *ioModeSpec, perm int64, permGiven b if !permGiven { perm = 0o666 } + // Anything that is not a regular file is handled by the notRegular branch + // below and by nothing else: truncating a fifo is meaningless, and probing it + // for writability by opening it would BLOCK until a reader appeared — an open + // that never returns, which is how an earlier wave's File.mkfifo cost this + // project 55 examples. A directory would refuse both as well. + special := exists && notRegular(p) switch { + case special: + // Nothing that is not a regular file can be read whole, because some of + // them do not end: File.open("/dev/zero") allocated 83 GB here and killed a + // CI runner before the open returned. A character device, a fifo, a socket + // or a directory opens with an empty buffer instead, so the position + // arithmetic works — which is all core/io/seek_spec.rb asks of /dev/zero — + // and a read sees end-of-file rather than the machine going away. + // + // That reads see nothing is a limit of a buffer-backed IO rather than a + // decision: see IOObj, whose whole model is the file's bytes in memory. case !exists: // O_CREAT: materialise the file now, as MRI's open does, so File.exist? // and File.stat see it (with its permissions) before any write is flushed. @@ -869,16 +907,6 @@ func openFileSpec(cls *RClass, p string, ms *ioModeSpec, perm int64, permGiven b if err := os.Truncate(p, 0); err != nil { raiseOpenErrno(err, p) } - case notRegular(p): - // Nothing that is not a regular file can be read whole, because some of - // them do not end: File.open("/dev/zero") allocated 83 GB here and killed a - // CI runner before the open returned. A character device, a fifo, a socket - // or a directory opens with an empty buffer instead, so the position - // arithmetic works — which is all core/io/seek_spec.rb asks of /dev/zero — - // and a read sees end-of-file rather than the machine going away. - // - // That reads see nothing is a limit of a buffer-backed IO rather than a - // decision: see IOObj, whose whole model is the file's bytes in memory. default: b, err := os.ReadFile(p) if err != nil { @@ -889,7 +917,7 @@ func openFileSpec(cls *RClass, p string, ms *ioModeSpec, perm int64, permGiven b // A write-only stream must still be able to reopen the path for writing; a // read-only one must not, so a permission failure shows up at open, not at // close. os.ReadFile above already reported an unreadable file. - if ms.writable && exists && !ms.trunc { + if ms.writable && exists && !ms.trunc && !special { if err := writableCheck(p); err != nil { raiseOpenErrno(err, p) } @@ -905,6 +933,15 @@ func openFileSpec(cls *RClass, p string, ms *ioModeSpec, perm int64, permGiven b return o } +// fileRecvClass is the class a File.open / File.new call must instantiate: the +// receiver when it is a class (File itself, or a subclass), falling back to File. +func fileRecvClass(self object.Value, fallback *RClass) *RClass { + if c, ok := self.(*RClass); ok { + return c + } + return fallback +} + // writableCheck reports the error opening p for writing would give, without // disturbing its contents — the permission failure MRI's open(2) raises at // File.open time but a buffer-backed stream would otherwise only meet at flush. diff --git a/internal/vm/io_buffer_internal_test.go b/internal/vm/io_buffer_internal_test.go index 417d96f4..0b4dcd77 100644 --- a/internal/vm/io_buffer_internal_test.go +++ b/internal/vm/io_buffer_internal_test.go @@ -68,7 +68,14 @@ func TestIOBuffer(t *testing.T) { {`p(IO::Buffer.new(2) ? "truthy" : "falsy")`, `"truthy"`}, // set_string coerces its argument via #to_str. {`class T; def to_str; "hi"; end; end; b = IO::Buffer.new(4); b.set_string(T.new); p b.get_string(0, 2)`, `"hi"`}, - {`p IO::Buffer.new(4, IO::Buffer::READONLY).readonly?`, `true`}, + // READONLY alone names no way to obtain the memory, so io_buffer_initialize + // never allocates and raises instead. This line asserted `true` until wave 28 + // measured MRI 4.0.5, which answers with the AllocationError: the assertion + // was wrong, not the reference. INTERNAL|READONLY is the read-only buffer + // that was meant. + {`begin; IO::Buffer.new(4, IO::Buffer::READONLY); rescue => e; p [e.class, e.message]; end`, + `[IO::Buffer::AllocationError, "Could not allocate buffer!"]`}, + {`p IO::Buffer.new(4, IO::Buffer::INTERNAL | IO::Buffer::READONLY).readonly?`, `true`}, {`b = IO::Buffer.new(6); b.set_string("abcdef", 0, 3); p b.get_string(0, 3)`, `"abc"`}, {`b = IO::Buffer.new(6); b.set_string("abcdef", 0, 2, 2); p b.get_string(0, 2)`, `"cd"`}, {`begin; IO::Buffer.new(2).set_value(:U32, 0, 1); rescue ArgumentError => e; p e.class; end`, `ArgumentError`}, diff --git a/internal/vm/io_class.go b/internal/vm/io_class.go index c5006fd5..46fb34ea 100644 --- a/internal/vm/io_class.go +++ b/internal/vm/io_class.go @@ -31,17 +31,7 @@ func (vm *VM) registerIOClassMethods(cIO, cFile *RClass) { if len(pos) < 1 || len(pos) > 2 { raise("ArgumentError", "wrong number of arguments (given %d, expected 1..2)", len(pos)) } - fd := int(vm.repeatLong(pos[0])) - src, ok := vm.fdTable[fd] - if !ok { - raise("Errno::EBADF", "Bad file descriptor - fd %d", fd) - } - if src.closed { - raise("IOError", "closed stream") - } - res := &IOObj{cls: cIO} - vm.ioAdoptDescriptor(res, src, pos, opts) - return res + return vm.ioAdoptFd(cIO, int(vm.repeatLong(pos[0])), pos, opts) } cIO.smethods["for_fd"] = &Method{name: "for_fd", owner: cIO, native: forFd} cIO.smethods["new"] = &Method{name: "new", owner: cIO, native: forFd} @@ -57,19 +47,7 @@ func (vm *VM) registerIOClassMethods(cIO, cFile *RClass) { if !ok || blk == nil { return oVal } - var ret object.Value - blockRec := recoverAny(func() { ret = vm.callBlock(blk, []object.Value{o}) }) - closeRec := recoverAny(func() { vm.send(o, "close", nil, nil) }) - if re, isRE := closeRec.(RubyError); isRE && re.Class == "IOError" && re.Message == "closed stream" { - closeRec = nil // MRI ignores a "closed stream" IOError from the ensure close - } - if blockRec != nil { - panic(blockRec) // the block's exception (or break/return) is primary - } - if closeRec != nil { - panic(closeRec) - } - return ret + return vm.ioYieldAndClose(o, blk) }} // IO.sysopen(path, mode = "r", perm = 0666) opens path and returns its raw // descriptor (io.c rb_io_s_sysopen → rb_sysopen). rbgo has no OS fds, so the @@ -211,6 +189,66 @@ func ioForeachMode(opts *object.Hash) string { // the descriptor's), the append position, and IO.new's optional path: override // (an explicit nil clears the inherited path). Shared by IO.for_fd/new and // IO#initialize so the two decode a descriptor identically (io.c io_initialize). +// ioYieldAndClose is the block form of IO.open and File.open — io.c rb_io_s_open +// with io_close in its ensure. The stream is closed through the RUBY-level +// #close, so a subclass that overrides it runs. +// +// A #close that raises propagates, EXCEPT an IOError "closed stream", which is +// what a block that closed the file itself leaves behind. When BOTH the block +// and #close raise, the one from #close is the one that escapes: it is raised +// from an ensure, and an ensure's exception supersedes the one it was unwinding. +// Measured on MRI 4.0.5 for File.open, for IO.open and for a bare +// begin/raise/ensure/raise alike — all three answer with the ensure's. +func (vm *VM) ioYieldAndClose(o *IOObj, blk *Proc) object.Value { + var ret object.Value + blockRec := recoverAny(func() { ret = vm.callBlock(blk, []object.Value{o}) }) + closeRec := recoverAny(func() { vm.send(o, "close", nil, nil) }) + if re, isRE := closeRec.(RubyError); isRE && re.Class == "IOError" && re.Message == "closed stream" { + closeRec = nil + } + if closeRec != nil { + panic(closeRec) + } + if blockRec != nil { + panic(blockRec) + } + return ret +} + +// ioFdArg is rb_check_to_int on a File.open/File.new first argument: an Integer, +// or an object that converts to one with #to_int, is a descriptor. Everything +// else — a String, a Pathname, anything answering only #to_path — is a path, and +// the bool says so rather than raising, because the caller has a path to try. +func (vm *VM) ioFdArg(v object.Value) (int, bool) { + if i, ok := v.(object.Integer); ok { + return int(i), true + } + if vm.respondsToDynamic(v, "to_int") { + return int(vm.repeatLong(v)), true + } + return 0, false +} + +// ioAdoptFd builds a stream of class cls over the descriptor fd — io_initialize, +// which IO.new, IO.for_fd and the File.new(fd) form all reach. rbgo has no real +// descriptors (see ioFd), so fd is looked up in the synthetic table #fileno +// fills; one that is not there is the EBADF an unknown descriptor gives. +func (vm *VM) ioAdoptFd(cls *RClass, fd int, pos []object.Value, opts *object.Hash) *IOObj { + src, ok := vm.fdTable[fd] + if !ok { + raise("Errno::EBADF", "Bad file descriptor - fd %d", fd) + } + if src.closed { + raise("IOError", "closed stream") + } + res := &IOObj{cls: cls} + vm.ioAdoptDescriptor(res, src, pos, opts) + // The wrapper answers #fileno with the descriptor it wraps, as dup-free + // io_initialize does — File.open(f.fileno).fileno == f.fileno. + res.fd = fd + return res +} + func (vm *VM) ioAdoptDescriptor(o, src *IOObj, pos []object.Value, opts *object.Hash) { ms := vm.ioResolveModeEnc(pos, opts) o.isStr, o.buf, o.path = true, src.buf, src.path diff --git a/internal/vm/io_modeenc_internal_test.go b/internal/vm/io_modeenc_internal_test.go index edd15db0..2482c6aa 100644 --- a/internal/vm/io_modeenc_internal_test.go +++ b/internal/vm/io_modeenc_internal_test.go @@ -211,7 +211,12 @@ func TestIOOpenBlock(t *testing.T) { t.Errorf("close raise: got %s/%q want RuntimeError/boom", cls, msg) } // The block's exception takes precedence over a #close exception. - if cls, msg := evalErr(t, ioFdProg(`IO.open(wo, "w") { |io| def io.close; raise "fromclose"; end; raise "fromblock" }`)); cls != "RuntimeError" || msg != "fromblock" { + // Both halves raise: the one from #close wins, because it is raised from + // rb_io_s_open's ensure and an ensure's exception supersedes the one it was + // unwinding. This line read "fromblock" until wave 28 measured MRI 4.0.5 — + // File.open, IO.open and a bare begin/raise/ensure/raise all answer with the + // ensure's exception, so the assertion was wrong, not the reference. + if cls, msg := evalErr(t, ioFdProg(`IO.open(wo, "w") { |io| def io.close; raise "fromclose"; end; raise "fromblock" }`)); cls != "RuntimeError" || msg != "fromclose" { t.Errorf("block precedence: got %s/%q want RuntimeError/fromblock", cls, msg) } } diff --git a/internal/vm/wave28_file_fd_open_test.go b/internal/vm/wave28_file_fd_open_test.go new file mode 100644 index 00000000..44a3217c --- /dev/null +++ b/internal/vm/wave28_file_fd_open_test.go @@ -0,0 +1,102 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm_test + +import ( + "os" + "path/filepath" + "testing" +) + +// TestWave28FileOpenDescriptor pins rb_file_initialize (io.c): with FEWER THAN +// THREE positional arguments, a first argument that converts through #to_int is +// a DESCRIPTOR and the call becomes io_initialize on it, not an open of a path. +// The permission argument is what rules that out — it is meaningless for an +// already-open descriptor, so File.new(fd, mode, perm) is the ordinary TypeError +// about the path instead. Every want string is MRI 4.0.5's stdout. +func TestWave28FileOpenDescriptor(t *testing.T) { + cases := []struct{ src, want string }{ + {`fh = File.open("f.txt"); c = File.open(fh.fileno); c.autoclose = false; [c.class, c.fileno == fh.fileno]`, "[File, true]\n"}, + {`fh = File.open("f.txt"); File.new(fh.fileno).read`, "\"hello file\\n\"\n"}, + // Three positional arguments mean the first one is a path again. + {`fh = File.open("f.txt"); File.new(fh.fileno, "r", 0644)`, "[TypeError, \"no implicit conversion of Integer into String\"]\n"}, + // An object that answers #to_int is a descriptor too. + {`fh = File.open("f.txt"); o = Object.new; n = fh.fileno; o.define_singleton_method(:to_int) { n }; File.open(o).read`, "\"hello file\\n\"\n"}, + // ...while one that answers only #to_path is a path. + {`o = Object.new; o.define_singleton_method(:to_path) { "f.txt" }; File.open(o).read`, "\"hello file\\n\"\n"}, + // A descriptor that was never handed out is EBADF, which is the + // SystemCallError core/file/open_spec.rb asks for. Only the class is + // asserted: MRI says "Bad file descriptor" where this VM names the + // synthetic descriptor it looked for (see ioFd). + {`begin; File.open(-1); rescue SystemCallError => e; e.class; end`, "Errno::EBADF\n"}, + {`fh = File.open("f.txt"); fh.close; File.open(fh.fileno)`, "[IOError, \"closed stream\"]\n"}, + // An explicit mode incompatible with the descriptor's is EINVAL. + {`fh = File.open("f.txt", "r"); File.new(fh.fileno, "w")`, "[Errno::EINVAL, \"Invalid argument\"]\n"}, + } + for _, c := range cases { + dir := fdOpenScratch(t) + src := "Dir.chdir(" + rubyString(dir) + ")\nbegin\n p(begin\n" + c.src + "\nend)\nrescue => e\n p [e.class, e.message]\nend\n" + if got := eval(t, src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + } +} + +// TestWave28FileOpenBlockClose pins the block form against rb_io_s_open and the +// io_close in its ensure: the stream is closed through the RUBY-level #close, so +// an override runs and an exception it raises propagates; a block that closed the +// file itself leaves an IOError "closed stream" that MRI swallows; and when both +// both raise, the one from #close escapes (an ensure's exception supersedes). +// File.open and File.new also instantiate the class they were CALLED on, so a +// subclass yields its own instances. MRI 4.0.5. +func TestWave28FileOpenBlockClose(t *testing.T) { + cases := []struct{ src, want string }{ + {`File.open("f.txt") { |f| 7 }`, "7\n"}, + {`File.open("f.txt") { |f| f.close; 3 }`, "3\n"}, + {`File.open("f.txt") { |f| def f.close; raise StandardError; end }`, "[StandardError, \"StandardError\"]\n"}, + {`File.open("f.txt") { |f| def f.close; raise Exception, "non-standard"; end }`, "[Exception, \"non-standard\"]\n"}, + // Both raise: the one from #close wins, being raised from the ensure. + {`File.open("f.txt") { |f| def f.close; raise "from close"; end; raise "from block" }`, "[RuntimeError, \"from close\"]\n"}, + // #close really is called, and really is the Ruby one. + {`File.open("f.txt") { |f| class << f; alias_method :oc, :close; def close; oc; REC << :closed; end; end }; REC`, "[:closed]\n"}, + {`class MyFile < File; end; MyFile.open("f.txt") { |f| f.class }`, "MyFile\n"}, + {`class MyFile2 < File; end; MyFile2.new("f.txt").class`, "MyFile2\n"}, + } + for _, c := range cases { + dir := fdOpenScratch(t) + // REC is the scratch array the "really is called" case records into; it is + // defined for every case so the source stays uniform. + src := "REC = []\nDir.chdir(" + rubyString(dir) + ")\nbegin\n p(begin\n" + + c.src + "\nend)\nrescue Exception => e\n p [e.class, e.message]\nend\n" + if got := eval(t, src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + } +} + +// TestWave28FileNewWarnsOnBlock pins rb_io_s_new's rb_warn: File.new always +// returns the open stream, so a block handed to it would never be called and MRI +// says so. rb_warn is silent while $VERBOSE is nil, which is how this VM starts, +// so the case sets it. MRI 4.0.5 prints the same line (with its own file:line +// prefix, which is not part of what the spec matches). +func TestWave28FileNewWarnsOnBlock(t *testing.T) { + dir := fdOpenScratch(t) + src := "Dir.chdir(" + rubyString(dir) + ")\n$VERBOSE = true\n" + + "$stderr = $stdout\nFile.new(\"f.txt\") { |f| 1 }\n" + const want = "warning: File::new() does not take block; use File::open() instead\n" + if got := eval(t, src); got != want { + t.Errorf("got=%q\nwant=%q", got, want) + } +} + +// fdOpenScratch builds a directory holding "f.txt" with "hello file\n". +func fdOpenScratch(t *testing.T) string { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "f.txt"), []byte("hello file\n"), 0o644); err != nil { + t.Fatalf("fixture: %v", err) + } + return dir +} From 43349fdae5c9c0ab2fc08f246a5585a26e04edb0 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 23 Sep 2026 23:50:47 +0200 Subject: [PATCH 06/10] Cover every branch this wave added, and drop the three it left unreachable The coverage gate reads a rounded total, so a function at 0% can hide inside a green number. Checked per FUNCTION instead: moving File.open off the fopen-style mode string left flagsToMode at 0% and vmodeString at 27%, because File.open had been their only busy caller. IO.sysopen, IO#reopen and IO.popen still reach them, so they are exercised there now. Three branches this wave wrote turned out to be unreachable, and were removed rather than left as untested code: - raiseOpenErrno's EISDIR and EEXIST arms: openFileSpec raises both itself, from the flags and the stat, before any call can report them; - timeIntervalFormat's "-Inf": arg_range_check refuses a negative interval before the range check can see it; - ioFlushClose, whose only caller is now the shared ioYieldAndClose. timeIntervalFromDivmod now reads its quotient with repeatLong rather than a switch of its own: NUM2TIMET IS rb_num2long, so "no implicit conversion of String into Integer" and "no implicit conversion from nil to integer" come out right by construction instead of by a second spelling of the rule. Both were byte-compared with MRI 4.0.5. Every function this wave added or touched is now at 100%; the two left under it in these files (defStringIORead, inspectStr) are untouched here. Full suite green, and the wasip1 and windows lanes build. Co-Authored-By: Claude Opus 5 --- internal/vm/io.go | 19 +- internal/vm/thread.go | 36 ++- .../vm/wave28_open_paths_internal_test.go | 236 ++++++++++++++++++ internal/vm/wave28_sleep_scheduler_test.go | 6 + 4 files changed, 262 insertions(+), 35 deletions(-) create mode 100644 internal/vm/wave28_open_paths_internal_test.go diff --git a/internal/vm/io.go b/internal/vm/io.go index f9b4b0ad..5bcb91bb 100644 --- a/internal/vm/io.go +++ b/internal/vm/io.go @@ -1063,16 +1063,13 @@ func raiseFlockErrno(err error, p string) { } // raiseOpenErrno turns a Go open/read failure into the Errno MRI's rb_sysopen -// reports, message and all. Anything unrecognised falls back to ENOENT, which is -// what this code raised for every failure before. +// reports, message and all. Only the permission failure is distinguished: +// openFileSpec raises EEXIST and EISDIR itself, from the flags and the stat, +// before any of these calls can report them. Anything else falls back to ENOENT, +// which is what this code raised for every failure before. func raiseOpenErrno(err error, p string) { - switch { - case errors.Is(err, fs.ErrPermission): + if errors.Is(err, fs.ErrPermission) { raise("Errno::EACCES", "Permission denied @ rb_sysopen - %s", p) - case errors.Is(err, syscall.EISDIR): - raise("Errno::EISDIR", "Is a directory @ rb_sysopen - %s", p) - case errors.Is(err, fs.ErrExist): - raise("Errno::EEXIST", "File exists @ rb_sysopen - %s", p) } raise("Errno::ENOENT", "No such file or directory @ rb_sysopen - %s", p) } @@ -1212,12 +1209,6 @@ func ioFlush(o *IOObj) { } } -// ioFlushClose flushes then marks the stream closed (the File.open block exit). -func ioFlushClose(o *IOObj) { - ioFlush(o) - o.closed = true -} - // curStdout / curStderr / curStdin return the IO currently bound to the global, // falling back to the raw VM writer when a host rebinds it to a non-IO value. func (vm *VM) curStdout() *IOObj { return vm.curIO("$stdout", vm.out, "STDOUT") } diff --git a/internal/vm/thread.go b/internal/vm/thread.go index 4fb02e96..dda5b2a5 100644 --- a/internal/vm/thread.go +++ b/internal/vm/thread.go @@ -1074,18 +1074,14 @@ func (vm *VM) timeInterval(v object.Value) float64 { // with NUM2LONG, i.e. truncated to whole nanoseconds — which is why a Rational // interval loses everything below a nanosecond rather than rounding. func (vm *VM) timeIntervalFromDivmod(q, rem object.Value) float64 { - var sec int64 - switch n := q.(type) { - case object.Integer: - sec = int64(n) - case object.Float: - // NUM2TIMET reaches rb_num2long, which truncates a Float rather than - // refusing it — so #divmod may answer with one and keep its fraction in the - // remainder, as `[0.5, 0]` does. - sec = int64(float64(n)) - default: - raise("TypeError", "can't convert %s into time interval", vm.timeIntervalClassName(q)) + // NUM2TIMET is rb_num2long, which TRUNCATES a Float rather than refusing it — + // so #divmod may answer with one and keep the fraction in the remainder, as + // `[0.5, 0]` does — and which names nil differently from everything else it + // cannot convert. + if object.IsNil(q) { + raise("TypeError", "no implicit conversion from nil to integer") } + sec := vm.repeatLong(q) if sec < 0 { raise("ArgumentError", "time interval must not be negative") } @@ -1096,9 +1092,10 @@ func (vm *VM) timeIntervalFromDivmod(q, rem object.Value) float64 { // timeIntervalRangeCheck is time_timespec's `if (f != t.tv_sec)` guard: the // integral part of the duration has to survive the trip through a time_t (an -// int64 here), so NaN and the infinities land in the same RangeError as a finite -// value that is simply too large. The number is formatted the way MRI's own -// vsnprintf renders `%f` — "NaN", "Inf", "-Inf", and six decimals otherwise. +// int64 here), so NaN and Infinity land in the same RangeError as a finite value +// that is simply too large. The number is formatted the way MRI's own vsnprintf +// renders `%f` — "NaN", "Inf", and six decimals otherwise. Negative infinity +// never reaches here: arg_range_check refuses a negative interval first. func timeIntervalRangeCheck(x float64) { i, _ := math.Modf(x) if !math.IsNaN(i) && i >= -9223372036854775808.0 && i < 9223372036854775808.0 { @@ -1115,8 +1112,6 @@ func timeIntervalFormat(x float64) string { return "NaN" case math.IsInf(x, 1): return "Inf" - case math.IsInf(x, -1): - return "-Inf" } return strconv.FormatFloat(x, 'f', 6, 64) } @@ -1125,11 +1120,10 @@ func timeIntervalFormat(x float64) string { // rb_obj_class, which knows Rational and Complex apart from a plain Object; the // VM-less caller falls back to the static table. func (vm *VM) timeIntervalClassName(v object.Value) string { - if vm == nil { - return classNameOf(v) - } - if c := vm.classOf(v); c != nil { - return c.name + if vm != nil { + if c := vm.classOf(v); c != nil { + return c.name + } } return classNameOf(v) } diff --git a/internal/vm/wave28_open_paths_internal_test.go b/internal/vm/wave28_open_paths_internal_test.go new file mode 100644 index 00000000..e6fb0c56 --- /dev/null +++ b/internal/vm/wave28_open_paths_internal_test.go @@ -0,0 +1,236 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm + +import ( + "os" + "path/filepath" + "runtime" + "syscall" + "testing" + + "github.com/go-embedded-ruby/ruby/internal/object" +) + +// TestWave28SysopenIntegerModes reaches vmodeString and flagsToMode, the +// lossy Integer→mode-string mapping the remaining callers still use: +// IO.sysopen, IO#reopen and IO.popen all take a mode that way. File.open no +// longer does — it works from the open(2) flags themselves — so these are the +// only paths left through it, and each flag combination maps to the fopen-style +// mode openFileIO understands. +func TestWave28SysopenIntegerModes(t *testing.T) { + dir := slash(t.TempDir()) + f := dir + "/m.txt" + if err := os.WriteFile(filepath.FromSlash(f), []byte("body"), 0o644); err != nil { + t.Fatal(err) + } + cases := []struct{ src, want string }{ + // Each Integer mode reaches flagsToMode; the resulting stream reports the + // access halves that mode implies. + {`fd = IO.sysopen("` + f + `", File::RDONLY); io = IO.for_fd(fd); p [io.read, io.closed?]`, "[\"body\", false]\n"}, + {`fd = IO.sysopen("` + f + `", File::RDWR); io = IO.for_fd(fd); p io.read`, "\"body\"\n"}, + {`fd = IO.sysopen("` + f + `", File::APPEND); io = IO.for_fd(fd); p io.class`, "IO\n"}, + {`fd = IO.sysopen("` + f + `", File::APPEND | File::RDWR); io = IO.for_fd(fd); p io.class`, "IO\n"}, + {`fd = IO.sysopen("` + dir + `/w.txt", File::WRONLY); p File.exist?("` + dir + `/w.txt")`, "true\n"}, + {`fd = IO.sysopen("` + dir + `/c.txt", File::RDWR | File::CREAT); p File.exist?("` + dir + `/c.txt")`, "true\n"}, + // A String mode is taken as it stands. + {`fd = IO.sysopen("` + f + `", "r"); p IO.for_fd(fd).read`, "\"body\"\n"}, + // An object converts through #to_int (an Integer mode) or #to_str (a + // String one); anything else is the TypeError naming String. + {`o = Object.new; def o.to_int; File::RDONLY; end; fd = IO.sysopen("` + f + `", o); p IO.for_fd(fd).read`, "\"body\"\n"}, + {`o = Object.new; def o.to_str; "r"; end; fd = IO.sysopen("` + f + `", o); p IO.for_fd(fd).read`, "\"body\"\n"}, + {`begin; IO.sysopen("` + f + `", :r); rescue => e; p [e.class, e.message]; end`, + "[TypeError, \"no implicit conversion of Symbol into String\"]\n"}, + // openFileIO's own refusals: an empty and an unrecognised mode. + {`begin; IO.sysopen("` + f + `", ""); rescue => e; p [e.class, e.message]; end`, + "[ArgumentError, \"invalid access mode \"]\n"}, + {`begin; IO.sysopen("` + f + `", "zz"); rescue => e; p [e.class, e.message]; end`, + "[ArgumentError, \"invalid access mode zz\"]\n"}, + // openFileIO's 'a' branch on a path that does not exist yet materialises it, + // because FMODE_APPEND carries O_CREAT. + {`fd = IO.sysopen("` + dir + `/a.txt", "a"); p File.exist?("` + dir + `/a.txt")`, "true\n"}, + {`fd = IO.sysopen("` + f + `", "a"); p File.exist?("` + f + `")`, "true\n"}, + // ...and its 'r' and 'a' branches on something that is not a regular file + // open with an empty buffer rather than trying to read the whole of it. + {`fd = IO.sysopen("` + dir + `"); p IO.for_fd(fd).read`, "\"\"\n"}, + {`fd = IO.sysopen("` + dir + `", "a"); p IO.for_fd(fd).class`, "IO\n"}, + // Both write branches materialise the file, and report the failure to do so + // as the ENOENT a missing directory gives. + {`begin; IO.sysopen("` + dir + `/nodir/x", File::WRONLY); rescue => e; p [e.class, e.message]; end`, + "[Errno::ENOENT, \"No such file or directory @ rb_sysopen - " + dir + "/nodir/x\"]\n"}, + {`begin; IO.sysopen("` + dir + `/nodir/y", "a"); rescue => e; p [e.class, e.message]; end`, + "[Errno::ENOENT, \"No such file or directory @ rb_sysopen - " + dir + "/nodir/y\"]\n"}, + } + for _, c := range cases { + if got := runFS(t, c.src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + } +} + +// TestWave28FileSizeFromTheDescriptor covers rb_file_size's two ends: it is an +// fstat on the OPEN descriptor, so a closed stream has none to ask ("closed +// stream") and an UNLINKED path still answers — the descriptor outlives the +// directory entry, which is what core/file/size_spec.rb calls "the cached size +// of the file if subsequently deleted". MRI 4.0.5. +func TestWave28FileSizeFromTheDescriptor(t *testing.T) { + dir := slash(t.TempDir()) + f := dir + "/s.txt" + if err := os.WriteFile(filepath.FromSlash(f), []byte("rubinius"), 0o644); err != nil { + t.Fatal(err) + } + cases := []struct{ src, want string }{ + {`p File.new("` + f + `").size`, "8\n"}, + // Another stream appends: the size is read afresh, not remembered. + {`io = File.new("` + f + `"); File.open("` + f + `", "a") { |g| g.write "!" }; p io.size`, "9\n"}, + // The path is gone, but the stream still has its bytes. + {`io = File.new("` + f + `"); File.delete("` + f + `"); p io.size`, "8\n"}, + {`io = File.new("` + f + `"); io.close; begin; io.size; rescue => e; p [e.class, e.message]; end`, + "[IOError, \"closed stream\"]\n"}, + } + for _, c := range cases { + if err := os.WriteFile(filepath.FromSlash(f), []byte("rubinius"), 0o644); err != nil { + t.Fatal(err) + } + if got := runFS(t, c.src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + } +} + +// TestWave28OpenFlagsCombinations reaches the arms of ioModeSpec.oflags that +// only the :flags option asks for — it is the one caller that has to turn an +// fmode back into an open(2) flag set (rb_io_fmode_oflags), so each of +// O_RDWR, O_APPEND and O_EXCL has to survive the round trip. +func TestWave28OpenFlagsCombinations(t *testing.T) { + dir := slash(t.TempDir()) + f := dir + "/f.txt" + if err := os.WriteFile(filepath.FromSlash(f), []byte("body"), 0o644); err != nil { + t.Fatal(err) + } + cases := []struct{ src, want string }{ + // "r+" is O_RDWR, and survives as a readable AND writable stream. + {`File.open("` + f + `", "r+", flags: 0) { |io| p [io.read, io.class] }`, "[\"body\", File]\n"}, + // "a" is O_WRONLY|O_APPEND|O_CREAT; the append half survives, so the write + // still lands at the end. + {`File.open("` + f + `", "a", flags: 0) { |io| io.write "!" }; p File.read("` + f + `")`, "\"body!\"\n"}, + // "wx" is O_EXCL, and the flag set it produces still refuses an existing + // file — the round trip did not drop it. + {`begin; File.open("` + f + `", "wx", flags: 0) {}; rescue => e; p [e.class, e.message]; end`, + "[Errno::EEXIST, \"File exists @ rb_sysopen - " + f + "\"]\n"}, + } + for _, c := range cases { + if got := runFS(t, c.src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + } +} + +// TestWave28OpenPermissionFailures covers the two places openFileSpec turns a +// refused system call into an Errno: reading a file the process may not read, +// and the writability probe that stands in for the open(2) a buffer-backed +// stream never makes. Both are EACCES, and both are what MRI 4.0.5 raises. +func TestWave28OpenPermissionFailures(t *testing.T) { + if runtime.GOOS == "windows" || os.Geteuid() == 0 { + t.Skip("permission bits do not refuse the owner here") + } + dir := t.TempDir() + unreadable := filepath.Join(dir, "none.txt") + readonly := filepath.Join(dir, "ro.txt") + for _, p := range []string{unreadable, readonly} { + if err := os.WriteFile(p, []byte("body"), 0o644); err != nil { + t.Fatal(err) + } + } + if err := os.Chmod(unreadable, 0o000); err != nil { + t.Fatal(err) + } + if err := os.Chmod(readonly, 0o444); err != nil { + t.Fatal(err) + } + defer func() { + _ = os.Chmod(unreadable, 0o644) + _ = os.Chmod(readonly, 0o644) + }() + + // The read of an unreadable file. + got := runFS(t, `begin; File.open("`+slash(unreadable)+`"); rescue => e; p [e.class, e.message]; end`) + want := "[Errno::EACCES, \"Permission denied @ rb_sysopen - " + slash(unreadable) + "\"]\n" + if got != want { + t.Errorf("unreadable: got %q, want %q", got, want) + } + // "r+" wants to write but does not truncate, so it is the writability probe — + // not the read and not the truncate — that refuses it. + got = runFS(t, `begin; File.open("`+slash(readonly)+`", "r+"); rescue => e; p [e.class, e.message]; end`) + want = "[Errno::EACCES, \"Permission denied @ rb_sysopen - " + slash(readonly) + "\"]\n" + if got != want { + t.Errorf("read-only: got %q, want %q", got, want) + } +} + +// TestWave28FlockDescriptorReuse covers lockDescriptor's second visit: a lock +// taken and then CHANGED — shared after exclusive — must act on the descriptor +// already held, because flock(2) upgrades and downgrades a lock in place and +// taking a second descriptor would instead have the process block against itself. +func TestWave28FlockDescriptorReuse(t *testing.T) { + p := flockScratch(t) + opens := 0 + restore := swapFlockSeam(func(string) (int, error) { opens++; return 7, nil }, + func(int, int) error { return nil }, + func(int) error { return nil }) + defer restore() + src := `f = File.open("` + p + `", "r"); p [f.flock(File::LOCK_EX), f.flock(File::LOCK_SH)]; f.close` + if got := runFS(t, src); got != "[0, 0]\n" { + t.Fatalf("got %q", got) + } + if opens != 1 { + t.Errorf("opened %d descriptors for two locks, want 1", opens) + } +} + +// TestWave28FlockClosedWhileWaiting covers the rb_io_check_closed rb_file_flock +// performs after each 0.1 s wait: a blocking lock request is not a commitment, +// and a stream closed from elsewhere while the request waits ends it with an +// IOError rather than spinning forever. The waiting fiber releases the GVL, which +// is what lets the closing thread run at all. +func TestWave28FlockClosedWhileWaiting(t *testing.T) { + p := flockScratch(t) + restore := swapFlockSeam(func(string) (int, error) { return 7, nil }, + func(int, int) error { return syscall.EWOULDBLOCK }, + func(int) error { return nil }) + defer restore() + src := ` +f = File.open("` + p + `", "r") +t = Thread.new do + begin + f.flock(File::LOCK_EX) + "no error" + rescue IOError => e + e.message + end +end +Thread.pass until t.status == "sleep" || !t.status +f.close +p t.value +` + if got := runFS(t, src); got != "\"closed stream\"\n" { + t.Errorf("got %q, want %q", got, "\"closed stream\"\n") + } +} + +// TestWave28FileRecvClassFallback covers fileRecvClass's guard. File.open and +// File.new always reach it with the class they were called on, so the fallback +// cannot be produced from Ruby — it is there because a native method's receiver +// is an object.Value and nothing in the type says it is a class. +func TestWave28FileRecvClassFallback(t *testing.T) { + cls := newClass("Fallback", nil) + if got := fileRecvClass(object.NilV, cls); got != cls { + t.Errorf("a non-class receiver must fall back to the given class, got %v", got) + } + other := newClass("Receiver", nil) + if got := fileRecvClass(other, cls); got != other { + t.Errorf("a class receiver must be used as-is, got %v", got) + } +} diff --git a/internal/vm/wave28_sleep_scheduler_test.go b/internal/vm/wave28_sleep_scheduler_test.go index e779e10a..b083b792 100644 --- a/internal/vm/wave28_sleep_scheduler_test.go +++ b/internal/vm/wave28_sleep_scheduler_test.go @@ -78,6 +78,12 @@ Fiber.set_scheduler(S.new) {`o = Object.new; def o.divmod(*); [0.5, 0]; end; p sleep(o)`, "0\n"}, // arg_range_check applies to the whole-seconds half only. {`begin; sleep(Rational(-1, 2)); rescue => e; p [e.class, e.message]; end`, "[ArgumentError, \"time interval must not be negative\"]\n"}, + // The quotient goes through NUM2TIMET, which is rb_num2long: it names nil + // differently from everything else it cannot convert. + {`o = Object.new; def o.divmod(*); [nil, 0]; end; begin; sleep(o); rescue => e; p [e.class, e.message]; end`, + "[TypeError, \"no implicit conversion from nil to integer\"]\n"}, + {`o = Object.new; def o.divmod(*); ["a", 0]; end; begin; sleep(o); rescue => e; p [e.class, e.message]; end`, + "[TypeError, \"no implicit conversion of String into Integer\"]\n"}, // A #divmod that does not answer with an Array is no conversion at all. {`o = Object.new; def o.divmod(*); 5; end; begin; sleep(o); rescue => e; p [e.class, e.message]; end`, "[TypeError, \"can't convert Object into time interval\"]\n"}, // Nothing to send #divmod to: a String has none. From 2bc50b3cf0c83a1a5c540c68929cec3e241660ea Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 24 Sep 2026 00:08:18 +0200 Subject: [PATCH 07/10] test: the flock stub speaks EAGAIN, which every platform defines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wasip1 lane failed to compile the tests: `undefined: syscall.EWOULDBLOCK` at three call sites. WASI's errno enum has no separate EWOULDBLOCK — errno.go says so in the comment beside `errnoAliases`, and filestat_wasm.go leaves `flockAgainErrs` empty for the same reason. The stub only needs an error value the retry list will match, not the real errno of any platform. `syscall.EAGAIN` is defined on wasip1, windows and unix alike, and on unix `flockAgainErrs` already carries it beside EWOULDBLOCK, so the POSIX lanes exercise the same path as before. Checked with `go vet ./internal/vm/` (which type-checks the test files) for wasip1/wasm, windows/amd64 and linux/amd64. Co-Authored-By: Claude Opus 5 --- internal/vm/wave28_flock_internal_test.go | 8 +++++--- internal/vm/wave28_open_paths_internal_test.go | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/vm/wave28_flock_internal_test.go b/internal/vm/wave28_flock_internal_test.go index 2ffcd3d8..68c8e962 100644 --- a/internal/vm/wave28_flock_internal_test.go +++ b/internal/vm/wave28_flock_internal_test.go @@ -67,7 +67,7 @@ func TestWave28FlockRetriesWithoutLockNB(t *testing.T) { func(fd, op int) error { calls++ if calls < 3 { - return syscall.EWOULDBLOCK + return syscall.EAGAIN } return nil }, @@ -148,8 +148,10 @@ func swapFlockSeam(open func(string) (int, error), lock func(int, int) error, cl oOpen, oLock, oClose, oAgain := lockFdFn, flockFn, closeFdFn, flockAgainErrs lockFdFn, flockFn, closeFdFn = open, lock, closeFd // The retry list is per platform and empty where there is no flock(2); the - // stub speaks EWOULDBLOCK, so give every platform that one entry. - flockAgainErrs = []error{syscall.EWOULDBLOCK} + // stub speaks EAGAIN, so give every platform that one entry. EAGAIN rather + // than EWOULDBLOCK because wasip1 defines no EWOULDBLOCK at all, and on unix + // the real list already carries EAGAIN beside it. + flockAgainErrs = []error{syscall.EAGAIN} return func() { lockFdFn, flockFn, closeFdFn, flockAgainErrs = oOpen, oLock, oClose, oAgain } } diff --git a/internal/vm/wave28_open_paths_internal_test.go b/internal/vm/wave28_open_paths_internal_test.go index e6fb0c56..928fab41 100644 --- a/internal/vm/wave28_open_paths_internal_test.go +++ b/internal/vm/wave28_open_paths_internal_test.go @@ -198,7 +198,7 @@ func TestWave28FlockDescriptorReuse(t *testing.T) { func TestWave28FlockClosedWhileWaiting(t *testing.T) { p := flockScratch(t) restore := swapFlockSeam(func(string) (int, error) { return 7, nil }, - func(int, int) error { return syscall.EWOULDBLOCK }, + func(int, int) error { return syscall.EAGAIN }, func(int) error { return nil }) defer restore() src := ` From 06ead2226622f0fffbb2bf6b10aac82121785165 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 24 Sep 2026 08:24:31 +0200 Subject: [PATCH 08/10] test: cover two branches this wave orphaned, and stop asserting POSIX on windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three failures, three different causes. The ubuntu lane failed the 100% gate at 99.9%. Diffing its below-100 list against main's shows exactly two functions newly under: IO#truncate's "not opened for writing" guard (io.go) and IO.select's NaN-timeout RangeError (spawn.go) — the second in a file this branch never touched, which is the orphaning signature: the change stopped exercising a path, it did not break it. Both answer exactly as MRI 4.0.5 does, verified byte for byte; they had simply lost their only caller. Whitebox tests added for each. The windows lane failed two perm assertions. Windows has no POSIX mode bits — its security model is ACL-based, and Go maps perm to nothing finer than a read-only flag — so a created file reads back 0666 whatever was asked for. MRI cannot honour 0744 there either, but there is no Windows MRI on this host to witness what it DOES report, so the two cases move into their own POSIX-guarded test naming issue #644, rather than being pinned to whatever Windows returns. The windows lane also failed three tests on TempDir cleanup, not on any assertion: they deliberately leave a File open (File.open(fh.fileno) needs fh alive) and Windows will not delete a file with an open handle. The underlying gap is the VM's, not the tests' — rbgo closes no IO at interpreter shutdown where MRI closes every one — and it is now issue #643. fdOpenScratch removes its directory best-effort and says so, so these cases report on what they test. Co-Authored-By: Claude Opus 5 --- internal/vm/wave28_cover_gaps_test.go | 47 +++++++++++++++++++++++ internal/vm/wave28_file_fd_open_test.go | 15 +++++++- internal/vm/wave28_file_open_mode_test.go | 31 +++++++++++++-- 3 files changed, 88 insertions(+), 5 deletions(-) create mode 100644 internal/vm/wave28_cover_gaps_test.go diff --git a/internal/vm/wave28_cover_gaps_test.go b/internal/vm/wave28_cover_gaps_test.go new file mode 100644 index 00000000..157a091d --- /dev/null +++ b/internal/vm/wave28_cover_gaps_test.go @@ -0,0 +1,47 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm + +import "testing" + +// TestWave28TruncateReadOnly covers IO#truncate's read-only guard. rb_io_truncate +// goes through GetOpenFile and then rb_io_check_writable (io.c), so a stream +// opened for reading answers IOError "not opened for writing" rather than +// attempting the syscall. MRI 4.0.5 prints the same pair. +func TestWave28TruncateReadOnly(t *testing.T) { + dir := t.TempDir() + src := `p1 = ` + rq(dir+"/ro.txt") + ` +File.write(p1, "abc") +f = File.open(p1, "r") +begin + f.truncate(1) +rescue => e + p [e.class.to_s, e.message] +ensure + f.close +end +` + if got, want := runFS(t, src), "[\"IOError\", \"not opened for writing\"]\n"; got != want { + t.Errorf("truncate on a read-only stream: got %q want %q", got, want) + } +} + +// TestWave28SelectNaNTimeout covers IO.select's timeout validation: a NaN +// interval is a RangeError before any waiting happens, which is what +// rb_time_interval's NaN check yields. Verified against MRI 4.0.5. +func TestWave28SelectNaNTimeout(t *testing.T) { + src := `r, w = IO.pipe +begin + IO.select([r], nil, nil, Float::NAN) +rescue => e + p [e.class.to_s, e.message] +ensure + r.close; w.close +end +` + if got, want := runFS(t, src), "[\"RangeError\", \"NaN out of Time range\"]\n"; got != want { + t.Errorf("IO.select with a NaN timeout: got %q want %q", got, want) + } +} diff --git a/internal/vm/wave28_file_fd_open_test.go b/internal/vm/wave28_file_fd_open_test.go index 44a3217c..a5b9bce3 100644 --- a/internal/vm/wave28_file_fd_open_test.go +++ b/internal/vm/wave28_file_fd_open_test.go @@ -94,7 +94,20 @@ func TestWave28FileNewWarnsOnBlock(t *testing.T) { // fdOpenScratch builds a directory holding "f.txt" with "hello file\n". func fdOpenScratch(t *testing.T) string { t.Helper() - dir := t.TempDir() + // Not t.TempDir(): these cases deliberately leave a File open when the + // snippet ends — `File.open(fh.fileno)` needs fh alive — and rbgo closes no + // IO at interpreter shutdown, where MRI closes every one. On POSIX nothing + // notices; on Windows the open handle makes t.TempDir()'s RemoveAll fail and + // the test errors on cleanup rather than on its assertion. + // + // So remove the directory on a best-effort basis and let the VM gap be + // tracked where it belongs, in issue #643, instead of being reported here as + // a failure of these cases. + dir, err := os.MkdirTemp("", "rbgo-fdopen-") + if err != nil { + t.Fatalf("scratch: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) if err := os.WriteFile(filepath.Join(dir, "f.txt"), []byte("hello file\n"), 0o644); err != nil { t.Fatalf("fixture: %v", err) } diff --git a/internal/vm/wave28_file_open_mode_test.go b/internal/vm/wave28_file_open_mode_test.go index 9e441ab1..25135039 100644 --- a/internal/vm/wave28_file_open_mode_test.go +++ b/internal/vm/wave28_file_open_mode_test.go @@ -7,6 +7,7 @@ package vm_test import ( "os" "path/filepath" + "runtime" "testing" ) @@ -60,10 +61,6 @@ func TestWave28FileOpenModeContract(t *testing.T) { {`File.open("f.txt", "r", mode: "w")`, "[ArgumentError, \"mode specified twice\"]\n"}, {`File.open("p3.txt", "w", 0744, perm: 0755) {}`, "[ArgumentError, \"perm specified twice\"]\n"}, - // --- the permission argument reaches the creation, in both spellings. - {`File.open("p2.txt", File::CREAT | File::RDWR, 0744) { |f| f.write "z" }; File.stat("p2.txt").mode.to_s(8)`, "\"100744\"\n"}, - {`File.open("p4.txt", "w", perm: 0745) {}; File.stat("p4.txt").mode.to_s(8)`, "\"100745\"\n"}, - // --- the :newline decorator. {`File.open("f.txt", "rb", newline: :universal) {}`, "[ArgumentError, \"newline decorator with binary mode\"]\n"}, {`File.open("f.txt", "r", newline: :bogus) {}`, "[ArgumentError, \"unexpected value for newline option: bogus\"]\n"}, @@ -139,3 +136,29 @@ func openModeScratch(t *testing.T) string { } return dir } + +// TestWave28FileOpenPerm covers the permission argument in both spellings — +// positional (File.open(path, flags, 0744)) and keyword (perm: 0745). +// +// It is POSIX-only. Windows has no mode bits: its file security model is +// ACL-based and Go maps perm to nothing but a read-only flag, so a created file +// reads back 0666 there whatever perm was asked for. MRI on Windows cannot +// honour 0744 either, but there is no Windows MRI on this host to witness what +// it DOES report — so rather than pin the assertion to whatever Windows +// currently returns, the case is left to issue #644. +func TestWave28FileOpenPerm(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX mode bits; see issue #644 for what MRI reports on Windows") + } + cases := []struct{ src, want string }{ + {`File.open("p2.txt", File::CREAT | File::RDWR, 0744) { |f| f.write "z" }; File.stat("p2.txt").mode.to_s(8)`, "\"100744\"\n"}, + {`File.open("p4.txt", "w", perm: 0745) {}; File.stat("p4.txt").mode.to_s(8)`, "\"100745\"\n"}, + } + for _, c := range cases { + dir := openModeScratch(t) + src := "Dir.chdir(" + rubyString(dir) + ")\nbegin\n p(begin\n" + c.src + "\nend)\nrescue => e\n p [e.class, e.message]\nend\n" + if got := eval(t, src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + } +} From 0795d8aded7267639db0184c4e58d1da5c200e2b Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 24 Sep 2026 09:37:36 +0200 Subject: [PATCH 09/10] Cover truncate's three branches, and delete a NaN guard that cannot run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My previous attempt at these two coverage gaps missed both, and the way it missed them is worth recording. IO#truncate: I read the uncovered block as the "not opened for writing" guard and wrote a File test for it. The uncovered block was the line ABOVE — the "closed stream" guard — and the method is registered inside defStringIORead, where it serves BOTH a StringIO and a path-backed File. Three cases are needed, not one: a closed File (closed stream), a read-only StringIO (not opened for writing), and a writable File (the ftruncate half, where the file on disk is that size at once). All three verified against MRI 4.0.5. IO.select's NaN guard is DEAD CODE, not an uncovered branch. It reads `if f := mutexSleepDur(args[3]); math.IsNaN(f)`, but mutexSleepDur is rb_time_interval and RAISES RangeError("NaN out of Time range") itself for a NaN or an infinity rather than returning one. The check after it could never run — which is why my test saw the right error and the branch still read zero: the message came from timeInterval, not from here. Deleted rather than covered. One divergence found and recorded, not papered over: MRI's rb_io_truncate is ftruncate(2) on the DESCRIPTOR, so it succeeds after the path is unlinked. rbgo's IOObj is buffer-backed with a synthetic fd, truncates by path, and answers ENOENT. Same root cause as File#stat on an unlinked file; added to issue #635, and the test says so where it asserts rbgo's answer. Co-Authored-By: Claude Opus 5 --- internal/vm/spawn.go | 8 +-- internal/vm/wave28_cover_gaps_test.go | 86 +++++++++++++++++++++++---- 2 files changed, 77 insertions(+), 17 deletions(-) diff --git a/internal/vm/spawn.go b/internal/vm/spawn.go index 97c0e23c..33352b5d 100644 --- a/internal/vm/spawn.go +++ b/internal/vm/spawn.go @@ -6,7 +6,6 @@ package vm import ( "io" - "math" "os" "path/filepath" "sort" @@ -247,10 +246,11 @@ func (vm *VM) registerSpawn() { if len(args) == 0 { raise("ArgumentError", "wrong number of arguments (given 0, expected 1..4)") } + // No NaN guard here: mutexSleepDur is rb_time_interval, and it RAISES + // RangeError("NaN out of Time range") itself for a NaN or an infinity + // rather than returning one. A second check after it could never run. if len(args) > 3 && !object.IsNil(args[3]) { - if f := mutexSleepDur(args[3]); math.IsNaN(f) { - raise("RangeError", "NaN out of Time range") - } + mutexSleepDur(args[3]) } readReady := object.NewArray() for _, v := range selectSet(vm, args, 0) { diff --git a/internal/vm/wave28_cover_gaps_test.go b/internal/vm/wave28_cover_gaps_test.go index 157a091d..6beb0e65 100644 --- a/internal/vm/wave28_cover_gaps_test.go +++ b/internal/vm/wave28_cover_gaps_test.go @@ -6,25 +6,27 @@ package vm import "testing" -// TestWave28TruncateReadOnly covers IO#truncate's read-only guard. rb_io_truncate -// goes through GetOpenFile and then rb_io_check_writable (io.c), so a stream -// opened for reading answers IOError "not opened for writing" rather than -// attempting the syscall. MRI 4.0.5 prints the same pair. +// TestWave28TruncateReadOnly covers StringIO#truncate's read-only guard — the +// one inside defStringIORead, which is a different implementation from File's. +// rb_io_truncate goes through GetOpenFile then rb_io_check_writable (io.c), so a +// stream opened for reading answers IOError "not opened for writing" rather than +// attempting the write. MRI 4.0.5 prints the same pair, and truncates to "a" +// when the stream IS writable. func TestWave28TruncateReadOnly(t *testing.T) { - dir := t.TempDir() - src := `p1 = ` + rq(dir+"/ro.txt") + ` -File.write(p1, "abc") -f = File.open(p1, "r") + src := `require "stringio" +s = StringIO.new("abc", "r") begin - f.truncate(1) + s.truncate(1) rescue => e p [e.class.to_s, e.message] -ensure - f.close end +w = StringIO.new("abc") +w.truncate(1) +p w.string ` - if got, want := runFS(t, src), "[\"IOError\", \"not opened for writing\"]\n"; got != want { - t.Errorf("truncate on a read-only stream: got %q want %q", got, want) + want := "[\"IOError\", \"not opened for writing\"]\n\"a\"\n" + if got := runFS(t, src); got != want { + t.Errorf("StringIO#truncate: got %q want %q", got, want) } } @@ -45,3 +47,61 @@ end t.Errorf("IO.select with a NaN timeout: got %q want %q", got, want) } } + +// TestWave28TruncateClosedStream covers the other guard in the same truncate: +// rb_io_truncate calls GetOpenFile BEFORE rb_io_check_writable, so a CLOSED +// stream answers "closed stream" whichever mode it had. The guard excludes +// StringIO, which tolerates a closed stream — hence a File here, not a StringIO. +// MRI 4.0.5 prints the same pair. +func TestWave28TruncateClosedStream(t *testing.T) { + dir := t.TempDir() + src := `p1 = ` + rq(dir+"/closed.txt") + ` +f = File.open(p1, "w") +f.write("abc") +f.close +begin + f.truncate(0) +rescue => e + p [e.class.to_s, e.message] +end +` + if got, want := runFS(t, src), "[\"IOError\", \"closed stream\"]\n"; got != want { + t.Errorf("truncate on a closed stream: got %q want %q", got, want) + } +} + +// TestWave28TruncateFileOnDisk covers the ftruncate half of IO#truncate: the +// file on disk is that size at once, which is what File.size and a second +// stream opened on the same path see straight afterwards. MRI 4.0.5 prints 3. +// +// The second case documents a DIVERGENCE rather than asserting a contract. +// MRI's rb_io_truncate is ftruncate(2) on the descriptor, so it succeeds after +// the path is unlinked; rbgo's IOObj is buffer-backed with a synthetic fd, so +// it truncates by path and answers ENOENT. That is the same root cause as +// File#stat on an unlinked file, and it is tracked in issue #635 — recorded +// here so the branch is exercised and the difference is visible at the point +// where someone would otherwise read rbgo's answer as correct. +func TestWave28TruncateFileOnDisk(t *testing.T) { + dir := t.TempDir() + src := `p1 = ` + rq(dir+"/sz.txt") + ` +File.write(p1, "abcdef") +f = File.open(p1, "r+") +f.truncate(3) +p File.size(p1) +f.close +p2 = ` + rq(dir+"/gone.txt") + ` +File.write(p2, "abcdef") +g = File.open(p2, "r+") +File.delete(p2) +begin + g.truncate(3) + p :ok +rescue => e + p e.class.to_s +end +` + // MRI prints 3 then :ok. rbgo prints 3 then the errno, per #635. + if got, want := runFS(t, src), "3\n\"Errno::ENOENT\"\n"; got != want { + t.Errorf("truncate on disk: got %q want %q", got, want) + } +} From f4cdba7dff18a0a2acdd9a1e4590341f09c2f3eb Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 24 Sep 2026 10:55:44 +0200 Subject: [PATCH 10/10] test: one scratch-directory rule, in one place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more tests failed the windows lane on TempDir cleanup rather than on any assertion — the same cause the fd-open cases already carried a note about: they hold a File open (the permission cases keep a handle across a chmod), rbgo closes no IO at interpreter shutdown where MRI closes every one (#643), and Windows will not delete a file with an open handle. Fixing the second helper by copying the first one's explanation would have left the same paragraph in two places to drift apart, which is the defect this wave already found in File::Stat#inspect — the member list written twice and disagreeing at the one member that can be nil. So the rule moves into ioScratchDir and both helpers call it. Co-Authored-By: Claude Opus 5 --- internal/vm/wave28_file_fd_open_test.go | 15 +--------- internal/vm/wave28_file_open_mode_test.go | 2 +- internal/vm/wave28_scratch_test.go | 34 +++++++++++++++++++++++ 3 files changed, 36 insertions(+), 15 deletions(-) create mode 100644 internal/vm/wave28_scratch_test.go diff --git a/internal/vm/wave28_file_fd_open_test.go b/internal/vm/wave28_file_fd_open_test.go index a5b9bce3..af61592a 100644 --- a/internal/vm/wave28_file_fd_open_test.go +++ b/internal/vm/wave28_file_fd_open_test.go @@ -94,20 +94,7 @@ func TestWave28FileNewWarnsOnBlock(t *testing.T) { // fdOpenScratch builds a directory holding "f.txt" with "hello file\n". func fdOpenScratch(t *testing.T) string { t.Helper() - // Not t.TempDir(): these cases deliberately leave a File open when the - // snippet ends — `File.open(fh.fileno)` needs fh alive — and rbgo closes no - // IO at interpreter shutdown, where MRI closes every one. On POSIX nothing - // notices; on Windows the open handle makes t.TempDir()'s RemoveAll fail and - // the test errors on cleanup rather than on its assertion. - // - // So remove the directory on a best-effort basis and let the VM gap be - // tracked where it belongs, in issue #643, instead of being reported here as - // a failure of these cases. - dir, err := os.MkdirTemp("", "rbgo-fdopen-") - if err != nil { - t.Fatalf("scratch: %v", err) - } - t.Cleanup(func() { _ = os.RemoveAll(dir) }) + dir := ioScratchDir(t) if err := os.WriteFile(filepath.Join(dir, "f.txt"), []byte("hello file\n"), 0o644); err != nil { t.Fatalf("fixture: %v", err) } diff --git a/internal/vm/wave28_file_open_mode_test.go b/internal/vm/wave28_file_open_mode_test.go index 25135039..f92a361c 100644 --- a/internal/vm/wave28_file_open_mode_test.go +++ b/internal/vm/wave28_file_open_mode_test.go @@ -127,7 +127,7 @@ p File.read("wro.txt") // holding "f.txt" (contents "hello file\n") and an empty directory "adir". func openModeScratch(t *testing.T) string { t.Helper() - dir := t.TempDir() + dir := ioScratchDir(t) if err := os.WriteFile(filepath.Join(dir, "f.txt"), []byte("hello file\n"), 0o644); err != nil { t.Fatalf("fixture: %v", err) } diff --git a/internal/vm/wave28_scratch_test.go b/internal/vm/wave28_scratch_test.go new file mode 100644 index 00000000..dd6ef309 --- /dev/null +++ b/internal/vm/wave28_scratch_test.go @@ -0,0 +1,34 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm_test + +import ( + "os" + "testing" +) + +// ioScratchDir is t.TempDir() for tests that deliberately leave a File open. +// +// Several cases here have to: File.open(fh.fileno) needs fh alive to have a +// descriptor to reopen, and the permission cases hold a handle across a chmod. +// rbgo closes no IO at interpreter shutdown, where MRI closes every one — issue +// #643 — so those handles are still open when the test ends. On POSIX nothing +// notices, because unlinking an open file is allowed. On Windows the open +// handle makes t.TempDir()'s RemoveAll fail, and the test then reports a +// cleanup error instead of its assertion, which hides what it was measuring. +// +// So: remove the directory best-effort, and let the VM gap be tracked in #643 +// rather than re-reported by every test that trips over it. One rule, one +// place — the previous shape had this explanation duplicated per helper and +// they would have drifted. +func ioScratchDir(t *testing.T) string { + t.Helper() + dir, err := os.MkdirTemp("", "rbgo-ioscratch-") + if err != nil { + t.Fatalf("scratch: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + return dir +}