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

On Tue, Oct 1, 2024 at 3:23 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> 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?

I've pushed this to the master branch now.

Thanks Nitin for the report.

--
Thanks, Amit Langote