From: Sebastian Harl <sh@tokkee.org>
Date: Wed, 23 Jan 2013 08:26:48 +0000 (+0100)
Subject: postgresql plugin: Don't access realloc'ed memory.
X-Git-Tag: collectd-5.2.1~1^2
X-Git-Url: https://git.tokkee.org/?a=commitdiff_plain;h=7bbba254348a7fa98c4337455a3e0eae88ce0e86;p=collectd.git

postgresql plugin: Don't access realloc'ed memory.

D'oh! This is the most stupidest bug: all database connection objects are
stored in a plugin-global array and a pointer to each object was passed around
to the worker functions. This array used to be an array of objects (rather
than pointers) and was realloc'ed every time a new connection was added. Now,
the realloc caused the old pointer to not be valid any more, thus, any but the
last database connection were later accessing memory out of range. This caused
the plugin to segfault if more than one database connection was used.

Thanks to Greg Swift for reporting this!
Fixes Github issue #238.
---

diff --git a/src/postgresql.c b/src/postgresql.c
index 6c9ab44e..e7b247e7 100644
--- a/src/postgresql.c
+++ b/src/postgresql.c
@@ -170,14 +170,14 @@ static char *def_queries[] = {
 };
 static int def_queries_num = STATIC_ARRAY_SIZE (def_queries);
 
-static c_psql_database_t *databases     = NULL;
-static size_t             databases_num = 0;
+static c_psql_database_t **databases     = NULL;
+static size_t              databases_num = 0;
 
-static udb_query_t      **queries       = NULL;
-static size_t             queries_num   = 0;
+static udb_query_t       **queries       = NULL;
+static size_t              queries_num   = 0;
 
-static c_psql_writer_t   *writers       = NULL;
-static size_t             writers_num   = 0;
+static c_psql_writer_t    *writers       = NULL;
+static size_t              writers_num   = 0;
 
 static int c_psql_begin (c_psql_database_t *db)
 {
@@ -220,17 +220,25 @@ static int c_psql_commit (c_psql_database_t *db)
 
 static c_psql_database_t *c_psql_database_new (const char *name)
 {
-	c_psql_database_t *db;
+	c_psql_database_t **tmp;
+	c_psql_database_t  *db;
 
-	db = (c_psql_database_t *)realloc (databases,
-			(databases_num + 1) * sizeof (*db));
+	db = (c_psql_database_t *)malloc (sizeof(*db));
 	if (NULL == db) {
 		log_err ("Out of memory.");
 		return NULL;
 	}
 
-	databases = db;
-	db = databases + databases_num;
+	tmp = (c_psql_database_t **)realloc (databases,
+			(databases_num + 1) * sizeof (*databases));
+	if (NULL == tmp) {
+		log_err ("Out of memory.");
+		sfree (db);
+		return NULL;
+	}
+
+	databases = tmp;
+	databases[databases_num] = db;
 	++databases_num;
 
 	db->conn = NULL;
@@ -324,7 +332,9 @@ static void c_psql_database_delete (void *data)
 	sfree (db->service);
 
 	/* don't care about freeing or reordering the 'databases' array
-	 * this is done in 'shutdown' */
+	 * this is done in 'shutdown'; also, don't free the database instance
+	 * object just to make sure that in case anybody accesses it before
+	 * shutdown won't segfault */
 	return;
 } /* c_psql_database_delete */
 
@@ -956,17 +966,17 @@ static int c_psql_flush (cdtime_t timeout,
 		__attribute__((unused)) const char *ident,
 		user_data_t *ud)
 {
-	c_psql_database_t *dbs = databases;
+	c_psql_database_t **dbs = databases;
 	size_t dbs_num = databases_num;
 	size_t i;
 
 	if ((ud != NULL) && (ud->data != NULL)) {
-		dbs = ud->data;
+		dbs = (c_psql_database_t **)&ud->data;
 		dbs_num = 1;
 	}
 
 	for (i = 0; i < dbs_num; ++i) {
-		c_psql_database_t *db = dbs + i;
+		c_psql_database_t *db = dbs[i];
 
 		/* don't commit if the timeout is larger than the regular commit
 		 * interval as in that case all requested data has already been
@@ -986,7 +996,7 @@ static int c_psql_shutdown (void)
 	plugin_unregister_read_group ("postgresql");
 
 	for (i = 0; i < databases_num; ++i) {
-		c_psql_database_t *db = databases + i;
+		c_psql_database_t *db = databases[i];
 
 		if (db->writers_num > 0) {
 			char cb_name[DATA_MAX_NAME_LEN];
@@ -1001,6 +1011,8 @@ static int c_psql_shutdown (void)
 			plugin_unregister_flush (cb_name);
 			plugin_unregister_write (cb_name);
 		}
+
+		sfree (db);
 	}
 
 	udb_query_free (queries, queries_num);