Fix possible double-release of spinlock in procsignal.c

michaelpq · Maxim Orlov · michaelpq · commit 62ec3e1f6786 · 2025-02-27T09:43:06.000+09:00
9d9b9d4 has added spinlocks to protect the fields in ProcSignal flags, introducing a code path in ProcSignalInit() where a spinlock could be released twice if the pss_pid field of a ProcSignalSlot is found as already set. Multiple spinlock releases have no effect with most spinlock implementations, but this could cause the code to run into issues when the spinlock is acquired concurrently by a different process. This sanity check on pss_pid generates a LOG that can be delayed until after the spinlock is released as, like older versions up to v17, the code expects the initialization of the ProcSignalSlot to happen even if pss_pid is found incorrect. The code is changed so as the old pss_pid is read while holding the slot's spinlock, with the LOG from the sanity check generated after releasing the spinlock, preventing the double release. Author: Maksim Melnikov <m.melnikov@postgrespro.ru> Co-authored-by: Maxim Orlov <orlovmg@gmail.com> Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru> Discussion: https://postgr.es/m/dca47527-2d8b-4e3b-b5a0-e2deb73371a4@postgrespro.ru
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
@@ -167,21 +167,18 @@ ProcSignalInit(bool cancel_key_valid, int32 cancel_key)
 {
 	ProcSignalSlot *slot;
 	uint64		barrier_generation;
+	uint32		old_pss_pid;
 
 	if (MyProcNumber < 0)
 		elog(ERROR, "MyProcNumber not set");
 	if (MyProcNumber >= NumProcSignalSlots)
 		elog(ERROR, "unexpected MyProcNumber %d in ProcSignalInit (max %d)", MyProcNumber, NumProcSignalSlots);
 	slot = &ProcSignal->psh_slot[MyProcNumber];
 
-	/* sanity check */
 	SpinLockAcquire(&slot->pss_mutex);
-	if (pg_atomic_read_u32(&slot->pss_pid) != 0)
-	{
-		SpinLockRelease(&slot->pss_mutex);
-		elog(LOG, "process %d taking over ProcSignal slot %d, but it's not empty",
-			 MyProcPid, MyProcNumber);
-	}
+
+	/* Value used for sanity check below */
+	old_pss_pid = pg_atomic_read_u32(&slot->pss_pid);
 
 	/* Clear out any leftover signal reasons */
 	MemSet(slot->pss_signalFlags, 0, NUM_PROCSIGNALS * sizeof(sig_atomic_t));
@@ -208,6 +205,11 @@ ProcSignalInit(bool cancel_key_valid, int32 cancel_key)
 
 	SpinLockRelease(&slot->pss_mutex);
 
+	/* Spinlock is released, do the check */
+	if (old_pss_pid != 0)
+		elog(LOG, "process %d taking over ProcSignal slot %d, but it's not empty",
+			 MyProcPid, MyProcNumber);
+
 	/* Remember slot location for CheckProcSignal */
 	MyProcSignalSlot = slot;
 
