From a210be772047575331fb6b0ab7b72043f81452ba Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon, 21 Feb 2011 21:18:04 -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/copy.c | 29 +++--
src/backend/commands/trigger.c | 155 +++++++++++++++++++------
src/backend/executor/nodeModifyTable.c | 103 +++-------------
src/include/commands/trigger.h | 18 +--
4 files changed, 161 insertions(+), 144 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index cac11a6c641..44f568f3964 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 8d996a87c77..dc6ee9c266f 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 42662bdc461..12a5b2a8953 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 c213ac7a4ef..80a779ed0bd 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,
--
GitLab