Hi, Valgrind on our internal buildfarm complained about use-after-free during currentEventTriggerState->commandList manipulations, e.g. lappend in EventTriggerCollectSimpleCommand. I've discovered that the source of problem is EventTriggerAlterTableEnd not bothering to switch into its own context before appending to the list. ced138e8cba fixed this in master and 13 but wasn't backpatched further, so I see the problem in 12-.
The particular reproducing scenario is somewhat involved; it combines pg_pathman [1] extension and SQL interface to it created in our fork of Postgres. Namely, we allow to create partitions in CREATE TABLE PARTITIONED by statement (similar to what [2] proposes). Each partition is created via separate SPI call which in turn ends up making AlterTableStmt as ProcessUtility PROCESS_UTILITY_SUBCOMMAND; so EventTriggerAlterTableStart/End is done, but additional EventTriggerQueryState is not allocated and commands are collected in toplevel EventTriggerQueryState. Of course SPI frees its memory between the calls which makes valgrind scream. Admittedly our case is tricky and I'm not sure it is possible to make up something like that in the pure core code, but I do believe other extension writers might run into this, so I propose to backpatch (the attached) 3 lines healing to all supported branches. Technically, all you (an extension author) have to do to encounter this is 1) Have toplevel EvenTriggerQueryState, i.e. catch utility statement. 2) Inside it, run AlterTable as PROCESS_UTILITY_SUBCOMMAND in some short-living context. [1] https://github.com/postgrespro/pg_pathman [2] https://www.postgresql.org/message-id/flat/CALT9ZEFBv05OhLMKO1Lbo_Zg9a0v%2BU9q9twe%3Dt-dixfR45RmVQ%40mail.gmail.com#f86f0fcfa62d5108fb81556a43f42457
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c index f7ee9838f7..d1972e2d7f 100644 --- a/src/backend/commands/event_trigger.c +++ b/src/backend/commands/event_trigger.c @@ -1807,9 +1807,15 @@ EventTriggerAlterTableEnd(void) /* If no subcommands, don't collect */ if (list_length(currentEventTriggerState->currentCommand->d.alterTable.subcmds) != 0) { + MemoryContext oldcxt; + + oldcxt = MemoryContextSwitchTo(currentEventTriggerState->cxt); + currentEventTriggerState->commandList = lappend(currentEventTriggerState->commandList, currentEventTriggerState->currentCommand); + + MemoryContextSwitchTo(oldcxt); } else pfree(currentEventTriggerState->currentCommand);
-- Arseny Sher Postgres Professional: http://www.postgrespro.com The Russian Postgres Company