From d4a1e5b0a3cd503de0f84e4c423643e0788bb066 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 25 Oct 2008 03:32:44 +0000 Subject: [PATCH] Fix an old bug in after-trigger handling: AfterTriggerEndQuery took the address of afterTriggers->query_stack[afterTriggers->query_depth] and hung onto it through all its firings of triggers. However, if a trigger causes sufficiently many nested query executions, query_stack will get repalloc'd bigger, leaving AfterTriggerEndQuery --- and hence afterTriggerInvokeEvents --- using a stale pointer. So far as I can find, the only consequence of this error is to stomp on a couple of words of already-freed memory; which would lead to a failure only if that chunk had already gotten re-allocated for something else. So it's hard to exhibit a simple failure case, but this is surely a bug. I noticed this while working on my recent patch to reduce pending-trigger space usage. The present patch is mighty ugly, because it requires making afterTriggerInvokeEvents know about all the possible event lists it might get called on. Fortunately, this is only needed in back branches because CVS HEAD avoids the problem in a different way: afterTriggerInvokeEvents only touches the passed AfterTriggerEventList pointer once at startup. Back branches are stable enough that wiring in knowledge of all possible call usages doesn't seem like a killer problem. Back-patch to 8.0. 7.4's trigger code is completely different and doesn't seem to have the problem (it doesn't even use repalloc). --- src/backend/commands/trigger.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 5bca2233279..33da6632cae 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -7,7 +7,7 @@ * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.227 2008/01/02 23:34:42 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.227.2.1 2008/10/25 03:32:44 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -2596,7 +2596,9 @@ afterTriggerMarkEvents(AfterTriggerEventList *events, * afterTriggerInvokeEvents() * * Scan the given event list for events that are marked as to be fired - * in the current firing cycle, and fire them. + * in the current firing cycle, and fire them. query_depth is the index in + * afterTriggers->query_stack, or -1 to examine afterTriggers->events. + * (We have to be careful here because query_stack could move under us.) * * If estate isn't NULL, we use its result relation info to avoid repeated * openings and closing of trigger target relations. If it is NULL, we @@ -2608,11 +2610,12 @@ afterTriggerMarkEvents(AfterTriggerEventList *events, * ---------- */ static void -afterTriggerInvokeEvents(AfterTriggerEventList *events, +afterTriggerInvokeEvents(int query_depth, CommandId firing_id, EState *estate, bool delete_ok) { + AfterTriggerEventList *events; AfterTriggerEvent event, prev_event; MemoryContext per_tuple_context; @@ -2638,6 +2641,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events, ALLOCSET_DEFAULT_MAXSIZE); prev_event = NULL; + events = (query_depth >= 0) ? &afterTriggers->query_stack[query_depth] : &afterTriggers->events; event = events->head; while (event != NULL) @@ -2699,7 +2703,10 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events, if (prev_event) prev_event->ate_next = next_event; else + { + events = (query_depth >= 0) ? &afterTriggers->query_stack[query_depth] : &afterTriggers->events; events->head = next_event; + } pfree(event); } else @@ -2712,6 +2719,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events, } /* Update list tail pointer in case we just deleted tail event */ + events = (query_depth >= 0) ? &afterTriggers->query_stack[query_depth] : &afterTriggers->events; events->tail = prev_event; /* Release working resources */ @@ -2831,8 +2839,6 @@ AfterTriggerBeginQuery(void) void AfterTriggerEndQuery(EState *estate) { - AfterTriggerEventList *events; - /* Must be inside a transaction */ Assert(afterTriggers != NULL); @@ -2849,18 +2855,20 @@ AfterTriggerEndQuery(EState *estate) * IMMEDIATE: all events we have decided to defer will be available for it * to fire. * - * We loop in case a trigger queues more events. + * We loop in case a trigger queues more events at the same query level + * (is that even possible?). Be careful here: firing a trigger could + * result in query_stack being repalloc'd, so we can't save its address + * across afterTriggerInvokeEvents calls. * * If we find no firable events, we don't have to increment * firing_counter. */ - events = &afterTriggers->query_stack[afterTriggers->query_depth]; - while (afterTriggerMarkEvents(events, &afterTriggers->events, true)) + while (afterTriggerMarkEvents(&afterTriggers->query_stack[afterTriggers->query_depth], &afterTriggers->events, true)) { CommandId firing_id = afterTriggers->firing_counter++; /* OK to delete the immediate events after processing them */ - afterTriggerInvokeEvents(events, firing_id, estate, true); + afterTriggerInvokeEvents(afterTriggers->query_depth, firing_id, estate, true); } afterTriggers->query_depth--; @@ -2906,7 +2914,7 @@ AfterTriggerFireDeferred(void) { CommandId firing_id = afterTriggers->firing_counter++; - afterTriggerInvokeEvents(events, firing_id, NULL, true); + afterTriggerInvokeEvents(-1, firing_id, NULL, true); } Assert(events->head == NULL); @@ -3471,7 +3479,7 @@ AfterTriggerSetState(ConstraintsSetStmt *stmt) * but we'd better not if inside a subtransaction, since the * subtransaction could later get rolled back. */ - afterTriggerInvokeEvents(events, firing_id, NULL, + afterTriggerInvokeEvents(-1, firing_id, NULL, !IsSubTransaction()); } }