Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit27c6619

Browse files
committed
Fix possible dangling pointer dereference in trigger.c.
AfterTriggerEndQuery correctly notes that the query_stack could getrepalloc'd during a trigger firing, but it nonetheless passes the addressof a query_stack entry to afterTriggerInvokeEvents, so that if such arepalloc occurs, afterTriggerInvokeEvents is already working with anobsolete dangling pointer while it scans the rest of the events. Oops.The only code at risk is its "delete_ok" cleanup code, so we canprevent unsafe behavior by passing delete_ok = false instead of true.However, that could have a significant performance penalty, because thepoint of passing delete_ok = true is to not have to re-scan possiblya 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 isdelete all the "chunks" in the event list except the last one, sincewe know all events in them must be dead. Deleting the chunks is workwe'd have had to do later in AfterTriggerEndQuery anyway, and it endsup saving rescanning of just about the same events we'd have gottenrid of with delete_ok = true.In v10 and HEAD, we also have to be careful to mop up any per-tableafter_trig_events pointers that would become dangling. This is slightlyannoying, but I don't think that normal use-cases will traverse this codepath often enough for it to be a performance problem.It's pretty hard to hit this in practice because of the unlikelihoodof the query_stack getting resized at just the wrong time. Nonetheless,it's definitely a live bug of ancient standing, so back-patch to allsupported branches.Discussion:https://postgr.es/m/2891.1505419542@sss.pgh.pa.us
1 parentfd31f9f commit27c6619

File tree

1 file changed

+71
-15
lines changed

1 file changed

+71
-15
lines changed

‎src/backend/commands/trigger.c

Lines changed: 71 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3831,14 +3831,12 @@ static void
38313831
afterTriggerFreeEventList(AfterTriggerEventList*events)
38323832
{
38333833
AfterTriggerEventChunk*chunk;
3834-
AfterTriggerEventChunk*next_chunk;
38353834

3836-
for (chunk=events->head;chunk!=NULL;chunk=next_chunk)
3835+
while ((chunk=events->head)!=NULL)
38373836
{
3838-
next_chunk=chunk->next;
3837+
events->head=chunk->next;
38393838
pfree(chunk);
38403839
}
3841-
events->head=NULL;
38423840
events->tail=NULL;
38433841
events->tailfree=NULL;
38443842
}
@@ -3882,6 +3880,45 @@ afterTriggerRestoreEventList(AfterTriggerEventList *events,
38823880
}
38833881
}
38843882

3883+
/* ----------
3884+
* afterTriggerDeleteHeadEventChunk()
3885+
*
3886+
*Remove the first chunk of events from the query level's event list.
3887+
*Keep any event list pointers elsewhere in the query level's data
3888+
*structures in sync.
3889+
* ----------
3890+
*/
3891+
staticvoid
3892+
afterTriggerDeleteHeadEventChunk(AfterTriggersQueryData*qs)
3893+
{
3894+
AfterTriggerEventChunk*target=qs->events.head;
3895+
ListCell*lc;
3896+
3897+
Assert(target&&target->next);
3898+
3899+
/*
3900+
* First, update any pointers in the per-table data, so that they won't be
3901+
* dangling. Resetting obsoleted pointers to NULL will make
3902+
* cancel_prior_stmt_triggers start from the list head, which is fine.
3903+
*/
3904+
foreach(lc,qs->tables)
3905+
{
3906+
AfterTriggersTableData*table= (AfterTriggersTableData*)lfirst(lc);
3907+
3908+
if (table->after_trig_done&&
3909+
table->after_trig_events.tail==target)
3910+
{
3911+
table->after_trig_events.head=NULL;
3912+
table->after_trig_events.tail=NULL;
3913+
table->after_trig_events.tailfree=NULL;
3914+
}
3915+
}
3916+
3917+
/* Now we can flush the head chunk */
3918+
qs->events.head=target->next;
3919+
pfree(target);
3920+
}
3921+
38853922

38863923
/* ----------
38873924
* AfterTriggerExecute()
@@ -4274,7 +4311,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
42744311
/*
42754312
* If it's last chunk, must sync event list's tailfree too. Note
42764313
* that delete_ok must NOT be passed as true if there could be
4277-
*stacked AfterTriggerEventList values pointing at this event
4314+
*additional AfterTriggerEventList values pointing at this event
42784315
* list, since we'd fail to fix their copies of tailfree.
42794316
*/
42804317
if (chunk==events->tail)
@@ -4522,6 +4559,8 @@ AfterTriggerBeginQuery(void)
45224559
void
45234560
AfterTriggerEndQuery(EState*estate)
45244561
{
4562+
AfterTriggersQueryData*qs;
4563+
45254564
/* Must be inside a query, too */
45264565
Assert(afterTriggers.query_depth >=0);
45274566

@@ -4555,23 +4594,40 @@ AfterTriggerEndQuery(EState *estate)
45554594
* If we find no firable events, we don't have to increment
45564595
* firing_counter.
45574596
*/
4597+
qs=&afterTriggers.query_stack[afterTriggers.query_depth];
4598+
45584599
for (;;)
45594600
{
4560-
AfterTriggersQueryData*qs;
4561-
4562-
/*
4563-
* Firing a trigger could result in query_stack being repalloc'd, so
4564-
* we must recalculate qs after each afterTriggerInvokeEvents call.
4565-
*/
4566-
qs=&afterTriggers.query_stack[afterTriggers.query_depth];
4567-
45684601
if (afterTriggerMarkEvents(&qs->events,&afterTriggers.events, true))
45694602
{
45704603
CommandIdfiring_id=afterTriggers.firing_counter++;
4604+
AfterTriggerEventChunk*oldtail=qs->events.tail;
45714605

4572-
/* OK to delete the immediate events after processing them */
4573-
if (afterTriggerInvokeEvents(&qs->events,firing_id,estate, true))
4606+
if (afterTriggerInvokeEvents(&qs->events,firing_id,estate, false))
45744607
break;/* all fired */
4608+
4609+
/*
4610+
* Firing a trigger could result in query_stack being repalloc'd,
4611+
* so we must recalculate qs after each afterTriggerInvokeEvents
4612+
* call. Furthermore, it's unsafe to pass delete_ok = true here,
4613+
* because that could cause afterTriggerInvokeEvents to try to
4614+
* access qs->events after the stack has been repalloc'd.
4615+
*/
4616+
qs=&afterTriggers.query_stack[afterTriggers.query_depth];
4617+
4618+
/*
4619+
* We'll need to scan the events list again. To reduce the cost
4620+
* of doing so, get rid of completely-fired chunks. We know that
4621+
* all events were marked IN_PROGRESS or DONE at the conclusion of
4622+
* afterTriggerMarkEvents, so any still-interesting events must
4623+
* have been added after that, and so must be in the chunk that
4624+
* was then the tail chunk, or in later chunks. So, zap all
4625+
* chunks before oldtail. This is approximately the same set of
4626+
* events we would have gotten rid of by passing delete_ok = true.
4627+
*/
4628+
Assert(oldtail!=NULL);
4629+
while (qs->events.head!=oldtail)
4630+
afterTriggerDeleteHeadEventChunk(qs);
45754631
}
45764632
else
45774633
break;

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp