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? -- Thanks, Amit Langote
v2-0001-Fix-expression-list-handling-in-ATExecAttachParti.patch
Description: Binary data
