Clean up impenetrable logic in pg_basebackup/receivelog.c.

tglsfdc · tglsfdc · commit fd602f29c19d · 2025-02-12T16:07:23.000-05:00
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
@@ -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;
 }
