There are two slow parts in the upload logic:
- Reading from the input stream character by character
- Checking each character one by one to normalize line endings
First of all, the line normalization isn't necessary for binary
transfers, so we can use a simple read while loop to read bytes into the
transfer buffer.
Second, for the ASCII transfer where we do have to normalize line
endings, we can be smarter than reading one character at a time. There's
a php_stream_get_line() function that we can repurpose if the flags for
the stream are set up properly.
This patch implements these fixes.
Results: I tested this on an 850 MiB file, transferring this to an FTP
server running locally.
Results before patch:
Binary/ASCII transfer (same code path): 8.21s
Results after patch:
Binary transfer: 0.65s
ASCII transfer: 0.74s
Further improvement is probably possible by having a larger send buffer.
Bumps the minimum required OpenSSL version from 1.0.2 to 1.1.1.
OpenSSL 1.1.1 is an LTS release, but has reached[^1] EOL from upstream. However, Linux distro/OS vendors
continue to ship OpenSSL 1.1.1, so 1.1.1 was picked as the minimum. The current minimum 1.0.2 reached
EOL in 2018.
Bumping the minimum required OpenSSL version makes it possible for ext-openssl to remove a bunch of
conditional code, and assume that TLS 1.3 (shipped with OpenSSL 1.1.1) will be supported everywhere.
- Debian buster: 1.1.1[^2]
- Ubuntu 20.04: 1.1.1[^3]
- CentOS/RHEL 7: 1.0.2
- RHEL 8/Rocky 8/EL 8: 1.1.1
- Fedora 38: 3.0.9 (`openssl11` provides OpenSSL 1.1 as well)
RHEL/CentOS 7 reaches EOL mid 2024, so for PHP 8.4 scheduled towards the end of this year, we can safely
bump the minimum OpenSSL version.
[^1]: https://www.openssl.org/blog/blog/2023/03/28/1.1.1-EOL/index.html
[^2]: https://packages.debian.org/buster/libssl-dev
[^3]: https://packages.ubuntu.com/focal/libssl-dev
The issue referenced here doesn't contain a reproducer, but I recently
received an email of a user with the exact same problem. I was able to
recreate the scenario locally using vsftpd and setting
`require_ssl_reuse=YES` in the vsftpd configuration.
It turns out that our session resumption code is broken. It only works a
single time: the first time a data connection opens. Subsequent data
connections fail to reuse the session. This is because on every data
connection a new session is negotiated, but the current code always
tries to reuse the (stale) session of the control connection.
To fix this, we use SSL_CTX_sess_set_new_cb() to setup a callback that
gets called every time a new session is negotiated. We take a strong
reference using SSL_get1_session() and store it in the ftpbuf_t struct.
Every time we open a data connection we'll take that session.
This works because every control connection has at most a single
associated data connection.
Also disable internal session caching storage to not fill the cache up
with useless sessions.
There is no phpt for this because PHP does not support enforcing SSL
session reuse.
It is however testable manually by setting up vsftpd and setting the
`require_ssl_reuse=YES` function from before.
Closes GH-12851.
When the user does not fully consume the data stream, but instead opens
a new one, a memory leak occurs. Moreover, the state is invalid: when
more commands arrive they'll be handled out-of-sync because the state of
the client does not match what the server is doing.
This leads to all sorts of weirdness, for example:
Warning: ftp_nb_fget(): OK.
Fix it by gracefully closing the old data stream when a new data stream
is started.
Closes GH-11606.
The char arrays were too small for a long on 64-bit systems, which
resulted in cutting off the string at the end with a NUL byte. Use a
size of MAX_LENGTH_OF_LONG to fix this issue instead of a fixed size
of 11 chars.
Closes GH-10525.
The docs say that this function returns true on success, and false on
error. This function always returns true in the current implementation
because the success return value from ftp_close() is never propagated to
userland. This affects one test: since the test server exits after an
invalid login, the ftp close correctly fails (because the server has
gone away).
@cname currently refers to the constant name in C. However, it is not always a (constant) name, but sometimes a function invocation, so naming it as @cvalue would be more appropriate.
Niels Dossche