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

According to the discussion in [1], it's not as safe as we supposed
to allow different threads to call bindtextdomain() concurrently.
Here is a patchset to prevent that by acquiring a mutex around
the libpq and ecpglib calls that are at risk.

In libpq, this would've required yet a third copy of the
Windows-specific ugliness in default_threadlock() and pgtls_init().
I didn't particularly want to do that, so I stole some ideas
from ecpglib to create a more POSIX-compliant emulation of
pthread_mutex_lock().  0001 attached is the refactoring needed
to make that happen, and then 0002 is the actual bug fix.

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.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/18312-bbbabc8113592b78%40postgresql.org

From be34eb602c1e28cb60141e46c79dcadc20933854 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon, 5 Feb 2024 14:23:20 -0500
Subject: [PATCH v1 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.

The idea of making pthread_mutex_t into a struct with a spinlock-like
field is borrowed from ecpg's similar emulation.  Perhaps we should
adopt this implementation there too, since ecpg also contains some
presently-bogus assumptions that mutex locking can't fail.
---
 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 +++++++++-
 4 files changed, 26 insertions(+), 47 deletions(-)

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 27c3c76e7a5e07da3ee8772d268f7f8b2ee8e2b4 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon, 5 Feb 2024 14:49:42 -0500
Subject: [PATCH v1 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.
---
 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 2b78caeaf5..b5bd7f0615 100644
--- a/src/interfaces/ecpg/ecpglib/misc.c
+++ b/src/interfaces/ecpg/ecpglib/misc.c
@@ -444,13 +444,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)
     {
@@ -460,14 +461,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