From 15907c36236d638437a1ed3efc0794fda2c9ad22 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 21 Feb 2011 21:18:13 -0500 Subject: [PATCH] Fix dangling-pointer problem in before-row update trigger processing. ExecUpdate checked for whether ExecBRUpdateTriggers had returned a new tuple value by seeing if the returned tuple was pointer-equal to the old one. But the "old one" was in estate->es_junkFilter's result slot, which would be scribbled on if we had done an EvalPlanQual update in response to a concurrent update of the target tuple; therefore we were comparing a dangling pointer to a live one. Given the right set of circumstances we could get a false match, resulting in not forcing the tuple to be stored in the slot we thought it was stored in. In the case reported by Maxim Boguk in bug #5798, this led to "cannot extract system attribute from virtual tuple" failures when trying to do "RETURNING ctid". I believe there is a very-low-probability chance of more serious errors, such as generating incorrect index entries based on the original rather than the trigger-modified version of the row. In HEAD, change all of ExecBRInsertTriggers, ExecIRInsertTriggers, ExecBRUpdateTriggers, and ExecIRUpdateTriggers so that they continue to have similar APIs. In the back branches I just changed ExecBRUpdateTriggers, since there is no bug in the ExecBRInsertTriggers case. --- src/backend/commands/trigger.c | 49 +++++++++++++++++++++----- src/backend/executor/nodeModifyTable.c | 27 +++----------- src/include/commands/trigger.h | 4 +-- 3 files changed, 47 insertions(+), 33 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 251c3e81a2..d67611b2ce 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -2172,18 +2172,19 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo) GetModifiedColumns(relinfo, estate)); } -HeapTuple +TupleTableSlot * ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, ResultRelInfo *relinfo, - ItemPointer tupleid, HeapTuple newtuple) + ItemPointer tupleid, TupleTableSlot *slot) { TriggerDesc *trigdesc = relinfo->ri_TrigDesc; int ntrigs = trigdesc->n_before_row[TRIGGER_EVENT_UPDATE]; int *tgindx = trigdesc->tg_before_row[TRIGGER_EVENT_UPDATE]; + HeapTuple slottuple = ExecMaterializeSlot(slot); + HeapTuple newtuple = slottuple; TriggerData LocTriggerData; HeapTuple trigtuple; HeapTuple oldtuple; - HeapTuple intuple = newtuple; TupleTableSlot *newSlot; int i; Bitmapset *modifiedCols; @@ -2194,12 +2195,22 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, return NULL; /* - * In READ COMMITTED isolation level it's possible that newtuple was + * In READ COMMITTED isolation level it's possible that target tuple was * changed due to concurrent update. In that case we have a raw subplan - * output tuple and need to run it through the junk filter. + * output tuple in newSlot, and need to run it through the junk filter to + * produce an insertable tuple. + * + * Caution: more than likely, the passed-in slot is the same as the + * junkfilter's output slot, so we are clobbering the original value of + * slottuple by doing the filtering. This is OK since neither we nor our + * caller have any more interest in the prior contents of that slot. */ if (newSlot != NULL) - intuple = newtuple = ExecRemoveJunk(relinfo->ri_junkFilter, newSlot); + { + slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot); + slottuple = ExecMaterializeSlot(slot); + newtuple = slottuple; + } modifiedCols = GetModifiedColumns(relinfo, estate); @@ -2226,13 +2237,33 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, relinfo->ri_TrigFunctions, relinfo->ri_TrigInstrument, GetPerTupleMemoryContext(estate)); - if (oldtuple != newtuple && oldtuple != intuple) + if (oldtuple != newtuple && oldtuple != slottuple) heap_freetuple(oldtuple); if (newtuple == NULL) - break; + { + heap_freetuple(trigtuple); + return NULL; /* "do nothing" */ + } } heap_freetuple(trigtuple); - return newtuple; + + if (newtuple != slottuple) + { + /* + * Return the modified tuple using the es_trig_tuple_slot. We assume + * the tuple was allocated in per-tuple memory context, and therefore + * will go away by itself. The tuple table slot should not try to + * clear it. + */ + TupleTableSlot *newslot = estate->es_trig_tuple_slot; + TupleDesc tupdesc = RelationGetDescr(relinfo->ri_RelationDesc); + + if (newslot->tts_tupleDescriptor != tupdesc) + ExecSetSlotDescriptor(newslot, tupdesc); + ExecStoreTuple(newtuple, newslot, InvalidBuffer, false); + slot = newslot; + } + return slot; } void diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index f4b2b16b69..ee6eef74f7 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -453,31 +453,14 @@ ExecUpdate(ItemPointer tupleid, if (resultRelInfo->ri_TrigDesc && resultRelInfo->ri_TrigDesc->n_before_row[TRIGGER_EVENT_UPDATE] > 0) { - HeapTuple newtuple; - - newtuple = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo, - tupleid, tuple); + slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo, + tupleid, slot); - if (newtuple == NULL) /* "do nothing" */ + if (slot == NULL) /* "do nothing" */ return NULL; - if (newtuple != tuple) /* modified by Trigger(s) */ - { - /* - * Put the modified tuple into a slot for convenience of routines - * below. We assume the tuple was allocated in per-tuple memory - * context, and therefore will go away by itself. The tuple table - * slot should not try to clear it. - */ - TupleTableSlot *newslot = estate->es_trig_tuple_slot; - TupleDesc tupdesc = RelationGetDescr(resultRelationDesc); - - if (newslot->tts_tupleDescriptor != tupdesc) - ExecSetSlotDescriptor(newslot, tupdesc); - ExecStoreTuple(newtuple, newslot, InvalidBuffer, false); - slot = newslot; - tuple = newtuple; - } + /* trigger might have changed tuple */ + tuple = ExecMaterializeSlot(slot); } /* diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index db79eb4be7..07ad698e7f 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -149,11 +149,11 @@ extern void ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo); extern void ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo); -extern HeapTuple ExecBRUpdateTriggers(EState *estate, +extern TupleTableSlot *ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, ResultRelInfo *relinfo, ItemPointer tupleid, - HeapTuple newtuple); + TupleTableSlot *slot); extern void ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo, ItemPointer tupleid, -- 2.40.0