Re: [PATCH] Memory leak in pg_config

2018-11-14 Thread Raúl Marín Rodríguez
Hi,

I understand it, as I said it's not an issue; just annoying when using
sanitizers. Thanks for the information.


--
Raúl Marín Rodríguez
carto.com



Re: [PATCH] Memory leak in pg_config

2018-11-14 Thread Tomas Vondra

On 11/14/18 3:59 PM, Tom Lane wrote:

=?UTF-8?B?UmHDumwgTWFyw61uIFJvZHLDrWd1ZXo=?=  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], and the conclusion was pretty much exactly the same.


[1] 
https://www.postgresql.org/message-id/flat/3fe1e38a-fb70-6260-9300-ce67ede21c32%40redhat.com



regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: [PATCH] Memory leak in pg_config

2018-11-14 Thread Tom Lane
=?UTF-8?B?UmHDumwgTWFyw61uIFJvZHLDrWd1ZXo=?=  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



[PATCH] Memory leak in pg_config

2018-11-14 Thread Raúl Marín Rodríguez
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
From 0d35d7a21b87554df7ef27b70dcf7e4ea512699f Mon Sep 17 00:00:00 2001
From: Raul Marin 
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