Be more wary about OpenSSL not setting errno on error.

OpenSSL will sometimes return SSL_ERROR_SYSCALL without having set
errno; this is apparently a reflection of recv(2)'s habit of not
setting errno when reporting EOF.  Ensure that we treat such cases
the same as read EOF.  Previously, we'd frequently report them like
"could not accept SSL connection: Success" which is confusing, or
worse report them with an unrelated errno left over from some
previous syscall.

To fix, ensure that errno is zeroed immediately before the call,
and report its value only when it's not zero afterwards; otherwise
report EOF.

For consistency, I've applied the same coding pattern in libpq's
pqsecure_raw_read().  Bare recv(2) shouldn't really return -1 without
setting errno, but in case it does we might as well cope.

Per report from Andres Freund.  Back-patch to all supported versions.

Discussion: https://postgr.es/m/20231208181451.deqnflwxqoehhxpe@awork3.anarazel.de

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 34f8f9e71ec08fd3b974a50df0d36a89e65b4b6a..571ae436d577f20e9a7d7e5067edb98f51f7ed17 100644 (file)
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -417,6 +417,7 @@ aloop:
         * per-thread error queue following another call to an OpenSSL I/O
         * routine.
         */
+       errno = 0;
        ERR_clear_error();
        r = SSL_accept(port->ssl);
        if (r <= 0)
@@ -453,7 +454,7 @@ aloop:
                                                                                 WAIT_EVENT_SSL_OPEN_SERVER);
                                goto aloop;
                        case SSL_ERROR_SYSCALL:
-                               if (r < 0)
+                               if (r < 0 && errno != 0)
                                        ereport(COMMERROR,
                                                        (errcode_for_socket_access(),
                                                         errmsg("could not accept SSL connection: %m")));
@@ -587,7 +588,7 @@ be_tls_read(Port *port, void *ptr, size_t len, int *waitfor)
                        break;
                case SSL_ERROR_SYSCALL:
                        /* leave it to caller to ereport the value of errno */
-                       if (n != -1)
+                       if (n != -1 || errno == 0)
                        {
                                errno = ECONNRESET;
                                n = -1;
@@ -645,8 +646,14 @@ be_tls_write(Port *port, void *ptr, size_t len, int *waitfor)
                        n = -1;
                        break;
                case SSL_ERROR_SYSCALL:
-                       /* leave it to caller to ereport the value of errno */
-                       if (n != -1)
+
+                       /*
+                        * Leave it to caller to ereport the value of errno.  However, if
+                        * errno is still zero then assume it's a read EOF situation, and
+                        * report ECONNRESET.  (This seems possible because SSL_write can
+                        * also do reads.)
+                        */
+                       if (n != -1 || errno == 0)
                        {
                                errno = ECONNRESET;
                                n = -1;

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index d7f425de88c0bde2b8b10dad9c165d3e6aea176c..283a29caa63d81210fcba2e0e71d248fec8c4681 100644 (file)
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -958,6 +958,8 @@ pq_recvbuf(void)
        {
                int                     r;
 
+               errno = 0;
+
                r = secure_read(MyProcPort, PqRecvBuffer + PqRecvLength,
                                                PQ_RECV_BUFFER_SIZE - PqRecvLength);
 
@@ -970,10 +972,13 @@ pq_recvbuf(void)
                         * Careful: an ereport() that tries to write to the client would
                         * cause recursion to here, leading to stack overflow and core
                         * dump!  This message must go *only* to the postmaster log.
+                        *
+                        * If errno is zero, assume it's EOF and let the caller complain.
                         */
-                       ereport(COMMERROR,
-                                       (errcode_for_socket_access(),
-                                        errmsg("could not receive data from client: %m")));
+                       if (errno != 0)
+                               ereport(COMMERROR,
+                                               (errcode_for_socket_access(),
+                                                errmsg("could not receive data from client: %m")));
                        return EOF;
                }
                if (r == 0)
@@ -1050,6 +1055,8 @@ pq_getbyte_if_available(unsigned char *c)
        /* Put the socket into non-blocking mode */
        socket_set_nonblocking(true);
 
+       errno = 0;
+
        r = secure_read(MyProcPort, c, 1);
        if (r < 0)
        {
@@ -1066,10 +1073,13 @@ pq_getbyte_if_available(unsigned char *c)
                         * Careful: an ereport() that tries to write to the client would
                         * cause recursion to here, leading to stack overflow and core
                         * dump!  This message must go *only* to the postmaster log.
+                        *
+                        * If errno is zero, assume it's EOF and let the caller complain.
                         */
-                       ereport(COMMERROR,
-                                       (errcode_for_socket_access(),
-                                        errmsg("could not receive data from client: %m")));
+                       if (errno != 0)
+                               ereport(COMMERROR,
+                                               (errcode_for_socket_access(),
+                                                errmsg("could not receive data from client: %m")));
                        r = EOF;
                }
        }

diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 6718dacfba4675b40f1c15862b000082ea66b5b7..2dee9323c69c7b8cac9ed1c2ab75f1ea53f8a886 100644 (file)
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -196,7 +196,7 @@ rloop:
                         */
                        goto rloop;
                case SSL_ERROR_SYSCALL:
-                       if (n < 0)
+                       if (n < 0 && SOCK_ERRNO != 0)
                        {
                                result_errno = SOCK_ERRNO;
                                if (result_errno == EPIPE ||
@@ -305,7 +305,13 @@ pgtls_write(PGconn *conn, const void *ptr, size_t len)
                        n = 0;
                        break;
                case SSL_ERROR_SYSCALL:
-                       if (n < 0)
+
+                       /*
+                        * If errno is still zero then assume it's a read EOF situation,
+                        * and report EOF.  (This seems possible because SSL_write can
+                        * also do reads.)
+                        */
+                       if (n < 0 && SOCK_ERRNO != 0)
                        {
                                result_errno = SOCK_ERRNO;
                                if (result_errno == EPIPE || result_errno == ECONNRESET)
@@ -1233,10 +1239,12 @@ open_client_SSL(PGconn *conn)
 {
        int                     r;
 
+       SOCK_ERRNO_SET(0);
        ERR_clear_error();
        r = SSL_connect(conn->ssl);
        if (r <= 0)
        {
+               int                     save_errno = SOCK_ERRNO;
                int                     err = SSL_get_error(conn->ssl, r);
                unsigned long ecode;
 
@@ -1253,10 +1261,10 @@ open_client_SSL(PGconn *conn)
                                {
                                        char            sebuf[PG_STRERROR_R_BUFLEN];
 
-                                       if (r == -1)
+                                       if (r == -1 && save_errno != 0)
                                                printfPQExpBuffer(&conn->errorMessage,
                                                                                  libpq_gettext("SSL SYSCALL error: %s\n"),
-                                                                                 SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
+                                                                                 SOCK_STRERROR(save_errno, sebuf, sizeof(sebuf)));
                                        else
                                                printfPQExpBuffer(&conn->errorMessage,
                                                                                  libpq_gettext("SSL SYSCALL error: EOF detected\n"));

diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index b8191b4c8f8a8f5314745b0fd97de8150a300d06..8e9d8b3c33f704f4786213d65f594dd0778ff41c 100644 (file)
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -242,6 +242,8 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
        int                     result_errno = 0;
        char            sebuf[PG_STRERROR_R_BUFLEN];
 
+       SOCK_ERRNO_SET(0);
+
        n = recv(conn->sock, ptr, len, 0);
 
        if (n < 0)
@@ -271,6 +273,11 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
                                break;
 #endif
 
+                       case 0:
+                               /* If errno didn't get set, treat it as regular EOF */
+                               n = 0;
+                               break;
+
                        default:
                                printfPQExpBuffer(&conn->errorMessage,
                                                                  libpq_gettext("could not receive data from server: %s\n"),