From 530249a06d2173ea54d9e3af4a6e56921bf4471b Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Wed, 6 May 2026 07:39:28 -0400 Subject: [PATCH] Prevent thread-unsafe sharing of an ECPG default connection ECPGconnect() published the new struct connection into all_connections, the calling thread's per-thread default-connection slot (held in thread-specific data via actual_connection_key), and the global actual_connection before calling PQconnectdbParams(). If PQconnectdbParams() then failed, ecpg_finish() removed the dead struct from the list and -- by the same logic that ecpg_finish() uses for DISCONNECT -- repointed the failing thread's per-thread slot at whatever now headed all_connections, which is typically a sibling thread's already-open connection. After such a failed CONNECT the caller's whenever-sqlerror handler often just logs and lets the thread continue; the next EXEC SQL statement issued without an explicit connection name then resolved through the poisoned per-thread slot or the actual_connection global default and ran libpq calls on the sibling's PGconn concurrently with the sibling thread itself. libpq is not thread-safe at the per-connection level, so the shared prep_stmts list got corrupted (segv in deallocate_one), the connection input buffer got reallocated underneath an in-flight reader (heap abort in pqCheckInBufferSpace), and conn->result occasionally went NULL while another thread was mid-row-processing. This was reproducible by running thread/prep and thread/alloc against a server with max_connections set low enough to refuse some of the worker threads' CONNECTs, e.g. max_connections = 10 in a TEMP_CONFIG passed to "make -C src/interfaces/ecpg/test check", and is the family of crashes Alexander Lakhin observed on the dikkop buildfarm animal. Fix: * Record on each successful CONNECT which thread opened that connection (new owner_thread field on struct connection). When an EXEC SQL statement is issued without a connection name and the calling thread has nothing in its per-thread default-connection slot, only fall back to actual_connection if its owner matches the caller; otherwise return NULL so ecpg_init() raises ECPG_NO_CONN cleanly. Threads that genuinely want to share a connection must do so explicitly by name (or via SET CONNECTION) and provide their own synchronization. * In ECPGconnect()'s failure path, snapshot the prior per-thread slot value and global default before publishing the unopened struct, and restore them after ecpg_finish() returns. This prevents the failing thread's slot from being aliased to a sibling's open connection. A new ecpg_thread_id abstraction (pthread_t on POSIX, DWORD on Win32) keeps ECPG_GET_THREAD_ID()/ECPG_THREAD_ID_EQUAL() portable across threading models. A new test thread/connfail forces a deterministic CONNECT failure (by naming a database that does not exist) and verifies that the worker thread's subsequent unnamed PREPARE returns ECPG_NO_CONN instead of silently using the main thread's connection. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/75460b3c-255d-47eb-b889-d99de38e6758@gmail.com --- src/interfaces/ecpg/ecpglib/connect.c | 73 +++++-- src/interfaces/ecpg/ecpglib/ecpglib_extern.h | 2 + .../ecpg/include/ecpg-pthread-win32.h | 17 ++ src/interfaces/ecpg/test/ecpg_schedule | 1 + .../ecpg/test/expected/thread-connfail.c | 192 ++++++++++++++++++ .../ecpg/test/expected/thread-connfail.stderr | 0 .../ecpg/test/expected/thread-connfail.stdout | 2 + src/interfaces/ecpg/test/thread/Makefile | 3 +- src/interfaces/ecpg/test/thread/connfail.pgc | 89 ++++++++ src/interfaces/ecpg/test/thread/meson.build | 1 + 10 files changed, 359 insertions(+), 21 deletions(-) create mode 100644 src/interfaces/ecpg/test/expected/thread-connfail.c create mode 100644 src/interfaces/ecpg/test/expected/thread-connfail.stderr create mode 100644 src/interfaces/ecpg/test/expected/thread-connfail.stdout create mode 100644 src/interfaces/ecpg/test/thread/connfail.pgc diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c index 78de9f298ba..e09f728a379 100644 --- a/src/interfaces/ecpg/ecpglib/connect.c +++ b/src/interfaces/ecpg/ecpglib/connect.c @@ -32,6 +32,30 @@ ecpg_pthreads_init(void) pthread_once(&actual_connection_key_once, ecpg_actual_connection_init); } +/* + * Resolve actual_connection as the implicit default for an unnamed EXEC SQL, + * but only if it was opened by the calling thread. Otherwise we'd let one + * thread silently issue libpq calls on another thread's PGconn, which is + * not safe. Threads that want to share a connection must do so explicitly + * by name (or via SET CONNECTION) and provide their own synchronization. + */ +static struct connection * +ecpg_default_connection(void) +{ + struct connection *ret; + + ret = pthread_getspecific(actual_connection_key); + if (ret != NULL) + return ret; + + if (actual_connection != NULL && + ECPG_THREAD_ID_EQUAL(actual_connection->owner_thread, + ECPG_GET_THREAD_ID())) + return actual_connection; + + return NULL; +} + static struct connection * ecpg_get_connection_nr(const char *connection_name) { @@ -41,16 +65,7 @@ ecpg_get_connection_nr(const char *connection_name) { ecpg_pthreads_init(); /* ensure actual_connection_key is valid */ - ret = pthread_getspecific(actual_connection_key); - - /* - * if no connection in TSD for this thread, get the global default - * connection and hope the user knows what they're doing (i.e. using - * their own mutex to protect that connection from concurrent accesses - */ - if (ret == NULL) - /* no TSD connection, going for global */ - ret = actual_connection; + ret = ecpg_default_connection(); } else { @@ -81,16 +96,7 @@ ecpg_get_connection(const char *connection_name) { ecpg_pthreads_init(); /* ensure actual_connection_key is valid */ - ret = pthread_getspecific(actual_connection_key); - - /* - * if no connection in TSD for this thread, get the global default - * connection and hope the user knows what they're doing (i.e. using - * their own mutex to protect that connection from concurrent accesses - */ - if (ret == NULL) - /* no TSD connection here either, using global */ - ret = actual_connection; + ret = ecpg_default_connection(); } else { @@ -262,6 +268,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p struct sqlca_t *sqlca = ECPGget_sqlca(); enum COMPAT_MODE compat = c; struct connection *this; + struct connection *prev_actual_connection; + struct connection *prev_thread_connection; int i, connect_params = 0; bool alloc_failed = (sqlca == NULL); @@ -540,12 +548,25 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p this->cache_head = NULL; this->prep_stmts = NULL; + this->owner_thread = ECPG_GET_THREAD_ID(); if (all_connections == NULL) this->next = NULL; else this->next = all_connections; + /* + * Snapshot this thread's per-thread default connection (kept in + * thread-specific data via actual_connection_key) and the global default, + * so we can restore them if PQconnectdbParams() fails below. Without + * this, ecpg_finish() would re-point this thread's slot at whatever then + * heads all_connections -- typically a sibling thread's already-open + * PGconn, which would later be returned by ecpg_get_connection(NULL) and + * used by libpq concurrently from two threads. + */ + prev_actual_connection = actual_connection; + prev_thread_connection = pthread_getspecific(actual_connection_key); + all_connections = this; pthread_setspecific(actual_connection_key, all_connections); actual_connection = all_connections; @@ -668,6 +689,18 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p ecpg_log("ECPGconnect: %s", errmsg); ecpg_finish(this); + + /* + * Restore this thread's per-thread default connection and the global + * default to whatever they were before we published "this". + * ecpg_finish() reset them to the new head of all_connections, but + * that may be a different thread's open connection -- letting our + * failed attempt aim this thread's slot at a sibling's PGconn would + * be unsafe. + */ + pthread_setspecific(actual_connection_key, prev_thread_connection); + actual_connection = prev_actual_connection; + pthread_mutex_unlock(&connections_mutex); ecpg_raise(lineno, ECPG_CONNECT, ECPG_SQLSTATE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION, db); diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h index c92f0aa1081..b6c391bf717 100644 --- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h +++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h @@ -4,6 +4,7 @@ #define _ECPG_ECPGLIB_EXTERN_H #include "ecpg_config.h" +#include "ecpg-pthread-win32.h" #include "ecpgtype.h" #include "libpq-fe.h" #include "sqlca.h" @@ -103,6 +104,7 @@ struct connection char *name; PGconn *connection; bool autocommit; + ecpg_thread_id owner_thread; /* thread that opened this connection */ struct ECPGtype_information_cache *cache_head; struct prepared_statement *prep_stmts; struct connection *next; diff --git a/src/interfaces/ecpg/include/ecpg-pthread-win32.h b/src/interfaces/ecpg/include/ecpg-pthread-win32.h index 7b6ba46b349..c742bb5232b 100644 --- a/src/interfaces/ecpg/include/ecpg-pthread-win32.h +++ b/src/interfaces/ecpg/include/ecpg-pthread-win32.h @@ -8,8 +8,25 @@ #ifndef WIN32 #include + +/* + * Abstraction for capturing and comparing a thread identity. Used by + * ecpglib to record which thread owns a given connection so that an + * unnamed EXEC SQL statement issued from a different thread (whose own + * CONNECT may have failed) does not silently fall through to it via the + * actual_connection global default -- libpq is not thread-safe at the + * per-connection level. + */ +typedef pthread_t ecpg_thread_id; +#define ECPG_GET_THREAD_ID() pthread_self() +#define ECPG_THREAD_ID_EQUAL(a, b) pthread_equal((a), (b)) + #else +typedef DWORD ecpg_thread_id; +#define ECPG_GET_THREAD_ID() GetCurrentThreadId() +#define ECPG_THREAD_ID_EQUAL(a, b) ((a) == (b)) + typedef struct pthread_mutex_t { /* initstate = 0: not initialized; 1: init done; 2: init in progress */ diff --git a/src/interfaces/ecpg/test/ecpg_schedule b/src/interfaces/ecpg/test/ecpg_schedule index d1f5d9452b7..d2c254fe9f3 100644 --- a/src/interfaces/ecpg/test/ecpg_schedule +++ b/src/interfaces/ecpg/test/ecpg_schedule @@ -64,3 +64,4 @@ test: thread/thread_implicit test: thread/prep test: thread/alloc test: thread/descriptor +test: thread/connfail diff --git a/src/interfaces/ecpg/test/expected/thread-connfail.c b/src/interfaces/ecpg/test/expected/thread-connfail.c new file mode 100644 index 00000000000..53d8865e9a8 --- /dev/null +++ b/src/interfaces/ecpg/test/expected/thread-connfail.c @@ -0,0 +1,192 @@ +/* Processed by ecpg (regression mode) */ +/* These include files are added by the preprocessor */ +#include +#include +#include +/* End of automatic include section */ +#define ECPGdebug(X,Y) ECPGdebug((X)+100,(Y)) + +#line 1 "connfail.pgc" +/* + * Verify that an EXEC SQL statement issued without a connection name from + * a worker thread whose own EXEC SQL CONNECT failed does not silently fall + * back to another thread's open PGconn. Before the corresponding ecpglib + * fix, ecpg_get_connection(NULL) would return the global default + * connection regardless of which thread had opened it, letting the worker + * issue libpq calls on the main thread's connection concurrently with the + * main thread itself -- libpq is not thread-safe at the per-connection + * level, so cross-thread sharing must be opt-in by name, not implicit. + * + * The test forces a deterministic CONNECT failure (target database does + * not exist), then issues an unnamed EXEC SQL. With the fix the unnamed + * statement must error with sqlcode = ECPG_NO_CONN (-220); without the + * fix it would silently succeed against the main thread's connection + * (or, under load, crash from concurrent libpq access). + */ +#include +#include +#include +#include "ecpg_config.h" + +#ifdef WIN32 +#define WIN32_LEAN_AND_MEAN +#include +#include +#else +#include +#endif + + +#line 1 "sqlca.h" +#ifndef POSTGRES_SQLCA_H +#define POSTGRES_SQLCA_H + +#ifndef PGDLLIMPORT +#if defined(WIN32) || defined(__CYGWIN__) +#define PGDLLIMPORT __declspec (dllimport) +#else +#define PGDLLIMPORT +#endif /* __CYGWIN__ */ +#endif /* PGDLLIMPORT */ + +#define SQLERRMC_LEN 150 + +#ifdef __cplusplus +extern "C" +{ +#endif + +struct sqlca_t +{ + char sqlcaid[8]; + long sqlabc; + long sqlcode; + struct + { + int sqlerrml; + char sqlerrmc[SQLERRMC_LEN]; + } sqlerrm; + char sqlerrp[8]; + long sqlerrd[6]; + /* Element 0: empty */ + /* 1: OID of processed tuple if applicable */ + /* 2: number of rows processed */ + /* after an INSERT, UPDATE or */ + /* DELETE statement */ + /* 3: empty */ + /* 4: empty */ + /* 5: empty */ + char sqlwarn[8]; + /* Element 0: set to 'W' if at least one other is 'W' */ + /* 1: if 'W' at least one character string */ + /* value was truncated when it was */ + /* stored into a host variable. */ + + /* + * 2: if 'W' a (hopefully) non-fatal notice occurred + */ /* 3: empty */ + /* 4: empty */ + /* 5: empty */ + /* 6: empty */ + /* 7: empty */ + + char sqlstate[5]; +}; + +struct sqlca_t *ECPGget_sqlca(void); + +#ifndef POSTGRES_ECPG_INTERNAL +#define sqlca (*ECPGget_sqlca()) +#endif + +#ifdef __cplusplus +} +#endif + +#endif + +#line 30 "connfail.pgc" + + +#line 1 "regression.h" + + + + + + +#line 31 "connfail.pgc" + + +#ifdef WIN32 +static unsigned __stdcall +fn(void *arg) +#else +static void * +fn(void *arg) +#endif +{ + /* exec sql begin declare section */ + + +#line 42 "connfail.pgc" + char * query = "SELECT 1" ; +/* exec sql end declare section */ +#line 43 "connfail.pgc" + + + (void) arg; + + /* + * Connect to a database name that the test instance does not have. + * This is guaranteed to fail with ECPG_CONNECT. + */ + { ECPGconnect(__LINE__, 0, "nonexistent_xyzzy_db" , NULL, NULL , NULL, 0); } +#line 51 "connfail.pgc" + + printf("worker connect sqlcode = %ld\n", sqlca.sqlcode); + + /* + * After our own connect failed, an unnamed EXEC SQL must fail with + * ECPG_NO_CONN (-220), NOT silently use the main thread's connection. + */ + { ECPGprepare(__LINE__, NULL, 0, "iworker", query);} +#line 58 "connfail.pgc" + + printf("worker prepare sqlcode = %ld\n", sqlca.sqlcode); + + return 0; +} + +int +main(void) +{ +#ifdef WIN32 + HANDLE t; + unsigned id; +#else + pthread_t t; +#endif + + { ECPGconnect(__LINE__, 0, "ecpg1_regression" , NULL, NULL , NULL, 0); } +#line 74 "connfail.pgc" + + { ECPGsetcommit(__LINE__, "on", NULL);} +#line 75 "connfail.pgc" + + +#ifdef WIN32 + t = (HANDLE) _beginthreadex(NULL, 0, fn, NULL, 0, &id); + WaitForSingleObject(t, INFINITE); + CloseHandle(t); +#else + pthread_create(&t, NULL, fn, NULL); + pthread_join(t, NULL); +#endif + + { ECPGdisconnect(__LINE__, "CURRENT");} +#line 86 "connfail.pgc" + + + return 0; +} diff --git a/src/interfaces/ecpg/test/expected/thread-connfail.stderr b/src/interfaces/ecpg/test/expected/thread-connfail.stderr new file mode 100644 index 00000000000..e69de29bb2d diff --git a/src/interfaces/ecpg/test/expected/thread-connfail.stdout b/src/interfaces/ecpg/test/expected/thread-connfail.stdout new file mode 100644 index 00000000000..9d91f3f7358 --- /dev/null +++ b/src/interfaces/ecpg/test/expected/thread-connfail.stdout @@ -0,0 +1,2 @@ +worker connect sqlcode = -402 +worker prepare sqlcode = -220 diff --git a/src/interfaces/ecpg/test/thread/Makefile b/src/interfaces/ecpg/test/thread/Makefile index 1b4ddcff61b..04b600420d9 100644 --- a/src/interfaces/ecpg/test/thread/Makefile +++ b/src/interfaces/ecpg/test/thread/Makefile @@ -8,6 +8,7 @@ TESTS = thread_implicit thread_implicit.c \ thread thread.c \ prep prep.c \ descriptor descriptor.c \ - alloc alloc.c + alloc alloc.c \ + connfail connfail.c all: $(TESTS) diff --git a/src/interfaces/ecpg/test/thread/connfail.pgc b/src/interfaces/ecpg/test/thread/connfail.pgc new file mode 100644 index 00000000000..3e2e5271d47 --- /dev/null +++ b/src/interfaces/ecpg/test/thread/connfail.pgc @@ -0,0 +1,89 @@ +/* + * Verify that an EXEC SQL statement issued without a connection name from + * a worker thread whose own EXEC SQL CONNECT failed does not silently fall + * back to another thread's open PGconn. Before the corresponding ecpglib + * fix, ecpg_get_connection(NULL) would return the global default + * connection regardless of which thread had opened it, letting the worker + * issue libpq calls on the main thread's connection concurrently with the + * main thread itself -- libpq is not thread-safe at the per-connection + * level, so cross-thread sharing must be opt-in by name, not implicit. + * + * The test forces a deterministic CONNECT failure (target database does + * not exist), then issues an unnamed EXEC SQL. With the fix the unnamed + * statement must error with sqlcode = ECPG_NO_CONN (-220); without the + * fix it would silently succeed against the main thread's connection + * (or, under load, crash from concurrent libpq access). + */ +#include +#include +#include +#include "ecpg_config.h" + +#ifdef WIN32 +#define WIN32_LEAN_AND_MEAN +#include +#include +#else +#include +#endif + +exec sql include sqlca; +exec sql include ../regression; + +#ifdef WIN32 +static unsigned __stdcall +fn(void *arg) +#else +static void * +fn(void *arg) +#endif +{ + exec sql begin declare section; + char *query = "SELECT 1"; + exec sql end declare section; + + (void) arg; + + /* + * Connect to a database name that the test instance does not have. + * This is guaranteed to fail with ECPG_CONNECT. + */ + exec sql connect to nonexistent_xyzzy_db; + printf("worker connect sqlcode = %ld\n", sqlca.sqlcode); + + /* + * After our own connect failed, an unnamed EXEC SQL must fail with + * ECPG_NO_CONN (-220), NOT silently use the main thread's connection. + */ + exec sql prepare iworker from :query; + printf("worker prepare sqlcode = %ld\n", sqlca.sqlcode); + + return 0; +} + +int +main(void) +{ +#ifdef WIN32 + HANDLE t; + unsigned id; +#else + pthread_t t; +#endif + + exec sql connect to REGRESSDB1; + exec sql set autocommit to on; + +#ifdef WIN32 + t = (HANDLE) _beginthreadex(NULL, 0, fn, NULL, 0, &id); + WaitForSingleObject(t, INFINITE); + CloseHandle(t); +#else + pthread_create(&t, NULL, fn, NULL); + pthread_join(t, NULL); +#endif + + exec sql disconnect; + + return 0; +} diff --git a/src/interfaces/ecpg/test/thread/meson.build b/src/interfaces/ecpg/test/thread/meson.build index b23289730b7..3af4604b90c 100644 --- a/src/interfaces/ecpg/test/thread/meson.build +++ b/src/interfaces/ecpg/test/thread/meson.build @@ -6,6 +6,7 @@ pgc_files = [ 'prep', 'descriptor', 'alloc', + 'connfail', ] foreach pgc_file : pgc_files -- 2.43.0