On Thu, Aug 29, 2024 at 6:50 AM Peter Eisentraut <pe...@eisentraut.org> wrote: > I took a look at this. It was quite a complicated discussion that led > to this, but I agree with the solution that was arrived at.
Sorry I missed/forgot this prior feedback when posting earlier today. Responses to the points you raised here: > Also, you're removing the configure test for _configthreadlocale(). > Presumably because you're removing all the uses. But wouldn't we need > that back later in the backend maybe? Or is that test even relevant > anymore, that is, are there Windows versions that don't have it? Yeah, this removes the uses. As it happens, in the thread about localeconv() I am also proposing to add a new user of it[1], but even then I don't think we'd need the configure test, because all relevant systems have it. It's in ucrt, the modern C runtime that replaced the old msvcrt, as used by Visual Studio 2015+ and Windows 10+, and also by MinGW in recent years. I think it was also in some versions of msvcrt, but MinGW had issues with that apparently. It's pretty confusing, so I went looking for proof and clues about the history. Here's a thread about 2019 animal jacana lacking the function: https://www.postgresql.org/message-id/flat/31420.1547783697%40sss.pgh.pa.us Once jacana was upgraded, it started finding the function, but next failed with -1 at runtime, cf 2cf91ccb. Grovelling through MinGW-w64's github, it looks like they added a dummy function that fails with -1 in 2016: https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-crt/misc/_configthreadlocale.c#L11 They only seem to interpose the dummy function when building against msvcrt. https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-crt/Makefile.am#L298 The default and recommended runtime library as of a few years ago is ucrt: https://www.msys2.org/docs/environments/#msvcrt-vs-ucrt The three MinGW environments we test today are using ucrt, and configure detects the symbol on all. Namely: fairwren (msys2/mingw64), the CI mingw64 task and the mingw cross-build that runs on Linux in the CI CompilerWarnings task. As far as I know these are the reasons for, and mechanism by which, we keep MinGW support working. We have no policy requiring arbitrary old MinGW systems work, and we wouldn't know anyway. I was paranoid that it might still be a dummy function, so I tried running a simple program on our CI with the MinGW compiler to confirm it's reaching the real library: int main() { printf("hello world\n"); printf("got %d\n", _configthreadlocale(_ENABLE_PER_THREAD_LOCALE)); printf("got %d\n", _configthreadlocale(_DISABLE_PER_THREAD_LOCALE)); } [06:16:39.580] c:\cirrus>call C:\msys64\usr\bin\bash.exe -l -c "gcc doit.c -o c:/doit" ... [06:16:41.474] c:\cirrus>call c:\doit.exe [06:16:41.482] hello world [06:16:41.482] got 2 [06:16:41.482] got 1 It's not clear to me whether or when we might have made any changes already that prevented PostgreSQL from working with msvcrt, of course, but I'm pretty sure if someone proposed that we *should* support it, we'd reject the proposal. That'd effectively be a different, obsolete one corresponding to Windows versions we've dropped, which wouldn't make any sense. Other mentions of msvcrt in win32env.c and pg_locale.c may also be effectively obsolete now too. > For Windows, we already have things like > > #define strcoll_l _strcoll_l > > in src/include/port/win32_port.h, so it would seem more sensible to add > strtod_l to that list, instead of in port.h. OK, I moved that line into that list. I left the implementation for Unixen (at a guess: probably just Solaris?) there. > The error handling with pg_ensure_c_locale(), that's the sort of thing > I'm afraid will be hard to test or even know how it will behave. And it > creates this weird coupling between pgtypeslib and ecpglib that you > mentioned earlier. And if there are other users of PG_C_LOCALE in the > future, there will be similar questions about the proper initialization > and error handling sequence. Ack. The coupling does become at least less weird (currently it must be capable of giving the wrong answers reliably if called directly I think, no?) and weaker, but I acknowledge that it's still non-ideal that out-of-memory would be handled nicely only if you used ecpg first, and subtle misbehaviour would ensure otherwise, and future users face the same sort of problem unless they design in a reasonable initialisation place that could check pg_ensure_c_locale() nicely. Classic retro-fitting problem, though. > I would consider instead making a local static variable in each function > that needs this. For example, numericvar_to_double() might do > > { > static locale_t c_locale; > > if (!c_locale) > { > c_locale = pg_get_c_locale(); > if (!c_locale) > return -1; /* local error reporting convention */ > } > > ... > } > > This is a bit more code in total, but then you only initialize what you > need and you can handle errors locally. Hmm. Seems like quite a lot of duplication, and also assumes that we can actually unwind and handle the error in a reasonable way at all callsites. Let me think about that some more... [1] https://www.postgresql.org/message-id/flat/ca+hukgjqve0+pv9dvc9dsums_pxxgo9swcxyambguwjugbw...@mail.gmail.com
From 2965979a7180265a8df91adb45c1ce32f05b190f Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Sat, 10 Aug 2024 11:01:21 +1200 Subject: [PATCH v6] Tidy up locale thread safety in ECPG library. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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.) Reviewed-by: Peter Eisentraut <pe...@eisentraut.org> Reviewed-by: Tristan Partin <tris...@partin.io> Discussion: https://postgr.es/m/CWZBBRR6YA8D.8EHMDRGLCKCD%40neon.tech --- configure | 13 +--- configure.ac | 3 +- meson.build | 3 +- src/include/pg_config.h.in | 9 ++- src/include/port.h | 31 ++++++++ src/include/port/win32_port.h | 1 + src/interfaces/ecpg/ecpglib/connect.c | 39 +--------- src/interfaces/ecpg/ecpglib/data.c | 2 +- src/interfaces/ecpg/ecpglib/descriptor.c | 37 --------- src/interfaces/ecpg/ecpglib/ecpglib_extern.h | 12 --- src/interfaces/ecpg/ecpglib/execute.c | 55 -------------- src/interfaces/ecpg/pgtypeslib/dt_common.c | 6 +- src/interfaces/ecpg/pgtypeslib/interval.c | 4 +- src/interfaces/ecpg/pgtypeslib/numeric.c | 2 +- src/port/Makefile | 1 + src/port/locale.c | 80 ++++++++++++++++++++ src/port/meson.build | 1 + src/port/snprintf.c | 55 ++++++++++++++ 18 files changed, 188 insertions(+), 166 deletions(-) create mode 100644 src/port/locale.c diff --git a/configure b/configure index f58eae1baa8..6a133c09d3b 100755 --- a/configure +++ b/configure @@ -15039,7 +15039,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 getifaddrs getpeerucred inet_pton kqueue 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 getifaddrs getpeerucred inet_pton kqueue mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast snprintf_l strtod_l strchrnul strsignal 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" @@ -15812,17 +15812,6 @@ fi # Win32 (really MinGW) support if test "$PORTNAME" = "win32"; then - for ac_func in _configthreadlocale -do : - ac_fn_c_check_func "$LINENO" "_configthreadlocale" "ac_cv_func__configthreadlocale" -if test "x$ac_cv_func__configthreadlocale" = xyes; then : - cat >>confdefs.h <<_ACEOF -#define HAVE__CONFIGTHREADLOCALE 1 -_ACEOF - -fi -done - case " $LIBOBJS " in *" dirmod.$ac_objext "* ) ;; *) LIBOBJS="$LIBOBJS dirmod.$ac_objext" diff --git a/configure.ac b/configure.ac index 82c5009e3e8..e73e0985862 100644 --- a/configure.ac +++ b/configure.ac @@ -1727,6 +1727,8 @@ AC_CHECK_FUNCS(m4_normalize([ pthread_is_threaded_np setproctitle setproctitle_fast + snprintf_l + strtod_l strchrnul strsignal syncfs @@ -1816,7 +1818,6 @@ fi # Win32 (really MinGW) support if test "$PORTNAME" = "win32"; then - AC_CHECK_FUNCS(_configthreadlocale) AC_LIBOBJ(dirmod) AC_LIBOBJ(kill) AC_LIBOBJ(open) diff --git a/meson.build b/meson.build index 5b0510cef78..dc557a317df 100644 --- a/meson.build +++ b/meson.build @@ -2614,7 +2614,6 @@ endif # XXX: Might be worth conditioning some checks on the OS, to avoid doing # unnecessary checks over and over, particularly on windows. func_checks = [ - ['_configthreadlocale', {'skip': host_system != 'windows'}], ['backtrace_symbols', {'dependencies': [execinfo_dep]}], ['clock_gettime', {'dependencies': [rt_dep], 'define': false}], ['copyfile'], @@ -2646,6 +2645,8 @@ func_checks = [ ['shm_open', {'dependencies': [rt_dep], 'define': false}], ['shm_unlink', {'dependencies': [rt_dep], 'define': false}], ['shmget', {'dependencies': [cygipc_dep], 'define': false}], + ['snprintf_l'], + ['strtod_l'], ['socket', {'dependencies': [socket_dep], 'define': false}], ['strchrnul'], ['strerror_r', {'dependencies': [thread_dep]}], diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index cdd9a6e9355..10c05775ff2 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -349,6 +349,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 @@ -397,6 +400,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 @@ -529,9 +535,6 @@ /* Define to 1 if your compiler understands __builtin_unreachable. */ #undef HAVE__BUILTIN_UNREACHABLE -/* Define to 1 if you have the `_configthreadlocale' function. */ -#undef HAVE__CONFIGTHREADLOCALE - /* Define to 1 if you have __cpuid. */ #undef HAVE__CPUID diff --git a/src/include/port.h b/src/include/port.h index ba9ab0d34f4..48292cb28f8 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -217,6 +217,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 diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h index 7789e0431aa..8dbde7da954 100644 --- a/src/include/port/win32_port.h +++ b/src/include/port/win32_port.h @@ -463,6 +463,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 diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c index b24b310ce59..78e501a2656 100644 --- a/src/interfaces/ecpg/ecpglib/connect.c +++ b/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 diff --git a/src/interfaces/ecpg/ecpglib/data.c b/src/interfaces/ecpg/ecpglib/data.c index fa562767585..856f4c9472d 100644 --- a/src/interfaces/ecpg/ecpglib/data.c +++ b/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++; diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c index ad279e245c4..855322e037f 100644 --- a/src/interfaces/ecpg/ecpglib/descriptor.c +++ b/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 HAVE__CONFIGTHREADLOCALE - 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 HAVE__CONFIGTHREADLOCALE - if (stmt.oldthreadlocale != -1) - (void) _configthreadlocale(stmt.oldthreadlocale); -#endif -#endif } else if (data_var.ind_type != ECPGt_NO_INDICATOR && data_var.ind_pointer != NULL) diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h index bad3cd9920b..40988d53575 100644 --- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h +++ b/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 HAVE__CONFIGTHREADLOCALE - int oldthreadlocale; -#endif -#endif int nparams; char **paramvalues; int *paramlengths; diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c index c578c21cf66..ec939d0f63a 100644 --- a/src/interfaces/ecpg/ecpglib/execute.c +++ b/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,40 +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 usually does have _configthreadlocale(). In some versions - * of MinGW, _configthreadlocale() exists but always returns -1 --- so - * treat that situation as if the function doesn't exist. - */ -#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 HAVE__CONFIGTHREADLOCALE - stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); -#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 @@ -2213,24 +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 HAVE__CONFIGTHREADLOCALE - - /* - * This is a bit trickier than it looks: if we failed partway through - * statement initialization, oldthreadlocale could still be 0. But that's - * okay because a call with 0 is defined to be a no-op. - */ - if (stmt->oldthreadlocale != -1) - (void) _configthreadlocale(stmt->oldthreadlocale); -#endif -#endif - free_statement(stmt); } diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c b/src/interfaces/ecpg/pgtypeslib/dt_common.c index c4119ab7932..f8349b66ce9 100644 --- a/src/interfaces/ecpg/pgtypeslib/dt_common.c +++ b/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; diff --git a/src/interfaces/ecpg/pgtypeslib/interval.c b/src/interfaces/ecpg/pgtypeslib/interval.c index 936a6883816..155c6cc7770 100644 --- a/src/interfaces/ecpg/pgtypeslib/interval.c +++ b/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; diff --git a/src/interfaces/ecpg/pgtypeslib/numeric.c b/src/interfaces/ecpg/pgtypeslib/numeric.c index 35e7b92da40..49938543d03 100644 --- a/src/interfaces/ecpg/pgtypeslib/numeric.c +++ b/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); diff --git a/src/port/Makefile b/src/port/Makefile index 366c814bd92..70d63963611 100644 --- a/src/port/Makefile +++ b/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 \ diff --git a/src/port/locale.c b/src/port/locale.c new file mode 100644 index 00000000000..387fb4a107a --- /dev/null +++ b/src/port/locale.c @@ -0,0 +1,80 @@ +/*------------------------------------------------------------------------- + * + * 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 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; +} diff --git a/src/port/meson.build b/src/port/meson.build index 83a06325209..93f6158c307 100644 --- a/src/port/meson.build +++ b/src/port/meson.build @@ -5,6 +5,7 @@ pgport_sources = [ 'chklocale.c', 'inet_net_ntop.c', 'noblock.c', + 'locale.c', 'path.c', 'pg_bitutils.c', 'pg_popcount_avx512.c', diff --git a/src/port/snprintf.c b/src/port/snprintf.c index 884f0262dd1..3cc49f01ea6 100644 --- a/src/port/snprintf.c +++ b/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. * @@ -1184,6 +1214,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 && @@ -1205,7 +1238,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 { @@ -1214,6 +1251,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; @@ -1336,12 +1378,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; -- 2.47.0
