Thread: Address the -Wuse-after-free warning in ATExecAttachPartition()
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. [1]: https://www.postgresql.org/message-id/flat/202311151802.ngj2la66jwgi%40alvherre.pgsql#4fc5622772ba0244c1ad203f5fc56701 Best Regards, Nitin Jadhav Azure Database for PostgreSQL Microsoft
Attachment
On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav <nitinjadhavpostgres@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. Curious how to reproduce the bug ;) > > [1]: https://www.postgresql.org/message-id/flat/202311151802.ngj2la66jwgi%40alvherre.pgsql#4fc5622772ba0244c1ad203f5fc56701 > > Best Regards, > Nitin Jadhav > Azure Database for PostgreSQL > Microsoft -- Regards Junwang Zhao
Hi Junwang, On Mon, Jul 8, 2024 at 7:08 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav > <nitinjadhavpostgres@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. > > 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
On Tue, Jul 9, 2024 at 6:18 PM Amit Langote <amitlangote09@gmail.com> wrote: > > Hi Junwang, > > On Mon, Jul 8, 2024 at 7:08 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav > > <nitinjadhavpostgres@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. > > > > 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 ;) -- Regards Junwang Zhao
On Tue, Jul 9, 2024 at 19:58 Junwang Zhao <zhjwpku@gmail.com> wrote:
On Tue, Jul 9, 2024 at 6:18 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi Junwang,
>
> On Mon, Jul 8, 2024 at 7:08 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav
> > <nitinjadhavpostgres@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.
> >
> > 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.
On Tue, Jul 9, 2024 at 7:39 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Tue, Jul 9, 2024 at 19:58 Junwang Zhao <zhjwpku@gmail.com> wrote: >> >> On Tue, Jul 9, 2024 at 6:18 PM Amit Langote <amitlangote09@gmail.com> wrote: >> > >> > Hi Junwang, >> > >> > On Mon, Jul 8, 2024 at 7:08 PM Junwang Zhao <zhjwpku@gmail.com> wrote: >> > > On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav >> > > <nitinjadhavpostgres@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. >> > > >> > > 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 “reproduceit”, I think Nitin meant the warning that suggests that use-after-free bug may occur. got it, thanks~ -- Regards Junwang Zhao