newlocale-once-ecpg-v1.patch

text/plain

Filename: newlocale-once-ecpg-v1.patch
Type: text/plain
Part: 0
Message: Re: Probable memory leak with ECPG and AIX

Patch

Format: unified
Series: patch v1
File+
src/interfaces/ecpg/ecpglib/connect.c 35 0
src/interfaces/ecpg/ecpglib/descriptor.c 10 5
src/interfaces/ecpg/ecpglib/ecpglib_extern.h 4 1
src/interfaces/ecpg/ecpglib/execute.c 17 23
Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    ecpglib: call newlocale() one per process.
    
    ecpglib has been calling it once per SQL query and once per EXEC SQL GET
    DESCRIPTOR.  Instead, if newlocale() has not succeeded before, call it
    while establishing a connection.  This mitigates three problems:
    - If newlocale() failed in EXEC SQL GET DESCRIPTOR, the command silently
      proceeded without the intended locale change.
    - On AIX, each newlocale()+freelocale() cycle leaked memory.
    - newlocale() CPU usage may have been nontrivial.
    
    Fail the connection attempt if newlocale() fails.  Rerrange
    ecpg_do_prologue() to validate the connection before its uselocale().
    
    The sort of program that may regress is one running in an environment
    where newlocale() fails.  If that program establishes connections
    without running SQL statements, it will stop working in response to this
    change.  I'm betting against the importance of such an ECPG use case.
    Most SQL execution (any using ECPGdo()) has long required newlocale()
    success, so there's little a connection could do without newlocale().
    
    Back-patch to v10 (all supported versions).
    
    Reviewed by FIXME.  Reported by Guillaume Lelarge.
    
    Discussion: https://postgr.es/m/20220101074055.GA54621@rfd.leadboat.com

diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 8dfcabe..2e4b003 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -10,6 +10,10 @@
 #include "ecpgtype.h"
 #include "sqlca.h"
 
+#ifdef HAVE_USELOCALE
+locale_t	ecpg_clocale;
+#endif
+
 #ifdef ENABLE_THREAD_SAFETY
 static pthread_mutex_t connections_mutex = PTHREAD_MUTEX_INITIALIZER;
 static pthread_key_t actual_connection_key;
@@ -502,6 +506,37 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
 #ifdef ENABLE_THREAD_SAFETY
 	pthread_mutex_lock(&connections_mutex);
 #endif
+#ifdef HAVE_USELOCALE
+	if (!ecpg_clocale)
+	{
+		ecpg_clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
+		if (!ecpg_clocale)
+		{
+#ifdef ENABLE_THREAD_SAFETY
+			pthread_mutex_unlock(&connections_mutex);
+#endif
+			ecpg_raise(lineno, ECPG_OUT_OF_MEMORY,
+					   ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, 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;
+		}
+	}
+#endif
+
 	if (connection_name != NULL)
 		this->name = ecpg_strdup(connection_name, lineno);
 	else
diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c
index 369c2f0..f1898de 100644
--- a/src/interfaces/ecpg/ecpglib/descriptor.c
+++ b/src/interfaces/ecpg/ecpglib/descriptor.c
@@ -486,9 +486,16 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
 		/* since the database gives the standard decimal point */
 		/* (see comments in execute.c) */
 #ifdef HAVE_USELOCALE
-		stmt.clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
-		if (stmt.clocale != (locale_t) 0)
-			stmt.oldlocale = uselocale(stmt.clocale);
+
+		/*
+		 * To get here, the above PQnfields() test must have found nonzero
+		 * fields.  One needs a connection to create such a descriptor.  (EXEC
+		 * SQL SET DESCRIPTOR can populate the descriptor's "items", but it
+		 * can't change the descriptor's PQnfields().)  Any successful
+		 * connection initializes ecpg_clocale.
+		 */
+		Assert(ecpg_clocale);
+		stmt.oldlocale = uselocale(ecpg_clocale);
 #else
 #ifdef HAVE__CONFIGTHREADLOCALE
 		stmt.oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
@@ -504,8 +511,6 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
 #ifdef HAVE_USELOCALE
 		if (stmt.oldlocale != (locale_t) 0)
 			uselocale(stmt.oldlocale);
-		if (stmt.clocale)
-			freelocale(stmt.clocale);
 #else
 		if (stmt.oldlocale)
 		{
diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 1a98dea..c438cfb 100644
--- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
+++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
@@ -59,6 +59,10 @@ struct ECPGtype_information_cache
 	enum ARRAY_TYPE isarray;
 };
 
+#ifdef HAVE_USELOCALE
+extern locale_t ecpg_clocale;	/* LC_NUMERIC=C */
+#endif
+
 /* structure to store one statement */
 struct statement
 {
@@ -73,7 +77,6 @@ struct statement
 	struct variable *inlist;
 	struct variable *outlist;
 #ifdef HAVE_USELOCALE
-	locale_t	clocale;
 	locale_t	oldlocale;
 #else
 	char	   *oldlocale;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 6a7ef0b..2ebe665 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -101,10 +101,7 @@ free_statement(struct statement *stmt)
 	free_variable(stmt->outlist);
 	ecpg_free(stmt->command);
 	ecpg_free(stmt->name);
-#ifdef HAVE_USELOCALE
-	if (stmt->clocale)
-		freelocale(stmt->clocale);
-#else
+#ifndef HAVE_USELOCALE
 	ecpg_free(stmt->oldlocale);
 #endif
 	ecpg_free(stmt);
@@ -1965,6 +1962,15 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
 		return false;
 	}
 
+#ifdef ENABLE_THREAD_SAFETY
+	ecpg_pthreads_init();
+#endif
+
+	con = ecpg_get_connection(connection_name);
+
+	if (!ecpg_init(con, connection_name, lineno))
+		return false;
+
 	stmt = (struct statement *) ecpg_alloc(sizeof(struct statement), lineno);
 
 	if (stmt == NULL)
@@ -1979,13 +1985,13 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
 	 * treat that situation as if the function doesn't exist.
 	 */
 #ifdef HAVE_USELOCALE
-	stmt->clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
-	if (stmt->clocale == (locale_t) 0)
-	{
-		ecpg_do_epilogue(stmt);
-		return false;
-	}
-	stmt->oldlocale = uselocale(stmt->clocale);
+
+	/*
+	 * Since ecpg_init() succeeded, we have a connection.  Any successful
+	 * connection initializes ecpg_clocale.
+	 */
+	Assert(ecpg_clocale);
+	stmt->oldlocale = uselocale(ecpg_clocale);
 	if (stmt->oldlocale == (locale_t) 0)
 	{
 		ecpg_do_epilogue(stmt);
@@ -2004,18 +2010,6 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
 	setlocale(LC_NUMERIC, "C");
 #endif
 
-#ifdef ENABLE_THREAD_SAFETY
-	ecpg_pthreads_init();
-#endif
-
-	con = ecpg_get_connection(connection_name);
-
-	if (!ecpg_init(con, connection_name, lineno))
-	{
-		ecpg_do_epilogue(stmt);
-		return false;
-	}
-
 	/*
 	 * If statement type is ECPGst_prepnormal we are supposed to prepare the
 	 * statement before executing them