Fix: FTP server: send 226 after closing the data connection

[pnfd]

Description

This PR fixes a critical state-machine issue where the NetX Duo FTP server sends the unconventional FTP reply code (250) after closing a data connection, instead of the RFC 959 compliant code (226).

Impact

Standard-compliant FTP clients (e.g., standard Yocto FTP clients, Rust FTP clients) hang indefinitely after downloading a file, uploading a file, or requesting a directory listing. The clients detect that the data connection was closed but wait endlessly for the 226 Transfer complete code on the control channel to properly finalize the transaction.

Root Cause

According to RFC 959:

• 250 Requested file action okay, completed should only be used when the command does not involve closing a data connection (e.g., RNTO, DELE).

• 226 Closing data connection must be sent to indicate that a data transfer (like RETR, STOR, LIST, NLST) has concluded successfully and the data port was closed.

In nxd_ftp_server.c, the macro NX_FTP_CODE_COMPLETED ("250") is currently hardcoded for all successful operations, including those that close the data socket (lines processing RETR, STOR, LIST, and NLST).

Wireshark traces clearly show the following sequence:

1. Data transferred via the data channel.
2. Server sends TCP [FIN, ACK] on the data channel (closing it correctly).
3. Server sends 250 File Sent / 250 List End on the control channel instead of 226.

Evidence of Protocol Violation

1. Application Layer (Control Channel)
The server incorrectly replies with 250 instead of 226 after the transfer is done:

227 Entering Passive Mode (10,0,0,27,192,5).
---> LIST
125 Sending List 
drw-rw-rw-  1 owner group          0 JAN 01 00:00 testfolder
-rw-rw-rw-  1 owner group         12 JAN 01 00:00 testdata.txt
250 List End

The Wireshark capture of the corresponding data port shows that the server (10.0.0.27) explicitly closes the socket by sending a [FIN, ACK] (Packet 201), proving that a 226 response was required:

Solution implemented

link macro NX_FTP_CODE_CLOSING_DATA correcly if a connection cleanup occurs previously

Replaced the incorrect NX_FTP_CODE_COMPLETED macro in nxd_ftp_server.c for the RETR, STOR, LIST, and NLST command processing blocks.

Operations without data channels (like DELE, RNTO, CWD) correctly retain the 250 response code.

This fully restores interoperability with strict FTP clients.
Greetings David

[fdesbiens]

Welcome, and thank you for a well-evidenced first contribution — the Wireshark capture showing the FIN/ACK on the data channel followed by 250 on the control channel is exactly the right way to make this case, and it made the review quick.

Two things I checked and can confirm in your favour:

The site selection is exactly right. After your change the remaining NX_FTP_CODE_COMPLETED uses are "File Renamed" (RNTO, :2359), "File Deleted" (DELE, :2427), "Directory Deleted" (RMD, :2501) and "set successfully" (CWD, :4064) — all commands with no data connection, all correctly keeping 250. So your claim that non-data operations retain 250 holds, and you did not miss a fifth data-transfer site.

No NetX-to-NetX regression. I was worried the change might break our own FTP client, but it validates control replies by first digit almost everywhere — if (buffer_ptr[0] != '2') at :1288, :1509, :2459, :2614, :2832, :3280, :3924, :4853 and :5484 in nxd_ftp_client.c. The two stricter 2 2 checks are both inside _nxd_ftp_client_connect() and apply to the 220 greeting, not to a post-transfer reply. So the client accepts 226 and 250 equally.

Now the blocking item. netx_ftp_commands_replys_test will fail. It matches the literal reply strings, including two you are changing, and then treats any unset flag as an error. Details on :2015 — the fix is a couple of lines in the test.

Beyond that, two things I would like changed and one nit:

The macro is misnamed and using it here will mislead people. NX_FTP_CODE_LOGOFF does expand to "226", so your change is functionally correct — but 226 is "Closing data connection", not a logoff, and reading NX_FTP_CODE_LOGOFF, "File Sent" looks wrong at a glance. See :2015.

The RFC framing is stronger than the RFC supports. This is worth getting right because the PR title goes into the history. RFC 959's reply listing for RETR, STOR, LIST and NLST includes both 226 and 250 as valid success replies, so the current code is permitted by the letter of the spec even though it is unconventional. Your change is still the right one — 226 is what real clients expect, and the hangs you observed are a genuine interoperability defect — but "non-compliance" and "protocol violation" overstate it. Suggested reframing in my comments. Worth a second pair of eyes on the RFC text before we settle the wording.

One stale comment your change leaves behind, on :4597.

Nothing here is a criticism of the diagnosis, which is sound. Thanks again for chasing it down to the packet level.

[fdesbiens]

test/regression/ftp_test/netx_ftp_commands_replys_test.c intercepts the server's control replies and matches them as literal strings, including two of the four you are changing:

    else if(!memcmp(message,"250 File Sent",13))
        reply_counter[3] = NX_TRUE;
    else if(!memcmp(message,"250 File Written ",16))
        reply_counter[4] = NX_TRUE;

That is :407-410. And at :302-303 the test treats any flag that was never set as a failure:

        if((command_counter[i] != NX_TRUE) || (reply_counter[i] != NX_TRUE))
            error_counter++;

With your change the server sends "226 File Sent" and "226 File Written", so reply_counter[3] and reply_counter[4] stay false and error_counter is incremented twice. The test fails.

The fix is to update those two comparisons to "226 ..." with the corresponding lengths — note the length constants are hand-written per string, so 13 and 16 both need rechecking against the new text, and the trailing space in "250 File Written " needs the same treatment.

I checked the other three tests that mention these codes so you do not have to:

• netx_ftp_establish_data_connection_08_test.c:308 already accepts either — if((!memcmp(message,"250",3)) || (!memcmp(message,"226",3))) — so it passes either way. Whoever wrote that anticipated this.
• netx_ftp_access_control_commands_04_test.c:347-350 asserts "250 ... set successfully", which is CWD and keeps 250, so it is unaffected.
• netx_ftp_service_commands_file_write_test.c:276 only has a comment saying the client should have received a 250; the check itself is status-based, so it still passes, but please update the comment while you are there.

Since your PR has no test-plan section, I am guessing the FTP suite was not run — worth doing before the next push. It builds and runs quickly with CMake and Ninja under test/cmake/.

[pnfd]

Last but not least, I updated the regression test (netx_ftp_commands_replys_test.c) to expect the 226 reply code for data connection operations. Verified locally with the following result:

❯ ./test/cmake/netxduo/build/netxduo/regression/netx_ftp_commands_replys_test
(c) 2024 Microsoft Corp. (c) 2026-present Eclipse ThreadX Contributors.  *  NetX Duo Linux/GNU Version 6.5.1.202602 *
IPv6 is built-in.
Tunnel is not built-in.
IP structure size: 4424
TCP control block size: 316
ARP table entry size: 52, ARP table size 1664
Packet structure size: 60
NetX Test:   FTP Reply Standard and Minimum Implementation Test........SUCCESS!

Over and Out. David

[fdesbiens]

To be clear, the value is correct — nxd_ftp_server.c:67 is #define NX_FTP_CODE_LOGOFF "226" /* Closing data connection. */, so what goes on the wire is exactly what you intend.

The problem is the name. 226 is "Closing data connection"; the logoff reply is 221, and that one lives at :66 as NX_FTP_CODE_CLOSE. So the two names are effectively swapped relative to their meanings, and NX_FTP_CODE_CLOSE is the one actually used for the QUIT reply at :1731 with the text "Logging Off".

The reason nobody has noticed is that NX_FTP_CODE_LOGOFF was completely unused before this PR — I grepped the whole add-on and :67 was its only occurrence. Your change is the first code to use it, which makes this the right moment to fix the name rather than propagate it.

I would rename it to something that says what it is, e.g.:

#define NX_FTP_CODE_CLOSING_DATA     "226"  /* Closing data connection; requested file action successful.  */

Since the macro has no other users, the rename touches only its definition and the four call sites you are already editing — no wider churn. If you would rather keep this PR minimal I will take it as-is with a follow-up, but doing it here is cheap and leaves the code readable.

Whether NX_FTP_CODE_CLOSE should also be renamed to something like NX_FTP_CODE_CLOSING_CONTROL is a separate question and definitely not yours to fix. That said, feel free to share your opinion!

[pnfd]

Hi Frédéric, thanks for the review and the great feedback! I took over your recommendation for the macro name. It is now much clearer to anyone reading the code what this response code stands for.

[fdesbiens]

I want to be careful here, because your underlying diagnosis is right and I do not want this to read as a rebuttal. The change should land. It is only the characterisation I would like corrected, since the PR title becomes the commit subject.

RFC 959's "Sequencing of Commands and Replies" listing gives, for RETR and STOR, the success replies as 125, 150 followed by 226, 250 — and the same pairing for LIST and NLST. Both codes are listed as valid. So a server replying 250 after a data transfer is doing something unconventional that many clients cope with, not something the specification forbids. The reply-code descriptions reinforce which is meant: 226 is "Closing data connection. Requested file action successful", 250 is the generic "Requested file action okay, completed".

So the accurate story is the one your Impact section already tells well: 226 is what clients expect after a data connection closes, several real clients wait for it specifically and hang without it, and the server should send it. That is an interoperability fix with a clear RFC-backed rationale — which is a perfectly good reason to merge — rather than a compliance defect.

Concretely I would drop "non-compliance" from the title in favour of something like "FTP server: send 226 after closing the data connection", and soften "Evidence of Protocol Violation" to something like "Observed client behaviour". Keep the Wireshark capture, it is the most convincing part.

Please do sanity-check the reply listing against the RFC text yourself before we settle the wording — I am working from the specification rather than a live copy, and if the listing turns out narrower than I remember then your original framing stands and I will withdraw this point.

[pnfd]

Good catch regarding the RFC 959 wording! I agree that protocol violation was slightly overstated since 250 is technically permitted by the letter of the spec, even if unconventional. I have renamed the PR title to 'FTP server: send 226 after closing the data connection'

[fdesbiens]

About the comment on line 4597.

This comment sits directly above the line your change to 226, so the two now contradict each other:

                    /* Now send "250" message to indicate successful file write.  */
                    _nx_ftp_server_response(&(client_req_ptr -> nx_ftp_client_request_control_socket), packet_ptr,
                                NX_FTP_CODE_LOGOFF, "File Written");

Worth rewording to name 226, or simply dropping the code number so the comment cannot drift again — the other three sites you touch just say "Now send a successful response to the client", which does not need maintaining.

[pnfd]

Good catch ! I totally missed that .... 👍