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

On Tue, Jul 9, 2024 at 7:39 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> On Tue, Jul 9, 2024 at 19:58 Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>>
>> On Tue, Jul 9, 2024 at 6:18 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>> >
>> > 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
>>
>> Thanks Amit,
>>
>> Good to know.
>>
>> When Nitin says:
>>
>> ```Thanks to Andres for spotting the bug and some off-list advice on how
>> to reproduce it.```
>>
>> I thought maybe there is some test case that can really trigger the
>> use-after-free bug, I might get it wrong though ;)
>
>
> I doubt one could write a regression test to demonstrate the use-after-free bug, though I may of course be wrong. By “reproduce it”, I think Nitin meant the warning that suggests that use-after-free bug may occur.

got it, thanks~

--
Regards
Junwang Zhao