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

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]: /messages/by-id/202311151802.ngj2la66jwgi@alvherre.pgsql

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 “reproduce it”, I think Nitin meant the warning that suggests that use-after-free bug may occur.

got it, thanks~

--
Regards
Junwang Zhao

Hi,

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.

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

On Tue, Oct 1, 2024 at 3:23 PM Amit Langote <amitlangote09@gmail.com> wrote:

Hi,

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.

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