From: Tom Lane Date: Tue, 22 Feb 2011 02:18:04 +0000 (-0500) Subject: Fix dangling-pointer problem in before-row update trigger processing. X-Git-Tag: REL9_1_ALPHA4~118 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a210be772047575331fb6b0ab7b72043f81452ba;p=postgresql 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. --- diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index cac11a6c64..44f568f396 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -1836,7 +1836,7 @@ CopyFrom(CopyState cstate) ResultRelInfo *resultRelInfo; EState *estate = CreateExecutorState(); /* for ExecConstraints() */ ExprContext *econtext; - TupleTableSlot *slot; + TupleTableSlot *myslot; MemoryContext oldcontext = CurrentMemoryContext; ErrorContextCallback errcontext; CommandId mycid = GetCurrentCommandId(true); @@ -1932,8 +1932,10 @@ CopyFrom(CopyState cstate) estate->es_result_relation_info = resultRelInfo; /* Set up a tuple slot too */ - slot = ExecInitExtraTupleSlot(estate); - ExecSetSlotDescriptor(slot, tupDesc); + myslot = ExecInitExtraTupleSlot(estate); + ExecSetSlotDescriptor(myslot, tupDesc); + /* Triggers might need a slot as well */ + estate->es_trig_tuple_slot = ExecInitExtraTupleSlot(estate); /* Prepare to catch AFTER triggers. */ AfterTriggerBeginQuery(); @@ -1960,6 +1962,7 @@ CopyFrom(CopyState cstate) for (;;) { + TupleTableSlot *slot; bool skip_tuple; Oid loaded_oid = InvalidOid; @@ -1983,32 +1986,28 @@ CopyFrom(CopyState cstate) /* Triggers and stuff need to be invoked in query context. */ MemoryContextSwitchTo(oldcontext); + /* Place tuple in tuple slot --- but slot shouldn't free it */ + slot = myslot; + ExecStoreTuple(tuple, slot, InvalidBuffer, false); + skip_tuple = false; /* BEFORE ROW INSERT Triggers */ if (resultRelInfo->ri_TrigDesc && resultRelInfo->ri_TrigDesc->trig_insert_before_row) { - HeapTuple newtuple; - - newtuple = ExecBRInsertTriggers(estate, resultRelInfo, tuple); + slot = ExecBRInsertTriggers(estate, resultRelInfo, slot); - if (newtuple == NULL) /* "do nothing" */ + if (slot == NULL) /* "do nothing" */ skip_tuple = true; - else if (newtuple != tuple) /* modified by Trigger(s) */ - { - heap_freetuple(tuple); - tuple = newtuple; - } + else /* trigger might have changed tuple */ + tuple = ExecMaterializeSlot(slot); } if (!skip_tuple) { List *recheckIndexes = NIL; - /* Place tuple in tuple slot */ - ExecStoreTuple(tuple, slot, InvalidBuffer, false); - /* Check the constraints of the tuple */ if (cstate->rel->rd_att->constr) ExecConstraints(resultRelInfo, slot, estate); diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 8d996a87c7..dc6ee9c266 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -1909,12 +1909,13 @@ ExecASInsertTriggers(EState *estate, ResultRelInfo *relinfo) false, NULL, NULL, NIL, NULL); } -HeapTuple +TupleTableSlot * ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo, - HeapTuple trigtuple) + TupleTableSlot *slot) { TriggerDesc *trigdesc = relinfo->ri_TrigDesc; - HeapTuple newtuple = trigtuple; + HeapTuple slottuple = ExecMaterializeSlot(slot); + HeapTuple newtuple = slottuple; HeapTuple oldtuple; TriggerData LocTriggerData; int i; @@ -1947,12 +1948,29 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo, relinfo->ri_TrigFunctions, relinfo->ri_TrigInstrument, GetPerTupleMemoryContext(estate)); - if (oldtuple != newtuple && oldtuple != trigtuple) + if (oldtuple != newtuple && oldtuple != slottuple) heap_freetuple(oldtuple); if (newtuple == NULL) - break; + return NULL; /* "do nothing" */ + } + + 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 newtuple; + return slot; } void @@ -1966,12 +1984,13 @@ ExecARInsertTriggers(EState *estate, ResultRelInfo *relinfo, true, NULL, trigtuple, recheckIndexes, NULL); } -HeapTuple +TupleTableSlot * ExecIRInsertTriggers(EState *estate, ResultRelInfo *relinfo, - HeapTuple trigtuple) + TupleTableSlot *slot) { TriggerDesc *trigdesc = relinfo->ri_TrigDesc; - HeapTuple newtuple = trigtuple; + HeapTuple slottuple = ExecMaterializeSlot(slot); + HeapTuple newtuple = slottuple; HeapTuple oldtuple; TriggerData LocTriggerData; int i; @@ -2004,12 +2023,29 @@ ExecIRInsertTriggers(EState *estate, ResultRelInfo *relinfo, relinfo->ri_TrigFunctions, relinfo->ri_TrigInstrument, GetPerTupleMemoryContext(estate)); - if (oldtuple != newtuple && oldtuple != trigtuple) + if (oldtuple != newtuple && oldtuple != slottuple) heap_freetuple(oldtuple); if (newtuple == NULL) - break; + return NULL; /* "do nothing" */ + } + + 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 newtuple; + return slot; } void @@ -2257,32 +2293,44 @@ 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; + HeapTuple slottuple = ExecMaterializeSlot(slot); + HeapTuple newtuple = slottuple; TriggerData LocTriggerData; HeapTuple trigtuple; HeapTuple oldtuple; - HeapTuple intuple = newtuple; TupleTableSlot *newSlot; int i; Bitmapset *modifiedCols; + /* get a copy of the on-disk tuple we are planning to update */ trigtuple = GetTupleForTrigger(estate, epqstate, relinfo, tupleid, &newSlot); if (trigtuple == NULL) - return NULL; + return NULL; /* cancel the update action */ /* - * 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); @@ -2314,13 +2362,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 @@ -2342,14 +2410,15 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo, } } -HeapTuple +TupleTableSlot * ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo, - HeapTuple oldtuple, HeapTuple newtuple) + HeapTuple trigtuple, TupleTableSlot *slot) { TriggerDesc *trigdesc = relinfo->ri_TrigDesc; + HeapTuple slottuple = ExecMaterializeSlot(slot); + HeapTuple newtuple = slottuple; TriggerData LocTriggerData; - HeapTuple intuple = newtuple; - HeapTuple rettuple; + HeapTuple oldtuple; int i; LocTriggerData.type = T_TriggerData; @@ -2367,26 +2436,42 @@ ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo, TRIGGER_TYPE_UPDATE)) continue; if (!TriggerEnabled(estate, relinfo, trigger, LocTriggerData.tg_event, - NULL, oldtuple, newtuple)) + NULL, trigtuple, newtuple)) continue; - LocTriggerData.tg_trigtuple = oldtuple; - LocTriggerData.tg_newtuple = newtuple; + LocTriggerData.tg_trigtuple = trigtuple; + LocTriggerData.tg_newtuple = oldtuple = newtuple; LocTriggerData.tg_trigtuplebuf = InvalidBuffer; LocTriggerData.tg_newtuplebuf = InvalidBuffer; LocTriggerData.tg_trigger = trigger; - rettuple = ExecCallTriggerFunc(&LocTriggerData, + newtuple = ExecCallTriggerFunc(&LocTriggerData, i, relinfo->ri_TrigFunctions, relinfo->ri_TrigInstrument, GetPerTupleMemoryContext(estate)); - if (newtuple != rettuple && newtuple != intuple) - heap_freetuple(newtuple); - newtuple = rettuple; + if (oldtuple != newtuple && oldtuple != slottuple) + heap_freetuple(oldtuple); if (newtuple == NULL) - break; + return NULL; /* "do nothing" */ + } + + 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 newtuple; + return slot; } void diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 42662bdc46..12a5b2a895 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -199,60 +199,26 @@ ExecInsert(TupleTableSlot *slot, if (resultRelInfo->ri_TrigDesc && resultRelInfo->ri_TrigDesc->trig_insert_before_row) { - HeapTuple newtuple; + slot = ExecBRInsertTriggers(estate, resultRelInfo, slot); - newtuple = ExecBRInsertTriggers(estate, resultRelInfo, tuple); - - 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); } /* INSTEAD OF ROW INSERT Triggers */ if (resultRelInfo->ri_TrigDesc && resultRelInfo->ri_TrigDesc->trig_insert_instead_row) { - HeapTuple newtuple; + slot = ExecIRInsertTriggers(estate, resultRelInfo, slot); - newtuple = ExecIRInsertTriggers(estate, resultRelInfo, tuple); - - 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); newId = InvalidOid; } @@ -533,31 +499,14 @@ ExecUpdate(ItemPointer tupleid, if (resultRelInfo->ri_TrigDesc && resultRelInfo->ri_TrigDesc->trig_update_before_row) { - HeapTuple newtuple; + slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo, + tupleid, slot); - newtuple = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo, - tupleid, tuple); - - 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); } /* INSTEAD OF ROW UPDATE Triggers */ @@ -565,7 +514,6 @@ ExecUpdate(ItemPointer tupleid, resultRelInfo->ri_TrigDesc->trig_update_instead_row) { HeapTupleData oldtup; - HeapTuple newtuple; Assert(oldtuple != NULL); oldtup.t_data = oldtuple; @@ -573,29 +521,14 @@ ExecUpdate(ItemPointer tupleid, ItemPointerSetInvalid(&(oldtup.t_self)); oldtup.t_tableOid = InvalidOid; - newtuple = ExecIRUpdateTriggers(estate, resultRelInfo, - &oldtup, tuple); + slot = ExecIRUpdateTriggers(estate, resultRelInfo, + &oldtup, 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); } else { diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index c213ac7a4e..80a779ed0b 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -132,16 +132,16 @@ extern void ExecBSInsertTriggers(EState *estate, ResultRelInfo *relinfo); extern void ExecASInsertTriggers(EState *estate, ResultRelInfo *relinfo); -extern HeapTuple ExecBRInsertTriggers(EState *estate, +extern TupleTableSlot *ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo, - HeapTuple trigtuple); + TupleTableSlot *slot); extern void ExecARInsertTriggers(EState *estate, ResultRelInfo *relinfo, HeapTuple trigtuple, List *recheckIndexes); -extern HeapTuple ExecIRInsertTriggers(EState *estate, +extern TupleTableSlot *ExecIRInsertTriggers(EState *estate, ResultRelInfo *relinfo, - HeapTuple trigtuple); + TupleTableSlot *slot); extern void ExecBSDeleteTriggers(EState *estate, ResultRelInfo *relinfo); extern void ExecASDeleteTriggers(EState *estate, @@ -160,20 +160,20 @@ 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, HeapTuple newtuple, List *recheckIndexes); -extern HeapTuple ExecIRUpdateTriggers(EState *estate, +extern TupleTableSlot *ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo, - HeapTuple oldtuple, - HeapTuple newtuple); + HeapTuple trigtuple, + TupleTableSlot *slot); extern void ExecBSTruncateTriggers(EState *estate, ResultRelInfo *relinfo); extern void ExecASTruncateTriggers(EState *estate,