Fix possible dangling pointer dereference in trigger.c.

AfterTriggerEndQuery correctly notes that the query_stack could get
repalloc'd during a trigger firing, but it nonetheless passes the address
of a query_stack entry to afterTriggerInvokeEvents, so that if such a
repalloc occurs, afterTriggerInvokeEvents is already working with an
obsolete dangling pointer while it scans the rest of the events.  Oops.
The only code at risk is its "delete_ok" cleanup code, so we can
prevent unsafe behavior by passing delete_ok = false instead of true.

However, that could have a significant performance penalty, because the
point of passing delete_ok = true is to not have to re-scan possibly
a large number of dead trigger events on the next time through the loop.
There's more than one way to skin that cat, though.  What we can do is
delete all the "chunks" in the event list except the last one, since
we know all events in them must be dead.  Deleting the chunks is work
we'd have had to do later in AfterTriggerEndQuery anyway, and it ends
up saving rescanning of just about the same events we'd have gotten
rid of with delete_ok = true.

In v10 and HEAD, we also have to be careful to mop up any per-table
after_trig_events pointers that would become dangling.  This is slightly
annoying, but I don't think that normal use-cases will traverse this code
path often enough for it to be a performance problem.

It's pretty hard to hit this in practice because of the unlikelihood
of the query_stack getting resized at just the wrong time.  Nonetheless,
it's definitely a live bug of ancient standing, so back-patch to all
supported branches.

Discussion: https://postgr.es/m/2891.1505419542@sss.pgh.pa.us

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 7b411c130bbc679e6901618c4dda675c95490b4a..e75a59d29958ef99e323cbb9133d4bc19b1025e9 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3831,14 +3831,12 @@ static void
 afterTriggerFreeEventList(AfterTriggerEventList *events)
 {
        AfterTriggerEventChunk *chunk;
-       AfterTriggerEventChunk *next_chunk;
 
-       for (chunk = events->head; chunk != NULL; chunk = next_chunk)
+       while ((chunk = events->head) != NULL)
        {
-               next_chunk = chunk->next;
+               events->head = chunk->next;
                pfree(chunk);
        }
-       events->head = NULL;
        events->tail = NULL;
        events->tailfree = NULL;
 }
@@ -3882,6 +3880,45 @@ afterTriggerRestoreEventList(AfterTriggerEventList *events,
        }
 }
 
+/* ----------
+ * afterTriggerDeleteHeadEventChunk()
+ *
+ *     Remove the first chunk of events from the query level's event list.
+ *     Keep any event list pointers elsewhere in the query level's data
+ *     structures in sync.
+ * ----------
+ */
+static void
+afterTriggerDeleteHeadEventChunk(AfterTriggersQueryData *qs)
+{
+       AfterTriggerEventChunk *target = qs->events.head;
+       ListCell   *lc;
+
+       Assert(target && target->next);
+
+       /*
+        * First, update any pointers in the per-table data, so that they won't be
+        * dangling.  Resetting obsoleted pointers to NULL will make
+        * cancel_prior_stmt_triggers start from the list head, which is fine.
+        */
+       foreach(lc, qs->tables)
+       {
+               AfterTriggersTableData *table = (AfterTriggersTableData *) lfirst(lc);
+
+               if (table->after_trig_done &&
+                       table->after_trig_events.tail == target)
+               {
+                       table->after_trig_events.head = NULL;
+                       table->after_trig_events.tail = NULL;
+                       table->after_trig_events.tailfree = NULL;
+               }
+       }
+
+       /* Now we can flush the head chunk */
+       qs->events.head = target->next;
+       pfree(target);
+}
+
 
 /* ----------
  * AfterTriggerExecute()
@@ -4274,7 +4311,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
                        /*
                         * If it's last chunk, must sync event list's tailfree too.  Note
                         * that delete_ok must NOT be passed as true if there could be
-                        * stacked AfterTriggerEventList values pointing at this event
+                        * additional AfterTriggerEventList values pointing at this event
                         * list, since we'd fail to fix their copies of tailfree.
                         */
                        if (chunk == events->tail)
@@ -4522,6 +4559,8 @@ AfterTriggerBeginQuery(void)
 void
 AfterTriggerEndQuery(EState *estate)
 {
+       AfterTriggersQueryData *qs;
+
        /* Must be inside a query, too */
        Assert(afterTriggers.query_depth >= 0);
 
@@ -4555,23 +4594,40 @@ AfterTriggerEndQuery(EState *estate)
         * If we find no firable events, we don't have to increment
         * firing_counter.
         */
+       qs = &afterTriggers.query_stack[afterTriggers.query_depth];
+
        for (;;)
        {
-               AfterTriggersQueryData *qs;
-
-               /*
-                * Firing a trigger could result in query_stack being repalloc'd, so
-                * we must recalculate qs after each afterTriggerInvokeEvents call.
-                */
-               qs = &afterTriggers.query_stack[afterTriggers.query_depth];
-
                if (afterTriggerMarkEvents(&qs->events, &afterTriggers.events, true))
                {
                        CommandId       firing_id = afterTriggers.firing_counter++;
+                       AfterTriggerEventChunk *oldtail = qs->events.tail;
 
-                       /* OK to delete the immediate events after processing them */
-                       if (afterTriggerInvokeEvents(&qs->events, firing_id, estate, true))
+                       if (afterTriggerInvokeEvents(&qs->events, firing_id, estate, false))
                                break;                  /* all fired */
+
+                       /*
+                        * Firing a trigger could result in query_stack being repalloc'd,
+                        * so we must recalculate qs after each afterTriggerInvokeEvents
+                        * call.  Furthermore, it's unsafe to pass delete_ok = true here,
+                        * because that could cause afterTriggerInvokeEvents to try to
+                        * access qs->events after the stack has been repalloc'd.
+                        */
+                       qs = &afterTriggers.query_stack[afterTriggers.query_depth];
+
+                       /*
+                        * We'll need to scan the events list again.  To reduce the cost
+                        * of doing so, get rid of completely-fired chunks.  We know that
+                        * all events were marked IN_PROGRESS or DONE at the conclusion of
+                        * afterTriggerMarkEvents, so any still-interesting events must
+                        * have been added after that, and so must be in the chunk that
+                        * was then the tail chunk, or in later chunks.  So, zap all
+                        * chunks before oldtail.  This is approximately the same set of
+                        * events we would have gotten rid of by passing delete_ok = true.
+                        */
+                       Assert(oldtail != NULL);
+                       while (qs->events.head != oldtail)
+                               afterTriggerDeleteHeadEventChunk(qs);
                }
                else
                        break;