Feature/compile config delay sending 226 #63
Conversation
A signed-unsigned conversion was making the PASV command sometimes report signed numbers. This was detected by the Windows firewall as a problematice connection and it therefore terminated the connection.
An FTP client implementation has been observed to close the data connection as soon as it receives the 226 status code - even though it hasn't received all data, yet. To improve interoper with such buggy clients, sending of the 226 status code can now be delayed a bit. The optional delay is controlled by means of preprocessor define that can be set from the cmake command line through a cmake cache variable.
fineftp-server/src/ftp_session.cpp
Outdated
| { | ||
| asio::error_code ec; | ||
| data_socket->shutdown(asio::socket_base::shutdown_both, ec); | ||
| data_socket->close(ec); | ||
| } | ||
|
|
There was a problem hiding this comment.
@bjuulp: This must always be executed, right? I think the current implementation does not close the data socket at all, after sending a file...
My old implementation did close the socket, but the new memory-mapped file implementation does not use the writeDataToSocket function anymore, which would close the socket. So I think this bug was introduced with the memory-mapped file implementation. Funny, that curl doesn't complain about that.
Edit: Ah, I think the shared-pointer goes out of scope and the destructor closes the socket, which saved us. I still think we should explicitly shutdown the connection properly.
There was a problem hiding this comment.
I've now tested what happens if I explicitly close the socket and set the delay to 0. This still triggers the bug in the old lwftp implementation. So, the only thing I've done in the latest commit is to move the explicit connection closure so that it is done irrespective of the presence or absence of the delay.
The data socket is now also closed explicitly after file fetch in the case where sending of code 226 is not delayed.
fineftp-server/src/ftp_session.cpp
Outdated
| me->sendFtpMessage(FtpReplyCode::CLOSING_DATA_CONNECTION, "Done"); | ||
| // Close Data Socket properly | ||
| { | ||
| asio::error_code ec; |
There was a problem hiding this comment.
Can you fix the "ec hides previous declaration" warning here?
There was a problem hiding this comment.
Actually. Where is that warning coming from? That is, why did you see it? I didn't see that in my builds.
There was a problem hiding this comment.
The warning came from Visual Studio 2022. It wasn't a problem in this case, but hiding variables can also be unintentional, so I like to keep the project as warning-free as possible.
An FTP client implementation has been
observed to close the data connection
as soon as it receives the 226 status
code - even though it hasn't received
all data, yet. To improve interoper
with such buggy clients, sending of
the 226 status code can now be delayed
a bit. The optional delay is controlled
by means of preprocessor define that
can be set from the cmake command
line through a cmake cache variable.