diff --git a/contrib/ruby/ext/trilogy-ruby/cext.c b/contrib/ruby/ext/trilogy-ruby/cext.c index 0e7940e7..7e128e1a 100644 --- a/contrib/ruby/ext/trilogy-ruby/cext.c +++ b/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; diff --git a/contrib/ruby/test/client_test.rb b/contrib/ruby/test/client_test.rb index 99cb784e..e25811cd 100644 --- a/contrib/ruby/test/client_test.rb +++ b/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 diff --git a/contrib/ruby/test/test_helper.rb b/contrib/ruby/test/test_helper.rb index 8937f07a..2c288ecc 100644 --- a/contrib/ruby/test/test_helper.rb +++ b/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 diff --git a/src/client.c b/src/client.c index 0b43f12c..9a68a05e 100644 --- a/src/client.c +++ b/src/client.c @@ -1,4 +1,3 @@ -#include #include #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; } diff --git a/src/socket.c b/src/socket.c index 324191b9..fc0eb624 100644 --- a/src/socket.c +++ b/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,6 +386,11 @@ 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); } @@ -375,6 +398,11 @@ static ssize_t _cb_ssl_read(trilogy_sock_t *_sock, void *buf, size_t nread) 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