On Tue, Oct 1, 2024 at 3:23 PM Amit Langote <amitlangot...@gmail.com> wrote: > Hi, > > On Mon, Jul 8, 2024 at 7:08 PM Junwang Zhao <zhjw...@gmail.com> wrote: > > On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav > > <nitinjadhavpostg...@gmail.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
