Repository navigation
Net::FTP: bound the connect and every read, as MRI does - #921
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The WIP commit below added open_timeout/read_timeout; this adds what it owed. A server that accepts and stays SILENT -- the production shape, and exactly what #916's SIGQUIT dump showed rbgo waiting on -- must raise Net::ReadTimeout. The connection is held OPEN on purpose: a server that closed it would give the reader an EOF, a different path (Net::FTPConnectionError), and the test would pass with no deadline at all. ⛔ THE ELAPSED TIME IS PART OF THE ASSERTION. Without it the test would also pass if the greeting read failed instantly for an unrelated reason, which is the one outcome that would NOT prove a deadline fired. It asserts the call took at least 150ms against a 0.2s read_timeout, and measures 0.21s. Control run in both directions: removing the single armRead() call makes that test HANG until the 40s wall instead of failing in 0.21s. The second test pins the defaults together -- open_timeout nil, read_timeout 60 -- because the ASYMMETRY is the point and is MRI's, from library/net-ftp/initialize_spec.rb:272 and :291. A tidier symmetric default would change behaviour MRI specifies. Refs #916 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#921's first push dropped internal/vm to 98.8% and the coverage ratchet refused it. The uncovered lines were all branches no socket test can reach, and each is a behaviour worth asserting on its own rather than a coverage chore: - ftpTimeoutSeconds across every shape: nil (MRI's unbounded default), Integer, Float, and ⛔ the non-positive cases, which are UNBOUNDED and not "expire immediately" -- a deadline of now would fail every read at once -- plus a non-numeric, which is ignored rather than guessed at. - armRead's two early paths: an ftpObj with no connection yet, and a nil read_timeout that must CLEAR a deadline an earlier setting left behind. The second is asserted by showing the read goes back to BLOCKING, since a cleared deadline has no other visible effect. - ftpTimeoutErr returning for an error that is not a timeout. A version that raised unconditionally would turn every connection failure into Net::ReadTimeout, which is the opposite of what #916 needs to stay distinguishable. Refs #916 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3cd3b06 was meant to answer the ratchet and did not move the measurement: CI came back with the same two functions, ftpApplyOpts at 95.0% and ftpTimeoutErr at 80.0%. A fix that does not move the number is not a fix, so this time the number is measured before pushing -- both now read 100.0% under `go tool cover -func` over the FTP tests. What was actually uncovered is the defect shape this repository keeps repeating: a mechanism wired at only SOME of its doors. - ftpApplyOpts reads open_timeout and read_timeout from the options Hash on two adjacent lines. Every socket test passes read_timeout ALONE, and the accessor tests go through the attr writer, not the Hash -- so the open_timeout line had never run. The new test goes through the Hash door for BOTH, and asserts the other half of that contract too: an explicit `read_timeout: nil` is a REQUEST for no bound and must not fall back to MRI's 60 s default, while a key absent from the Hash must leave the field untouched. Those are the two cases the key-presence check exists to tell apart. - ftpTimeoutErr has two raises and the existing test returns before reaching either: it only asserts that a NON-timeout is passed through. The new test drives a real net.Error timeout through both phases and asserts the CLASS -- Net::OpenTimeout for open, Net::ReadTimeout otherwise -- because MRI distinguishes them and ruby/spec matches on the class, so a version that collapsed the two would still satisfy any check that only asked "did it raise?". The error is WRAPPED, as the standard library hands it to the callers, so the test fails if the classification is ever narrowed to a type assertion. Refs #916 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tannevaled
added a commit
that referenced
this pull request
Nov 13, 2026
…et (#924) * WIP: Net::FTP open_timeout/read_timeout (test still to add) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Net::FTP: the tests for the connect and read bounds The WIP commit below added open_timeout/read_timeout; this adds what it owed. A server that accepts and stays SILENT -- the production shape, and exactly what #916's SIGQUIT dump showed rbgo waiting on -- must raise Net::ReadTimeout. The connection is held OPEN on purpose: a server that closed it would give the reader an EOF, a different path (Net::FTPConnectionError), and the test would pass with no deadline at all. ⛔ THE ELAPSED TIME IS PART OF THE ASSERTION. Without it the test would also pass if the greeting read failed instantly for an unrelated reason, which is the one outcome that would NOT prove a deadline fired. It asserts the call took at least 150ms against a 0.2s read_timeout, and measures 0.21s. Control run in both directions: removing the single armRead() call makes that test HANG until the 40s wall instead of failing in 0.21s. The second test pins the defaults together -- open_timeout nil, read_timeout 60 -- because the ASYMMETRY is the point and is MRI's, from library/net-ftp/initialize_spec.rb:272 and :291. A tidier symmetric default would change behaviour MRI specifies. Refs #916 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WIP: Net::FTP releases the GVL around its blocking socket calls Measurement pending; see #916 for the mechanism. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Net::FTP: cover every branch the timeout helpers added #921's first push dropped internal/vm to 98.8% and the coverage ratchet refused it. The uncovered lines were all branches no socket test can reach, and each is a behaviour worth asserting on its own rather than a coverage chore: - ftpTimeoutSeconds across every shape: nil (MRI's unbounded default), Integer, Float, and ⛔ the non-positive cases, which are UNBOUNDED and not "expire immediately" -- a deadline of now would fail every read at once -- plus a non-numeric, which is ignored rather than guessed at. - armRead's two early paths: an ftpObj with no connection yet, and a nil read_timeout that must CLEAR a deadline an earlier setting left behind. The second is asserted by showing the read goes back to BLOCKING, since a cleared deadline has no other visible effect. - ftpTimeoutErr returning for an error that is not a timeout. A version that raised unconditionally would turn every connection failure into Net::ReadTimeout, which is the opposite of what #916 needs to stay distinguishable. Refs #916 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * A gate for the mistake this repository made eight times in one week rbgo emulates a GVL: one Ruby thread runs VM code at a time, and a thread that blocks in a native call MUST release it (ioBlock). Net::HTTP has done that since it was written. Net::FTP did it at NONE of its sites, so every read held the GVL across a blocking read(2). The cost was not slowness, it was a DEADLOCK. A Ruby program that serves itself -- a spec fixture, a test that binds a socket and connects to it, an embedder running a listener beside its own client -- has its server thread return from Accept and then park on the lock the client sits on. Dose-response: a 2s client deadline put the accept at 2.308s, 5s at 5.307s, ioBlock at 0.305s. In ruby/spec: 39 of 56 library/net-ftp files killed by the clock, none under MRI (#916). ⛔ THE POINT IS NOT THIS BUG, IT IS ITS SHAPE. Eight times in one week a mechanism existed and was wired at only some of its doors: Array.new vs Array#first, expand_path vs Dir.home, raiseWithIvars vs Kernel#raise, the vcall flag, Set's constructors, the stubbed-helper list in three surfaces, the README's derived numbers, and now ioBlock. "Is it implemented?" was always the wrong question; "who else should be calling it?" is answerable from the AST without running anything. What it found beyond net-ftp, on its first run: - THREE more sites in Net::FTP itself -- the DATA connection, which my own fix in #921 missed; - Net::IMAP, whose readLine/readLiteral are handed to the library as callbacks with no ioBlock anywhere: the same shape, untouched; - Net::SMTP's response reader; httprb and hanami's Accept. ⛔ And it corrected me twice while being written. I had said "Net::HTTP has four unwrapped sites": false. All six nethttp findings are helpers whose callers already wrap them, which a purely lexical rule cannot see. That exemption is NAMED, never inferred -- //gvl:released-by-caller <reason>, a reason required, so every waiver is one grep away. A gate that cries wolf is switched off within a week. It lands as a RATCHET, not a big bang: testdata/gvl-blocking-backlog.txt records what was already there, so the gate fires on what happens NEXT. The list may only shrink -- an entry that stops reporting fails too, because a stale allowance is where the next regression hides. Control run in both directions: removing the single ftpIO wrapper makes it name net_ftp_bind.go:249 immediately. Refs #916 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Net::FTP: the two doors the first coverage pass still had not opened 3cd3b06 was meant to answer the ratchet and did not move the measurement: CI came back with the same two functions, ftpApplyOpts at 95.0% and ftpTimeoutErr at 80.0%. A fix that does not move the number is not a fix, so this time the number is measured before pushing -- both now read 100.0% under `go tool cover -func` over the FTP tests. What was actually uncovered is the defect shape this repository keeps repeating: a mechanism wired at only SOME of its doors. - ftpApplyOpts reads open_timeout and read_timeout from the options Hash on two adjacent lines. Every socket test passes read_timeout ALONE, and the accessor tests go through the attr writer, not the Hash -- so the open_timeout line had never run. The new test goes through the Hash door for BOTH, and asserts the other half of that contract too: an explicit `read_timeout: nil` is a REQUEST for no bound and must not fall back to MRI's 60 s default, while a key absent from the Hash must leave the field untouched. Those are the two cases the key-presence check exists to tell apart. - ftpTimeoutErr has two raises and the existing test returns before reaching either: it only asserts that a NON-timeout is passed through. The new test drives a real net.Error timeout through both phases and asserts the CLASS -- Net::OpenTimeout for open, Net::ReadTimeout otherwise -- because MRI distinguishes them and ruby/spec matches on the class, so a version that collapsed the two would still satisfy any check that only asked "did it raise?". The error is WRAPPED, as the standard library hands it to the callers, so the test fails if the classification is ever narrowed to a type assertion. Refs #916 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * The gate had the defect it was written to catch, and a waiver I wrote was false Two findings, both about this gate rather than about a binding, and both found by pulling on the one thread the gate could not see. 1. ⛔ THE GATE WAS BLIND TO conn.Read -- the commonest blocking socket call of all. It caught retrlines at net_ftp_bind.go:485 because that reads through bufio's ReadString, and MISSED retrbinary nineteen lines below it, reading the same data connection through conn.Read. Same connection, same defect, invisible because only one of the two spellings was in the list. The gate was wired at some of its doors -- which is the exact defect shape it exists to refuse. Read is now checked, but only in files that import "net": a Read on a bytes.Reader, a file or a protocol decoder is not a socket call, and a gate that cried wolf on those would be switched off within a week. On this tree the rule is exact for Read -- the six non-socket Read sites (crypto/rand, mail, images, openbao, grpc, sftp) are all in files importing no net, and the restriction leaves exactly two, both real. The gate now checks 33 calls where it checked 27. ⛔ WRITE IS DELIBERATELY ABSENT, and the measurement is the reason. I added it first and claimed in a comment that the file rule was exact for it too. It is NOT: Write reported eleven sites, EIGHT of them not network writes at all -- seven *bufio.Writer.Write, which only buffers (the socket write happens at Flush), and one bytes.Buffer in a file that does import net. Separating those needs the receiver's TYPE, not its file. Three real unwrapped conn writes do exist; they are written down as a COMMENT at the foot of the backlog, not as entries, because an entry that never reports fails this gate by design. The gap is on the page instead of in somebody's memory. 2. ⛔ A WAIVER I WROTE IN THIS BRANCH WAS FALSE. nethttpRawDial carried //gvl:released-by-caller every call site is inside the ioBlock at nethttpDial, so the GVL is already dropped when this runs There is no function called nethttpDial. The only ioBlock in nethttp_bind.go is in nethttpExchangeFramed, and the dial path does not go through it: nethttpRawExchange called nethttpDialXfer directly. So the whole dial -- the TCP connect, a proxy CONNECT (write, then read the reply a byte at a time) and the TLS handshake -- held the emulated GVL, stopping every other Ruby thread for the duration of a connect to an unreachable host. The slowest part of a request was the unwrapped part. A bare waiver is a silent one; a waiver with a reason that cannot be followed to a real release point is WORSE, because it reads as verified and nobody re-checks it. Fixed where it belongs: nethttpRawExchange now runs nethttpDialXfer inside an ioBlock, which covers dial, CONNECT and handshake at once. The two other waivers in that file name nethttpExchangeFramed and are correct. nethttpProxyConnect gets a waiver of its own, true as of this commit, and the negative control confirms the gate names nethttp_bind.go:646 the moment it is removed. Measured: gate green at 33 calls over 542 files; 127 tests pass, including the whole Net::HTTP suite with the dial now wrapped. Refs #916 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
tannevaled
added a commit
that referenced
this pull request
Nov 13, 2026
…changed (#926) The "a deadline belongs to the engine's API" section is the worked example for the whole shared-engine argument, and it described the binding in the past tense as having no timeout and no read deadline "anywhere". That was true when it was written and is not true now: #921 is merged. A reader arriving at that page would conclude rbgo still has neither. The argument is unchanged -- an unbounded read in an engine four languages share is still a shared denial of service, and the neutral engine should still take a deadline in its own API. What changed is the consumer's half, so that is what the new subsection records: - open_timeout (default nil, unbounded) and read_timeout (default 60), from the options Hash and from the attr writers, with the deadline armed PER READ. Non-positive means no bound, as in MRI. - the GVL released around the control connection, plus an AST gate that refuses a new blocking socket call holding it (#924), with the dose-response that proved the mechanism. ⛔ And the honest limit, which is the reason the extraction warning STAYS: the measurement did not move. The library/net-ftp specs are byte-identical after both changes -- still 39 killed, 459/17, -442. Both fixes are necessary and proved on a minimal reproduction, but at least one cause remains and it is in the spec-harness path. The three candidates eliminated so far are named, with how they were eliminated (by reading, not by timing, since the machine was too loaded for a timeout to mean anything). Writing "fixed" here while the number sits where it was is exactly how a docs page stops being evidence. Refs #916 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
tannevaled
added a commit
that referenced
this pull request
Nov 13, 2026
…hrinks (#927) #921 bounded the control connection and #924 released the GVL around it. Both left the DATA connection holding it, and the gate #924 added said so in its seeded backlog -- four entries I wrote down rather than fixed, so the gate could land without a sprawling change. This is the change. dialData, acceptData, sendData, retrlines and retrbinary now run their blocking call inside ftpIO, and the four entries are DELETED from testdata/gvl-blocking-backlog.txt. That list may only shrink, and an entry that stops reporting fails the gate too, so a fix cannot leave its allowance behind. ⛔ acceptData is the one #916 is actually about. In ACTIVE mode the client listens and the server dials back, so a Ruby program that serves itself has both ends in one process -- and holding the GVL in that Accept parks every other Ruby thread, INCLUDING the one that would make the call it is waiting for. ruby/spec's net-ftp fixtures are exactly that shape, and their spec_helper.rb puts Net::FTP into active mode (see #916's comment: rbgo has no Net::FTP.default_passive, so today that helper is skipped in silence and the suite never reaches this path at all). Only the blocking call goes inside each closure. ftpConnErr raises, and a panic crossing a released GVL would unwind without re-acquiring it, so every raise stays outside. ⛔ NO BEHAVIOURAL TEST, AND THE REASON IS NOT LAZINESS. The in-repo FTP test server is Go-side, so it cannot reproduce this deadlock at all: nothing about rbgo's emulated GVL blocks a Go goroutine, and a test built on it would pass whether or not the fix is present. A real regression test needs BOTH ends in Ruby -- a dummy FTP server in a Ruby Thread, which is what ruby/spec's fixture is -- and that is worth doing properly rather than approximating with something that looks like proof and is not. What does guard the property is the gate: TestEveryBlockingSocketCallReleasesTheGVL fails in about a second if any of these five is unwrapped again, and the backlog deletion means it also fails if one silently stops being reported. The measurement that would show this moving -- the library/net-ftp sweep -- is deferred: this machine has been carrying a 1-minute load average between 23 and 86, and a per-file timeout measured under that cannot tell a hang from a slow file. Refs #916 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
tannevaled
added a commit
that referenced
this pull request
Nov 13, 2026
#928) * Net::FTP: the DATA connection releases the GVL too, and the backlog shrinks #921 bounded the control connection and #924 released the GVL around it. Both left the DATA connection holding it, and the gate #924 added said so in its seeded backlog -- four entries I wrote down rather than fixed, so the gate could land without a sprawling change. This is the change. dialData, acceptData, sendData, retrlines and retrbinary now run their blocking call inside ftpIO, and the four entries are DELETED from testdata/gvl-blocking-backlog.txt. That list may only shrink, and an entry that stops reporting fails the gate too, so a fix cannot leave its allowance behind. ⛔ acceptData is the one #916 is actually about. In ACTIVE mode the client listens and the server dials back, so a Ruby program that serves itself has both ends in one process -- and holding the GVL in that Accept parks every other Ruby thread, INCLUDING the one that would make the call it is waiting for. ruby/spec's net-ftp fixtures are exactly that shape, and their spec_helper.rb puts Net::FTP into active mode (see #916's comment: rbgo has no Net::FTP.default_passive, so today that helper is skipped in silence and the suite never reaches this path at all). Only the blocking call goes inside each closure. ftpConnErr raises, and a panic crossing a released GVL would unwind without re-acquiring it, so every raise stays outside. ⛔ NO BEHAVIOURAL TEST, AND THE REASON IS NOT LAZINESS. The in-repo FTP test server is Go-side, so it cannot reproduce this deadlock at all: nothing about rbgo's emulated GVL blocks a Go goroutine, and a test built on it would pass whether or not the fix is present. A real regression test needs BOTH ends in Ruby -- a dummy FTP server in a Ruby Thread, which is what ruby/spec's fixture is -- and that is worth doing properly rather than approximating with something that looks like proof and is not. What does guard the property is the gate: TestEveryBlockingSocketCallReleasesTheGVL fails in about a second if any of these five is unwrapped again, and the backlog deletion means it also fails if one silently stops being reported. The measurement that would show this moving -- the library/net-ftp sweep -- is deferred: this machine has been carrying a 1-minute load average between 23 and 86, and a per-file timeout measured under that cannot tell a hang from a slow file. Refs #916 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Net::FTP.default_passive, and the options a nil host was throwing away ruby/spec's library/net-ftp/spec_helper.rb — which all 56 files require — opens with: if defined?(Net::FTP.default_passive) Net::FTP.default_passive = false end rbgo had no such accessor, so the guard was skipped in SILENCE and the whole suite ran against a connection rbgo kept in PASSIVE mode, while the fixture server implements PORT/EPRT and has no `pasv` method at all. That is the `451 Exception: undefined method 'pasv'` those specs report (#916). Implemented against the reference, not against expectations: net-ftp 0.3.9 lib/net/ftp.rb:96 (`@@default_passive = true`), :159-167 (the accessors) and :286-290 (`if options[:passive].nil?` → the default). The value lives in the CLASS VARIABLE MRI uses, so Net::FTP.class_variable_get(:@@default_passive) answers here as it does there and there is one place holding it, not two that can disagree. Two defects the reference comparison turned up that reading our own code would not have: - ⛔ `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 — binary, timeouts, credentials, all of it. MRI applies the options first and connects separately, and ruby/spec drives exactly that shape: `@ftp.send(:initialize, nil, options)` then asserts `passive` (initialize_spec.rb:201-226). A mechanism wired at only some of its doors, again. - ⛔ `passive: nil` meant FALSE. ftpApplyOpts tested the KEY's presence, but MRI tests the VALUE: nil is "no opinion" and falls back to the default. Note this is the OPPOSITE of read_timeout in the same Hash three lines away, where nil IS a request (for no bound) — easy to conflate, so both now say which they are in a comment. Tests: a 12-line table run through rbgo, each line annotated with the reference line it comes from, plus ftpDefaultPassive's fallbacks (nil class, absent or nil class variable → true), which no Ruby program can reach and which decide the transfer mode of every connection. ⛔ Measured before pushing with the tool added in #925, which caught my own gap: `covratchet -require ftpDefaultPassive,ftpApplyOpts,ftpInit` first reported ftpDefaultPassive at 66.7%, and reports all three at 100% now. Refs #916 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Net::FTPdialled withnet.Dialand set no read deadline anywhere, so a peer that accepted the connection and then said nothing held the interpreter for ever.Measured 2026-11-10: 39 of 56
library/net-ftpruby/spec files had to be killed by the clock under rbgo, and none under MRI (#916). ASIGQUITgoroutine dump put the process innet.(*conn).Readbeneathgo-ruby-net-ftp'sGetMultiline, waiting for the 220 greeting.That makes it a security defect and not only a conformance one: an unbounded read on a socket the peer controls is a denial of service needing no exploit — only silence.
⛔ The defaults are not symmetric, and the asymmetry is MRI's
Taken from the executable spec rather than guessed —
library/net-ftp/initialize_spec.rbpinsopen_timeoutto nil (:272) andread_timeoutto 60 (:291). MRI leaves the connect unbounded and bounds every read. A tidier symmetric default would change behaviour MRI specifies.net.DialTimeoutdriven byopen_timeout;DialTimeout(0)is Go's spelling of "unbounded", so the default path is unchanged.SetReadDeadlinebefore every read, not once at connect: a deadline is absolute, so one set at connect would expire mid-session and turn every later read into a timeout.open_timeout/read_timeoutand their writers. The setter arms at once, becauseNet::FTP#closelowersread_timeoutbefore the QUIT exchange precisely so a wedged server cannot hold the close.Net::OpenTimeout/Net::ReadTimeout, following the shapenethttp_bind.go'sraiseTransportErralready used. The HTTP binding has bounded its reads all along; this one simply never got the same treatment.Tests, with the control run in both directions
A server that accepts and stays silent — the production shape, and exactly what the SIGQUIT dump showed rbgo waiting on — must raise
Net::ReadTimeout. The connection is held open on purpose: a server that closed it would give the reader an EOF, a different path, and the test would pass with no deadline at all.The elapsed time is part of the assertion. Without it the test would also pass if the greeting read failed instantly for an unrelated reason — the one outcome that would not prove a deadline fired. It requires ≥150 ms against a 0.2 s
read_timeout, and measures 0.21 s.Control: removing the single
armRead()call makes that test hang until the 40 s wall instead of failing in 0.21 s.⛔ What this does not do
It bounds the hang; it does not make the net-ftp specs pass. After the fix the connect and login complete and the block moves to
voidcmd, so rbgo still sends something the server does not honour. #916 stays open.Refs #916
🤖 Generated with Claude Code