[PATCH] Memory leak in pg_config
Hi,
I've been trying to run Postgis regress tests under Clang sanitizers and one of
the issues I'm facing is the constant stream of errors during the `configure`
step coming from calls to `pg_config`.
Example:
```
$ pg_config --cc
clang
=================================================================
==14521==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 368 byte(s) in 1 object(s) allocated from:
#0 0x55de20d161d9 in malloc (/usr/bin/pg_config+0xf81d9)
[...]
SUMMARY: AddressSanitizer: 2610 byte(s) leaked in 47 allocation(s).
```
The leaked memory is part of the `configdata` array which isn't freed before
exiting. It doesn't have any long term impact but it's annoying.
A similar thing happens in the `pg_config` SQL function. Since the memory
will be released at the end of the transaction, releasing it is optional but
I've done it anyway.
I'm attaching a the patch with the changes.
Greetings,
Greetings,
--
Raúl Marín Rodríguez
carto.com
Attachments:
configdata_leak-v1.patchtext/x-patch; charset=US-ASCII; name=configdata_leak-v1.patchDownload
From 0d35d7a21b87554df7ef27b70dcf7e4ea512699f Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Wed, 14 Nov 2018 09:08:50 +0100
Subject: [PATCH] pg_config: Avoid leaking configdata
---
src/backend/utils/misc/pg_config.c | 1 +
src/bin/pg_config/pg_config.c | 4 ++++
src/common/config_info.c | 15 ++++++++++++++-
src/include/common/config_info.h | 2 ++
4 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/misc/pg_config.c b/src/backend/utils/misc/pg_config.c
index aa434bc3ab..62d1aea287 100644
--- a/src/backend/utils/misc/pg_config.c
+++ b/src/backend/utils/misc/pg_config.c
@@ -78,6 +78,7 @@ pg_config(PG_FUNCTION_ARGS)
tuple = BuildTupleFromCStrings(attinmeta, values);
tuplestore_puttuple(tupstore, tuple);
}
+ free_configdata(configdata, configdata_len);
/*
* no longer need the tuple descriptor reference created by
diff --git a/src/bin/pg_config/pg_config.c b/src/bin/pg_config/pg_config.c
index a341b756de..c53a802422 100644
--- a/src/bin/pg_config/pg_config.c
+++ b/src/bin/pg_config/pg_config.c
@@ -160,6 +160,7 @@ main(int argc, char **argv)
{
for (i = 0; i < configdata_len; i++)
printf("%s = %s\n", configdata[i].name, configdata[i].setting);
+ free_configdata(configdata, configdata_len);
exit(0);
}
@@ -180,9 +181,12 @@ main(int argc, char **argv)
fprintf(stderr, _("%s: invalid argument: %s\n"),
progname, argv[i]);
advice();
+ free_configdata(configdata, configdata_len);
exit(1);
}
}
+ free_configdata(configdata, configdata_len);
+
return 0;
}
diff --git a/src/common/config_info.c b/src/common/config_info.c
index 55e688e656..f8da71c598 100644
--- a/src/common/config_info.c
+++ b/src/common/config_info.c
@@ -27,7 +27,7 @@
* get_configdata(const char *my_exec_path, size_t *configdata_len)
*
* Get configure-time constants. The caller is responsible
- * for pfreeing the result.
+ * for pfreeing the result [free_configdata]
*/
ConfigData *
get_configdata(const char *my_exec_path, size_t *configdata_len)
@@ -203,3 +203,16 @@ get_configdata(const char *my_exec_path, size_t *configdata_len)
return configdata;
}
+
+
+void
+free_configdata(ConfigData *configdata, size_t configdata_len)
+{
+ int i;
+ for (i = 0; i < configdata_len; i++)
+ {
+ pfree(configdata[i].name);
+ pfree(configdata[i].setting);
+ }
+ pfree(configdata);
+}
diff --git a/src/include/common/config_info.h b/src/include/common/config_info.h
index 72014a915a..26f85e86a9 100644
--- a/src/include/common/config_info.h
+++ b/src/include/common/config_info.h
@@ -18,4 +18,6 @@ typedef struct ConfigData
extern ConfigData *get_configdata(const char *my_exec_path,
size_t *configdata_len);
+extern void free_configdata(ConfigData *configdata, size_t configdata_len);
+
#endif /* COMMON_CONFIG_INFO_H */
--
2.19.1
=?UTF-8?B?UmHDumwgTWFyw61uIFJvZHLDrWd1ZXo=?= <rmrodriguez@carto.com> writes:
I've been trying to run Postgis regress tests under Clang sanitizers and one of
the issues I'm facing is the constant stream of errors during the `configure`
step coming from calls to `pg_config`.
TBH, I do not think we should do anything about this. It has never been
project policy that short-lived programs should free everything before
exiting, and I don't think we should change that. initdb, in particular,
would need a huge amount of work to meet such a policy, and it would
really be entirely wasted effort. Just because you've configured your
tools to enforce an unreasonable policy doesn't make it a reasonable one.
regards, tom lane
On 11/14/18 3:59 PM, Tom Lane wrote:
=?UTF-8?B?UmHDumwgTWFyw61uIFJvZHLDrWd1ZXo=?= <rmrodriguez@carto.com> writes:
I've been trying to run Postgis regress tests under Clang sanitizers and one of
the issues I'm facing is the constant stream of errors during the `configure`
step coming from calls to `pg_config`.TBH, I do not think we should do anything about this. It has never been
project policy that short-lived programs should free everything before
exiting, and I don't think we should change that. initdb, in particular,
would need a huge amount of work to meet such a policy, and it would
really be entirely wasted effort. Just because you've configured your
tools to enforce an unreasonable policy doesn't make it a reasonable one.
Yeah. Incidentally we had the same discussion about initdb a few days
ago [1]/messages/by-id/3fe1e38a-fb70-6260-9300-ce67ede21c32@redhat.com, and the conclusion was pretty much exactly the same.
[1]: /messages/by-id/3fe1e38a-fb70-6260-9300-ce67ede21c32@redhat.com
/messages/by-id/3fe1e38a-fb70-6260-9300-ce67ede21c32@redhat.com
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services