pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedUse-after-free in 12- EventTriggerAlterTableEnd
2+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2020-10-27 93f726c04f 66c036b4f1 cc623ed2f4 7978ad0254 41c742a432 Fix use-after-free bug with event triggers and ALTER TABLE.
* Use-after-free in 12- EventTriggerAlterTableEnd
@ 2020-10-27 09:36 Arseny Sher <a.sher@postgrespro.ru>
2020-10-27 19:21 ` Re: Use-after-free in 12- EventTriggerAlterTableEnd Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 2+ messages in thread
From: Arseny Sher @ 2020-10-27 09:36 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
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%4...
--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
use_after_free_EventTriggerAlterTableEnd.diff (text/x-diff, 735B) download
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);
* Re: Use-after-free in 12- EventTriggerAlterTableEnd
2020-10-27 09:36 Use-after-free in 12- EventTriggerAlterTableEnd Arseny Sher <a.sher@postgrespro.ru>
@ 2020-10-27 19:21 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 2+ messages in thread
From: Tom Lane @ 2020-10-27 19:21 UTC (permalink / raw)
To: Arseny Sher <a.sher@postgrespro.ru>; +Cc: pgsql-hackers; Alvaro Herrera <alvherre@alvh.no-ip.org>
Arseny Sher <a.sher@postgrespro.ru> writes:
> 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-.
Yeah, that clearly should have been back-patched --- the fact that it
accidentally didn't fail in the most common case wasn't a good reason
for leaving the bug in place. I'm not excited about the test case
ced138e8cba added though, so I think your proposed patch is fine.
Will push shortly.
regards, tom lane
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2020-10-27 19:21 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-10-27 09:36 Use-after-free in 12- EventTriggerAlterTableEnd Arseny Sher <a.sher@postgrespro.ru>
2020-10-27 19:21 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox