From bb71702191757bc086a3e4e2d27529264e074d86 Mon Sep 17 00:00:00 2001 From: Vlad Lesin Date: Fri, 15 May 2026 14:27:46 +0300 Subject: [PATCH 4/5] Fix lock-group teardown double-push and leak This commit fixes two latent bugs in ProcKill()'s lock-group teardown freelist publication: a double push of the leader's PGPROC that corrupts the freelist, and a leak of the last follower's PGPROC slot. ProcKill()'s lock-group teardown had two PGPROC freelist publications scattered through the function: a follower's inline push of the leader's PGPROC inside the lock-group block (taken when the follower is the last group member exiting), and every backend's lockGroupLeader-NULL-driven self-push at the bottom of the function. The two were coordinated only by inspection of proc->lockGroupLeader, which the follower cleared as a side effect of pushing the leader. This coordination was broken: in the canonical two-backend scenario, * the follower cleared leader->lockGroupLeader and pushed the leader's PGPROC inline under leader_lwlock; * the follower did NOT clear its own proc->lockGroupLeader (that clearing happens only on the "else if (leader != MyProc)" branch, which the follower skipped); * when the leader reached the bottom of ProcKill, it saw proc->lockGroupLeader == NULL (the follower cleared it) and pushed itself -- a second dlist_push_tail() of the same node onto the same freelist; * the follower at the bottom saw its own proc->lockGroupLeader != NULL (never cleared) and skipped its own push; its slot leaked. The second push corrupts the dlist's prev/next pointers, after which dlist_pop_head_node() can return invalid memory or hand the same slot to two backends. Replace the inline dlist_push_head() in the lock-group block (the follower-returns-leader path) and the lockGroupLeader-NULL-driven self-push that followed it with two booleans -- push_leader and push_self -- decided under leader_lwlock, and consolidate every freelist push into a single ProcGlobal->freeProcsLock critical section at the bottom of the function. Holding leader_lwlock across the decisions is what makes this reorganization safe: (a) The leader's "should I self-push?" choice and a follower's "should I push the leader?" choice are both made inside the same leader_lwlock critical section, so they cannot interleave. At most one pusher sets each boolean, and every backend's proc->lockGroupLeader is cleared whenever the corresponding push_self is true. This eliminates the double-push and the leak. (b) A follower only sets push_leader when it observes an empty group after removing itself, so the leader has already passed its own lwlock-held dlist_delete by the time a pusher runs. This commit's structural change does NOT close the related procLatch recycle race: the leader-slot push is now deferred from inside the lock-group block to the consolidated critical section below the follower's own DisownLatch, so the slot is no longer on the freelist while the follower is parked at the injection point, yet the follower's deferred push of the leader slot still races the leader's own DisownLatch, which lives in the leader's process and is serialized against the follower's push only by unrelated locks. The following commit hoists DisownLatch and closes that window. --- src/backend/storage/lmgr/proc.c | 92 +++++++++++++++++++++++---------- 1 file changed, 66 insertions(+), 26 deletions(-) diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 9ba0d4790fd..7201ad9caa8 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -924,7 +924,10 @@ static void ProcKill(int code, Datum arg) { PGPROC *proc; + PGPROC *leader; dlist_head *procgloballist; + bool push_leader; + bool push_self; Assert(MyProc != NULL); @@ -960,35 +963,74 @@ ProcKill(int code, Datum arg) /* Cancel any pending condition variable sleep, too */ ConditionVariableCancelSleep(); + proc = MyProc; + procgloballist = proc->procgloballist; + /* - * Detach from any lock group of which we are a member. If the leader - * exits before all other group members, its PGPROC will remain allocated - * until the last group process exits; that process must return the - * leader's PGPROC to the appropriate list. + * Detach from any lock group of which we are a member, deciding under + * leader_lwlock whether we (and possibly the leader) need to be pushed + * onto a freelist. The actual pushes happen below, under + * ProcGlobal->freeProcsLock. + * + * Holding leader_lwlock across the decisions is what makes this safe: + * + * (a) The leader's "should I self-push?" choice and a follower's "should + * I push the leader?" choice are both made inside the same leader_lwlock + * critical section, so they cannot interleave. (That interleaving was + * the source of a double push.) + * + * (b) A follower only sets push_leader when it observes an empty group + * after removing itself, so the leader has already passed its own + * lwlock-held dlist_delete by the time a pusher runs. + * + * NOTE: this commit does NOT establish the latch-ownership invariant that + * the freelist pushes must follow DisownLatch -- see the next commit for + * that. */ - if (MyProc->lockGroupLeader != NULL) + push_leader = false; + push_self = true; + leader = NULL; + + if (proc->lockGroupLeader != NULL) { - PGPROC *leader = MyProc->lockGroupLeader; - LWLock *leader_lwlock = LockHashPartitionLockByProc(leader); + LWLock *leader_lwlock; + + leader = proc->lockGroupLeader; + leader_lwlock = LockHashPartitionLockByProc(leader); LWLockAcquire(leader_lwlock, LW_EXCLUSIVE); Assert(!dlist_is_empty(&leader->lockGroupMembers)); - dlist_delete(&MyProc->lockGroupLink); + dlist_delete(&proc->lockGroupLink); if (dlist_is_empty(&leader->lockGroupMembers)) { leader->lockGroupLeader = NULL; - if (leader != MyProc) + if (leader != proc) { - procgloballist = leader->procgloballist; - - /* Leader exited first; return its PGPROC. */ - SpinLockAcquire(&ProcGlobal->freeProcsLock); - dlist_push_head(procgloballist, &leader->freeProcsLink); - SpinLockRelease(&ProcGlobal->freeProcsLock); + /* + * We are the last follower and the leader exited earlier; its + * PGPROC is still allocated and must be pushed here. + */ + push_leader = true; + proc->lockGroupLeader = NULL; } + /* else: leader == proc; the clear above covered us */ + } + else if (leader != proc) + { + /* Non-last follower; leader still present in the group. */ + proc->lockGroupLeader = NULL; + } + else + { + /* + * We are the leader and followers remain. Skip our own push; the + * last follower to exit will push us. Leave + * proc->lockGroupLeader set until that point so InitProcess's + * freshly-picked-up invariant (lockGroupLeader == NULL) is + * re-established only when the PGPROC is actually free. + */ + push_self = false; } - else if (leader != MyProc) - MyProc->lockGroupLeader = NULL; LWLockRelease(leader_lwlock); /* @@ -1021,7 +1063,6 @@ ProcKill(int code, Datum arg) SwitchBackToLocalLatch(); pgstat_reset_wait_event_storage(); - proc = MyProc; MyProc = NULL; MyProcNumber = INVALID_PROC_NUMBER; DisownLatch(&proc->procLatch); @@ -1031,16 +1072,15 @@ ProcKill(int code, Datum arg) proc->vxid.procNumber = INVALID_PROC_NUMBER; proc->vxid.lxid = InvalidTransactionId; - procgloballist = proc->procgloballist; SpinLockAcquire(&ProcGlobal->freeProcsLock); - - /* - * If we're still a member of a locking group, that means we're a leader - * which has somehow exited before its children. The last remaining child - * will release our PGPROC. Otherwise, release it now. - */ - if (proc->lockGroupLeader == NULL) + if (push_leader) + { + /* Return leader PGPROC (and semaphore) to appropriate freelist */ + dlist_push_head(leader->procgloballist, &leader->freeProcsLink); + } + if (push_self) { + Assert(proc->lockGroupLeader == NULL); /* Since lockGroupLeader is NULL, lockGroupMembers should be empty. */ Assert(dlist_is_empty(&proc->lockGroupMembers)); -- 2.43.0