Re: Avoiding concurrent calls to bindtextdomain() - Mailing list pgsql-hackers

I wrote:
> 0001 also gets rid of the possibility that pthread_mutex_init/
> pthread_mutex_lock could fail due to malloc failure.  This seems
> important since default_threadlock() assumes that pthread_mutex_lock
> cannot fail in practice.  I observe that ecpglib also assumes that,
> although it's using CreateMutex() which has documented failure
> conditions.  So I wonder if we ought to copy this implementation
> back into ecpglib; but I've not done that here.

The cfbot seemed happy with v1, so here's a v2 that does copy that
code back into ecpglib.  (I kind of wonder why this code exists in
libpq + ecpglib at all, rather than in src/port/; but that seems
like a refactoring exercise for another day.)

I double-checked that all the pthread_mutex_t variables in libpq
and ecpglib are static, so the change in that struct should not
pose an ABI hazard for back-patching.

Barring objections, I plan to push this pretty soon.

            regards, tom lane

From 8d7ebd1fc5b556850fcbe117f44f1e0918edf4b9 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 8 Feb 2024 12:54:08 -0500
Subject: [PATCH v2 1/2] Clean up unnecessarily Windows-dependent code in
 libpq.

Fix pthread-win32.h and pthread-win32.c to provide a more complete
emulation of POSIX pthread mutexes: define PTHREAD_MUTEX_INITIALIZER
and make sure that pthread_mutex_lock() can operate on a mutex
object that's been initialized that way.  Then we don't need the
duplicative platform-specific logic in default_threadlock() and
pgtls_init(), which we'd otherwise need yet a third copy of for
an upcoming bug fix.

Also, since default_threadlock() supposes that pthread_mutex_lock()
cannot fail, try to ensure that that's actually true, by getting
rid of the malloc call that was formerly involved in initializing
an emulated mutex.  We can define an extra state for the spinlock
field instead.

Also, replace the similar code in ecpglib/misc.c with this version.
While ecpglib's version had at least a POSIX-compliant API, it
likewise had the potential of failing during mutex init (but here,
because of CreateMutex failure rather than malloc failure).  Since
all of misc.c's callers ignore failures, it seems like a wise idea
to avoid failures here too.

A further improvement in this area could be to unify libpq's and
ecpglib's implementations into a src/port/pthread-win32.c file.
But that doesn't seem like a bug fix, so I'll desist for now.

Discussion: https://postgr.es/m/264860.1707163416@sss.pgh.pa.us
---
 src/interfaces/ecpg/ecpglib/misc.c            | 37 +++++++++++++++----
 .../ecpg/include/ecpg-pthread-win32.h         | 22 ++++-------
 src/interfaces/libpq/fe-connect.c             | 16 --------
 src/interfaces/libpq/fe-secure-openssl.c      | 20 ----------
 src/interfaces/libpq/pthread-win32.c          | 26 ++++++++-----
 src/port/pthread-win32.h                      | 11 +++++-
 6 files changed, 63 insertions(+), 69 deletions(-)

diff --git a/src/interfaces/ecpg/ecpglib/misc.c b/src/interfaces/ecpg/ecpglib/misc.c
index 2b78caeaf5..58fff10697 100644
--- a/src/interfaces/ecpg/ecpglib/misc.c
+++ b/src/interfaces/ecpg/ecpglib/misc.c
@@ -407,17 +407,38 @@ ECPGis_noind_null(enum ECPGttype type, const void *ptr)

 #ifdef WIN32

-void
-win32_pthread_mutex(volatile pthread_mutex_t *mutex)
+int
+pthread_mutex_init(pthread_mutex_t *mp, void *attr)
+{
+    mp->initstate = 0;
+    return 0;
+}
+
+int
+pthread_mutex_lock(pthread_mutex_t *mp)
 {
-    if (mutex->handle == NULL)
+    /* Initialize the csection if not already done */
+    if (mp->initstate != 1)
     {
-        while (InterlockedExchange((LONG *) &mutex->initlock, 1) == 1)
-            Sleep(0);
-        if (mutex->handle == NULL)
-            mutex->handle = CreateMutex(NULL, FALSE, NULL);
-        InterlockedExchange((LONG *) &mutex->initlock, 0);
+        LONG        istate;
+
+        while ((istate = InterlockedExchange(&mp->initstate, 2)) == 2)
+            Sleep(0);            /* wait, another thread is doing this */
+        if (istate != 1)
+            InitializeCriticalSection(&mp->csection);
+        InterlockedExchange(&mp->initstate, 1);
     }
+    EnterCriticalSection(&mp->csection);
+    return 0;
+}
+
+int
+pthread_mutex_unlock(pthread_mutex_t *mp)
+{
+    if (mp->initstate != 1)
+        return EINVAL;
+    LeaveCriticalSection(&mp->csection);
+    return 0;
 }

 static pthread_mutex_t win32_pthread_once_lock = PTHREAD_MUTEX_INITIALIZER;
diff --git a/src/interfaces/ecpg/include/ecpg-pthread-win32.h b/src/interfaces/ecpg/include/ecpg-pthread-win32.h
index 8252a17809..7b6ba46b34 100644
--- a/src/interfaces/ecpg/include/ecpg-pthread-win32.h
+++ b/src/interfaces/ecpg/include/ecpg-pthread-win32.h
@@ -12,28 +12,22 @@

 typedef struct pthread_mutex_t
 {
-    HANDLE        handle;
-    LONG        initlock;
+    /* initstate = 0: not initialized; 1: init done; 2: init in progress */
+    LONG        initstate;
+    CRITICAL_SECTION csection;
 } pthread_mutex_t;

 typedef DWORD pthread_key_t;
 typedef bool pthread_once_t;

-#define PTHREAD_MUTEX_INITIALIZER    { NULL, 0 }
+#define PTHREAD_MUTEX_INITIALIZER    { 0 }
 #define PTHREAD_ONCE_INIT            false

-void        win32_pthread_mutex(volatile pthread_mutex_t *mutex);
-void        win32_pthread_once(volatile pthread_once_t *once, void (*fn) (void));
+int            pthread_mutex_init(pthread_mutex_t *, void *attr);
+int            pthread_mutex_lock(pthread_mutex_t *);
+int            pthread_mutex_unlock(pthread_mutex_t *);

-#define pthread_mutex_lock(mutex) \
-    do { \
-        if ((mutex)->handle == NULL) \
-            win32_pthread_mutex((mutex)); \
-        WaitForSingleObject((mutex)->handle, INFINITE); \
-    } while(0)
-
-#define pthread_mutex_unlock(mutex) \
-    ReleaseMutex((mutex)->handle)
+void        win32_pthread_once(volatile pthread_once_t *once, void (*fn) (void));

 #define pthread_getspecific(key) \
     TlsGetValue((key))
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 64c0b628b3..d4e10a0c4f 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7426,24 +7426,8 @@ error:
 static void
 default_threadlock(int acquire)
 {
-#ifndef WIN32
     static pthread_mutex_t singlethread_lock = PTHREAD_MUTEX_INITIALIZER;
-#else
-    static pthread_mutex_t singlethread_lock = NULL;
-    static long mutex_initlock = 0;

-    if (singlethread_lock == NULL)
-    {
-        while (InterlockedExchange(&mutex_initlock, 1) == 1)
-             /* loop, another thread own the lock */ ;
-        if (singlethread_lock == NULL)
-        {
-            if (pthread_mutex_init(&singlethread_lock, NULL))
-                Assert(false);
-        }
-        InterlockedExchange(&mutex_initlock, 0);
-    }
-#endif
     if (acquire)
     {
         if (pthread_mutex_lock(&singlethread_lock))
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 6bc216956d..8110882262 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -91,12 +91,7 @@ static bool ssl_lib_initialized = false;

 static long crypto_open_connections = 0;

-#ifndef WIN32
 static pthread_mutex_t ssl_config_mutex = PTHREAD_MUTEX_INITIALIZER;
-#else
-static pthread_mutex_t ssl_config_mutex = NULL;
-static long win32_ssl_create_mutex = 0;
-#endif

 static PQsslKeyPassHook_OpenSSL_type PQsslKeyPassHook = NULL;
 static int    ssl_protocol_version_to_openssl(const char *protocol);
@@ -773,20 +768,6 @@ pq_lockingcallback(int mode, int n, const char *file, int line)
 int
 pgtls_init(PGconn *conn, bool do_ssl, bool do_crypto)
 {
-#ifdef WIN32
-    /* Also see similar code in fe-connect.c, default_threadlock() */
-    if (ssl_config_mutex == NULL)
-    {
-        while (InterlockedExchange(&win32_ssl_create_mutex, 1) == 1)
-             /* loop, another thread own the lock */ ;
-        if (ssl_config_mutex == NULL)
-        {
-            if (pthread_mutex_init(&ssl_config_mutex, NULL))
-                return -1;
-        }
-        InterlockedExchange(&win32_ssl_create_mutex, 0);
-    }
-#endif
     if (pthread_mutex_lock(&ssl_config_mutex))
         return -1;

@@ -874,7 +855,6 @@ static void
 destroy_ssl_system(void)
 {
 #if defined(HAVE_CRYPTO_LOCK)
-    /* Mutex is created in pgtls_init() */
     if (pthread_mutex_lock(&ssl_config_mutex))
         return;

diff --git a/src/interfaces/libpq/pthread-win32.c b/src/interfaces/libpq/pthread-win32.c
index e607bee89a..b40872898d 100644
--- a/src/interfaces/libpq/pthread-win32.c
+++ b/src/interfaces/libpq/pthread-win32.c
@@ -34,27 +34,33 @@ pthread_getspecific(pthread_key_t key)
 int
 pthread_mutex_init(pthread_mutex_t *mp, void *attr)
 {
-    *mp = (CRITICAL_SECTION *) malloc(sizeof(CRITICAL_SECTION));
-    if (!*mp)
-        return 1;
-    InitializeCriticalSection(*mp);
+    mp->initstate = 0;
     return 0;
 }

 int
 pthread_mutex_lock(pthread_mutex_t *mp)
 {
-    if (!*mp)
-        return 1;
-    EnterCriticalSection(*mp);
+    /* Initialize the csection if not already done */
+    if (mp->initstate != 1)
+    {
+        LONG        istate;
+
+        while ((istate = InterlockedExchange(&mp->initstate, 2)) == 2)
+            Sleep(0);            /* wait, another thread is doing this */
+        if (istate != 1)
+            InitializeCriticalSection(&mp->csection);
+        InterlockedExchange(&mp->initstate, 1);
+    }
+    EnterCriticalSection(&mp->csection);
     return 0;
 }

 int
 pthread_mutex_unlock(pthread_mutex_t *mp)
 {
-    if (!*mp)
-        return 1;
-    LeaveCriticalSection(*mp);
+    if (mp->initstate != 1)
+        return EINVAL;
+    LeaveCriticalSection(&mp->csection);
     return 0;
 }
diff --git a/src/port/pthread-win32.h b/src/port/pthread-win32.h
index 97ccc17a12..5f33269057 100644
--- a/src/port/pthread-win32.h
+++ b/src/port/pthread-win32.h
@@ -5,7 +5,16 @@
 #define __PTHREAD_H

 typedef ULONG pthread_key_t;
-typedef CRITICAL_SECTION *pthread_mutex_t;
+
+typedef struct pthread_mutex_t
+{
+    /* initstate = 0: not initialized; 1: init done; 2: init in progress */
+    LONG        initstate;
+    CRITICAL_SECTION csection;
+} pthread_mutex_t;
+
+#define PTHREAD_MUTEX_INITIALIZER    { 0 }
+
 typedef int pthread_once_t;

 DWORD        pthread_self(void);
--
2.39.3

From 89862c5c145d5f66f284098abf0ef6384a96a8d7 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 8 Feb 2024 12:59:44 -0500
Subject: [PATCH v2 2/2] Avoid concurrent calls to bindtextdomain().

We previously supposed that it was okay for different threads to
call bindtextdomain() concurrently (cf. commit 1f655fdc3).
It now emerges that there's at least one gettext implementation
in which that triggers an abort() crash, so let's stop doing that.
Add mutexes guarding libpq's and ecpglib's calls, which are the
only ones that need worry about multithreaded callers.

Note: in libpq, we could perhaps have piggybacked on
default_threadlock() to avoid defining a new mutex variable.
I judge that not terribly safe though, since libpq_gettext could
be called from code that is holding the default mutex.  If that
were the first such call in the process, it'd fail.  An extra
mutex is cheap insurance against unforeseen interactions.

Per bug #18312 from Christian Maurer.  Back-patch to all
supported versions.

Discussion: https://postgr.es/m/18312-bbbabc8113592b78@postgresql.org
Discussion: https://postgr.es/m/264860.1707163416@sss.pgh.pa.us
---
 src/interfaces/ecpg/ecpglib/misc.c | 39 ++++++++++++++++++++----------
 src/interfaces/libpq/fe-misc.c     | 39 ++++++++++++++++++++----------
 2 files changed, 52 insertions(+), 26 deletions(-)

diff --git a/src/interfaces/ecpg/ecpglib/misc.c b/src/interfaces/ecpg/ecpglib/misc.c
index 58fff10697..8b9aefcd9c 100644
--- a/src/interfaces/ecpg/ecpglib/misc.c
+++ b/src/interfaces/ecpg/ecpglib/misc.c
@@ -465,13 +465,14 @@ char *
 ecpg_gettext(const char *msgid)
 {
     /*
-     * If multiple threads come through here at about the same time, it's okay
-     * for more than one of them to call bindtextdomain().  But it's not okay
-     * for any of them to reach dgettext() before bindtextdomain() is
-     * complete, so don't set the flag till that's done.  Use "volatile" just
-     * to be sure the compiler doesn't try to get cute.
+     * At least on Windows, there are gettext implementations that fail if
+     * multiple threads call bindtextdomain() concurrently.  Use a mutex and
+     * flag variable to ensure that we call it just once per process.  It is
+     * not known that similar bugs exist on non-Windows platforms, but we
+     * might as well do it the same way everywhere.
      */
     static volatile bool already_bound = false;
+    static pthread_mutex_t binddomain_mutex = PTHREAD_MUTEX_INITIALIZER;

     if (!already_bound)
     {
@@ -481,14 +482,26 @@ ecpg_gettext(const char *msgid)
 #else
         int            save_errno = errno;
 #endif
-        const char *ldir;
-
-        /* No relocatable lookup here because the binary could be anywhere */
-        ldir = getenv("PGLOCALEDIR");
-        if (!ldir)
-            ldir = LOCALEDIR;
-        bindtextdomain(PG_TEXTDOMAIN("ecpglib"), ldir);
-        already_bound = true;
+
+        (void) pthread_mutex_lock(&binddomain_mutex);
+
+        if (!already_bound)
+        {
+            const char *ldir;
+
+            /*
+             * No relocatable lookup here because the calling executable could
+             * be anywhere
+             */
+            ldir = getenv("PGLOCALEDIR");
+            if (!ldir)
+                ldir = LOCALEDIR;
+            bindtextdomain(PG_TEXTDOMAIN("ecpglib"), ldir);
+            already_bound = true;
+        }
+
+        (void) pthread_mutex_unlock(&binddomain_mutex);
+
 #ifdef WIN32
         SetLastError(save_errno);
 #else
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 47a28b0a3a..f2fc78a481 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -1225,13 +1225,14 @@ static void
 libpq_binddomain(void)
 {
     /*
-     * If multiple threads come through here at about the same time, it's okay
-     * for more than one of them to call bindtextdomain().  But it's not okay
-     * for any of them to return to caller before bindtextdomain() is
-     * complete, so don't set the flag till that's done.  Use "volatile" just
-     * to be sure the compiler doesn't try to get cute.
+     * At least on Windows, there are gettext implementations that fail if
+     * multiple threads call bindtextdomain() concurrently.  Use a mutex and
+     * flag variable to ensure that we call it just once per process.  It is
+     * not known that similar bugs exist on non-Windows platforms, but we
+     * might as well do it the same way everywhere.
      */
     static volatile bool already_bound = false;
+    static pthread_mutex_t binddomain_mutex = PTHREAD_MUTEX_INITIALIZER;

     if (!already_bound)
     {
@@ -1241,14 +1242,26 @@ libpq_binddomain(void)
 #else
         int            save_errno = errno;
 #endif
-        const char *ldir;
-
-        /* No relocatable lookup here because the binary could be anywhere */
-        ldir = getenv("PGLOCALEDIR");
-        if (!ldir)
-            ldir = LOCALEDIR;
-        bindtextdomain(PG_TEXTDOMAIN("libpq"), ldir);
-        already_bound = true;
+
+        (void) pthread_mutex_lock(&binddomain_mutex);
+
+        if (!already_bound)
+        {
+            const char *ldir;
+
+            /*
+             * No relocatable lookup here because the calling executable could
+             * be anywhere
+             */
+            ldir = getenv("PGLOCALEDIR");
+            if (!ldir)
+                ldir = LOCALEDIR;
+            bindtextdomain(PG_TEXTDOMAIN("libpq"), ldir);
+            already_bound = true;
+        }
+
+        (void) pthread_mutex_unlock(&binddomain_mutex);
+
 #ifdef WIN32
         SetLastError(save_errno);
 #else
--
2.39.3