[PATCH] Fix Potential Memory Leak in pg_amcheck Code
Dear pgsql-hackers,
While reviewing the code of pg_amcheck, I discovered a potential issue that could lead to a memory leak. Specifically, there is a missing PQclear(result) call before the database connection is disconnected, which could leave the query result unfreed.
To resolve this, I have created a patch that ensures PQclear(result) is called prior to the disconnectDatabase(conn) function. This small change ensures proper memory management by cleaning up the query result before closing the connection.
Please find the patch attached for your review.
I would appreciate any feedback or suggestions on this fix. If it looks good, I would be happy to proceed with any necessary steps to contribute it to the project.
Thank you for your time and consideration.
--
Best regards,
JiaoShuntian,
Highgo Software.
Attachments:
0001-pg_amcheck-prevent-potential-memory-leak.patchapplication/octet-stream; charset=gb18030; name=0001-pg_amcheck-prevent-potential-memory-leak.patchDownload
From 5226bbe1eac49bf072ce06b00d20a1aab3ea8529 Mon Sep 17 00:00:00 2001
From: JiaoShuntian <jiaoshuntian@highgo.com>
Date: Fri, 7 Feb 2025 14:42:50 +0800
Subject: pg_amcheck prevent potential memory leak by adding PQclear before
disconnectDatabase
---
src/bin/pg_amcheck/pg_amcheck.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index c5ec25be22..996833ed40 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -587,6 +587,7 @@ main(int argc, char *argv[])
/* Querying the catalog succeeded, but amcheck is missing. */
pg_log_warning("skipping database \"%s\": amcheck is not installed",
PQdb(conn));
+ PQclear(result);
disconnectDatabase(conn);
conn = NULL;
continue;
--
2.43.5
On 7 Feb 2025, at 08:04, Saladin <312199339@qq.com> wrote:
Dear pgsql-hackers,
While reviewing the code of pg_amcheck, I discovered a potential issue that could lead to a memory leak. Specifically, there is a missing PQclear(result) call before the database connection is disconnected, which could leave the query result unfreed.
To resolve this, I have created a patch that ensures PQclear(result) is called prior to the disconnectDatabase(conn) function. This small change ensures proper memory management by cleaning up the query result before closing the connection.
We typically don't worry too much about small leaks in client applications, as
they will be cleaned up when the program exits. That being said, there is
value in doing the right thing and setting good examples in our own code as
many do read it and reference it. This fix ties in nicely with f3e4aeb744da
which fixed another libpq usage pattern (albeit a more interesting one).
There are two more cases of not calling PQclear() before disconnecting, both of
which are immediately followed by exit(1) calls. We call PQclear on such a
case elsewhere in the file so it's not entirely consistent, but it's not all
that important as the memory will be reclaimed at exit.
I would appreciate any feedback or suggestions on this fix. If it looks good, I would be happy to proceed with any necessary steps to contribute it to the project.
Since you've submitted a patch you have already performed the necessary steps,
thanks for your contribution!
--
Daniel Gustafsson
Hi!
On Fri, 7 Feb 2025 at 13:28, Daniel Gustafsson <daniel@yesql.se> wrote:
That being said, there is
value in doing the right thing and setting good examples in our own code as
many do read it and reference it.
We call PQclear on such a
case elsewhere in the file so it's not entirely consistent, but it's not all
that important as the memory will be reclaimed at exit.
So, are we +1 or -1 on moving this forward?
--
Best regards,
Kirill Reshke
On Sun, 23 Feb 2025 at 07:49, Kirill Reshke <reshkekirill@gmail.com> wrote:
So, are we +1 or -1 on moving this forward?
+1 from me. Marked as ready for committer.
On 23 Feb 2025, at 13:03, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
On Sun, 23 Feb 2025 at 07:49, Kirill Reshke <reshkekirill@gmail.com> wrote:
So, are we +1 or -1 on moving this forward?
+1 from me. Marked as ready for committer.
Agreed, I have it on my TODO for the week to revisit.
./daniel