Re: [PATCH] Segfault in pool_do_auth() when failover races with a new connection

Hi,

The new patch looks fine to me. There's however one thing Claude
pointed out. According to it, my_main_node_id,
private_backend_status[], and my_backend_status[] are per-process
globals (copied on fork), not shared memory. So, making any of those
volatile should not make any difference.

Best regards,
Emond

Op do 2 jul 2026 om 09:39 schreef Tatsuo Ishii <ishii(at)postgresql(dot)org>:
>
> 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