Fix #80880: SSL_read on shutdown, ftp/proc_open

[cmb69]

When SSL_read() after SSL_shutdown() fails with SSL_ERROR_SYSCALL,
we should not warn about this on Windows for WSAECONNABORTED.

---

I'm not sure whether this is a Windows specific issue, or whether SSL_ERROR_SYSCALL may happen on other systems as well, so I went with a Windows specific patch. @manuelm, since you've implemented the proper SSL shutdown, maybe you want to have a look.

[cmb69]

Oh forgot to mention that adding a test for a Windows specific FTP issue wouldn't make sense, since the FTP test suite doesn't run there anyway. :(

[manuelm]

I'm not sure whether this is a Windows specific issue, or whether SSL_ERROR_SYSCALL may happen on other systems as well

I'm not sure either. Maybe we should simply bail out without any warning message at all (unless we receive SSL_ERROR_WANT_READ ofc)? There isn't much a user can do anyway.

[cmb69]

Maybe we should simply bail out without any warning message at all (unless we receive SSL_ERROR_WANT_READ ofc)? There isn't much a user can do anyway.

That may indeed be a better solution, although a meaningful error message might help users to indentify the problem.

Anyhow, the php_openssl_sockop_close() is totally different from the FTP shutdown. After SSL_shutdown() there are no further SSL_read()s, and on Windows receives are even shutdown:

php-src/ext/openssl/xp_ssl.c

Lines 2279 to 2319 in ac47162

	if (close_handle) {
	if (sslsock->ssl_active) {
	SSL_shutdown(sslsock->ssl_handle);
	sslsock->ssl_active = 0;
	}
	if (sslsock->ssl_handle) {
	SSL_free(sslsock->ssl_handle);
	sslsock->ssl_handle = NULL;
	}
	if (sslsock->ctx) {
	SSL_CTX_free(sslsock->ctx);
	sslsock->ctx = NULL;
	}
	#ifdef HAVE_TLS_ALPN
	if (sslsock->alpn_ctx.data) {
	pefree(sslsock->alpn_ctx.data, php_stream_is_persistent(stream));
	}
	#endif
	#ifdef PHP_WIN32
	if (sslsock->s.socket == -1)
	sslsock->s.socket = SOCK_ERR;
	#endif
	if (sslsock->s.socket != SOCK_ERR) {
	#ifdef PHP_WIN32
	/* prevent more data from coming in */
	shutdown(sslsock->s.socket, SHUT_RD);
	/* try to make sure that the OS sends all data before we close the connection.
	* Essentially, we are waiting for the socket to become writeable, which means
	* that all pending data has been sent.
	* We use a small timeout which should encourage the OS to send the data,
	* but at the same time avoid hanging indefinitely.
	* */
	do {
	n = php_pollfd_for_ms(sslsock->s.socket, POLLOUT, 500);
	} while (n == -1 && php_socket_errno() == EINTR);
	#endif
	closesocket(sslsock->s.socket);
	sslsock->s.socket = SOCK_ERR;
	}
	}

Maybe @bukka has some idea about how to implement a proper SSL shutdown?

[bukka]

Yeah I think you should just bail out on all platforms if there's SSL_ERROR_SYSCALL.

I think the php_openssl_sockop_close doesn't deal with this case - it doesn't try to read before closing connection. The whole shutdown is not really right. Interestingly I started looking again to #2605 more than a week ago and planning to do some work on it hopefully in the coming months. It needs some refactoring (especially for non-blocking case) and this could be added too probably. Think php_openssl_sockop_close should be always blocking anyway so this would apply to both cases. The non blocking socket should use changed stream_socket_enable_crypto if it needs to do shutdown nicely.

I have been also looking to FTP handling and there are quite a lot of duplication so it would be good to maybe switch it to use stream internally once the shutdown is handled there as well. Btw. this SSL_ERROR_WANT_READ logic (polling) might not be needed from 1.1.1 where SSL_MODE_AUTO_RETRY is enabled by default. We should probably look into enabling in the stream for blocking socket explicitly so it's available on 1.0.2 and 1.1.0 where it's not enabled by default. That could make it a bit simpler.

[cmb69]

Thanks, @bukka! I've implemented an uncoditional bailout on SSL_ERROR_SYSCALL now.

Regarding ext/ftp: not sure how much work we should put into this. I assume that SFTP is more important nowadays, and that the need for FTPS will further decrease. We may want consider to deprecate the extension (and maybe want to consider whether to bundle ext/ssh2 instead). Those still needing FTP(S) have still basic support via streams.

[bukka]

Thanks, @bukka! I've implemented an uncoditional bailout on SSL_ERROR_SYSCALL now.

Looks good! Just noted that it might be worth to mention the reason in the comment.

Regarding ext/ftp: not sure how much work we should put into this. I assume that SFTP is more important nowadays, and that the need for FTPS will further decrease. We may want consider to deprecate the extension (and maybe want to consider whether to bundle ext/ssh2 instead). Those still needing FTP(S) have still basic support via streams.

I think the extension is quite widely used so I don't agree that it should be deprecated. The sftp api in ext/ssh2 is not that straight forward but might be good idea to add that ext to core as it has got some other advantages. If it exposes the API, it could be potentially used by ftp ext. You are right that FTPS is not that used, was just thinking that it would be better to have it in a single place. Anyway this is probably not the best place to discuss it :)

[cmb69]

I agree that deprecating ext/ftp and/or bundling ext/ssh2 should not be discussed here. :)