Re: Address the -Wuse-after-free warning in ATExecAttachPartition()

From: Amit Langote <amitlangote09(at)gmail(dot)com>
To: Junwang Zhao <zhjwpku(at)gmail(dot)com>
Cc: Nitin Jadhav <nitinjadhavpostgres(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>
Subject: Re: Address the -Wuse-after-free warning in ATExecAttachPartition()
Date: 2024-07-09 10:18:28
Message-ID: CA+HiwqGyW__AqcZ4HiThZ99VLNYKHHPk9q28=9sEMOiA_+HkLg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Junwang,

On Mon, Jul 8, 2024 at 7:08 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
> On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav
> <nitinjadhavpostgres(at)gmail(dot)com> wrote:
> >
> > In [1], Andres reported a -Wuse-after-free bug in the
> > ATExecAttachPartition() function. I've created a patch to address it
> > with pointers from Amit offlist.
> >
> > The issue was that the partBoundConstraint variable was utilized after
> > the list_concat() function. This could potentially lead to accessing
> > the partBoundConstraint variable after its memory has been freed.
> >
> > The issue was resolved by using the return value of the list_concat()
> > function, instead of using the list1 argument of list_concat(). I
> > copied the partBoundConstraint variable to a new variable named
> > partConstraint and used it for the previous references before invoking
> > get_proposed_default_constraint(). I confirmed that the
> > eval_const_expressions(), make_ands_explicit(),
> > map_partition_varattnos(), QueuePartitionConstraintValidation()
> > functions do not modify the memory location pointed to by the
> > partBoundConstraint variable. Therefore, it is safe to use it for the
> > next reference in get_proposed_default_constraint()
> >
> > Attaching the patch. Please review and share the comments if any.
> > Thanks to Andres for spotting the bug and some off-list advice on how
> > to reproduce it.
>
> The patch LGTM.
>
> Curious how to reproduce the bug ;)

Download and apply (`git am`) Andres' patch to "add allocator
attributes" here (note it's not merged into the tree yet!):
https://github.com/anarazel/postgres/commit/99067d5c944e7bd29a8702689f470f623723f4e7.patch

Then configure using meson with -Dc_args="-Wuse-after-free=3"
--buildtype=release

Compiling with gcc-12 or gcc-13 should give you the warning that looks
as follows:

[713/2349] Compiling C object
src/backend/postgres_lib.a.p/commands_tablecmds.c.o
../src/backend/commands/tablecmds.c: In function ‘ATExecAttachPartition’:
../src/backend/commands/tablecmds.c:18593:25: warning: pointer
‘partBoundConstraint’ may be used after ‘list_concat’
[-Wuse-after-free]
18593 |
get_proposed_default_constraint(partBoundConstraint);
|
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/backend/commands/tablecmds.c:18546:26: note: call to ‘list_concat’ here
18546 | partConstraint = list_concat(partBoundConstraint,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
18547 |
RelationGetPartitionQual(rel));
|
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

--
Thanks, Amit Langote

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2024-07-09 10:26:12 Re: tests fail on windows with default git settings
Previous Message Amul Sul 2024-07-09 09:53:39 pg_verifybackup: TAR format backup verification