From 6c98a97bf4f0886a0dcbeaaecbd395c4e8ecb657 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Wed, 30 Nov 2022 22:23:00 +0200 Subject: Improve TLS error handling. (#11557) * Remove duplicate code, propagating SSL errors into connection state. * Add missing error handling in synchronous IO functions. * Fix connection error reporting in some replication flows. --- src/replication.c | 8 ++-- src/tls.c | 113 +++++++++++++++++++++++++----------------------------- 2 files changed, 57 insertions(+), 64 deletions(-) diff --git a/src/replication.c b/src/replication.c index 3674bea05..a57cd8528 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1837,7 +1837,7 @@ void readSyncBulkPayload(connection *conn) { if (nread == -1) { serverLog(LL_WARNING, "I/O error reading bulk count from MASTER: %s", - strerror(errno)); + connGetLastError(conn)); goto error; } else { /* nread here is returned by connSyncReadLine(), which calls syncReadLine() and @@ -1909,7 +1909,7 @@ void readSyncBulkPayload(connection *conn) { return; } serverLog(LL_WARNING,"I/O error trying to sync with MASTER: %s", - (nread == -1) ? strerror(errno) : "connection lost"); + (nread == -1) ? connGetLastError(conn) : "connection lost"); cancelReplicationHandshake(1); return; } @@ -2254,7 +2254,7 @@ char *receiveSynchronousResponse(connection *conn) { /* Read the reply from the server. */ if (connSyncReadLine(conn,buf,sizeof(buf),server.repl_syncio_timeout*1000) == -1) { - serverLog(LL_WARNING, "Failed to read response from the server: %s", strerror(errno)); + serverLog(LL_WARNING, "Failed to read response from the server: %s", connGetLastError(conn)); return NULL; } server.repl_transfer_lastio = server.unixtime; @@ -2815,7 +2815,7 @@ void syncWithMaster(connection *conn) { serverLog(LL_NOTICE,"Retrying with SYNC..."); if (connSyncWrite(conn,"SYNC\r\n",6,server.repl_syncio_timeout*1000) == -1) { serverLog(LL_WARNING,"I/O error writing to MASTER: %s", - strerror(errno)); + connGetLastError(conn)); goto error; } } diff --git a/src/tls.c b/src/tls.c index d16246511..d938fbe19 100644 --- a/src/tls.c +++ b/src/tls.c @@ -517,6 +517,7 @@ static connection *connCreateAcceptedTLS(int fd, void *priv) { } static void tlsEventHandler(struct aeEventLoop *el, int fd, void *clientData, int mask); +static void updateSSLEvent(tls_connection *conn); /* Process the return code received from OpenSSL> * Update the want parameter with expected I/O. @@ -550,6 +551,47 @@ static int handleSSLReturnCode(tls_connection *conn, int ret_value, WantIOType * return 0; } +/* Handle OpenSSL return code following SSL_write() or SSL_read(): + * + * - Updates conn state and last_errno. + * - If update_event is nonzero, calls updateSSLEvent() when necessary. + * + * Returns ret_value, or -1 on error or dropped connection. + */ +static int updateStateAfterSSLIO(tls_connection *conn, int ret_value, int update_event) { + /* If system call was interrupted, there's no need to go through the full + * OpenSSL error handling and just report this for the caller to retry the + * operation. + */ + if (errno == EINTR) { + conn->c.last_errno = EINTR; + return -1; + } + + if (ret_value <= 0) { + WantIOType want = 0; + int ssl_err; + if (!(ssl_err = handleSSLReturnCode(conn, ret_value, &want))) { + if (want == WANT_READ) conn->flags |= TLS_CONN_FLAG_WRITE_WANT_READ; + if (want == WANT_WRITE) conn->flags |= TLS_CONN_FLAG_READ_WANT_WRITE; + if (update_event) updateSSLEvent(conn); + errno = EAGAIN; + return -1; + } else { + if (ssl_err == SSL_ERROR_ZERO_RETURN || + ((ssl_err == SSL_ERROR_SYSCALL && !errno))) { + conn->c.state = CONN_STATE_CLOSED; + return -1; + } else { + conn->c.state = CONN_STATE_ERROR; + return -1; + } + } + } + + return ret_value; +} + static void registerSSLEvent(tls_connection *conn, WantIOType want) { int mask = aeGetFileEvents(server.el, conn->c.fd); @@ -830,39 +872,12 @@ static int connTLSConnect(connection *conn_, const char *addr, int port, const c static int connTLSWrite(connection *conn_, const void *data, size_t data_len) { tls_connection *conn = (tls_connection *) conn_; - int ret, ssl_err; + int ret; if (conn->c.state != CONN_STATE_CONNECTED) return -1; ERR_clear_error(); ret = SSL_write(conn->ssl, data, data_len); - /* If system call was interrupted, there's no need to go through the full - * OpenSSL error handling and just report this for the caller to retry the - * operation. - */ - if (errno == EINTR) { - conn->c.last_errno = EINTR; - return -1; - } - if (ret <= 0) { - WantIOType want = 0; - if (!(ssl_err = handleSSLReturnCode(conn, ret, &want))) { - if (want == WANT_READ) conn->flags |= TLS_CONN_FLAG_WRITE_WANT_READ; - updateSSLEvent(conn); - errno = EAGAIN; - return -1; - } else { - if (ssl_err == SSL_ERROR_ZERO_RETURN || - ((ssl_err == SSL_ERROR_SYSCALL && !errno))) { - conn->c.state = CONN_STATE_CLOSED; - return -1; - } else { - conn->c.state = CONN_STATE_ERROR; - return -1; - } - } - } - - return ret; + return updateStateAfterSSLIO(conn, ret, 1); } static int connTLSWritev(connection *conn_, const struct iovec *iov, int iovcnt) { @@ -905,40 +920,11 @@ static int connTLSWritev(connection *conn_, const struct iovec *iov, int iovcnt) static int connTLSRead(connection *conn_, void *buf, size_t buf_len) { tls_connection *conn = (tls_connection *) conn_; int ret; - int ssl_err; if (conn->c.state != CONN_STATE_CONNECTED) return -1; ERR_clear_error(); ret = SSL_read(conn->ssl, buf, buf_len); - /* If system call was interrupted, there's no need to go through the full - * OpenSSL error handling and just report this for the caller to retry the - * operation. - */ - if (errno == EINTR) { - conn->c.last_errno = EINTR; - return -1; - } - if (ret <= 0) { - WantIOType want = 0; - if (!(ssl_err = handleSSLReturnCode(conn, ret, &want))) { - if (want == WANT_WRITE) conn->flags |= TLS_CONN_FLAG_READ_WANT_WRITE; - updateSSLEvent(conn); - - errno = EAGAIN; - return -1; - } else { - if (ssl_err == SSL_ERROR_ZERO_RETURN || - ((ssl_err == SSL_ERROR_SYSCALL) && !errno)) { - conn->c.state = CONN_STATE_CLOSED; - return 0; - } else { - conn->c.state = CONN_STATE_ERROR; - return -1; - } - } - } - - return ret; + return updateStateAfterSSLIO(conn, ret, 1); } static const char *connTLSGetLastError(connection *conn_) { @@ -1005,7 +991,9 @@ static ssize_t connTLSSyncWrite(connection *conn_, char *ptr, ssize_t size, long setBlockingTimeout(conn, timeout); SSL_clear_mode(conn->ssl, SSL_MODE_ENABLE_PARTIAL_WRITE); + ERR_clear_error(); int ret = SSL_write(conn->ssl, ptr, size); + ret = updateStateAfterSSLIO(conn, ret, 0); SSL_set_mode(conn->ssl, SSL_MODE_ENABLE_PARTIAL_WRITE); unsetBlockingTimeout(conn); @@ -1016,7 +1004,9 @@ static ssize_t connTLSSyncRead(connection *conn_, char *ptr, ssize_t size, long tls_connection *conn = (tls_connection *) conn_; setBlockingTimeout(conn, timeout); + ERR_clear_error(); int ret = SSL_read(conn->ssl, ptr, size); + ret = updateStateAfterSSLIO(conn, ret, 0); unsetBlockingTimeout(conn); return ret; @@ -1032,7 +1022,10 @@ static ssize_t connTLSSyncReadLine(connection *conn_, char *ptr, ssize_t size, l while(size) { char c; - if (SSL_read(conn->ssl,&c,1) <= 0) { + ERR_clear_error(); + int ret = SSL_read(conn->ssl, &c, 1); + ret = updateStateAfterSSLIO(conn, ret, 0); + if (ret <= 0) { nread = -1; goto exit; } -- cgit v1.2.1