Hi, On 2022-11-16 17:42:30 -0800, Andres Freund wrote: > Afaict the problem is that > proc = (PGPROC *) &(waitQueue->links); > > is a gross gross hack - this isn't actually a PGPROC, it's pointing to an > SHM_QUEUE, but *not* one embedded in PGPROC. It kinda works because ->links > is at offset 0 in PGPROC, which means that > SHMQueueInsertBefore(&(proc->links), &(MyProc->links)); > will turn &proc->links back into waitQueue->links. Which we then can enqueue > again. > > I don't see the point of this hack, even leaving ubsan's valid complaints > aside. Why bother having this, sometimes, fake PGPROC pointer when we could > just use a SHM_QUEUE* to determine the insertion point?
As done in the attached patch. With this ubsan passes both on 32bit and 64bit. Greetings, Andres Freund
>From 776f002952f92c764e98dfe8c180a00c1f60ab09 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Wed, 16 Nov 2022 20:00:59 -0800 Subject: [PATCH 1/3] Fix mislabeling of PROC_QUEUE->links as PGPROC, fixing ubsan on 32bit Author: Reviewed-by: Discussion: https://postgr.es/m/20221117014230.op5kmgypdv2dt...@awork3.anarazel.de Backpatch: --- src/backend/storage/lmgr/proc.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 13fa07b0ff7..214866502ed 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -1050,13 +1050,13 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable) uint32 hashcode = locallock->hashcode; LWLock *partitionLock = LockHashPartitionLock(hashcode); PROC_QUEUE *waitQueue = &(lock->waitProcs); + SHM_QUEUE *waitQueuePos; LOCKMASK myHeldLocks = MyProc->heldLocks; TimestampTz standbyWaitStart = 0; bool early_deadlock = false; bool allow_autovacuum_cancel = true; bool logged_recovery_conflict = false; ProcWaitStatus myWaitStatus; - PGPROC *proc; PGPROC *leader = MyProc->lockGroupLeader; int i; @@ -1104,13 +1104,16 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable) * we are only considering the part of the wait queue before my insertion * point. */ - if (myHeldLocks != 0) + if (myHeldLocks != 0 && waitQueue->size > 0) { LOCKMASK aheadRequests = 0; + SHM_QUEUE *proc_node; - proc = (PGPROC *) waitQueue->links.next; + proc_node = waitQueue->links.next; for (i = 0; i < waitQueue->size; i++) { + PGPROC *proc = (PGPROC *) proc_node; + /* * If we're part of the same locking group as this waiter, its * locks neither conflict with ours nor contribute to @@ -1118,7 +1121,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable) */ if (leader != NULL && leader == proc->lockGroupLeader) { - proc = (PGPROC *) proc->links.next; + proc_node = proc->links.next; continue; } /* Must he wait for me? */ @@ -1157,20 +1160,21 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable) } /* - * If we fall out of loop normally, proc points to waitQueue head, so - * we will insert at tail of queue as desired. + * If we iterated through the whole queue, cur points to the waitQueue + * head, so we will insert at tail of queue as desired. */ + waitQueuePos = proc_node; } else { /* I hold no locks, so I can't push in front of anyone. */ - proc = (PGPROC *) &(waitQueue->links); + waitQueuePos = &waitQueue->links; } /* - * Insert self into queue, ahead of the given proc (or at tail of queue). + * Insert self into queue, at the position determined above. */ - SHMQueueInsertBefore(&(proc->links), &(MyProc->links)); + SHMQueueInsertBefore(waitQueuePos, &MyProc->links); waitQueue->size++; lock->waitMask |= LOCKBIT_ON(lockmode); -- 2.38.0
