public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tatsuo Ishii <[email protected]>
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH] Segfault in pool_do_auth() when failover races with a new connection
Date: Thu, 02 Jul 2026 16:39:22 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAGXsc+Y7OKp93zSqt1WUCh7ABC7kyZqrxJPFA8AC1woB30S8EQ@mail.gmail.com>
References: <CAGXsc+Y7OKp93zSqt1WUCh7ABC7kyZqrxJPFA8AC1woB30S8EQ@mail.gmail.com>
Hi Emond,
> Hi,
>
> Last night one of CI builds detected a segmentation fault in pgpool
> (4.7.2). I've ran an analysis on the coredump with Claude and came to
> the conclusion that this can happen when trying to open a connection
> in the middle of a failover.
>
> Symptom / backtrace (resolved from the core dump):
> pool_do_auth (src/auth/pool_auth.c:349) <-- SIGSEGV, fault addr 0x0
> connect_backend (src/protocol/child.c:1099) (PG_TRY block)
> get_backend_connection (src/protocol/child.c:2109)
> do_child (src/protocol/child.c:233)
>
> The crashing statement is:
> protoMajor = MAIN_CONNECTION(cp)->sp->major; /* pool_auth.c:349 */
> i.e. cp->slots[MAIN_NODE_ID] is NULL. From the core, the pool had
> slots[0] == NULL while slots[1]/slots[2] were valid, and MAIN_NODE_ID == 0.
>
> pool_initialize_private_backend_status() snapshots
> private_backend_status[] and my_main_node_id non‑atomically w.r.t. the
> parent's failover, so a child can capture my_main_node_id == 0 while
> node 0 is already CON_DOWN. new_connection() then never creates
> slots[0], and pool_do_auth() dereferences the NULL main slot. (Same
> class as the NULL‑MAIN_CONNECTION case noted in
> pool_virtual_main_db_node_id().)
From pool_initialize_private_backend_status():
for (i = 0; i < MAX_NUM_BACKENDS; i++)
{
private_backend_status[i] = BACKEND_INFO(i).backend_status;
/* my_backend_status is referred to by VALID_BACKEND macro. */
my_backend_status[i] = &private_backend_status[i];
}
my_main_node_id = REAL_MAIN_NODE_ID;
BACKEND_INFO(i).backend_status are updated in the pgpool main process
at line 1793 of failover(pgpool_main.c) by calling
handle_failover_request(), but REAL_MAIN_NODE_ID (actually
Req_info->main_node_id, shared memory) is updated at 1846 of
failover() by calling save_node_info(). So updating REAL_MAIN_NODE_ID
tends to slightly delay after updating
BACKEND_INFO(i).backend_status. This may explain why my_main_node is
set the stale data (1).
Another possibility is my_main_node_id being lacking volatile
qualification. This also could set stale data to my_main_node_id (2).
Of course (1) and (2) are not mutually exclusive. So your patch for
(1) and fix for (2) need to be both applied.
By the way in your patch there is loops like this. This can be
replaced by existing get_next_main_node().
+ for (i = 0; i < NUM_BACKENDS; i++)
+ {
+ if (private_backend_status[i] == CON_UP ||
+ private_backend_status[i] == CON_CONNECT_WAIT)
+ {
+ my_main_node_id = i;
+ break;
+ }
+ }
All of the above in mind, I propose attached patch.
What do you think?
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
[text/x-patch] connect_crash_fix_v2.patch (3.6K, ../[email protected]/2-connect_crash_fix_v2.patch)
download | inline diff:
diff --git a/src/include/pool.h b/src/include/pool.h
index 549aed30f..7ead2e926 100644
--- a/src/include/pool.h
+++ b/src/include/pool.h
@@ -345,8 +345,8 @@ extern int pool_get_major_version(void);
extern bool pool_is_node_to_be_sent_in_current_query(int node_id);
extern int pool_virtual_main_db_node_id(void);
-extern BACKEND_STATUS *my_backend_status[];
-extern int my_main_node_id;
+extern volatile BACKEND_STATUS *my_backend_status[];
+extern volatile int my_main_node_id;
#define VALID_BACKEND(backend_id) \
((RAW_MODE && (backend_id) == REAL_MAIN_NODE_ID) || \
@@ -662,7 +662,7 @@ extern volatile sig_atomic_t ignore_sigusr1;
#define QUERY_STRING_BUFFER_LEN 1024
extern char query_string_buffer[]; /* last query string sent to simpleQuery() */
-extern BACKEND_STATUS private_backend_status[MAX_NUM_BACKENDS];
+extern volatile BACKEND_STATUS private_backend_status[MAX_NUM_BACKENDS];
extern char remote_host[]; /* client host */
extern char remote_port[]; /* client port */
diff --git a/src/main/pgpool_main.c b/src/main/pgpool_main.c
index 19d89cb79..a99b1b5b5 100644
--- a/src/main/pgpool_main.c
+++ b/src/main/pgpool_main.c
@@ -226,7 +226,7 @@ static pid_t health_check_pids[MAX_NUM_BACKENDS];
/*
* Private copy of backend status
*/
-BACKEND_STATUS private_backend_status[MAX_NUM_BACKENDS];
+volatile BACKEND_STATUS private_backend_status[MAX_NUM_BACKENDS];
/*
* shmem connection info table
@@ -278,7 +278,7 @@ static pid_t wd_lifecheck_pid = 0; /* pid for child process handling watchdog
* lifecheck */
BACKEND_STATUS *my_backend_status[MAX_NUM_BACKENDS]; /* Backend status buffer */
-int my_main_node_id; /* Main node id buffer */
+volatile int my_main_node_id; /* Main node id buffer */
/*
* Dummy variable to suppress compiler warnings by discarding return values
diff --git a/src/protocol/child.c b/src/protocol/child.c
index 4a527c84c..67a3c6c9a 100644
--- a/src/protocol/child.c
+++ b/src/protocol/child.c
@@ -1483,7 +1483,20 @@ pool_initialize_private_backend_status(void)
my_backend_status[i] = &private_backend_status[i];
}
- my_main_node_id = REAL_MAIN_NODE_ID;
+ my_main_node_id = REAL_MAIN_NODE_ID;
+
+ /*
+ * REAL_MAIN_NODE_ID and the per-node status are read from shared memory
+ * non-atomically, so a concurrent failover can leave my_main_node_id
+ * pointing at a node we just captured as down. Re-point it at the first
+ * node that is up in our private snapshot; otherwise MAIN_CONNECTION()
+ * would dereference a connection slot that is never created for a down
+ * node.
+ */
+ if (my_main_node_id < 0 || my_main_node_id >= NUM_BACKENDS ||
+ (private_backend_status[my_main_node_id] != CON_UP &&
+ private_backend_status[my_main_node_id] != CON_CONNECT_WAIT))
+ my_main_node_id = get_next_main_node();
}
static void
diff --git a/src/protocol/pool_connection_pool.c b/src/protocol/pool_connection_pool.c
index b16ccc39e..a939b08a7 100644
--- a/src/protocol/pool_connection_pool.c
+++ b/src/protocol/pool_connection_pool.c
@@ -1035,6 +1035,26 @@ new_connection(POOL_CONNECTION_POOL *p)
if (active_backend_count > 0)
{
+ /*
+ * A concurrent failover may have changed backend status while we were
+ * creating connections, leaving my_main_node_id pointing at a node we
+ * skipped (its slot is NULL). Re-point it at a node we did connect to
+ * so that MAIN_CONNECTION() never dereferences a NULL slot (e.g. in
+ * pool_do_auth()).
+ */
+ if (my_main_node_id < 0 || my_main_node_id >= NUM_BACKENDS ||
+ p->slots[my_main_node_id] == NULL)
+ {
+ for (i = 0; i < NUM_BACKENDS; i++)
+ {
+ if (p->slots[i] != NULL)
+ {
+ my_main_node_id = i;
+ break;
+ }
+ }
+ }
+
return p;
}
view thread (8+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Segfault in pool_do_auth() when failover races with a new connection
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox