Repository navigation
Conversation
PORT created a socket in env->data_fd, but ftp_data_open() closed env->data_fd first and then called connect() on the closed descriptor, so every transfer after PORT failed with EBADF (curl: "Uploaded unaligned file size (0 out of N bytes)", "The server failed to connect to data port"), and STOR left an empty file behind. If another thread had reused the descriptor number in the meantime, connect() would even have operated on that thread's descriptor. Create the socket in ftp_data_open() right before connecting to the address given by PORT, and close the data connection (resetting env->data_fd) when connect() or setsockopt() fails, so no half-open descriptor is left behind. ftp_data_close() now also resets env->data_fd when close() reports an error, so the descriptor is never closed twice. Co-Authored-By: Claude Opus 5.5 <[email protected]>
ftp_data_open() uses the address given by PORT whenever one was ever set, so after a PORT command a later PASV was ignored: the server tried to connect to the old PORT address instead of accepting the connection the client opened to the advertised passive port. PASV now forgets the PORT address, and PORT closes the listening socket of a previous PASV instead of leaving it open until the next PASV or the end of the session. The error paths of PASV closed env->passive_fd without resetting it, so the next PASV (or the end of the session) closed that descriptor number again, possibly after another thread had reused it; reset it to -1. Co-Authored-By: Claude Opus 5.5 <[email protected]>
ftp_data_open() passed an uninitialized socklen_t to accept() for passive mode connections. accept() reads it as the size of the address buffer, so depending on what was left on the stack the call could fail with EINVAL (a value that is negative as an int) or report a truncated peer address. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The receive loop stored the result of recv() in a size_t, so an error
(-1, e.g. ECONNRESET when the client resets the data connection) became
SIZE_MAX, passed the "> 0" test and was handed to io_nwrite() as the
number of bytes to write from the 1 MiB receive buffer. Linux rejects
that length with EFAULT, so the client got "550 Bad address" instead of
the real error, and the code relied on the kernel refusing a write far
past the end of the buffer.
Keep the result signed and report the recv() error itself
("550 Connection reset by peer").
Co-Authored-By: Claude Opus 5.5 <[email protected]>
RETR only reset env->data_offset after a successful transfer. When it
failed (e.g. "550 No such file or directory"), the offset given by REST
was kept and used by the next STOR, which then wrote its data after
that many zero bytes ("REST 5", failed RETR, STOR "hello" created
"\0\0\0\0\0hello"). Take the offset at the start of RETR, as STOR
already does.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Add a writer that frames data as RFC 959 block mode blocks and a reader that decodes them, for the block transfer mode added next: - io_block_write() gathers data in a caller-supplied buffer and sends it in blocks of at most 65535 bytes, one write per block; io_block_write_eof() sends the remaining data and the EOF block (40 00 00) with a single write. - io_block_read() returns file data, 0 once the EOF block was read, or an error. It handles headers and data split across recv() calls in any way, accepts EOF on a zero-length block or on the last data block, ignores EOR and suspected-error flags and zero-length blocks, discards restart marker data, rejects descriptors with bits outside 0xF0 (EBADMSG), reports a connection closed before the EOF block as an error (EPIPE), and never reads past the EOF block, since the next bytes on a kept data connection belong to the next transfer. Co-Authored-By: Claude Opus 5.5 <[email protected]>
In stream mode, closing the data connection is what ends a file, so every transfer needs a new PASV/PORT and a new TCP connection. In block mode (RFC 959 section 3.4.2) the end of a file is an EOF block, so the data connection can stay open and be reused: a client uploading thousands of small files sets it up once instead of once per file. - MODE S replies "200 Mode set to S." and closes a kept data connection; MODE B replies "200 Mode set to B."; any other argument gets "504 Unsupported transfer mode." and no argument "501 Usage: MODE <S|B>". - PASV and PORT close a kept data connection and mark a new one as pending. In block mode, a transfer command opens a pending connection and replies 150, reuses the kept connection and replies "125 Data connection already open; transfer starting.", or, with neither, replies "425 Use PORT or PASV first." before any file is opened. - STOR, APPE, RETR (with REST offsets and SELF to ELF conversion; the multi-line preliminary reply keeps one code, 150 or 125), LIST and MLSD frame their data as blocks. A successful transfer replies "250 Transfer complete." and keeps the connection. A failure during the transfer closes it: "426 Connection closed; transfer aborted." for a connection closed before the EOF block, an invalid descriptor or a send/receive error, and the usual 550 for local file errors. An upload that fails keeps the data received so far, truncating the file there. Errors found before the transfer starts (no such file, ...) do not touch the data connection. - Data connections opened in block mode use TCP_NODELAY, so the last block of a transfer is not held back waiting for the acknowledgment of earlier data on the long-lived connection. Stream mode keeps its behaviour, replies and copy paths. Co-Authored-By: Claude Opus 5.5 <[email protected]>
test/test-mode-b.py (Python 3.9+ standard library only) starts a server binary on a free port, or targets a running one, and checks: - MODE S/B/other/no-argument replies; - many uploads over one PASV (0 B to 6 MiB, around the 65535-byte block size), 150 then 125 replies, byte-identical files; - 425 without PASV/PORT or a kept connection, and no file created; - APPE, REST+STOR, and resuming an upload cut short with APPE; - headers and data split across reads down to one byte at a time, EOF on a data block, EOR/suspected-error flags, empty blocks, restart marker data discarded, and nothing read past the EOF block; - truncated uploads (426, data received so far kept, next PASV works), invalid descriptors, and local read/write errors closing the connection while earlier errors leave it open; - RETR with REST, empty files, LIST and MLSD (also larger than a block) matching stream mode, and SELF to ELF conversion on and off with the 125/150 multi-line preliminary reply; - MODE S, PASV and PORT closing the kept connection, active mode in both modes, PASV after PORT and PORT after PASV, and stream mode replies. --bench N times N small uploads in stream and in block mode. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Describe block mode with a persistent data connection under Features, fix the Linux build command (the makefile is Makefile.linux, not Makefile.posix) and show how to run the block mode tests. Co-Authored-By: Claude Opus 5.5 <[email protected]>
LIST opened a relative path with opendir() as is, that is relative to the working directory of the server process, while it built the paths it passed to stat() from the FTP working directory. After "CWD /data", "LIST sub" therefore answered "550 No such file or directory" (or listed another directory) instead of listing /data/sub. Resolve the argument with ftp_abspath() like the other commands do. MLSD is not affected: it always lists the FTP working directory. Co-Authored-By: Claude Opus 5.5 <[email protected]>
APPE looked up the size of the file to append to and answered "550 No such file or directory" when it did not exist, while RFC 959 says the file shall be created in that case (curl --append of a new file failed). Treat ENOENT as an empty file, so STOR creates it; other stat() errors are still reported, and an offset given by REST is still used as is. Co-Authored-By: Claude Opus 5.5 <[email protected]>
NLST was answered "502 Command not recognized". Implement it as in RFC 959 and RFC 3659 section 2.2: "NLST [path]" sends one name per line (CRLF) over the data connection, the bare names of the entries of a directory (without "." and "..", with names starting with a dot), or the path as given when it names a file. Relative paths are resolved against the FTP working directory, ls-style options such as -a are skipped, an empty directory gives an empty listing and a success reply, and a missing path is answered 550 before any preliminary reply. Like LIST, it replies 150 and 226 in stream mode, and uses block framing and the 150/125/250/425/426 replies in block mode. A failure to send the names is answered 426 in both modes. Clients can use it to learn which files exist in a directory with a single transfer, instead of one SIZE command per file. Co-Authored-By: Claude Opus 5.5 <[email protected]>
- nlst: names only with CRLF, dot files without "." and "..", -a and other options skipped, relative and absolute directories, a file path given back as is, UTF-8 names and names with spaces, an empty directory (an empty listing, a lone EOF block in block mode), a missing path answered 550 without a preliminary reply and without touching a kept connection, the same output in stream and block mode (150, then 125), and active mode. - nlst_curl: curl -l with the multicwd, nocwd and singlecwd methods, in passive and active mode, including a directory whose names take more than one 65535-byte block (skipped when curl is not installed). - list_many_entries: NLST larger than one block in both modes. - appe_new_file: APPE creates a missing file in both modes, REST still sets the offset, and other errors are still answered 550. - no_data_connection: NLST, and APPE of a missing file, get 425 in block mode without a data connection, and no file is created. - list_relative_path: LIST of a path relative to the FTP working directory in both modes. The runner reports skipped tests. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Author
|
Sorry, mistake. |
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.
No description provided.