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).

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 276efb8477e90f8c202b1099e1049ecb563c64b8..1660ef0f04e5d9aaad866a0b2f9715a93fc011ff 100644 (file)
--- 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.195.2.5 2008/05/27 21:13:39 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.195.2.6 2008/10/25 03:32:58 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -2256,7 +2256,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, then we expect that all the firable events are
  *     for triggers of the relations included in the estate's result relation
@@ -2269,11 +2271,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;
@@ -2292,6 +2295,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)
@@ -2409,7 +2413,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
@@ -2422,6 +2429,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 */
@@ -2532,8 +2540,6 @@ AfterTriggerBeginQuery(void)
 void
 AfterTriggerEndQuery(EState *estate)
 {
-       AfterTriggerEventList *events;
-
        /* Must be inside a transaction */
        Assert(afterTriggers != NULL);
 
@@ -2550,18 +2556,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--;
@@ -2607,7 +2615,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);
@@ -3056,7 +3064,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());
                }
        }