Thread: Potential use-after-free in partion related code
Hi, A while back I had proposed annotations for palloc() et al that let the compiler know about which allocators pair with what freeing functions. One thing that allows the compiler to do is to detect use after free. One such complaint is: ../../../../../home/andres/src/postgresql/src/backend/commands/tablecmds.c: In function ‘ATExecAttachPartition’: ../../../../../home/andres/src/postgresql/src/backend/commands/tablecmds.c:18758:25: warning: pointer ‘partBoundConstraint’may be used after ‘list_concat’ [-Wuse-after-free] 18758 | get_proposed_default_constraint(partBoundConstraint); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../../../../home/andres/src/postgresql/src/backend/commands/tablecmds.c:18711:26: note: call to ‘list_concat’ here 18711 | partConstraint = list_concat(partBoundConstraint, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 18712 | RelationGetPartitionQual(rel)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ And it seems quite right: partConstraint = list_concat(partBoundConstraint, RelationGetPartitionQual(rel)); At this point partBoundConstraint may not be used anymore, because list_concat() might have reallocated. But then a few lines later: /* we already hold a lock on the default partition */ defaultrel = table_open(defaultPartOid, NoLock); defPartConstraint = get_proposed_default_constraint(partBoundConstraint); We use partBoundConstraint again. I unfortunately can't quickly enough identify what partConstraint, defPartConstraint, partBoundConstraint are, so I don't don't really know what the fix here is. Greetings, Andres Freund
On 2023-Nov-15, Andres Freund wrote: > partConstraint = list_concat(partBoundConstraint, > RelationGetPartitionQual(rel)); > > At this point partBoundConstraint may not be used anymore, because > list_concat() might have reallocated. > > But then a few lines later: > > /* we already hold a lock on the default partition */ > defaultrel = table_open(defaultPartOid, NoLock); > defPartConstraint = > get_proposed_default_constraint(partBoundConstraint); > > We use partBoundConstraint again. Yeah, this is wrong if partBoundConstraint is reallocated by list_concat. One possible fix is to change list_concat to list_concat_copy(), which leaves the original list unmodified. AFAICT the bug came in with 6f6b99d1335b, which added default partitions. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Now I have my system running, not a byte was off the shelf; It rarely breaks and when it does I fix the code myself. It's stable, clean and elegant, and lightning fast as well, And it doesn't cost a nickel, so Bill Gates can go to hell."