Thanks for the review.
LeakSanitizer is widely used tools, so I think it
would be useful to make PostgreSQL work cleanly with them. We could
either fix reported leaks where cleanup is straightforward, or maintain
suppressions for intentional process-lifetime allocations. I prefer
fixing the leaks where possible.
We use these sanitizers in our own work, so we can continue examining
their reports across PostgreSQL, fix actual leaks, add suppressions where
appropriate, and submit the changes as separate patches.
For pg_config, the cleanup is small and straightforward. A broad
suppression for get_configdata() could also hide future leaks from the
same call path.
In v2, I restructured the control flow using if/else, so
free_configdata() is called only once before returning from main().
Invalid arguments now set the exit code and break out of the loop
instead of calling exit().
Personally, I prefer the simpler control flow used in v1.
I also corrected the function definition style.
The updated patch is attached.
On 26-07-21 21:35, Andrey Borodin wrote:
Hi Ivan,
LeakSanitizer reports 2829 bytes leaked in 47 allocations.
I can confirm the leak and the fix. The 47 allocations are exactly 23
names + 23 settings + 1 array. With your patch the tool reports zero.
Fun archaeology: this leak celebrates its 10th anniversary this year.
It arrived with a5c43b88694 (PG 9.6), which introduced get_configdata()
with the comment "the caller is responsible for pfreeing the result".
pg_config has been ignoring that contract ever since. Before 9.6 it
used static buffers and had nothing to leak at all. Simpler times.
One thing to be aware of: our CI runs sanitizers with
detect_leaks=0, and the comment there says "too many uninteresting
leak errors in short-lived binaries". This leak is a prime specimen
of the genre, so the answer here may well be "the OS frees it faster
than we do". If the goal is quiet LSan runs, a suppression list might
scale better: there are many binaries other than pg_config that leak
too. Anyway, I think it would be good to quite LSan if possible...
Two small notes:
- Validating the arguments before calling get_configdata() would
leave a single free before return instead of three call sites.
- The function definition puts "static" alone on its own line. The
usual style is "static void" together, then the function name on
the next line.
Thank you!
Best regards, Andrey Borodin.
--
Best wishes,
Ivan Kush
Tantor Labs LLC
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Ivan Kush <[email protected]>
Date: Tue, 21 Jul 2026 15:25:38 +0000
Subject: [PATCH v2] Fix memory leak in pg_config
---
src/bin/pg_config/pg_config.c | 48 ++++++++++++++++++++++++++++++++----------------
1 file changed, 32 insertions(+), 16 deletions(-)
diff --git a/src/bin/pg_config/pg_config.c b/src/bin/pg_config/pg_config.c
--- a/src/bin/pg_config/pg_config.c
+++ b/src/bin/pg_config/pg_config.c
@@ -11,6 +11,17 @@
}
}
+static void
+free_configdata(ConfigData *configdata, size_t configdata_len)
+{
+ for (size_t i = 0; i < configdata_len; i++)
+ {
+ pfree(configdata[i].name);
+ pfree(configdata[i].setting);
+ }
+ pfree(configdata);
+}
+
int
main(int argc, char **argv)
{
@@ -19,6 +30,7 @@
char my_exec_path[MAXPGPATH];
int i;
int j;
+ int exit_code = 0;
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_config"));
@@ -49,29 +61,33 @@
{
for (i = 0; i < configdata_len; i++)
printf("%s = %s\n", configdata[i].name, configdata[i].setting);
- exit(0);
}
-
- /* otherwise print requested items */
- for (i = 1; i < argc; i++)
+ else
{
- for (j = 0; info_items[j].switchname != NULL; j++)
+ /* otherwise print requested items */
+ for (i = 1; i < argc; i++)
{
- if (strcmp(argv[i], info_items[j].switchname) == 0)
+ for (j = 0; info_items[j].switchname != NULL; j++)
+ {
+ if (strcmp(argv[i], info_items[j].switchname) == 0)
+ {
+ show_item(info_items[j].configname,
+ configdata, configdata_len);
+ break;
+ }
+ }
+ if (info_items[j].switchname == NULL)
{
- show_item(info_items[j].configname,
- configdata, configdata_len);
+ fprintf(stderr, _("%s: invalid argument: %s\n"),
+ progname, argv[i]);
+ advice();
+ exit_code = 1;
break;
}
}
- if (info_items[j].switchname == NULL)
- {
- fprintf(stderr, _("%s: invalid argument: %s\n"),
- progname, argv[i]);
- advice();
- exit(1);
- }
}
- return 0;
+ free_configdata(configdata, configdata_len);
+
+ return exit_code;
}
--
2.39.2
