Thread: Possible segfault when sending notification within a ProcessUtility hook
Possible segfault when sending notification within a ProcessUtility hook
From
Anthonin Bonnefoy
Date:
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
Attachment
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
Re: Possible segfault when sending notification within a ProcessUtility hook
From
Anthonin Bonnefoy
Date:
> 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.