Merge pull request #112 from jhawthorn/openssl_fixes

Fixes to OpenSSL error handling

contrib/ruby/ext/trilogy-ruby/cext.c

@@ -94,12 +94,26 @@ static void trilogy_syserr_fail_str(int e, VALUE msg)
         rb_raise(Trilogy_ConnectionRefusedError, "%" PRIsVALUE, msg);
     } else if (e == ECONNRESET) {
         rb_raise(Trilogy_ConnectionResetError, "%" PRIsVALUE, msg);
+    } else if (e == EPIPE) {
+        // Backwards compatibility: This error class makes no sense, but matches legacy behavior
+        rb_raise(Trilogy_QueryError, "%" PRIsVALUE ": TRILOGY_CLOSED_CONNECTION", msg);
     } else {
         VALUE exc = rb_funcall(Trilogy_SyscallError, id_from_errno, 2, INT2NUM(e), msg);
         rb_exc_raise(exc);
     }
 }
 
+static int trilogy_error_recoverable_p(int rc)
+{
+    // TRILOGY_OPENSSL_ERR and TRILOGY_SYSERR (which can result from an SSL error) must shut down the socket, as further
+    // SSL calls would be invalid.
+    // TRILOGY_ERR, which represents an error message sent to us from the server, is recoverable.
+    // TRILOGY_MAX_PACKET_EXCEEDED is also recoverable as we do not send data when it occurs.
+    // For other exceptions we will also close the socket to prevent further use, as the connection is probably in an
+    // invalid state.
+    return rc == TRILOGY_ERR || rc == TRILOGY_MAX_PACKET_EXCEEDED;
+}
+
 NORETURN(static void handle_trilogy_error(struct trilogy_ctx *, int, const char *, ...));
 static void handle_trilogy_error(struct trilogy_ctx *ctx, int rc, const char *msg, ...)
 {
@@ -108,14 +122,20 @@ static void handle_trilogy_error(struct trilogy_ctx *ctx, int rc, const char *ms
     VALUE rbmsg = rb_vsprintf(msg, args);
     va_end(args);
 
+    if (!trilogy_error_recoverable_p(rc)) {
+        if (ctx->conn.socket != NULL) {
+            // trilogy_sock_shutdown may affect errno
+            int errno_was = errno;
+            trilogy_sock_shutdown(ctx->conn.socket);
+            errno = errno_was;
+        }
+    }
+
     switch (rc) {
     case TRILOGY_SYSERR:
         trilogy_syserr_fail_str(errno, rbmsg);
 
     case TRILOGY_TIMEOUT:
-        if (ctx->conn.socket != NULL) {
-            trilogy_sock_shutdown(ctx->conn.socket);
-        }
         rb_raise(Trilogy_TimeoutError, "%" PRIsVALUE, rbmsg);
 
     case TRILOGY_ERR: {
@@ -131,11 +151,6 @@ static void handle_trilogy_error(struct trilogy_ctx *ctx, int rc, const char *ms
             int err_reason = ERR_GET_REASON(ossl_error);
             trilogy_syserr_fail_str(err_reason, rbmsg);
         }
-        // We can't recover from OpenSSL level errors if there's
-        // an active connection.
-        if (ctx->conn.socket != NULL) {
-            trilogy_sock_shutdown(ctx->conn.socket);
-        }
         rb_raise(Trilogy_SSLError, "%" PRIsVALUE ": SSL Error: %s", rbmsg, ERR_reason_error_string(ossl_error));
     }
 
@@ -982,6 +997,10 @@ static VALUE rb_trilogy_close(VALUE self)
         }
     }
 
+    // We aren't checking or raising errors here (we need close to always close the socket and free the connection), so
+    // we must clear any SSL errors left in the queue from a read/write.
+    ERR_clear_error();
+
     trilogy_free(&ctx->conn);
 
     return Qnil;

contrib/ruby/test/client_test.rb

@@ -773,6 +773,8 @@ def test_packet_size_lower_than_trilogy_max_packet_len
     end
 
     assert_equal "trilogy_query_send: TRILOGY_MAX_PACKET_EXCEEDED", exception.message
+
+    assert client.ping
   ensure
     ensure_closed client
   end
@@ -791,6 +793,8 @@ def test_packet_size_greater_than_trilogy_max_packet_len
     end
 
     assert_equal "trilogy_query_send: TRILOGY_MAX_PACKET_EXCEEDED", exception.message
+
+    assert client.ping
   ensure
     ensure_closed client
   end
@@ -809,6 +813,8 @@ def test_configured_max_packet_below_server
     end
 
     assert_equal "trilogy_query_send: TRILOGY_MAX_PACKET_EXCEEDED", exception.message
+
+    assert client.ping
   ensure
     ensure_closed client
   end
@@ -833,6 +839,10 @@ def test_configured_max_packet_above_server
     end
 
     refute_match(/TRILOGY_MAX_PACKET_EXCEEDED/, exception.message)
+
+    assert_raises_connection_error do
+      client.ping
+    end
   ensure
     ensure_closed client
   end
@@ -851,6 +861,8 @@ def test_absolute_maximum_packet_size
     end
 
     assert_equal "trilogy_query_send: TRILOGY_MAX_PACKET_EXCEEDED", exception.message
+
+    assert client.ping
   ensure
     ensure_closed client
   end

contrib/ruby/test/test_helper.rb

@@ -139,6 +139,8 @@ def assert_raises_connection_error(&block)
 
     if err.is_a?(Trilogy::QueryError)
       assert_includes err.message, "TRILOGY_CLOSED_CONNECTION"
+    elsif err.is_a?(Trilogy::SSLError)
+      assert_includes err.message, "unexpected eof while reading"
     else
       assert_instance_of Trilogy::ConnectionResetError, err
     end

src/client.c

@@ -1,4 +1,3 @@
-#include <errno.h>
 #include <fcntl.h>
 
 #include "trilogy/client.h"
@@ -76,11 +75,6 @@ static int read_packet(trilogy_conn_t *conn)
 
         if (nread < 0) {
             int rc = (int)nread;
-            if (rc == TRILOGY_SYSERR) {
-                if (errno == EINTR || errno == EAGAIN) {
-                    return TRILOGY_AGAIN;
-                }
-            }
             return rc;
         }
 
@@ -162,16 +156,6 @@ int trilogy_flush_writes(trilogy_conn_t *conn)
 
     if (bytes < 0) {
         int rc = (int)bytes;
-        if (rc == TRILOGY_SYSERR) {
-            if (errno == EINTR || errno == EAGAIN) {
-                return TRILOGY_AGAIN;
-            }
-
-            if (errno == EPIPE) {
-                return TRILOGY_CLOSED_CONNECTION;
-            }
-        }
-
         return rc;
     }
 

src/socket.c

@@ -66,7 +66,11 @@ static ssize_t _cb_raw_read(trilogy_sock_t *_sock, void *buf, size_t nread)
     struct trilogy_sock *sock = (struct trilogy_sock *)_sock;
     ssize_t data_read = read(sock->fd, buf, nread);
     if (data_read < 0) {
-        return (ssize_t)TRILOGY_SYSERR;
+        if (errno == EINTR || errno == EAGAIN) {
+            return (ssize_t)TRILOGY_AGAIN;
+        } else {
+            return (ssize_t)TRILOGY_SYSERR;
+        }
     }
     return data_read;
 }
@@ -76,6 +80,14 @@ static ssize_t _cb_raw_write(trilogy_sock_t *_sock, const void *buf, size_t nwri
     struct trilogy_sock *sock = (struct trilogy_sock *)_sock;
     ssize_t data_written = write(sock->fd, buf, nwrite);
     if (data_written < 0) {
+        if (errno == EINTR || errno == EAGAIN) {
+            return (ssize_t)TRILOGY_AGAIN;
+        }
+
+        if (errno == EPIPE) {
+            return (ssize_t)TRILOGY_CLOSED_CONNECTION;
+        }
+
         return (ssize_t)TRILOGY_SYSERR;
     }
     return data_written;
@@ -353,12 +365,18 @@ int trilogy_sock_resolve(trilogy_sock_t *_sock)
 
 static ssize_t ssl_io_return(struct trilogy_sock *sock, ssize_t ret)
 {
-    if (ret < 0) {
+    if (ret <= 0) {
         int rc = SSL_get_error(sock->ssl, (int)ret);
         if (rc == SSL_ERROR_WANT_WRITE || rc == SSL_ERROR_WANT_READ) {
             return (ssize_t)TRILOGY_AGAIN;
-        } else if (rc == SSL_ERROR_SYSCALL && errno != 0) {
-            return (ssize_t)TRILOGY_SYSERR;
+        } else if (rc == SSL_ERROR_SYSCALL && !ERR_peek_error()) {
+            if (errno == 0) {
+                // On OpenSSL <= 1.1.1, SSL_ERROR_SYSCALL with an errno value
+                // of 0 indicates unexpected EOF from the peer.
+                return (ssize_t)TRILOGY_CLOSED_CONNECTION;
+            } else {
+                return (ssize_t)TRILOGY_SYSERR;
+            }
         }
         return (ssize_t)TRILOGY_OPENSSL_ERR;
     }
@@ -368,13 +386,23 @@ static ssize_t ssl_io_return(struct trilogy_sock *sock, ssize_t ret)
 static ssize_t _cb_ssl_read(trilogy_sock_t *_sock, void *buf, size_t nread)
 {
     struct trilogy_sock *sock = (struct trilogy_sock *)_sock;
+
+    // This shouldn't be necessary, but protects against other libraries in the same process incorrectly leaving errors
+    // in the queue.
+    ERR_clear_error();
+
     ssize_t data_read = (ssize_t)SSL_read(sock->ssl, buf, (int)nread);
     return ssl_io_return(sock, data_read);
 }
 
 static ssize_t _cb_ssl_write(trilogy_sock_t *_sock, const void *buf, size_t nwrite)
 {
     struct trilogy_sock *sock = (struct trilogy_sock *)_sock;
+
+    // This shouldn't be necessary, but protects against other libraries in the same process incorrectly leaving errors
+    // in the queue.
+    ERR_clear_error();
+
     ssize_t data_written = (ssize_t)SSL_write(sock->ssl, buf, (int)nwrite);
     return ssl_io_return(sock, data_written);
 }
@@ -398,7 +426,10 @@ static int _cb_ssl_close(trilogy_sock_t *_sock)
     struct trilogy_sock *sock = (struct trilogy_sock *)_sock;
     if (sock->ssl != NULL) {
         if (SSL_in_init(sock->ssl) == 0) {
-            SSL_shutdown(sock->ssl);
+            (void)SSL_shutdown(sock->ssl);
+            // SSL_shutdown might return WANT_WRITE or WANT_READ. Ideally we would retry but we don't want to block.
+            // It may also push an error onto the OpenSSL error queue, so clear that.
+            ERR_clear_error();
         }
         SSL_free(sock->ssl);
         sock->ssl = NULL;
@@ -580,6 +611,10 @@ int trilogy_sock_upgrade_ssl(trilogy_sock_t *_sock)
 {
     struct trilogy_sock *sock = (struct trilogy_sock *)_sock;
 
+    // This shouldn't be necessary, but protects against other libraries in the same process incorrectly leaving errors
+    // in the queue.
+    ERR_clear_error();
+
     SSL_CTX *ctx = trilogy_ssl_ctx(&sock->base.opts);
 
     if (!ctx) {
@@ -622,6 +657,10 @@ int trilogy_sock_upgrade_ssl(trilogy_sock_t *_sock)
         goto fail;
 
     for (;;) {
+        // This shouldn't be necessary, but protects against other libraries in the same process incorrectly leaving errors
+        // in the queue.
+        ERR_clear_error();
+
         int ret = SSL_connect(sock->ssl);
         if (ret == 1) {
 #if OPENSSL_VERSION_NUMBER < 0x1000200fL