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

Hi,

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.

I reviewed the faulty code in ATExecAttachPartition() that this patch
aims to address, and it seems that the fix suggested by Alvaro in the
original report [1] might be more appropriate. It also allows us to
resolve a minor issue related to how partBoundConstraint is handled
later in the function. Specifically, the constraint checked against
the default partition, if one exists on the parent table, should not
include the negated version of the parent's constraint, which the
current code mistakenly does. This occurs because the list_concat()
operation modifies the partBoundConstraint when combining it with the
parent table’s partition constraint. Although this doesn't cause a
live bug, the inclusion of the redundant parent constraint introduces
unnecessary complexity in the combined constraint expression. This can
cause slight delays in evaluating the constraint, as the redundant
check always evaluates to false for rows in the default partition.
Ultimately, the constraint failure is still determined by the negated
version of the partition constraint of the table being attached.

I'm inclined to apply the attached patch only to the master branch as
the case for applying this to back-branches doesn't seem all that
strong to me. Thoughts?

--
Thanks, Amit Langote