Fix a crash on short response lines and a hang on mid-reply hangup - #4
Open
onslaughtq wants to merge 3 commits into
Open
Fix a crash on short response lines and a hang on mid-reply hangup#4onslaughtq wants to merge 3 commits into
onslaughtq wants to merge 3 commits into
Conversation
getResponse called C.head on the remainder after the three digit code, so any response line shorter than four bytes died with "Prelude.head: empty list" instead of an FTPException. A bare "200", or the empty line a closed socket yields, both hit this. Validate the three digit code up front and match on the remainder with C.uncons, which also makes the later read of the code total. loopMultiLine never terminated when the peer hung up mid response, since no line ever matched the closing code. It now stops at end of input. End of input and a blank line are deliberately kept distinct. recvLine signals end of input by throwing, while an empty ByteString is legitimate reply text -- RFC 959 lets the intermediate lines of a multiline reply carry arbitrary text. Collapsing the two would truncate any reply containing a blank line and leave its real terminator unread, so every later command would pick up the previous reply.
Both changes on this branch are bug fixes with no API change, so PVP asks for a patch bump.
There was a problem hiding this comment.
Pull request overview
Fixes FTP response parsing failures and incomplete multiline replies.
Changes:
- Validates three-digit response codes without partial operations.
- Distinguishes blank lines from EOF in multiline responses.
- Adds regression tests, changelog notes, and bumps version to 0.5.1.8.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ftp-client/src/Network/FTP/Client.hs |
Updates response parsing and EOF handling. |
ftp-client/test/test.hs |
Adds response parsing regression tests. |
ftp-client/package.yaml |
Bumps the package version. |
ftp-client/ftp-client.cabal |
Synchronizes the generated package version. |
ftp-client/CHANGELOG.md |
Documents the fixes. |
Suppressed comments (1)
ftp-client/src/Network/FTP/Client.hs:297
- This treats any intermediate line whose first three bytes equal the reply code as the terminator, although a multiline reply closes only when that code is followed by a space. For example, an intermediate
220-not a terminatorwould end the response here and leave the real terminator buffered for the next command. Check the required separator as well.
if nextCode == code
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Terminating instead of looping forever was only half of the fix. Returning the lines collected so far handed back a fragment as though it were a whole reply, so a greeting cut off after "220-" produced a well formed FTPResponse with code 220 and status Success, and withFTP carried on against a control connection that was already gone. End of input mid reply now raises BadProtocolResponseException carrying what did arrive. The distinction that motivated the original fix is unchanged: end of input is not a blank line. RFC 959 lets the intermediate lines of a multiline reply hold arbitrary text, blank lines included, so a blank line still has to be kept and the loop still has to continue past it. The existing test asserted the old behaviour -- it expected exactly the Success 220 that this commit rejects -- so it becomes a shouldThrow rather than a new case beside it. Reported by Copilot on #4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
onslaughtq
marked this pull request as ready for review
August 25, 2026 01:04
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.
Two bugs in
getResponse.C.headwas called on the bytes after the three digit code, so any reply line shorter than four bytes died withPrelude.head: empty listinstead of anFTPException. A bare200does it, as does the empty line a closed socket yields. The code is now checked up front and the remainder matched withC.uncons, which also makes the laterreadtotal.loopMultiLinenever terminated when the peer hung up part way through a multiline reply, because no line ever matched the closing code.The second fix is fussier than it looks, on purpose.
recvLinesignals end of input by throwing, while an emptyByteStringis ordinary reply text — RFC 959 lets the intermediate lines of a multiline reply carry anything, blank lines included. Treating a blank line as the end would cut the reply short and leave its real terminator unread, so the next command would read the previous reply and everything after that would be off by one. The two cases are kept apart.8 hspec examples, up from 3. The blank line test fails against the old code.
Stacked:
add-ci-multi-ghc→fix-response-line-partiality→pr-2-multiline-and-handle-exports→add-ci-flag→add-henforcer-fourmolu→fix-audit-findings. Merge bottom-up.