Notice when a control-connection write fails - #105
Conversation
Every write to the control connection discarded the send() return value. A reply that never reached the client was not logged, not retried and not noticed -- the session carried on until some later recv() happened to fail. The trace line was written unconditionally too, so the log claimed a reply had gone out when it had not, which is misleading precisely when reconstructing a failed session. Rather than touch ~200 call sites, the four places that write to the control socket now funnel through one ftpd_session_send(): it completes short writes, reports a failure once per session, sets ctrl_dead and moves the session to SESS_CLOSING. Callers may keep ignoring the return value and still terminate correctly, because the command loop ends on its next iteration. ctrl_dead is tracked separately from the session state on purpose. A handler that assigns its own state after a failed reply would otherwise clear SESS_CLOSING again -- the 220 greeting did exactly that, so it now only advances the state when the write succeeded, and the loop tests the flag as well. Deliberately no retry and no interpretation of transport-level return codes here. The control connection is a blocking socket (FIONBIO is set only on the listener), so waiting out a full send buffer belongs in the C library; anything negative reaching FTPD means the peer is gone. See mvslovers/libc370#120, which has to land before this behaves correctly on an emulator whose SEND can report "would block". Also stops ftpd_data_send() from logging errno for a non-negative failure return, where it only prints a leftover value from an earlier call. Fixes #104
|
Live auf MVS getestet (mvsdev, Build Der neue Pfad feuert — und genau einmal pro Session: Drei Läufe, drei Meldungen. Das ist der eigentliche Punkt: Jeder Lauf hatte 36 Kommandos vorab gepipelined, deren Antworten alle in den toten Socket geschrieben worden wären. Ohne den Testaufbau: einloggen, 36 Kommandos (FEAT/SYST/PWD im Wechsel) auf einmal absetzen, ohne eine Antwort zu lesen, dann die Verbindung mit Nebenbei bestätigt das die Analyse aus #104: Ein erster Versuch mit 0,2 s Pause vor dem Abriss erzeugte keine Fehlermeldung — die kleine Antwort ging noch in den Sendepuffer, bevor das RST ankam. Der Fehlerfall braucht wirklich einen Schreibversuch nach dem Abriss. Keine Regression, keine Leichen:
Hinweis für Nachtestende: Unverändert gilt: Merge/Deployment erst nach mvslovers/libc370#120, siehe PR-Beschreibung. |
|
Korrektur zu meinem vorigen Kommentar: die Warnung „erst nach libc370#120 deployen" ist gegenstandslos — #120 ist längst drin. libc370 Der Live-Test lief damit nicht in einem Zwischenzustand, sondern in der vollständigen Zielkonfiguration: gepatchtes Hercules (SEND non-blocking), libc370 mit Zur Vollständigkeit, was der Test damit abgedeckt hat und was nicht:
Aus meiner Sicht steht dem Merge damit nichts mehr im Weg. |
1 participant
Fixes #104
Approach: one sink, not 200 call sites
ftpd_session_reply()alone has ~197 callers across eight files. Rewriting them would be a large diff with real regression risk and no structural guarantee — the next unchecked call reintroduces the problem. Instead, the four places that actually write to the control socket (ftpd_session_reply(), both writes inftpd_session_reply_multi(), andcmd_feat()) now funnel through a newftpd_session_send(), which:ctrl_deadand moves the session toSESS_CLOSING.Callers may keep ignoring the return value and still terminate correctly — the command loop ends on its next iteration.
ftpd_session_reply()andftpd_session_reply_multi()now returnint; that is source-compatible, since C permits discarding a return value, and the build confirms no call site needed touching.The trace line moved behind the success check: a reply that never reached the client no longer appears in the trace as if it had.
Why
ctrl_deadis separate from the session stateA handler that assigns its own state after a failed reply would clear
SESS_CLOSINGagain. That is not hypothetical — the 220 greeting did exactly that (ftpd_session_reply(...)followed unconditionally bysess->state = SESS_AUTH_USER;). The greeting now only advances the state when the write succeeded, and the command loop tests the flag as well, so the guarantee does not depend on all ~200 handlers behaving.Deliberately not done here
No retry, and no interpretation of transport-level return codes. The control connection is a blocking socket —
FIONBIOis set only on the listening socket, for the accept poll (src/ftpd.c:418) — so waiting out a temporarily full send buffer belongs in the C library, not here. Anything negative that reaches FTPD means the peer is gone.That split is the coordination agreed in mvslovers/libc370#120: per socket type, exactly one layer waits. libc370#120 should land before this is deployed. Until it does, a patched Hercules can return a "would block" code straight through to FTPD, and this change would then close a session on a transiently full buffer. The window is small (control replies are under 512 bytes) and only exists on a system with a patched emulator and an unpatched libc370.
Also fixed
ftpd_data_send()(src/ftpd#dat.c) loggederrnofor every non-positive return, thougherrnoonly carries a diagnosis whensend()itself failed. For any other value it printed a leftover from an earlier call that reads like a real cause. It now logs the actual return code and the byte counts, and only reportserrnowhen it means something.Verification
makeclean (cc370,-Wall -Werror),make test-host72 assertions pass (TSTADR 29, TSTDSN 43).control write failed ... closing sessionin the log, the session ending promptly, and the worker returning to the pool.