Tidy up locale thread safety in ECPG library.

Remove setlocale() and _configthreadlocal() as fallback strategy on
systems that don't have uselocale(), where ECPG tries to control
LC_NUMERIC formatting on input and output of floating point numbers.  It
was probably broken on some systems (NetBSD), and the code was also
quite messy and complicated, with obsolete configure tests (Windows).
It was also arguably broken, or at least had unstated environmental
requirements, if pgtypeslib code was called directly.

Instead, introduce PG_C_LOCALE to refer to the "C" locale as a locale_t
value.  It maps to the special constant LC_C_LOCALE when defined by libc
(macOS, NetBSD), or otherwise uses a process-lifetime locale_t that is
allocated on first use, just as ECPG previously did itself.  The new
replacement might be more widely useful.  Then change the float parsing
and printing code to pass that to _l() functions where appropriate.

Unfortunately the portability of those functions is a bit complicated.
First, many obvious and useful _l() functions are missing from POSIX,
though most standard libraries define some of them anyway.  Second,
although the thread-safe save/restore technique can be used to replace
the missing ones, Windows and NetBSD refused to implement standard
uselocale().  They might have a point: "wide scope" uselocale() is hard
to combine with other code and error-prone, especially in library code.
Luckily they have the  _l() functions we want so far anyway.  So we have
to be prepared for both ways of doing things:

1.  In ECPG, use strtod_l() for parsing, and supply a port.h replacement
using uselocale() over a limited scope if missing.

2.  Inside our own snprintf.c, use three different approaches to format
floats.  For frontend code, call libc's snprintf_l(), or wrap libc's
snprintf() in uselocale() if it's missing.  For backend code, snprintf.c
can keep assuming that the global locale's LC_NUMERIC is "C" and call
libc's snprintf() without change, for now.

(It might eventually be possible to call our in-tree Ryū routines to
display floats in snprintf.c, given the C-locale-always remit of our
in-tree snprintf(), but this patch doesn't risk changing anything that
complicated.)

Author: Thomas Munro <thomas.munro@gmail.com>
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Reviewed-by: Tristan Partin <tristan@partin.io>
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://postgr.es/m/CWZBBRR6YA8D.8EHMDRGLCKCD%40neon.tech

configure

@@ -15401,7 +15401,7 @@ fi
 LIBS_including_readline="$LIBS"
 LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
 
-for ac_func in backtrace_symbols copyfile copy_file_range elf_aux_info getauxval getifaddrs getpeerucred inet_pton kqueue localeconv_l mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast strchrnul strsignal syncfs sync_file_range uselocale wcstombs_l
+for ac_func in backtrace_symbols copyfile copy_file_range elf_aux_info getauxval getifaddrs getpeerucred inet_pton kqueue localeconv_l mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast snprintf_l strchrnul strsignal strtod_l syncfs sync_file_range uselocale wcstombs_l
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"

configure.ac

@@ -1772,8 +1772,10 @@ AC_CHECK_FUNCS(m4_normalize([
 	pthread_is_threaded_np
 	setproctitle
 	setproctitle_fast
+	snprintf_l
 	strchrnul
 	strsignal
+	strtod_l
 	syncfs
 	sync_file_range
 	uselocale

meson.build

@@ -2753,6 +2753,7 @@ func_checks = [
   ['shm_open', {'dependencies': [rt_dep], 'define': false}],
   ['shm_unlink', {'dependencies': [rt_dep], 'define': false}],
   ['shmget', {'dependencies': [cygipc_dep], 'define': false}],
+  ['snprintf_l'],
   ['socket', {'dependencies': [socket_dep], 'define': false}],
   ['strchrnul'],
   ['strerror_r', {'dependencies': [thread_dep]}],
@@ -2761,6 +2762,7 @@ func_checks = [
   ['strnlen'],
   ['strsep'],
   ['strsignal'],
+  ['strtod_l'],
   ['sync_file_range'],
   ['syncfs'],
   ['uselocale'],

src/include/pg_config.h.in

@@ -355,6 +355,9 @@
 /* Define to 1 if you have the `setproctitle_fast' function. */
 #undef HAVE_SETPROCTITLE_FAST
 
+/* Define to 1 if you have the `snprintf_l' function. */
+#undef HAVE_SNPRINTF_L
+
 /* Define to 1 if the system has the type `socklen_t'. */
 #undef HAVE_SOCKLEN_T
 
@@ -400,6 +403,9 @@
 /* Define to 1 if you have the `strsignal' function. */
 #undef HAVE_STRSIGNAL
 
+/* Define to 1 if you have the `strtod_l' function. */
+#undef HAVE_STRTOD_L
+
 /* Define to 1 if the system has the type `struct option'. */
 #undef HAVE_STRUCT_OPTION
 

src/include/port.h

@@ -218,6 +218,37 @@ extern int	pg_fprintf(FILE *stream, const char *fmt,...) pg_attribute_printf(2,
 extern int	pg_vprintf(const char *fmt, va_list args) pg_attribute_printf(1, 0);
 extern int	pg_printf(const char *fmt,...) pg_attribute_printf(1, 2);
 
+/*
+ * A couple of systems offer a fast constant locale_t value representing the
+ * "C" locale.  We use that if possible, but fall back to creating a singleton
+ * object otherwise.  To check that it is available, call pg_ensure_c_locale()
+ * and assume out of memory if it returns false.
+ */
+#ifdef LC_C_LOCALE
+#define PG_C_LOCALE LC_C_LOCALE
+#define pg_ensure_c_locale() true
+#else
+extern locale_t pg_get_c_locale(void);
+#define PG_C_LOCALE pg_get_c_locale()
+#define pg_ensure_c_locale() (PG_C_LOCALE != 0)
+#endif
+
+#if !defined(HAVE_STRTOD_L) && !defined(WIN32)
+/*
+ * POSIX doesn't define this function, but we can implement it with thread-safe
+ * save-and-restore.
+ */
+static inline double
+strtod_l(const char *nptr, char **endptr, locale_t loc)
+{
+	locale_t	save = uselocale(loc);
+	double		result = strtod(nptr, endptr);
+
+	uselocale(save);
+	return result;
+}
+#endif
+
 #ifndef WIN32
 /*
  * We add a pg_ prefix as a warning that the Windows implementations have the

src/include/port/win32_port.h

@@ -453,6 +453,7 @@ extern int	_pglstat64(const char *name, struct stat *buf);
 #define isspace_l _isspace_l
 #define iswspace_l _iswspace_l
 #define strcoll_l _strcoll_l
+#define strtod_l _strtod_l
 #define strxfrm_l _strxfrm_l
 #define wcscoll_l _wcscoll_l
 

src/interfaces/ecpg/ecpglib/connect.c

@@ -10,10 +10,6 @@
 #include "ecpgtype.h"
 #include "sqlca.h"
 
-#ifdef HAVE_USELOCALE
-locale_t	ecpg_clocale = (locale_t) 0;
-#endif
-
 static pthread_mutex_t connections_mutex = PTHREAD_MUTEX_INITIALIZER;
 static pthread_key_t actual_connection_key;
 static pthread_once_t actual_connection_key_once = PTHREAD_ONCE_INIT;
@@ -268,7 +264,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
 	const char **conn_keywords;
 	const char **conn_values;
 
-	if (sqlca == NULL)
+	if (sqlca == NULL || !pg_ensure_c_locale())
 	{
 		ecpg_raise(lineno, ECPG_OUT_OF_MEMORY,
 				   ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
@@ -483,39 +479,6 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
 	/* add connection to our list */
 	pthread_mutex_lock(&connections_mutex);
 
-	/*
-	 * ... but first, make certain we have created ecpg_clocale.  Rely on
-	 * holding connections_mutex to ensure this is done by only one thread.
-	 */
-#ifdef HAVE_USELOCALE
-	if (!ecpg_clocale)
-	{
-		ecpg_clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
-		if (!ecpg_clocale)
-		{
-			pthread_mutex_unlock(&connections_mutex);
-			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

src/interfaces/ecpg/ecpglib/data.c

@@ -466,7 +466,7 @@ ecpg_get_data(const PGresult *results, int act_tuple, int act_field, int lineno,
 						pval++;
 
 					if (!check_special_value(pval, &dres, &scan_length))
-						dres = strtod(pval, &scan_length);
+						dres = strtod_l(pval, &scan_length, PG_C_LOCALE);
 
 					if (isarray && *scan_length == '"')
 						scan_length++;

src/interfaces/ecpg/ecpglib/descriptor.c

@@ -475,46 +475,9 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
 		memset(&stmt, 0, sizeof stmt);
 		stmt.lineno = lineno;
 
-		/* Make sure we do NOT honor the locale for numeric input */
-		/* since the database gives the standard decimal point */
-		/* (see comments in execute.c) */
-#ifdef HAVE_USELOCALE
-
-		/*
-		 * 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 WIN32
-		stmt.oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
-#endif
-		stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
-		setlocale(LC_NUMERIC, "C");
-#endif
-
 		/* desperate try to guess something sensible */
 		stmt.connection = ecpg_get_connection(NULL);
 		ecpg_store_result(ECPGresult, index, &stmt, &data_var);
-
-#ifdef HAVE_USELOCALE
-		if (stmt.oldlocale != (locale_t) 0)
-			uselocale(stmt.oldlocale);
-#else
-		if (stmt.oldlocale)
-		{
-			setlocale(LC_NUMERIC, stmt.oldlocale);
-			ecpg_free(stmt.oldlocale);
-		}
-#ifdef WIN32
-		if (stmt.oldthreadlocale != -1)
-			_configthreadlocale(stmt.oldthreadlocale);
-#endif
-#endif
 	}
 	else if (data_var.ind_type != ECPGt_NO_INDICATOR && data_var.ind_pointer != NULL)
 

src/interfaces/ecpg/ecpglib/ecpglib_extern.h

@@ -56,10 +56,6 @@ 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,14 +69,6 @@ struct statement
 	bool		questionmarks;
 	struct variable *inlist;
 	struct variable *outlist;
-#ifdef HAVE_USELOCALE
-	locale_t	oldlocale;
-#else
-	char	   *oldlocale;
-#ifdef WIN32
-	int			oldthreadlocale;
-#endif
-#endif
 	int			nparams;
 	char	  **paramvalues;
 	int		   *paramlengths;

src/interfaces/ecpg/ecpglib/execute.c

@@ -101,9 +101,6 @@ free_statement(struct statement *stmt)
 	free_variable(stmt->outlist);
 	ecpg_free(stmt->command);
 	ecpg_free(stmt->name);
-#ifndef HAVE_USELOCALE
-	ecpg_free(stmt->oldlocale);
-#endif
 	ecpg_free(stmt);
 }
 
@@ -1973,43 +1970,6 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
 	if (stmt == NULL)
 		return false;
 
-	/*
-	 * Make sure we do NOT honor the locale for numeric input/output since the
-	 * database wants the standard decimal point.  If available, use
-	 * uselocale() for this because it's thread-safe.  Windows doesn't have
-	 * that, but it does have _configthreadlocale().
-	 */
-#ifdef HAVE_USELOCALE
-
-	/*
-	 * 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);
-		return false;
-	}
-#else
-#ifdef WIN32
-	stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
-	if (stmt->oldthreadlocale == -1)
-	{
-		ecpg_do_epilogue(stmt);
-		return false;
-	}
-#endif
-	stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
-	if (stmt->oldlocale == NULL)
-	{
-		ecpg_do_epilogue(stmt);
-		return false;
-	}
-	setlocale(LC_NUMERIC, "C");
-#endif
-
 	/*
 	 * If statement type is ECPGst_prepnormal we are supposed to prepare the
 	 * statement before executing them
@@ -2216,19 +2176,6 @@ ecpg_do_epilogue(struct statement *stmt)
 	if (stmt == NULL)
 		return;
 
-#ifdef HAVE_USELOCALE
-	if (stmt->oldlocale != (locale_t) 0)
-		uselocale(stmt->oldlocale);
-#else
-	if (stmt->oldlocale)
-	{
-		setlocale(LC_NUMERIC, stmt->oldlocale);
-#ifdef WIN32
-		_configthreadlocale(stmt->oldthreadlocale);
-#endif
-	}
-#endif
-
 	free_statement(stmt);
 }
 

src/interfaces/ecpg/pgtypeslib/dt_common.c

@@ -1218,7 +1218,7 @@ DecodeNumber(int flen, char *str, int fmask,
 			return DecodeNumberField(flen, str, (fmask | DTK_DATE_M),
 									 tmask, tm, fsec, is2digits);
 
-		*fsec = strtod(cp, &cp);
+		*fsec = strtod_l(cp, &cp, PG_C_LOCALE);
 		if (*cp != '\0')
 			return -1;
 	}
@@ -2030,7 +2030,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
 							{
 								double		frac;
 
-								frac = strtod(cp, &cp);
+								frac = strtod_l(cp, &cp, PG_C_LOCALE);
 								if (*cp != '\0')
 									return -1;
 								*fsec = frac * 1000000;
@@ -2054,7 +2054,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
 							{
 								double		time;
 
-								time = strtod(cp, &cp);
+								time = strtod_l(cp, &cp, PG_C_LOCALE);
 								if (*cp != '\0')
 									return -1;
 

src/interfaces/ecpg/pgtypeslib/interval.c

@@ -60,7 +60,7 @@ ParseISO8601Number(const char *str, char **endptr, int *ipart, double *fpart)
 	if (!(isdigit((unsigned char) *str) || *str == '-' || *str == '.'))
 		return DTERR_BAD_FORMAT;
 	errno = 0;
-	val = strtod(str, endptr);
+	val = strtod_l(str, endptr, PG_C_LOCALE);
 	/* did we not see anything that looks like a double? */
 	if (*endptr == str || errno != 0)
 		return DTERR_BAD_FORMAT;
@@ -455,7 +455,7 @@ DecodeInterval(char **field, int *ftype, int nf,	/* int range, */
 				else if (*cp == '.')
 				{
 					errno = 0;
-					fval = strtod(cp, &cp);
+					fval = strtod_l(cp, &cp, PG_C_LOCALE);
 					if (*cp != '\0' || errno != 0)
 						return DTERR_BAD_FORMAT;
 

src/interfaces/ecpg/pgtypeslib/numeric.c

@@ -1455,7 +1455,7 @@ numericvar_to_double(numeric *var, double *dp)
 	 * strtod does not reset errno to 0 in case of success.
 	 */
 	errno = 0;
-	val = strtod(tmp, &endptr);
+	val = strtod_l(tmp, &endptr, PG_C_LOCALE);
 	if (errno == ERANGE)
 	{
 		free(tmp);

src/port/Makefile

@@ -41,6 +41,7 @@ OBJS = \
 	bsearch_arg.o \
 	chklocale.o \
 	inet_net_ntop.o \
+	locale.o \
 	noblock.o \
 	path.o \
 	pg_bitutils.o \

src/port/locale.c

@@ -0,0 +1,84 @@
+/*-------------------------------------------------------------------------
+ *
+ * locale.c
+ *		Helper routines for thread-safe system locale usage.
+ *
+ *
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *	  src/port/locale.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "c.h"
+
+#ifndef LC_C_LOCALE
+
+#ifndef WIN32
+#include <pthread.h>
+#else
+#include <synchapi.h>
+#endif
+
+/* A process-lifetime singleton, allocated on first need. */
+static locale_t c_locale;
+
+#ifndef WIN32
+static void
+init_c_locale_once(void)
+{
+	c_locale = newlocale(LC_ALL, "C", NULL);
+}
+#else
+static BOOL
+init_c_locale_once(PINIT_ONCE once, PVOID parameter, PVOID *context)
+{
+	c_locale = _create_locale(LC_ALL, "C");
+	return true;
+}
+#endif
+
+/*
+ * Access a process-lifetime singleton locale_t object.  Use the macro
+ * PG_C_LOCALE instead of calling this directly, as it can skip the function
+ * call on some systems.
+ */
+locale_t
+pg_get_c_locale(void)
+{
+	/*
+	 * Fast path if already initialized.  This assumes that we can read a
+	 * locale_t (in practice, a pointer) without tearing in a multi-threaded
+	 * program.
+	 */
+	if (c_locale != (locale_t) 0)
+		return c_locale;
+
+	/* Make a locale_t.  It will live until process exit. */
+	{
+#ifndef WIN32
+		static pthread_once_t once = PTHREAD_ONCE_INIT;
+
+		pthread_once(&once, init_c_locale_once);
+#else
+		static INIT_ONCE once;
+		InitOnceExecuteOnce(&once, init_c_locale_once, NULL, NULL);
+#endif
+	}
+
+	/*
+	 * It's possible that the allocation of the locale failed due to low
+	 * memory, and then (locale_t) 0 will be returned.  Users of PG_C_LOCALE
+	 * should defend against that by checking pg_ensure_c_locale() at a
+	 * convenient time, so that they can treat it as a simple constant after
+	 * that.
+	 */
+
+	return c_locale;
+}
+
+#endif							/* not LC_C_LOCALE */

src/port/meson.build

@@ -4,6 +4,7 @@ pgport_sources = [
   'bsearch_arg.c',
   'chklocale.c',
   'inet_net_ntop.c',
+  'locale.c',
   'noblock.c',
   'path.c',
   'pg_bitutils.c',

src/port/snprintf.c

@@ -109,6 +109,36 @@
 #undef	vprintf
 #undef	printf
 
+#if defined(FRONTEND)
+/*
+ * Frontend code might be multi-threaded.  When calling the system snprintf()
+ * for floats, we have to use either the non-standard snprintf_l() variant, or
+ * save-and-restore the calling thread's locale using uselocale(), depending on
+ * availability.
+ */
+#if defined(HAVE_SNPRINTF_L)
+/* At least macOS and the BSDs have the snprintf_l() extension. */
+#define USE_SNPRINTF_L
+#elif defined(WIN32)
+/* Windows has a version with a different name and argument order. */
+#define snprintf_l(str, size, loc, format, ...) _snprintf_l(str, size, format, loc, __VA_ARGS__)
+#define USE_SNPRINTF_L
+#else
+/* We have to do a thread-safe save-and-restore around snprintf(). */
+#define NEED_USE_LOCALE
+#endif
+#else
+/*
+ * Backend code doesn't need to worry about locales here, because LC_NUMERIC is
+ * set to "C" in main() and doesn't change.  Plain old snprintf() is always OK
+ * without uselocale().
+ *
+ * XXX If the backend were multithreaded, we would have to be 100% certain that
+ * no one is calling setlocale() concurrently, even transiently, to be able to
+ * keep this small optimization.
+ */
+#endif
+
 /*
  * Info about where the formatted output is going.
  *
@@ -1220,6 +1250,9 @@ fmtfloat(double value, char type, int forcesign, int leftjust,
 		 * according to == but not according to memcmp.
 		 */
 		static const double dzero = 0.0;
+#ifdef NEED_USE_LOCALE
+		locale_t	save_locale = uselocale(PG_C_LOCALE);
+#endif
 
 		if (adjust_sign((value < 0.0 ||
 						 (value == 0.0 &&
@@ -1241,7 +1274,11 @@ fmtfloat(double value, char type, int forcesign, int leftjust,
 			fmt[2] = '*';
 			fmt[3] = type;
 			fmt[4] = '\0';
+#ifdef USE_SNPRINTF_L
+			vallen = snprintf_l(convert, sizeof(convert), PG_C_LOCALE, fmt, prec, value);
+#else
 			vallen = snprintf(convert, sizeof(convert), fmt, prec, value);
+#endif
 		}
 		else
 		{
@@ -1250,6 +1287,11 @@ fmtfloat(double value, char type, int forcesign, int leftjust,
 			fmt[2] = '\0';
 			vallen = snprintf(convert, sizeof(convert), fmt, value);
 		}
+
+#ifdef NEED_USE_LOCALE
+		uselocale(save_locale);
+#endif
+
 		if (vallen < 0)
 			goto fail;
 
@@ -1372,12 +1414,25 @@ pg_strfromd(char *str, size_t count, int precision, double value)
 		}
 		else
 		{
+#ifdef NEED_USE_LOCALE
+			locale_t	save_locale = uselocale(PG_C_LOCALE);
+#endif
+
 			fmt[0] = '%';
 			fmt[1] = '.';
 			fmt[2] = '*';
 			fmt[3] = 'g';
 			fmt[4] = '\0';
+#ifdef USE_SNPRINTF_L
+			vallen = snprintf_l(convert, sizeof(convert), PG_C_LOCALE, fmt, precision, value);
+#else
 			vallen = snprintf(convert, sizeof(convert), fmt, precision, value);
+#endif
+
+#ifdef NEED_USE_LOCALE
+			uselocale(save_locale);
+#endif
+
 			if (vallen < 0)
 			{
 				target.failed = true;