Fix #77151 ftp_close(): SSL_read on shutdown #3666
Conversation
| if (data_available(ftp, fd)) { | ||
| ERR_clear_error(); | ||
| nread = SSL_read(ssl_handle, buf, sizeof(buf)); | ||
| while (!done && data_available(ftp, fd)) { |
There was a problem hiding this comment.
test grouped to avoid (possible) dead loop when !done and no data available.
| while (!done && data_available(ftp, fd)) { | ||
| ERR_clear_error(); | ||
| nread = SSL_read(ssl_handle, buf, sizeof(buf)); | ||
| if (nread <= 0) { |
There was a problem hiding this comment.
manage error, only if read fails.
There was a problem hiding this comment.
SSL_get_error returns SSL_ERROR_NONE if nread > 0
There was a problem hiding this comment.
That's not my point. My point is this doesn't look like a fix as it doesn't actually change anything.
Also please note that there are at least two more cases with SSL_get_error in the same file with the same pattern.
|
second commit make warning conditional by sslerror. |
| if ((sslerror = ERR_get_error())) { | ||
| ERR_error_string_n(sslerror, buf, sizeof(buf)); | ||
| ERR_error_string_n(sslerror, buf, sizeof(buf)); | ||
| php_error_docref(NULL, E_WARNING, "SSL_read on shutdown: %s", buf); |
There was a problem hiding this comment.
This seems wrong. SSL_get_error may also return SSL_ERROR_SYSCALL in which case OpenSSL indicates that the underlying syscall has failed. This changes hides this error case from the user.
There was a problem hiding this comment.
Still better that current broken implementation.
Please understand that we have released 7.1.24/7.2.12 with a regression, for which we need an urgent fix.
|
So I found a ftp service that let me reproduce the bug report. In this case
Edit: I'm suggesting something like this (together with the --- ext/ftp/ftp.c 2018-11-15 17:58:32.454527562 +0100
+++ ext/ftp/ftp.c 2018-11-15 18:05:38.706094266 +0100
@@ -1930,6 +1930,12 @@
/* SSL wants a write. Really odd. Let's bail out. */
done = 1;
break;
+ case SSL_ERROR_SYSCALL:
+ if (errno == 0) {
+ /* The connection has already been closed */
+ done = 1;
+ break;
+ }
default:
if ((sslerror = ERR_get_error())) {
ERR_error_string_n(sslerror, buf, sizeof(buf)); |
00a9144 to
dd1d951
Compare
|
I have re-add warning when errno is set. So no message will be output when sslerror=0 and errno=0 |
Regression introduced in fix for #76972 only display the error message when sslerror or if errno is set (for SSL_ERROR_SYSCALL case)
dd1d951 to
2966b88
Compare
|
@cmb69 I'd like to merge this one before today RCs |
|
@remicollet Okay, please go ahead. |
nikic
left a comment
There was a problem hiding this comment.
The newest version looks good to me.
|
Merged. |
Regression introduced in fix for #76972