Merge branch 'fix to SSLSocket' into main

luisremis · luisremis · commit a2a2adc28443 · 2022-03-15T08:09:57.000-07:00
diff --git a/include/comm/Exception.h b/include/comm/Exception.h
@@ -31,6 +31,7 @@
 #pragma once
 
 #include <string>
+#include <iostream>
 
 namespace comm {
 
@@ -65,37 +66,50 @@ namespace comm {
         // Additional information
         const std::string msg;
         const int errno_val;
+        const int ssl_err_code;
 
         // Where it was thrown
         const char * const file;   // Source file name
         const int line;           // Source line number
 
         Exception(int exc, const char *exc_name, const char *f, int l)
             : num(exc), name(exc_name),
-              msg(), errno_val(0),
+              msg(), errno_val(0), ssl_err_code(0),
               file(f), line(l)
         {}
 
         Exception(int exc, const char *exc_name,
                   const std::string &m,
                   const char *f, int l)
             : num(exc), name(exc_name),
-              msg(m), errno_val(0),
+              msg(m), errno_val(0), ssl_err_code(0),
+              file(f), line(l)
+        {}
+
+        Exception(int exc, const char *exc_name,
+                  int err, const char* m,
+                  int ssl_err,
+                  const char *f, int l)
+            : num(exc), name(exc_name),
+              msg(m), errno_val(err), ssl_err_code(ssl_err),
               file(f), line(l)
         {}
 
         Exception(int exc, const char *exc_name,
                   int err, const std::string &m,
+                  int ssl_err,
                   const char *f, int l)
             : num(exc), name(exc_name),
-              msg(m), errno_val(err),
+              msg(m), errno_val(err), ssl_err_code(ssl_err),
               file(f), line(l)
         {}
         Exception() = delete;
         Exception(const Exception&) = default;
         Exception& operator=(const Exception&) = delete;
+        friend std::ostream& operator<<(std::ostream&, const Exception&);
     };
 
+    std::ostream& operator<<(std::ostream&, const Exception&);
 };
 
 #define THROW_EXCEPTION(name, ...) \
diff --git a/src/comm/Connection.cc b/src/comm/Connection.cc
@@ -97,10 +97,10 @@ const std::basic_string<uint8_t>& Connection::recv_message()
     };
 
     size_t bytes_recv = recv_and_check(reinterpret_cast<uint8_t*>(&recv_message_size),
-                                       sizeof(uint32_t));
+                                       sizeof(recv_message_size));
 
     if (bytes_recv != sizeof(recv_message_size)) {
-        THROW_EXCEPTION(ReadFail);
+        THROW_EXCEPTION(ReadFail, "Short read msg header");
     }
 
     if (recv_message_size > _max_buffer_size) {
@@ -115,11 +115,11 @@ const std::basic_string<uint8_t>& Connection::recv_message()
     bytes_recv = recv_and_check(const_cast<uint8_t*>(_buffer_str.data()), recv_message_size);
 
     if (recv_message_size != bytes_recv) {
-        THROW_EXCEPTION(ReadFail);
+        THROW_EXCEPTION(ReadFail, "Short read msg body");
     }
 
     if (recv_message_size != _buffer_str.size()) {
-        THROW_EXCEPTION(ReadFail);
+        THROW_EXCEPTION(ReadFail, "Short read other");
     }
 
     if (_metrics) {
diff --git a/src/comm/Exception.cc b/src/comm/Exception.cc
@@ -28,10 +28,12 @@
  *
  */
 
+#include "comm/Exception.h"
+
 #include <stdio.h>
 #include <string.h>
-
-#include "comm/Exception.h"
+#include <openssl/ssl.h>
+#include <openssl/err.h>
 
 void print_exception(const comm::Exception &e, FILE *f)
 {
@@ -41,3 +43,59 @@ void print_exception(const comm::Exception &e, FILE *f)
     else if (!e.msg.empty())
         fprintf(f, "%s\n", e.msg.c_str());
 }
+
+std::ostream& comm::operator<<(std::ostream& os, const comm::Exception& o)
+{
+    static const size_t len = 255;
+    char buf[len+1];
+    buf[len] = 0;
+
+    os << o.file << ':' << o.line << ':'
+       << " comm::Exception={"
+       << "num=" << o.num
+       << ",name='" << o.name << "'"
+       ;
+    if (!o.msg.empty()) {
+        os << ",msg='" << o.msg << "'"
+            ;
+    }
+    int e = o.errno_val;
+    if (e) {
+        buf[0] = 0;
+        os << ",Error={"
+           << "errno=" << e
+           << ",errmsg='" << strerror_r(e, buf, len) << "'"
+           << '}'
+           ;
+    }
+    auto s = o.ssl_err_code;
+    if (s) {
+        const char* err_str = "?";
+        if (false) {
+# define _O(X) } else if (s == X) { err_str = #X;
+       _O(SSL_ERROR_NONE)
+       _O(SSL_ERROR_ZERO_RETURN)
+       _O(SSL_ERROR_WANT_READ)
+       _O(SSL_ERROR_WANT_WRITE)
+       _O(SSL_ERROR_WANT_CONNECT)
+       _O(SSL_ERROR_WANT_ACCEPT)
+       _O(SSL_ERROR_WANT_X509_LOOKUP)
+       _O(SSL_ERROR_WANT_ASYNC)
+       _O(SSL_ERROR_WANT_ASYNC_JOB)
+       _O(SSL_ERROR_WANT_CLIENT_HELLO_CB)
+       _O(SSL_ERROR_SYSCALL)
+       _O(SSL_ERROR_SSL)
+# undef _O
+        }
+        const char* r = ERR_reason_error_string(s);
+        os << ",sslError={"
+           << "err=" << err_str
+           << ",reason='" << (r ? r : "") << "'"
+           << '}'
+           ;
+    }
+    os << '}';
+
+    return os;
+}
+
diff --git a/src/comm/TCPConnection.cc b/src/comm/TCPConnection.cc
@@ -59,22 +59,25 @@ size_t TCPConnection::read(uint8_t* buffer, size_t length)
         THROW_EXCEPTION(SocketFail);
     }
 
+    errno = 0;
     auto count = ::recv(_tcp_socket->_socket_fd, buffer, length, MSG_WAITALL);
+    int errno_r = errno;
 
     if (count < 0) {
         DISABLE_WARNING(logical-op)
         if (errno == EAGAIN || errno == EWOULDBLOCK) {
-            THROW_EXCEPTION(ConnectionShutDown, "Connection being cleaned cause of timeout");
+            THROW_EXCEPTION(ConnectionShutDown, // Not, really. Read expired for blocking socket.
+                    errno_r, "recv()", 0);
         }
         else {
-            THROW_EXCEPTION(ReadFail);
+            THROW_EXCEPTION(ReadFail, errno_r, "recv()", 0);
         }
         ENABLE_WARNING(logical-op)
     }
     // When a stream socket peer has performed an orderly shutdown, the
     // return value will be 0 (the traditional "end-of-file" return).
     else if (count == 0) {
-        THROW_EXCEPTION(ConnectionShutDown, "Closing Connection.");
+        THROW_EXCEPTION(ConnectionShutDown, "Peer Closed Connection.");
     }
 
     return static_cast<size_t>(count);
diff --git a/src/comm/TCPSocket.cc b/src/comm/TCPSocket.cc
@@ -9,6 +9,7 @@
 #include <netinet/ip.h>
 #include <sys/socket.h>
 #include <unistd.h>
+#include <glog/logging.h>
 
 #include "comm/Exception.h"
 #include "comm/Variables.h"
@@ -27,19 +28,42 @@ TCPSocket::~TCPSocket()
     }
 }
 
+static long msec_diff(const struct timespec& a,
+                      const struct timespec& b)
+{
+    return
+        (a.tv_sec  - b.tv_sec)  * 1000 +
+        (a.tv_nsec - b.tv_nsec) / 1000000;
+}
+
+
 std::unique_ptr<TCPSocket> TCPSocket::accept(const std::unique_ptr<TCPSocket>& listening_socket)
 {
     struct sockaddr_in clnt_addr;
     socklen_t len = sizeof(clnt_addr); //store size of the address
 
     // This is where client connects.
     // Server will stall here until incoming connection
-    // unless the socket is marked and nonblocking
-    int connected_socket = ::accept(listening_socket->_socket_fd, reinterpret_cast<sockaddr*>(&clnt_addr), &len);
+    // unless the socket is marked as nonblocking
+again:
+    timespec t1;
+    clock_gettime(CLOCK_REALTIME_COARSE, &t1);
 
+    errno = 0;
+    int connected_socket = ::accept(listening_socket->_socket_fd, reinterpret_cast<sockaddr*>(&clnt_addr), &len);
+    int errno_r = errno;
     if (connected_socket < 0) {
-        THROW_EXCEPTION(ConnectionError);
+        if (errno_r == EAGAIN) {
+            timespec t2;
+            clock_gettime(CLOCK_REALTIME_COARSE, &t2);
+            auto dt = msec_diff(t2, t1);
+            VLOG(3) << "accept(): no activity for " << dt << " msec. Going back to listening";
+            goto again;
+        } else {
+            THROW_EXCEPTION(ConnectionError, errno_r, "accept()", 0);
+        }
     }
+    // MAGICK can be done here
 
     return std::unique_ptr<TCPSocket>(new TCPSocket(connected_socket));
 }
diff --git a/src/comm/TLSConnection.cc b/src/comm/TLSConnection.cc
@@ -10,6 +10,7 @@
 #include <netdb.h>
 #include <string>
 #include <unistd.h>
+#include <glog/logging.h>
 
 #include "comm/Exception.h"
 #include "util/gcc_util.h"
@@ -31,35 +32,64 @@ TLSConnection::TLSConnection(
 {
 }
 
+static long msec_diff(const struct timespec& a,
+                      const struct timespec& b)
+{
+    return
+        (a.tv_sec  - b.tv_sec)  * 1000 +
+        (a.tv_nsec - b.tv_nsec) / 1000000;
+}
+
 size_t TLSConnection::read(uint8_t* buffer, size_t length)
 {
+again:
+    timespec t1;
+    clock_gettime(CLOCK_REALTIME_COARSE, &t1);
+
+    errno = 0;
     auto count = SSL_read(_tls_socket->_ssl, buffer, length);
+    int errno_r = errno;
 
     if (count <= 0) {
         auto error = SSL_get_error(_tls_socket->_ssl, count);
 
-        DISABLE_WARNING(logical-op)
         if (error == SSL_ERROR_ZERO_RETURN) {
-            THROW_EXCEPTION(ConnectionShutDown, "Closing Connection.");
+            THROW_EXCEPTION(ConnectionShutDown, // FIXME -- misleading 'ConnectionShutDown'
+                     errno_r, "SSL_read()", error); // Peer closed conn for writing, so no more reads
         }
-        else if (error == SSL_ERROR_SYSCALL && (!errno || errno == EAGAIN)) {
-            THROW_EXCEPTION(ConnectionShutDown, "Closing Connection.");
+        else if (error == SSL_ERROR_WANT_READ) {
+            if (errno_r == EAGAIN) {
+                timespec t2;
+                clock_gettime(CLOCK_REALTIME_COARSE, &t2);
+                auto dt = msec_diff(t2, t1);
+                VLOG(3) << "SSL_read(): no activity for " << dt << " msec. Going back to reading";
+                goto again;
+            } else {
+                // This error is recoverable. Consider handling it.
+                THROW_EXCEPTION(ReadFail, errno_r, "SSL_read()", error);
+            }
+        }
+        else if (error == SSL_ERROR_SYSCALL ||
+                 error == SSL_ERROR_SSL) {
+            THROW_EXCEPTION(ConnectionShutDown, errno_r, "SSL_read()", error);
+            // FIXME: *we* must close the conn
         }
         else {
-            THROW_EXCEPTION(ReadFail);
+            THROW_EXCEPTION(ReadFail, errno_r, "SSL_read()", error);
         }
-        ENABLE_WARNING(logical-op)
     }
 
     return static_cast<size_t>(count);
 }
 
 size_t TLSConnection::write(const uint8_t* buffer, size_t length)
 {
+    errno = 0;
     auto count = SSL_write(_tls_socket->_ssl, buffer, static_cast<int>(length));
-
+    int errno_r = errno;
     if (count <= 0) {
-        THROW_EXCEPTION(WriteFail, "Error sending message.");
+        auto error = SSL_get_error(_tls_socket->_ssl, count);
+        THROW_EXCEPTION(WriteFail, errno_r, "SSL_write()", error);
     }
 
     return static_cast<size_t>(count);
diff --git a/src/comm/TLSSocket.cc b/src/comm/TLSSocket.cc
@@ -25,7 +25,8 @@ TLSSocket::TLSSocket(std::unique_ptr<TCPSocket> tcp_socket, SSL* ssl) :
 TLSSocket::~TLSSocket()
 {
     if (_ssl) {
-        if (!(SSL_get_shutdown(_ssl) & SSL_SENT_SHUTDOWN)) {
+        int status = SSL_get_shutdown(_ssl);
+        if ((status & (SSL_SENT_SHUTDOWN | SSL_RECEIVED_SHUTDOWN)) == 0) {
             SSL_shutdown(_ssl);
         }
         SSL_free(_ssl);
@@ -34,21 +35,22 @@ TLSSocket::~TLSSocket()
 
 void TLSSocket::accept()
 {
+    errno = 0;
     auto result = ::SSL_accept(_ssl);
-
+    int errno_r = errno;
     if (result < 1) {
-        THROW_EXCEPTION(TLSError,
-                        "Error accepting connection. SSL Error: " +
-                        std::to_string(result));
+        THROW_EXCEPTION(TLSError, errno_r, "SSL_accept()", result);
     }
 }
 
 void TLSSocket::connect()
 {
+    errno = 0;
     auto result = ::SSL_connect(_ssl);
+    int errno_r = errno;
 
     if (result < 1) {
-        THROW_EXCEPTION(TLSError);
+        THROW_EXCEPTION(TLSError, errno_r, "SSL_connect()", result);
     }
 }
 
diff --git a/tools/prometheus_ambassador/SConscript b/tools/prometheus_ambassador/SConscript
@@ -18,6 +18,7 @@ prom_amb_env.Replace(
                 'aperturedb-client',
                 'prometheus-cpp-core',
                 'prometheus-cpp-pull',
+                'glog',
               ],
     LIBPATH = [ '/usr/local/lib/',
                 os.getenv('AD_COMM_LIB',         default='../../lib'),
@@ -39,7 +40,7 @@ prom_amb_env.Program('prometheus_ambassador', src)
 
 test_env = prom_amb_env.Clone()
 test_env.Replace(
-    LIBS = prom_amb_env['LIBS'] + ['gtest', 'pthread', 'protobuf'],
+    LIBS = prom_amb_env['LIBS'] + ['pthread', 'protobuf', 'gtest'],
     CPPPATH = prom_amb_env['CPPPATH'] + ['../../test', '../../src'],
 )
 

Original file line number	Diff line number	Diff line change
`@@ -59,22 +59,25 @@ size_t TCPConnection::read(uint8_t* buffer, size_t length)`
`59`	`59`	`THROW_EXCEPTION(SocketFail);`
`60`	`60`	`}`
`61`	`61`
	`62`	`+ errno = 0;`
`62`	`63`	`auto count = ::recv(_tcp_socket->_socket_fd, buffer, length, MSG_WAITALL);`
	`64`	`+ int errno_r = errno;`
`63`	`65`
`64`	`66`	`if (count < 0) {`
`65`	`67`	`DISABLE_WARNING(logical-op)`
`66`	`68`	`if (errno == EAGAIN \|\| errno == EWOULDBLOCK) {`
`67`		`- THROW_EXCEPTION(ConnectionShutDown, "Connection being cleaned cause of timeout");`
	`69`	`+ THROW_EXCEPTION(ConnectionShutDown, // Not, really. Read expired for blocking socket.`
	`70`	`+ errno_r, "recv()", 0);`
`68`	`71`	`}`
`69`	`72`	`else {`
`70`		`- THROW_EXCEPTION(ReadFail);`
	`73`	`+ THROW_EXCEPTION(ReadFail, errno_r, "recv()", 0);`
`71`	`74`	`}`
`72`	`75`	`ENABLE_WARNING(logical-op)`
`73`	`76`	`}`
`74`	`77`	`// When a stream socket peer has performed an orderly shutdown, the`
`75`	`78`	`// return value will be 0 (the traditional "end-of-file" return).`
`76`	`79`	`else if (count == 0) {`
`77`		`- THROW_EXCEPTION(ConnectionShutDown, "Closing Connection.");`
	`80`	`+ THROW_EXCEPTION(ConnectionShutDown, "Peer Closed Connection.");`
`78`	`81`	`}`
`79`	`82`
`80`	`83`	`return static_cast<size_t>(count);`
Original file line number	Diff line number	Diff line change
`@@ -25,7 +25,8 @@ TLSSocket::TLSSocket(std::unique_ptr<TCPSocket> tcp_socket, SSL* ssl) :`
`25`	`25`	`TLSSocket::~TLSSocket()`
`26`	`26`	`{`
`27`	`27`	`if (_ssl) {`
`28`		`- if (!(SSL_get_shutdown(_ssl) & SSL_SENT_SHUTDOWN)) {`
	`28`	`+ int status = SSL_get_shutdown(_ssl);`
	`29`	`+ if ((status & (SSL_SENT_SHUTDOWN \| SSL_RECEIVED_SHUTDOWN)) == 0) {`
`29`	`30`	`SSL_shutdown(_ssl);`
`30`	`31`	`}`
`31`	`32`	`SSL_free(_ssl);`
`@@ -34,21 +35,22 @@ TLSSocket::~TLSSocket()`
`34`	`35`
`35`	`36`	`void TLSSocket::accept()`
`36`	`37`	`{`
	`38`	`+ errno = 0;`
`37`	`39`	`auto result = ::SSL_accept(_ssl);`
`38`		`-`
	`40`	`+ int errno_r = errno;`
`39`	`41`	`if (result < 1) {`
`40`		`- THROW_EXCEPTION(TLSError,`
`41`		`- "Error accepting connection. SSL Error: " +`
`42`		`- std::to_string(result));`
	`42`	`+ THROW_EXCEPTION(TLSError, errno_r, "SSL_accept()", result);`
`43`	`43`	`}`
`44`	`44`	`}`
`45`	`45`
`46`	`46`	`void TLSSocket::connect()`
`47`	`47`	`{`
	`48`	`+ errno = 0;`
`48`	`49`	`auto result = ::SSL_connect(_ssl);`
	`50`	`+ int errno_r = errno;`
`49`	`51`
`50`	`52`	`if (result < 1) {`
`51`		`- THROW_EXCEPTION(TLSError);`
	`53`	`+ THROW_EXCEPTION(TLSError, errno_r, "SSL_connect()", result);`
`52`	`54`	`}`
`53`	`55`	`}`
`54`	`56`