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

@@ -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.

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.