Horton

Thread

Commits

  1. Fix mislabeling of PROC_QUEUE->links as PGPROC, fixing UBSan on 32bit

  1. ubsan fails on 32bit builds

    Andres Freund <andres@anarazel.de> — 2022-11-17T01:42:30Z

    Hi,

I am working on polishing my patch to make CI use sanitizers. Unfortunately
using -fsanitize=alignment,undefined causes tests to fail on 32bit builds.

https://cirrus-ci.com/task/5092504471601152
https://api.cirrus-ci.com/v1/artifact/task/5092504471601152/testrun/build-32/testrun/recovery/022_crash_temp_files/log/022_crash_temp_files_node_crash.log

../src/backend/storage/lmgr/proc.c:1173:2: runtime error: member access within misaligned address 0xf4019e54 for type 'struct PGPROC', which requires 8 byte alignment
0xf4019e54: note: pointer points here
  e0 0d 09 f4 54 9e 01 f4  54 9e 01 f4 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
              ^
==65203==Using libbacktrace symbolizer.
    #0 0x57076f46 in ProcSleep ../src/backend/storage/lmgr/proc.c:1173
    #1 0x57054cf7 in WaitOnLock ../src/backend/storage/lmgr/lock.c:1859
    #2 0x57058e4f in LockAcquireExtended ../src/backend/storage/lmgr/lock.c:1101
    #3 0x57058f82 in LockAcquire ../src/backend/storage/lmgr/lock.c:752
    #4 0x57051bb8 in XactLockTableWait ../src/backend/storage/lmgr/lmgr.c:702
    #5 0x569c31b3 in _bt_doinsert ../src/backend/access/nbtree/nbtinsert.c:225
    #6 0x569cff09 in btinsert ../src/backend/access/nbtree/nbtree.c:200
    #7 0x569ac19d in index_insert ../src/backend/access/index/indexam.c:193
    #8 0x56c72af6 in ExecInsertIndexTuples ../src/backend/executor/execIndexing.c:416
    #9 0x56d014c7 in ExecInsert ../src/backend/executor/nodeModifyTable.c:1065
...


I can reproduce this locally.

At first I thought the problem was caused by:
46d6e5f5679 Display the time when the process started waiting for the lock, in pg_locks, take 2

as pg_atomic_uint64 is 8 byte aligned on x86 - otherwise one gets into
terrible terrible performance territory because atomics can be split across
cachelines - but 46d6e5f5679 didn't teach ProcGlobalShmemSize() /
InitProcGlobal() that allocations need to be aligned to a larger
size. However, we've made ShmemAllocRaw() use cacheline alignment, which
should suffice.  And indeed - ProcGlobal->allProcs is aligned correctly, and
sizeof(PGPROC) % 8 == 0.  It doesn't seem great to rely on that, but ...


Printing out *proc in proc.c:1173 seems indicates clearly that it's not a
valid proc for some reason.

(gdb) p myHeldLocks
$26 = 0
(gdb) p lock->waitProcs
$27 = {links = {prev = 0xf33c4b5c, next = 0xf33c4b5c}, size = 0}
(gdb) p &(waitQueue->links)
$29 = (SHM_QUEUE *) 0xf33c4b5c
(gdb) p proc
$30 = (PGPROC *) 0xf33c4b5c

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?

Greetings,

Andres Freund

  2. Re: ubsan fails on 32bit builds

    Andres Freund <andres@anarazel.de> — 2022-11-17T07:28:21Z

    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

  3. Re: ubsan fails on 32bit builds

    Robert Haas <robertmhaas@gmail.com> — 2022-11-17T19:20:47Z

    On Wed, Nov 16, 2022 at 8:42 PM Andres Freund <andres@anarazel.de> 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.

Not that I object to a targeted fix, but it's been 10 years since
slist and dlist were committed, and we really ought to eliminate
SHM_QUEUE entirely in favor of using those. It's basically an
open-coded implementation of something for which we now have a
toolkit. Not that it's impossible to make this kind of mistake with a
toolkit, but in general open-coding the same logic in multiple places
increases the risk of bugs.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  4. Re: ubsan fails on 32bit builds

    Andres Freund <andres@anarazel.de> — 2022-11-17T20:13:04Z

    Hi,

On 2022-11-17 14:20:47 -0500, Robert Haas wrote:
> On Wed, Nov 16, 2022 at 8:42 PM Andres Freund <andres@anarazel.de> 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.
> 
> Not that I object to a targeted fix

Should we backpatch this fix? Likely this doesn't cause active breakage
outside of 32bit builds under ubsan, but that's not an unreasonable thing to
want to do in the backbranches.


> but it's been 10 years since
> slist and dlist were committed, and we really ought to eliminate
> SHM_QUEUE entirely in favor of using those. It's basically an
> open-coded implementation of something for which we now have a
> toolkit. Not that it's impossible to make this kind of mistake with a
> toolkit, but in general open-coding the same logic in multiple places
> increases the risk of bugs.

Agreed. I had started on a set of patches for some of the SHM_QUEUE uses, but
somehow we ended up a bit stuck on the naming of dlist_delete variant that
afterwards zeroes next/prev so we can replace SHMQueueIsDetached() uses.

Should probably find and rebase those patches...

Greetings,

Andres Freund

  5. Re: ubsan fails on 32bit builds

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-11-17T20:15:04Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-11-17 14:20:47 -0500, Robert Haas wrote:
>> Not that I object to a targeted fix

> Should we backpatch this fix? Likely this doesn't cause active breakage
> outside of 32bit builds under ubsan, but that's not an unreasonable thing to
> want to do in the backbranches.

+1 for backpatching what you showed.

>> but it's been 10 years since
>> slist and dlist were committed, and we really ought to eliminate
>> SHM_QUEUE entirely in favor of using those.

> Agreed. I had started on a set of patches for some of the SHM_QUEUE uses, but
> somehow we ended up a bit stuck on the naming of dlist_delete variant that
> afterwards zeroes next/prev so we can replace SHMQueueIsDetached() uses.

Also +1, but of course for HEAD only.

			regards, tom lane

  6. Re: ubsan fails on 32bit builds

    Thomas Munro <thomas.munro@gmail.com> — 2022-11-17T21:13:09Z

    On Fri, Nov 18, 2022 at 9:13 AM Andres Freund <andres@anarazel.de> wrote:
> Agreed. I had started on a set of patches for some of the SHM_QUEUE uses, but
> somehow we ended up a bit stuck on the naming of dlist_delete variant that
> afterwards zeroes next/prev so we can replace SHMQueueIsDetached() uses.
>
> Should probably find and rebase those patches...

https://www.postgresql.org/message-id/flat/20200211042229.msv23badgqljrdg2%40alap3.anarazel.de