public inbox for [email protected]  

help / color / mirror / Atom feed

From: Emond Papegaaij <[email protected]>
To: Tatsuo Ishii <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] Segfault in pool_do_auth() when failover races with a new connection
Date: Thu, 2 Jul 2026 11:21:13 +0200
Message-ID: <CAGXsc+aizSg6Z22=pV8NsFb-FukzT2XP1YriOkOJjOBLVdN-vg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAGXsc+Y7OKp93zSqt1WUCh7ABC7kyZqrxJPFA8AC1woB30S8EQ@mail.gmail.com>
	<[email protected]>

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 <[email protected]>:
>
> 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

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: <CAGXsc+aizSg6Z22=pV8NsFb-FukzT2XP1YriOkOJjOBLVdN-vg@mail.gmail.com>

* 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