Обсуждение: Possible segfault when sending notification within a ProcessUtility hook

Hi,

I've encountered the following segfault:

#0: 0x0000000104e821a8 postgres`list_head(l=0x7f7f7f7f7f7f7f7f) at
pg_list.h:130:17
#1: 0x0000000104e81c9c postgres`PreCommit_Notify at async.c:932:16
#2: 0x0000000104dd02f8 postgres`CommitTransaction at xact.c:2236:2
#3: 0x0000000104dcfc24 postgres`CommitTransactionCommand at xact.c:3061:4
#4: 0x000000010528a880 postgres`finish_xact_command at postgres.c:2777:3
#5: 0x00000001052883ac postgres`exec_simple_query(query_string="notify
test;") at postgres.c:1298:4

This happens when a transaction block fails and a ProcessUtility hook
sends a notification during the rollback command.

When a transaction block fails, it will enter in a TBLOCK_ABORT state,
waiting for a rollback. Calling rollback will switch to a
TBLOCK_ABORT_END state and will only go through CleanupTransaction.
If a hook sends a notification during the rollback command, a
notification will be queued but its content will be wiped when the
TopTransactionContext is destroyed.
Trying to send a notification immediately after will segfault in
PreCommit_Notify as pendingNotifies->events will be invalid.

There's a test_notify_rollback test module attached to the patch that reproduces
the issue.

Moving notification clean up from AbortTransaction to CleanupTransaction fixes
the issue as it will clear pendingActions in the same function that destroys the
TopTransactionContext.

Regards,
Anthonin

Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> writes:
> This happens when a transaction block fails and a ProcessUtility hook
> sends a notification during the rollback command.

Why should we regard that as anything other than a bug in the
ProcessUtility hook?  A failed transaction should not send any
notifies.

> Moving notification clean up from AbortTransaction to CleanupTransaction fixes
> the issue as it will clear pendingActions in the same function that destroys the
> TopTransactionContext.

Maybe that would be okay, or maybe not, but I'm disinclined to
mess with it without a better argument for changing it.  It seems
like there would still be room to break things with mistimed
calls to async.c functions.

            regards, tom lane

> On Tue, Dec 5, 2023 at 9:03 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Why should we regard that as anything other than a bug in the
> ProcessUtility hook?  A failed transaction should not send any
> notifies.

Fair point. That was also my initial assumption but I thought that the
transaction
state was not available from a hook as I've missed
IsAbortedTransactionBlockState.

I will rely on IsAbortedTransactionBlockState to avoid this case,
thanks for the input.

Regards,
Anthonin.