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 01018819010bf316f8000b703a94c09d4b264cd2..6f5f90f9af4a6fd3c89e63a04d758b2621102784 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.177.4.5 2008/05/27 21:13:50 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.177.4.6 2008/10/25 03:33:05 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -2100,17 +2100,20 @@ 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.)
  *
  *     When delete_ok is TRUE, it's okay to delete fully-processed events.
  *     The events list pointers are updated.
  * ----------
  */
 static void
-afterTriggerInvokeEvents(AfterTriggerEventList *events,
+afterTriggerInvokeEvents(int query_depth,
                                                 CommandId firing_id,
                                                 bool delete_ok)
 {
+       AfterTriggerEventList *events;
        AfterTriggerEvent event,
                                prev_event;
        MemoryContext per_tuple_context;
@@ -2127,6 +2130,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)
@@ -2207,7 +2211,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
@@ -2220,6 +2227,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 */
@@ -2322,8 +2330,6 @@ AfterTriggerBeginQuery(void)
 void
 AfterTriggerEndQuery(void)
 {
-       AfterTriggerEventList *events;
-
        /* Must be inside a transaction */
        Assert(afterTriggers != NULL);
 
@@ -2340,17 +2346,19 @@ AfterTriggerEndQuery(void)
         * SET CONSTRAINTS ... 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, true);
+               afterTriggerInvokeEvents(afterTriggers->query_depth, firing_id, true);
        }
 
        afterTriggers->query_depth--;
@@ -2396,7 +2404,7 @@ AfterTriggerFireDeferred(void)
        {
                CommandId               firing_id = afterTriggers->firing_counter++;
 
-               afterTriggerInvokeEvents(events, firing_id, true);
+               afterTriggerInvokeEvents(-1, firing_id, true);
        }
 
        Assert(events->head == NULL);
@@ -2845,7 +2853,7 @@ AfterTriggerSetState(ConstraintsSetStmt *stmt)
                         * level, but we'd better not if inside a subtransaction, since
                         * the subtransaction could later get rolled back.
                         */
-                       afterTriggerInvokeEvents(events, firing_id,
+                       afterTriggerInvokeEvents(-1, firing_id,
                                                                         !IsSubTransaction());
                }
        }