public inbox for [email protected]  

help / color / mirror / Atom feed

From: 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 22:45:17 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAGXsc+aizSg6Z22=pV8NsFb-FukzT2XP1YriOkOJjOBLVdN-vg@mail.gmail.com>
References: <CAGXsc+Y7OKp93zSqt1WUCh7ABC7kyZqrxJPFA8AC1woB30S8EQ@mail.gmail.com>
	<[email protected]>
	<CAGXsc+aizSg6Z22=pV8NsFb-FukzT2XP1YriOkOJjOBLVdN-vg@mail.gmail.com>

Hi Emond,

> 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.

You are right. Attached is the v3 patch.

Regards,
--
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_v3.patch (2.1K, ../[email protected]/2-connect_crash_fix_v3.patch)
  download | inline diff:
diff --git a/src/protocol/child.c b/src/protocol/child.c
index 4a527c84c..a4ec13dfd 100644
--- a/src/protocol/child.c
+++ b/src/protocol/child.c
@@ -1478,12 +1478,26 @@ pool_initialize_private_backend_status(void)
 
 	for (i = 0; i < MAX_NUM_BACKENDS; i++)
 	{
-		private_backend_status[i] = BACKEND_INFO(i).backend_status;
+		private_backend_status[i] =
+			*(volatile BACKEND_STATUS *) &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;
+ 	my_main_node_id = *(volatile int *) &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