Fix assorted new memory leaks in libpq.

tglsfdc · tglsfdc · commit b7ab88ddb1b9 · 2025-05-22T20:35:32.000-04:00
Valgrind'ing the postgres_fdw tests showed me that libpq was leaking PGconn.be_cancel_key. It looks like freePGconn is expecting pqDropServerData to release it ... but in a cancel connection object, that doesn't happen. Looking a little closer, I was dismayed to find that freePGconn also missed freeing the pgservice, min_protocol_version, max_protocol_version, sslkeylogfile, scram_client_key_binary, and scram_server_key_binary strings. There's much less excuse for those oversights. Worse, that's from five different commits (a460251, 4b99fed, 285613c, 2da74d8, 761c795), some of them by extremely senior hackers. Fortunately, all of these are new in v18, so we haven't shipped any leaky versions of libpq. While at it, reorder the operations in freePGconn to match the order of the fields in struct PGconn. Some of those free's seem to have been inserted with the aid of a dartboard.
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
@@ -2027,13 +2027,11 @@ pqConnectOptions2(PGconn *conn)
 		if (len < 0)
 		{
 			libpq_append_conn_error(conn, "invalid SCRAM client key");
-			free(conn->scram_client_key_binary);
 			return false;
 		}
 		if (len != SCRAM_MAX_KEY_LEN)
 		{
 			libpq_append_conn_error(conn, "invalid SCRAM client key length: %d", len);
-			free(conn->scram_client_key_binary);
 			return false;
 		}
 		conn->scram_client_key_len = len;
@@ -2052,13 +2050,11 @@ pqConnectOptions2(PGconn *conn)
 		if (len < 0)
 		{
 			libpq_append_conn_error(conn, "invalid SCRAM server key");
-			free(conn->scram_server_key_binary);
 			return false;
 		}
 		if (len != SCRAM_MAX_KEY_LEN)
 		{
 			libpq_append_conn_error(conn, "invalid SCRAM server key length: %d", len);
-			free(conn->scram_server_key_binary);
 			return false;
 		}
 		conn->scram_server_key_len = len;
@@ -5053,21 +5049,19 @@ freePGconn(PGconn *conn)
 		free(conn->events[i].name);
 	}
 
-	release_conn_addrinfo(conn);
-	pqReleaseConnHosts(conn);
-
-	free(conn->client_encoding_initial);
-	free(conn->events);
+	/* free everything not freed in pqClosePGconn */
 	free(conn->pghost);
 	free(conn->pghostaddr);
 	free(conn->pgport);
 	free(conn->connect_timeout);
 	free(conn->pgtcp_user_timeout);
+	free(conn->client_encoding_initial);
 	free(conn->pgoptions);
 	free(conn->appname);
 	free(conn->fbappname);
 	free(conn->dbName);
 	free(conn->replication);
+	free(conn->pgservice);
 	free(conn->pguser);
 	if (conn->pgpass)
 	{
@@ -5082,8 +5076,9 @@ freePGconn(PGconn *conn)
 	free(conn->keepalives_count);
 	free(conn->sslmode);
 	free(conn->sslnegotiation);
-	free(conn->sslcert);
+	free(conn->sslcompression);
 	free(conn->sslkey);
+	free(conn->sslcert);
 	if (conn->sslpassword)
 	{
 		explicit_bzero(conn->sslpassword, strlen(conn->sslpassword));
@@ -5093,32 +5088,40 @@ freePGconn(PGconn *conn)
 	free(conn->sslrootcert);
 	free(conn->sslcrl);
 	free(conn->sslcrldir);
-	free(conn->sslcompression);
 	free(conn->sslsni);
 	free(conn->requirepeer);
-	free(conn->require_auth);
-	free(conn->ssl_min_protocol_version);
-	free(conn->ssl_max_protocol_version);
 	free(conn->gssencmode);
 	free(conn->krbsrvname);
 	free(conn->gsslib);
 	free(conn->gssdelegation);
-	free(conn->connip);
-	/* Note that conn->Pfdebug is not ours to close or free */
-	free(conn->write_err_msg);
-	free(conn->inBuffer);
-	free(conn->outBuffer);
-	free(conn->rowBuf);
+	free(conn->min_protocol_version);
+	free(conn->max_protocol_version);
+	free(conn->ssl_min_protocol_version);
+	free(conn->ssl_max_protocol_version);
 	free(conn->target_session_attrs);
+	free(conn->require_auth);
 	free(conn->load_balance_hosts);
 	free(conn->scram_client_key);
 	free(conn->scram_server_key);
+	free(conn->sslkeylogfile);
 	free(conn->oauth_issuer);
 	free(conn->oauth_issuer_id);
 	free(conn->oauth_discovery_uri);
 	free(conn->oauth_client_id);
 	free(conn->oauth_client_secret);
 	free(conn->oauth_scope);
+	/* Note that conn->Pfdebug is not ours to close or free */
+	free(conn->events);
+	pqReleaseConnHosts(conn);
+	free(conn->connip);
+	release_conn_addrinfo(conn);
+	free(conn->scram_client_key_binary);
+	free(conn->scram_server_key_binary);
+	/* if this is a cancel connection, be_cancel_key may still be allocated */
+	free(conn->be_cancel_key);
+	free(conn->inBuffer);
+	free(conn->outBuffer);
+	free(conn->rowBuf);
 	termPQExpBuffer(&conn->errorMessage);
 	termPQExpBuffer(&conn->workBuffer);
 
@@ -5147,6 +5150,7 @@ pqReleaseConnHosts(PGconn *conn)
 			}
 		}
 		free(conn->connhost);
+		conn->connhost = NULL;
 	}
 }
 
