From 0e76f476ac4edb8a4527b4c26d5d0768fbed4778 Mon Sep 17 00:00:00 2001 From: Wiebe Cazemier Date: Tue, 26 Nov 2024 15:13:27 +0100 Subject: [PATCH] Fix possible hanging SSL read loop Actual case not reproduced, but it was confirmed that doing an extra SSL_read() on a socket that had already returned EOF made it get stuck in a loop. This refactor makes the code more in line with the documentation for OpenSSL 3, instead of OpenSSL 1, meaning we don't communicate the SSL_read() error upwards, but stick to our own convention. --- client.cpp | 2 +- iowrapper.cpp | 112 ++++++++++++++++++++++++++------------------------ 2 files changed, 60 insertions(+), 54 deletions(-) diff --git a/client.cpp b/client.cpp index c25def0..c965ea8 100644 --- a/client.cpp +++ b/client.cpp @@ -224,7 +224,7 @@ DisconnectStage Client::readFdIntoBuffer() if (error == IoWrapResult::Interrupted) continue; - if (error == IoWrapResult::Wouldblock) + if (error == IoWrapResult::Wouldblock || error == IoWrapResult::Disconnected) break; // Make sure we either always have enough space for a next call of this method, or stop reading the fd. diff --git a/iowrapper.cpp b/iowrapper.cpp index 22e3dd5..c86fb38 100644 --- a/iowrapper.cpp +++ b/iowrapper.cpp @@ -400,10 +400,9 @@ void IoWrapper::setFakeUpgraded() ssize_t IoWrapper::readOrSslRead(int fd, void *buf, size_t nbytes, IoWrapResult *error) { *error = IoWrapResult::Success; - ssize_t n = 0; if (!ssl) { - n = read(fd, buf, nbytes); + ssize_t n = read(fd, buf, nbytes); if (n < 0) { if (errno == EINTR) @@ -417,73 +416,80 @@ ssize_t IoWrapper::readOrSslRead(int fd, void *buf, size_t nbytes, IoWrapResult { *error = IoWrapResult::Disconnected; } + + return n; } else { this->sslReadWantsWrite = false; ERR_clear_error(); - char sslErrorBuf[OPENSSL_ERROR_STRING_SIZE]; - n = SSL_read(ssl, buf, nbytes); + ssize_t n = SSL_read(ssl, buf, nbytes); - if (n <= 0) - { - int err = SSL_get_error(ssl, n); - unsigned long error_code = ERR_get_error(); + if (n > 0) + return n; - if (err == SSL_ERROR_WANT_READ || err == SSL_ERROR_WANT_WRITE) - { - *error = IoWrapResult::Wouldblock; - if (err == SSL_ERROR_WANT_WRITE) - { - sslReadWantsWrite = true; - parentClient->setReadyForWriting(true); - } - n = -1; - } - else if (err == SSL_ERROR_ZERO_RETURN) + int err = SSL_get_error(ssl, n); + unsigned long error_code = ERR_get_error(); + + if (err == SSL_ERROR_WANT_READ || err == SSL_ERROR_WANT_WRITE) + { + *error = IoWrapResult::Wouldblock; + if (err == SSL_ERROR_WANT_WRITE) { - parentClient->setDisconnectReason("SSL socket close with close_notify"); - *error = IoWrapResult::Disconnected; + sslReadWantsWrite = true; + parentClient->setReadyForWriting(true); } + return -1; + } + + if (err == SSL_ERROR_ZERO_RETURN) + { + parentClient->setDisconnectReason("SSL socket close with close_notify"); + *error = IoWrapResult::Disconnected; + return 0; + } + #if OPENSSL_VERSION_NUMBER >= 0x30000000L - else if (err == SSL_ERROR_SSL && ERR_GET_REASON(error_code) == SSL_R_UNEXPECTED_EOF_WHILE_READING) - { - parentClient->setDisconnectReason("SSL socket close without close_notify"); - *error = IoWrapResult::Disconnected; - } + if (err == SSL_ERROR_SSL && ERR_GET_REASON(error_code) == SSL_R_UNEXPECTED_EOF_WHILE_READING) + { + parentClient->setDisconnectReason("SSL socket close without close_notify"); + *error = IoWrapResult::Disconnected; + return 0; + } #endif - else if (err == SSL_ERROR_SYSCALL && errno == 0) - { - // See https://www.openssl.org/docs/man1.1.1/man3/SSL_get_error.html "BUGS" why unexpected EOF is seen as SSL_ERROR_SYSCALL. - *error = IoWrapResult::Disconnected; - parentClient->setDisconnectReason("SSL<3 socket close without close_notify"); - } - else + if (err == SSL_ERROR_SYSCALL && errno == 0) + { + // See https://www.openssl.org/docs/man1.1.1/man3/SSL_get_error.html "BUGS" why unexpected EOF is seen as SSL_ERROR_SYSCALL. + + *error = IoWrapResult::Disconnected; + parentClient->setDisconnectReason("SSL<3 socket close without close_notify"); + return 0; + } + + if (err == SSL_ERROR_SYSCALL) + { + // I don't actually know if OpenSSL hides this or passes EINTR on. The docs say + // 'Some non-recoverable, fatal I/O error occurred' for SSL_ERROR_SYSCALL, so it + // implies EINTR is not included? Also, we use non-blocking sockets, which don't + // return EINTR. + if (errno == EINTR) { - if (err == SSL_ERROR_SYSCALL) - { - // I don't actually know if OpenSSL hides this or passes EINTR on. The docs say - // 'Some non-recoverable, fatal I/O error occurred' for SSL_ERROR_SYSCALL, so it - // implies EINTR is not included? - if (errno == EINTR) - *error = IoWrapResult::Interrupted; - else - { - char *err = strerror(errno); - std::string msg(err); - throw std::runtime_error("SSL read error: " + msg); - } - } - ERR_error_string(error_code, sslErrorBuf); - std::string errorString(sslErrorBuf, OPENSSL_ERROR_STRING_SIZE); - ERR_print_errors_cb(logSslError, NULL); - throw std::runtime_error("SSL socket error reading: " + errorString); + *error = IoWrapResult::Interrupted; + return -1; } + + char *err = strerror(errno); + std::string msg(err); + throw std::runtime_error("SSL read syscall error: " + msg); } - } - return n; + char sslErrorBuf[OPENSSL_ERROR_STRING_SIZE]; + ERR_error_string(error_code, sslErrorBuf); + std::string errorString(sslErrorBuf, OPENSSL_ERROR_STRING_SIZE); + ERR_print_errors_cb(logSslError, NULL); + throw std::runtime_error("SSL socket error reading: " + errorString); + } } // SSL and non-SSL sockets behave differently. This wrapper unifies behavor for the caller.