diff --git a/internal/vm/net_ftp.go b/internal/vm/net_ftp.go index a217045c..d5ab9522 100644 --- a/internal/vm/net_ftp.go +++ b/internal/vm/net_ftp.go @@ -77,6 +77,29 @@ func (vm *VM) registerFTPErrors(netMod, std *RClass) { // options) connects (and logs in when a username is supplied); open is new plus // the ensured-close block form. func (vm *VM) registerFTPClassMethods(ftp *RClass) { + // Net::FTP.default_passive, held in the CLASS VARIABLE the reference + // implementation uses (net-ftp 0.3.9 lib/net/ftp.rb:96 `@@default_passive = + // true`, accessors at :159-167). Storing it there rather than in a Go field + // is not decoration: a Ruby program can reach it through + // Net::FTP.class_variable_get(:@@default_passive), as it can under MRI, and + // there is one place holding the value instead of two that can disagree. + // + // ⛔ ruby/spec's library/net-ftp/spec_helper.rb sets this to FALSE for the + // whole suite, and its fixture server implements PORT/EPRT and has no `pasv` + // method at all. Without the accessor the helper's `if + // defined?(Net::FTP.default_passive)` guard was skipped in SILENCE and every + // spec ran against a mode the fixture cannot speak (#916). + ftp.cvars["@@default_passive"] = object.Bool(true) + ftp.smethods["default_passive"] = &Method{name: "default_passive", owner: ftp, + native: func(_ *VM, _ object.Value, _ []object.Value, _ *Proc) object.Value { + return object.Bool(ftpDefaultPassive(ftp)) + }} + ftp.smethods["default_passive="] = &Method{name: "default_passive=", owner: ftp, + native: func(_ *VM, _ object.Value, a []object.Value, _ *Proc) object.Value { + ftp.cvars["@@default_passive"] = object.Bool(a[0].Truthy()) + return a[0] + }} + newFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { f := newFTPObj(vm, ftp) vm.ftpInit(f, args) @@ -109,20 +132,33 @@ func (vm *VM) ftpWithClose(f *ftpObj, blk *Proc) (res object.Value) { // the legacy positional user / passwd / acct, and a login when a username is // present. func (vm *VM) ftpInit(f *ftpObj, args []object.Value) { - if len(args) == 0 || object.IsNil(args[0]) { - return - } - host := strArg(args[0]) port := 21 user, passwd, acct := "", "", "" userGiven, passwdGiven := false, false + // ⛔ THE OPTIONS ARE APPLIED WHETHER OR NOT A HOST WAS GIVEN. This used to + // return before reading them when args[0] was nil, so + // `Net::FTP.new(nil, passive: true)` silently ignored EVERY option -- found + // by running the settings against the reference rather than against itself: + // rbgo answered `false` where net-ftp 0.3.9 answers `true`. + // + // MRI's initialize sets @binary / @passive / @debug_mode / the timeouts + // first and only then, separately, connects when a host is present + // (lib/net/ftp.rb:285-300). ruby/spec drives exactly that shape: + // `@ftp.send(:initialize, nil, options)` then asserts `passive` + // (library/net-ftp/initialize_spec.rb:201-226). if len(args) >= 2 { if h, ok := args[1].(*object.Hash); ok { port, user, userGiven, passwd, passwdGiven, acct = vm.ftpApplyOpts(f, h, port) } } + // No host: configured, not connected. + if len(args) == 0 || object.IsNil(args[0]) { + return + } + host := strArg(args[0]) + vm.ftpConnect(f, host, port) if userGiven { vm.ftpLogin(f, user, passwd, passwdGiven, acct) @@ -139,7 +175,13 @@ func (vm *VM) ftpApplyOpts(f *ftpObj, h *object.Hash, port int) (int, string, bo port = clampInt(n) } } - if v, ok := ftpOptHash(h, "passive"); ok { + // ⛔ MRI tests the VALUE, not the key: `if options[:passive].nil?` falls back + // to @@default_passive (ftp.rb:286-290). So `passive: nil` is NOT a request + // for active mode — it is "no opinion". Reading the key's presence instead + // would make an explicit nil mean false, which is the opposite. This differs + // from read_timeout in the same Hash, where nil IS a request (for no bound), + // and the two are easy to conflate because they sit three lines apart. + if v, ok := ftpOptHash(h, "passive"); ok && !object.IsNil(v) { f.passive = v.Truthy() } // open_timeout and read_timeout are set from the options Hash exactly as diff --git a/internal/vm/net_ftp_bind.go b/internal/vm/net_ftp_bind.go index f96b87d2..0817d95b 100644 --- a/internal/vm/net_ftp_bind.go +++ b/internal/vm/net_ftp_bind.go @@ -98,11 +98,28 @@ const ftpDefaultReadTimeout = 60 // newFTPObj builds a Net::FTP with MRI's timeout defaults in place. Every // construction site goes through it so a new one cannot be born unbounded. +// ftpDefaultPassive reads Net::FTP's @@default_passive. An absent or non-boolean +// value reads as MRI's documented default, true: this is consulted on every +// construction, and a class variable someone deleted must not silently flip the +// transfer mode of the next connection. +func ftpDefaultPassive(cls *RClass) bool { + if cls == nil { + return true + } + v, ok := cls.cvars["@@default_passive"] + if !ok || object.IsNil(v) { + return true + } + return v.Truthy() +} + func newFTPObj(vm *VM, cls *RClass) *ftpObj { return &ftpObj{ - cls: cls, - binary: true, - passive: true, + cls: cls, + binary: true, + // Read at CONSTRUCTION, as MRI does: Net::FTP.default_passive = false + // affects connections made after it, never one already open. + passive: ftpDefaultPassive(cls), openTimeout: object.NilV, readTimeout: object.IntValue(ftpDefaultReadTimeout), vm: vm, diff --git a/internal/vm/net_ftp_internal_test.go b/internal/vm/net_ftp_internal_test.go index 98b733fe..71cf7000 100644 --- a/internal/vm/net_ftp_internal_test.go +++ b/internal/vm/net_ftp_internal_test.go @@ -1696,6 +1696,83 @@ func TestFTPTimeoutErrIgnoresANonTimeout(t *testing.T) { ftpTimeoutErr(nil, "open") } +// Net::FTP.default_passive, and the two things that made it worth running +// against the reference rather than against our own expectations. +// +// ⛔ WHY IT MATTERS BEYOND THE ACCESSOR: ruby/spec's library/net-ftp/spec_helper.rb +// is `if defined?(Net::FTP.default_passive) then Net::FTP.default_passive = false`, +// and its fixture server implements PORT/EPRT with NO `pasv` method at all. +// While the accessor was missing, that guard was skipped in SILENCE and all 56 +// files ran in a mode the fixture cannot speak (#916). +// +// ⛔ AND THE DEFECT THE COMPARISON FOUND: `Net::FTP.new(nil, passive: true)` +// answered false. ftpInit returned before reading the options Hash whenever the +// host was nil, so EVERY option was dropped on that path -- the shape of a +// mechanism wired at only some of its doors, again. MRI applies the options +// first and connects separately (net-ftp 0.3.9 lib/net/ftp.rb:285-300), which is +// exactly what library/net-ftp/initialize_spec.rb:201-226 drives with +// `@ftp.send(:initialize, nil, options)`. +func TestFTPDefaultPassiveMatchesTheReference(t *testing.T) { + got := runSrc(t, `require "net/ftp" +p Net::FTP.default_passive +p Net::FTP.new.passive +before = Net::FTP.new +Net::FTP.default_passive = false +p Net::FTP.default_passive +p before.passive +p Net::FTP.new.passive +p Net::FTP.new(nil, passive: nil).passive +p Net::FTP.new(nil, passive: true).passive +p Net::FTP.new(nil, passive: false).passive +Net::FTP.default_passive = true +p Net::FTP.new(nil, passive: nil).passive +p Net::FTP.new(nil, passive: false).passive +p(Net::FTP.default_passive = false) +p Net::FTP.class_variable_get(:@@default_passive) +`) + // Each line is net-ftp 0.3.9's answer, read off lib/net/ftp.rb:96 (the + // default), :159-167 (the accessors) and :286-290 (nil means "no opinion", + // so it falls back to the default while false and true are requests). + want := "true\n" + // default_passive starts true + "true\n" + // so does a fresh connection + "false\n" + // the setter takes + "true\n" + // ⛔ a connection built BEFORE the change keeps its mode + "false\n" + // one built after follows it + "false\n" + // passive: nil is no opinion -> the default + "true\n" + // passive: true is a request, even with a nil host + "false\n" + // and so is passive: false + "true\n" + // default back to true: nil follows it + "false\n" + // while an explicit false still wins + "false\n" + // the writer returns its argument + "false" // and the value lives in the class variable MRI uses (runSrc trims the last newline) + if got != want { + t.Errorf("default_passive table:\ngot %q\nwant %q", got, want) + } +} + +// ftpDefaultPassive's fallbacks, which no Ruby program can reach: it is +// consulted on EVERY construction, so a missing or nil class variable must read +// as MRI's documented default rather than silently flipping the transfer mode of +// the next connection. Asserted directly because "it cannot happen" is how a +// default nobody chose gets shipped. +func TestFTPDefaultPassiveFallsBackToMRIsDefault(t *testing.T) { + if !ftpDefaultPassive(nil) { + t.Error("a nil class must read as true, MRI's default") + } + cls := &RClass{cvars: map[string]object.Value{}} + if !ftpDefaultPassive(cls) { + t.Error("an absent @@default_passive must read as true") + } + cls.cvars["@@default_passive"] = object.NilV + if !ftpDefaultPassive(cls) { + t.Error("a nil @@default_passive must read as true, not as false") + } + cls.cvars["@@default_passive"] = object.Bool(false) + if ftpDefaultPassive(cls) { + t.Error("an explicit false must be honoured") + } +} + // ⛔ THE DEFECT SHAPE THIS REPOSITORY KEEPS REPEATING: a mechanism wired at only // SOME of its doors. open_timeout and read_timeout each have three doors -- the // constructor's options Hash, the attr writer, and the default -- and the Hash