From fa703b317e9d261ffd34bbf5651ea29aff3ff0f0 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 13 Sep 2021 13:23:50 +0900
Subject: [PATCH] Fix error handling with threads on OOM in ECPG connection
 logic

An out-of-memory failure happening when allocating the structures to
store the connection parameter keywords and values would mess up with
the set of connections saved, as on failure the pthread mutex would
still be hold with the new connection object listed but free()'d.

Rather than just unlocking the mutex, which would leave the static list
of connections into an inconsistent state, move the allocation for the
structures of the connection parameters before beginning the test
manipulation.  This ensures that the list of connections and the
connection mutex remain consistent all the time in this code path.

This error is unlikely going to happen, but this could mess up badly
with ECPG clients in surprising ways, so backpatch all the way down.

Reported-by: ryancaicse
Discussion: https://postgr.es/m/17186-b4cfd8f0eb4d1dee@postgresql.org
Backpatch-through: 9.6
---
 src/interfaces/ecpg/ecpglib/connect.c | 75 +++++++++++++++------------
 1 file changed, 41 insertions(+), 34 deletions(-)

diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 60564b176c..a2166b1e12 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -458,6 +458,47 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
 	else
 		realname = NULL;
 
+	/*
+	 * Count options for the allocation done below (this may produce an
+	 * overestimate, it's ok).
+	 */
+	if (options)
+		for (i = 0; options[i]; i++)
+			if (options[i] == '=')
+				connect_params++;
+
+	if (user && strlen(user) > 0)
+		connect_params++;
+	if (passwd && strlen(passwd) > 0)
+		connect_params++;
+
+	/*
+	 * Allocate enough space for all connection parameters.  These allocations
+	 * are done before manipulating the list of connections to ease the error
+	 * handling on failure.
+	 */
+	conn_keywords = (const char **) ecpg_alloc((connect_params + 1) * sizeof(char *), lineno);
+	conn_values = (const char **) ecpg_alloc(connect_params * sizeof(char *), lineno);
+	if (conn_keywords == NULL || conn_values == NULL)
+	{
+		if (host)
+			ecpg_free(host);
+		if (port)
+			ecpg_free(port);
+		if (options)
+			ecpg_free(options);
+		if (realname)
+			ecpg_free(realname);
+		if (dbname)
+			ecpg_free(dbname);
+		if (conn_keywords)
+			ecpg_free(conn_keywords);
+		if (conn_values)
+			ecpg_free(conn_values);
+		free(this);
+		return false;
+	}
+
 	/* add connection to our list */
 #ifdef ENABLE_THREAD_SAFETY
 	pthread_mutex_lock(&connections_mutex);
@@ -488,40 +529,6 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
 			 options ? "with options " : "", options ? options : "",
 			 (user && strlen(user) > 0) ? "for user " : "", user ? user : "");
 
-	/* count options (this may produce an overestimate, it's ok) */
-	if (options)
-		for (i = 0; options[i]; i++)
-			if (options[i] == '=')
-				connect_params++;
-
-	if (user && strlen(user) > 0)
-		connect_params++;
-	if (passwd && strlen(passwd) > 0)
-		connect_params++;
-
-	/* allocate enough space for all connection parameters */
-	conn_keywords = (const char **) ecpg_alloc((connect_params + 1) * sizeof(char *), lineno);
-	conn_values = (const char **) ecpg_alloc(connect_params * sizeof(char *), lineno);
-	if (conn_keywords == NULL || conn_values == NULL)
-	{
-		if (host)
-			ecpg_free(host);
-		if (port)
-			ecpg_free(port);
-		if (options)
-			ecpg_free(options);
-		if (realname)
-			ecpg_free(realname);
-		if (dbname)
-			ecpg_free(dbname);
-		if (conn_keywords)
-			ecpg_free(conn_keywords);
-		if (conn_values)
-			ecpg_free(conn_values);
-		free(this);
-		return false;
-	}
-
 	i = 0;
 	if (realname)
 	{