Re: BUG #17391: While using --with-ssl=openssl and PG_TEST_EXTRA='ssl' options, SSL tests fail on OpenBSD 7.0 - Mailing list pgsql-bugs

I wrote:
> Just to note that I have a plan for fixing this part.  I've concluded
> that it was a design error to implement the write_failed error
> postponement mechanism in pqSendSome, and instead we should shove it
> down a couple of abstraction layers into pqsecure_raw_write.  This'd
> visibly have no effect on non-encrypted connections, because
> pqSendSome is the only caller of pqsecure_write.  But in encrypted
> connections, we'd be additionally allowing write-failure postponement
> during OpenSSL's internal machinations, which seems to be exactly
> what's wanted now that we have seen this failure mode.

Here's a draft patch for that.  It seems to fix the OpenBSD problem
reliably.

I'd originally thought that everything under pqsecure_write needed to be
converted to use the write_failed mechanism, which would have required
rather invasive changes in fe-secure-openssl.c and fe-secure-gssapi.c.
But now I think we can leave those modules alone, as long as the
bottom-level physical I/O uses write_failed.  If we get some other
error out of the SSL/GSS layer, there's no harm in reporting it
right away instead of postponing it.  This reflects the fact that
it's not that easy to tell whether an OpenSSL error ought to be
classified as "read" or "write", since both I/O directions might be
going on under the hood.

BTW, this also changes what is looking to me like a thinko in
my_sock_write(): if we get a zero result from pqsecure_raw_write(),
we should not be consulting errno, because it wouldn't have
been set.  Am I missing something there?

Thoughts?

            regards, tom lane

diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 90a312bf2c..09c731729e 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -777,19 +777,19 @@ definitelyFailed:
  * (putting it in conn->inBuffer) in any situation where we can't send
  * all the specified data immediately.
  *
- * Upon write failure, conn->write_failed is set and the error message is
- * saved in conn->write_err_msg, but we clear the output buffer and return
- * zero anyway; this is because callers should soldier on until it's possible
- * to read from the server and check for an error message.  write_err_msg
- * should be reported only when we are unable to obtain a server error first.
- * (Thus, a -1 result is returned only for an internal *read* failure.)
+ * If a socket-level write failure occurs, conn->write_failed is set and the
+ * error message is saved in conn->write_err_msg, but we clear the output
+ * buffer and return zero anyway; this is because callers should soldier on
+ * until it's possible to read from the server and check for an error message.
+ * write_err_msg should be reported only when we are unable to obtain a server
+ * error first.  Much of that behavior is implemented at lower levels, but
+ * this function deals with some edge cases.
  */
 static int
 pqSendSome(PGconn *conn, int len)
 {
     char       *ptr = conn->outBuffer;
     int            remaining = conn->outCount;
-    int            oldmsglen = conn->errorMessage.len;
     int            result = 0;

     /*
@@ -817,7 +817,7 @@ pqSendSome(PGconn *conn, int len)
     if (conn->sock == PGINVALID_SOCKET)
     {
         conn->write_failed = true;
-        /* Insert error message into conn->write_err_msg, if possible */
+        /* Store error message in conn->write_err_msg, if possible */
         /* (strdup failure is OK, we'll cope later) */
         conn->write_err_msg = strdup(libpq_gettext("connection not open\n"));
         /* Discard queued data; no chance it'll ever be sent */
@@ -859,24 +859,6 @@ pqSendSome(PGconn *conn, int len)
                     continue;

                 default:
-                    /* pqsecure_write set the error message for us */
-                    conn->write_failed = true;
-
-                    /*
-                     * Transfer error message to conn->write_err_msg, if
-                     * possible (strdup failure is OK, we'll cope later).
-                     *
-                     * We only want to transfer whatever has been appended to
-                     * conn->errorMessage since we entered this routine.
-                     */
-                    if (!PQExpBufferBroken(&conn->errorMessage))
-                    {
-                        conn->write_err_msg = strdup(conn->errorMessage.data +
-                                                     oldmsglen);
-                        conn->errorMessage.len = oldmsglen;
-                        conn->errorMessage.data[oldmsglen] = '\0';
-                    }
-
                     /* Discard queued data; no chance it'll ever be sent */
                     conn->outCount = 0;

@@ -886,7 +868,18 @@ pqSendSome(PGconn *conn, int len)
                         if (pqReadData(conn) < 0)
                             return -1;
                     }
-                    return 0;
+
+                    /*
+                     * Lower-level code should already have filled
+                     * conn->write_err_msg (and set conn->write_failed) or
+                     * conn->errorMessage.  In the former case, we pretend
+                     * there's no problem; the write_failed condition will be
+                     * dealt with later.  Otherwise, report the error now.
+                     */
+                    if (conn->write_failed)
+                        return 0;
+                    else
+                        return -1;
             }
         }
         else
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 9f735ba437..f6e563a2e5 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1682,7 +1682,7 @@ my_sock_write(BIO *h, const char *buf, int size)

     res = pqsecure_raw_write((PGconn *) BIO_get_data(h), buf, size);
     BIO_clear_retry_flags(h);
-    if (res <= 0)
+    if (res < 0)
     {
         /* If we were interrupted, tell caller to retry */
         switch (SOCK_ERRNO)
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index 0b998e254d..a1dc7b796d 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -280,9 +280,22 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
 /*
  *    Write data to a secure connection.
  *
- * On failure, this function is responsible for appending a suitable message
- * to conn->errorMessage.  The caller must still inspect errno, but only
- * to determine whether to continue/retry after error.
+ * Returns the number of bytes written, or a negative value (with errno
+ * set) upon failure.  The write count could be less than requested.
+ *
+ * Note that socket-level hard failures are masked from the caller,
+ * instead setting conn->write_failed and storing an error message
+ * in conn->write_err_msg; see pqsecure_raw_write.  This allows us to
+ * postpone reporting of write failures until we're sure no error
+ * message is available from the server.
+ *
+ * However, errors detected in the SSL or GSS management level are reported
+ * via a negative result, with message appended to conn->errorMessage.
+ * It's frequently unclear whether such errors should be considered read or
+ * write errors, so we don't attempt to postpone reporting them.
+ *
+ * The caller must still inspect errno upon failure, but only to determine
+ * whether to continue/retry; a message has been saved someplace in any case.
  */
 ssize_t
 pqsecure_write(PGconn *conn, const void *ptr, size_t len)
@@ -310,16 +323,50 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
     return n;
 }

+/*
+ * Low-level implementation of pqsecure_write.
+ *
+ * This is used directly for an unencrypted connection.  For encrypted
+ * connections, this does the physical I/O on behalf of pgtls_write or
+ * pg_GSS_write.
+ *
+ * This function reports failure (i.e., returns a negative result) only
+ * for retryable errors such as EINTR.  Looping for such cases is to be
+ * handled at some outer level, maybe all the way up to the application.
+ * For hard failures, we set conn->write_failed and store an error message
+ * in conn->write_err_msg, but then claim to have written the data anyway.
+ * This is because we don't want to report write failures so long as there
+ * is a possibility of reading from the server and getting an error message
+ * that could explain why the connection dropped.  Many TCP stacks have
+ * race conditions such that a write failure may or may not be reported
+ * before all incoming data has been read.
+ *
+ * Note that this error behavior happens below the SSL management level when
+ * we are using SSL.  That's because at least some versions of OpenSSL are
+ * too quick to report a write failure when there's still a possibility to
+ * get a more useful error from the server.
+ */
 ssize_t
 pqsecure_raw_write(PGconn *conn, const void *ptr, size_t len)
 {
     ssize_t        n;
     int            flags = 0;
     int            result_errno = 0;
+    char        msgbuf[1024];
     char        sebuf[PG_STRERROR_R_BUFLEN];

     DECLARE_SIGPIPE_INFO(spinfo);

+    /*
+     * If we already had a write failure, we will never again try to send data
+     * on that connection.  Even if the kernel would let us, we've probably
+     * lost message boundary sync with the server.  conn->write_failed
+     * therefore persists until the connection is reset, and we just discard
+     * all data presented to be written.
+     */
+    if (conn->write_failed)
+        return len;
+
 #ifdef MSG_NOSIGNAL
     if (conn->sigpipe_flag)
         flags |= MSG_NOSIGNAL;
@@ -369,17 +416,29 @@ retry_masked:
                 /* FALL THRU */

             case ECONNRESET:
-                appendPQExpBufferStr(&conn->errorMessage,
-                                     libpq_gettext("server closed the connection unexpectedly\n"
-                                                   "\tThis probably means the server terminated abnormally\n"
-                                                   "\tbefore or while processing the request.\n"));
+                conn->write_failed = true;
+                /* Store error message in conn->write_err_msg, if possible */
+                /* (strdup failure is OK, we'll cope later) */
+                snprintf(msgbuf, sizeof(msgbuf),
+                         libpq_gettext("server closed the connection unexpectedly\n"
+                                       "\tThis probably means the server terminated abnormally\n"
+                                       "\tbefore or while processing the request.\n"));
+                conn->write_err_msg = strdup(msgbuf);
+                /* Now claim the write succeeded */
+                n = len;
                 break;

             default:
-                appendPQExpBuffer(&conn->errorMessage,
-                                  libpq_gettext("could not send data to server: %s\n"),
-                                  SOCK_STRERROR(result_errno,
-                                                sebuf, sizeof(sebuf)));
+                conn->write_failed = true;
+                /* Store error message in conn->write_err_msg, if possible */
+                /* (strdup failure is OK, we'll cope later) */
+                snprintf(msgbuf, sizeof(msgbuf),
+                         libpq_gettext("could not send data to server: %s\n"),
+                         SOCK_STRERROR(result_errno,
+                                       sebuf, sizeof(sebuf)));
+                conn->write_err_msg = strdup(msgbuf);
+                /* Now claim the write succeeded */
+                n = len;
                 break;
         }
     }