Clean up impenetrable logic in pg_basebackup/receivelog.c.

Coverity complained about possible double free of HandleCopyStream's
"copybuf".  AFAICS it's mistaken, but it is easy to see why it's
confused, because management of that buffer is impossibly confusing.
It's unreasonable that HandleEndOfCopyStream frees the buffer in some
cases but not others, updates the caller's state for that in no case,
and has not a single comment about how complicated that makes things.

Let's put all the responsibility for freeing copybuf in the actual
owner of that variable, HandleCopyStream.  This results in one more
PQfreemem call than before, but the logic is far easier to follow,
both for humans and machines.

Since this isn't (quite) actually broken, no back-patch.

diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index f7ba79188c1ae7595619cd30b33e58cb4c60905b..6b6e32dfbdf5615fcd4417d2419c90d748e15010 100644 (file)
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -803,6 +803,10 @@ HandleCopyStream(PGconn *conn, StreamCtl *stream,
        sleeptime = CalculateCopyStreamSleeptime(now, stream->standby_message_timeout,
                                                 last_status);
 
+       /* Done with any prior message */
+       PQfreemem(copybuf);
+       copybuf = NULL;
+
        r = CopyStreamReceive(conn, sleeptime, stream->stop_socket, &copybuf);
        while (r != 0)
        {
@@ -814,8 +818,8 @@ HandleCopyStream(PGconn *conn, StreamCtl *stream,
 
                if (res == NULL)
                    goto error;
-               else
-                   return res;
+               PQfreemem(copybuf);
+               return res;
            }
 
            /* Check the message type. */
@@ -844,6 +848,10 @@ HandleCopyStream(PGconn *conn, StreamCtl *stream,
                goto error;
            }
 
+           /* Done with that message */
+           PQfreemem(copybuf);
+           copybuf = NULL;
+
            /*
             * Process the received data, and any subsequent data we can read
             * without blocking.
@@ -920,8 +928,8 @@ CopyStreamPoll(PGconn *conn, long timeout_ms, pgsocket stop_socket)
  * maximum of 'timeout' ms.
  *
  * If data was received, returns the length of the data. *buffer is set to
- * point to a buffer holding the received message. The buffer is only valid
- * until the next CopyStreamReceive call.
+ * point to a buffer holding the received message. The caller must eventually
+ * free the buffer with PQfreemem().
  *
  * Returns 0 if no data was available within timeout, or if wait was
  * interrupted by signal or stop_socket input.
@@ -934,8 +942,8 @@ CopyStreamReceive(PGconn *conn, long timeout, pgsocket stop_socket,
    char       *copybuf = NULL;
    int         rawlen;
 
-   PQfreemem(*buffer);
-   *buffer = NULL;
+   /* Caller should have cleared any prior buffer */
+   Assert(*buffer == NULL);
 
    /* Try to receive a CopyData message */
    rawlen = PQgetCopyData(conn, &copybuf, 1);
@@ -1198,7 +1206,6 @@ HandleEndOfCopyStream(PGconn *conn, StreamCtl *stream, char *copybuf,
        }
        still_sending = false;
    }
-   PQfreemem(copybuf);
    *stoppos = blockpos;
    return res;
 }