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

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