8000 Fix possible dangling pointer dereference in trigger.c. · MSPawanRanjith/postgres@4cd6cd2 · GitHub
[go: up one dir, main page]
Skip to content

Commit 4cd6cd2

Browse files
tglsfdctglsfdc
committed
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
1 parent a64a0bf commit 4cd6cd2

File tree

1 file changed

+53
-12
lines changed

1 file changed

+53
-12
lines changed

‎src/backend/commands/trigger.c

Lines changed: 53 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3263,14 +3263,12 @@ static void
32633263
afterTriggerFreeEventList(AfterTriggerEventList *events)
32643264
{
32653265
AfterTriggerEventChunk *chunk;
3266-
AfterTriggerEventChunk *next_chunk;
32673266

3268-
for (chunk = events->head; chunk != NULL; chunk = next_chunk)
3267+
while ((chunk = events->head) != NULL)
32693268
{
3270-
next_chunk = chunk->next;
3269+
events->head = chunk->next;
32713270
pfree(chunk);
32723271
}
3273-
events->head = NULL;
32743272
events->tail = NULL;
32753273
events->tailfree = NULL;
32763274
}
@@ -3314,6 +3312,23 @@ afterTriggerRestoreEventList(AfterTriggerEventList *events,
33143312
}
33153313
}
33163314

3315+
/* ----------
3316+
* afterTriggerDeleteHeadEventChunk()
3317+
*
3318+
* Remove the first chunk of events from the given event list.
3319+
* ----------
3320+
*/
3321+
static void
3322+
afterTriggerDeleteHeadEventChunk(AfterTriggerEventList *events)
3323+
{
3324+
AfterTriggerEventChunk *target = events->head;
3325+
3326+
Assert(target && target->next);
3327+
3328+
events->head = target->next;
3329+
pfree(target);
3330+
}
3331+
33173332

33183333
/* ----------
33193334
* AfterTriggerExecute()
@@ -3624,7 +3639,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
36243639
/*
36253640
* If it's last chunk, must sync event list's tailfree too. Note
36263641
* that delete_ok must NOT be passed as true if there could be
3627-
* stacked AfterTriggerEventList values pointing at this event
3642+
* additional AfterTriggerEventList values pointing at this event
36283643
* list, since we'd fail to fix their copies of tailfree.
36293644
*/
36303645
if (chunk == events->tail)
@@ -3773,24 +3788,50 @@ AfterTriggerEndQuery(EState *estate)
37733788
* IMMEDIATE: all events we have decided to defer will be available for it
37743789
* to fire.
37753790
*
3776-
* We loop in case a trigger queues more events at the same query level
3777-
* (is that even possible?). Be careful here: firing a trigger could
3778-
* result in query_stack being repalloc'd, so we can't save its address
3779-
* across afterTriggerInvokeEvents calls.
3791+
* We loop in case a trigger queues more events at the same query level.
3792+
* Ordinary trigger functions, including all PL/pgSQL trigger functions,
3793+
* will instead fire any triggers in a dedicated query level. Foreign key
3794+
* enforcement triggers do add to the current query level, thanks to their
3795+
* passing fire_triggers = false to SPI_execute_snapshot(). Other
3796+
* C-language triggers might do likewise.
37803797
*
37813798
* If we find no firable events, we don't have to increment
37823799
* firing_counter.
37833800
*/
3801+
events = &afterTriggers->query_stack[afterTriggers->query_depth];
3802+
37843803
for (;;)
37853804
{
3786-
events = &afterTriggers->query_stack[afterTriggers->query_depth];
37873805
if (afterTriggerMarkEvents(events, &afterTriggers->events, true))
37883806
{
37893807
CommandId firing_id = afterTriggers->firing_counter++;
3808+
AfterTriggerEventChunk *oldtail = events->tail;
37903809

3791-
/* OK to delete the immediate events after processing them */
3792-
if (afterTriggerInvokeEvents(events, firing_id, estate, true))
3810+
if (afterTriggerInvokeEvents(events, firing_id, estate, false))
37933811
break; /* all fired */
3812+
3813+
/*
3814+
* Firing a trigger could result in query_stack being repalloc'd,
3815+
* so we must recalculate ptr after each afterTriggerInvokeEvents
3816+
* call. Furthermore, it's unsafe to pass delete_ok = true here,
3817+
* because that could cause afterTriggerInvokeEvents to try to
3818+
* access *events after the stack has been repalloc'd.
3819+
*/
3820+
events = &afterTriggers->query_stack[afterTriggers->query_depth];
3821+
3822+
/*
3823+
* We'll need to scan the events list again. To reduce the cost
3824+
* of doing so, get rid of completely-fired chunks. We know that
3825+
* all events were marked IN_PROGRESS or DONE at the conclusion of
3826+
* afterTriggerMarkEvents, so any still-interesting events must
3827+
* have been added after that, and so must be in the chunk that
3828+
* was then the tail chunk, or in later chunks. So, zap all
3829+
* chunks before oldtail. This is approximately the same set of
3830+
* events we would have gotten rid of by passing delete_ok = true.
3831+
*/
3832+
Assert(oldtail != NULL);
3833+
while (events->head != oldtail)
3834+
afterTriggerDeleteHeadEventChunk(events);
37943835
}
37953836
else
37963837
break;

0 commit comments

Comments
 (0)
0