From 88e6ad3054ddd5aa0dee12e5def2c335fe92a414 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Fri, 19 Apr 2019 11:33:37 -0700 Subject: [PATCH] Fix two memory leaks around force-storing tuples in slots. As reported by Tom, when ExecStoreMinimalTuple() had to perform a conversion to store the minimal tuple in the slot, it forgot to respect the shouldFree flag, and leaked the tuple into the current memory context if true. Fix that by freeing the tuple in that case. Looking at the relevant code made me (Andres) realize that not having the shouldFree parameter to ExecForceStoreHeapTuple() was a bad idea. Some callers had to locally implement the necessary logic, and in one case it was missing, creating a potential per-group leak in non-hashed aggregation. The choice to not free the tuple in ExecComputeStoredGenerated() is not pretty, but not introduced by this commit - I'll start a separate discussion about it. Reported-By: Tom Lane Discussion: https://postgr.es/m/366.1555382816@sss.pgh.pa.us --- contrib/postgres_fdw/postgres_fdw.c | 4 +--- src/backend/commands/trigger.c | 20 ++++++++++---------- src/backend/executor/execTuples.c | 20 ++++++++++++++++++-- src/backend/executor/nodeAgg.c | 2 +- src/backend/executor/nodeIndexonlyscan.c | 2 +- src/backend/executor/nodeModifyTable.c | 9 +++++++-- src/include/executor/tuptable.h | 4 +++- 7 files changed, 41 insertions(+), 20 deletions(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index db62caf6d9..25e219dd82 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -3711,9 +3711,7 @@ store_returning_result(PgFdwModifyState *fmstate, * The returning slot will not necessarily be suitable to store * heaptuples directly, so allow for conversion. */ - ExecForceStoreHeapTuple(newtup, slot); - ExecMaterializeSlot(slot); - pfree(newtup); + ExecForceStoreHeapTuple(newtup, slot, true); } PG_CATCH(); { diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index c81ccc8fcf..2beb378145 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -2586,7 +2586,7 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo, } else if (newtuple != oldtuple) { - ExecForceStoreHeapTuple(newtuple, slot); + ExecForceStoreHeapTuple(newtuple, slot, false); if (should_free) heap_freetuple(oldtuple); @@ -2668,7 +2668,7 @@ ExecIRInsertTriggers(EState *estate, ResultRelInfo *relinfo, } else if (newtuple != oldtuple) { - ExecForceStoreHeapTuple(newtuple, slot); + ExecForceStoreHeapTuple(newtuple, slot, false); if (should_free) heap_freetuple(oldtuple); @@ -2797,7 +2797,7 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate, else { trigtuple = fdw_trigtuple; - ExecForceStoreHeapTuple(trigtuple, slot); + ExecForceStoreHeapTuple(trigtuple, slot, false); } LocTriggerData.type = T_TriggerData; @@ -2869,7 +2869,7 @@ ExecARDeleteTriggers(EState *estate, ResultRelInfo *relinfo, slot, NULL); else - ExecForceStoreHeapTuple(fdw_trigtuple, slot); + ExecForceStoreHeapTuple(fdw_trigtuple, slot, false); AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_DELETE, true, slot, NULL, NIL, NULL, @@ -2898,7 +2898,7 @@ ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo, LocTriggerData.tg_oldtable = NULL; LocTriggerData.tg_newtable = NULL; - ExecForceStoreHeapTuple(trigtuple, slot); + ExecForceStoreHeapTuple(trigtuple, slot, false); for (i = 0; i < trigdesc->numtriggers; i++) { @@ -3057,7 +3057,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, } else { - ExecForceStoreHeapTuple(fdw_trigtuple, oldslot); + ExecForceStoreHeapTuple(fdw_trigtuple, oldslot, false); trigtuple = fdw_trigtuple; } @@ -3107,7 +3107,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, } else if (newtuple != oldtuple) { - ExecForceStoreHeapTuple(newtuple, newslot); + ExecForceStoreHeapTuple(newtuple, newslot, false); /* * If the tuple returned by the trigger / being stored, is the old @@ -3164,7 +3164,7 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo, oldslot, NULL); else if (fdw_trigtuple != NULL) - ExecForceStoreHeapTuple(fdw_trigtuple, oldslot); + ExecForceStoreHeapTuple(fdw_trigtuple, oldslot, false); AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE, true, oldslot, newslot, recheckIndexes, @@ -3192,7 +3192,7 @@ ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo, LocTriggerData.tg_oldtable = NULL; LocTriggerData.tg_newtable = NULL; - ExecForceStoreHeapTuple(trigtuple, oldslot); + ExecForceStoreHeapTuple(trigtuple, oldslot, false); for (i = 0; i < trigdesc->numtriggers; i++) { @@ -3228,7 +3228,7 @@ ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo, } else if (newtuple != oldtuple) { - ExecForceStoreHeapTuple(newtuple, newslot); + ExecForceStoreHeapTuple(newtuple, newslot, false); if (should_free) heap_freetuple(oldtuple); diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c index 546db02cad..55d1669db0 100644 --- a/src/backend/executor/execTuples.c +++ b/src/backend/executor/execTuples.c @@ -1430,11 +1430,12 @@ ExecStoreMinimalTuple(MinimalTuple mtup, */ void ExecForceStoreHeapTuple(HeapTuple tuple, - TupleTableSlot *slot) + TupleTableSlot *slot, + bool shouldFree) { if (TTS_IS_HEAPTUPLE(slot)) { - ExecStoreHeapTuple(tuple, slot, false); + ExecStoreHeapTuple(tuple, slot, shouldFree); } else if (TTS_IS_BUFFERTUPLE(slot)) { @@ -1447,6 +1448,9 @@ ExecForceStoreHeapTuple(HeapTuple tuple, oldContext = MemoryContextSwitchTo(slot->tts_mcxt); bslot->base.tuple = heap_copytuple(tuple); MemoryContextSwitchTo(oldContext); + + if (shouldFree) + pfree(tuple); } else { @@ -1454,6 +1458,12 @@ ExecForceStoreHeapTuple(HeapTuple tuple, heap_deform_tuple(tuple, slot->tts_tupleDescriptor, slot->tts_values, slot->tts_isnull); ExecStoreVirtualTuple(slot); + + if (shouldFree) + { + ExecMaterializeSlot(slot); + pfree(tuple); + } } } @@ -1481,6 +1491,12 @@ ExecForceStoreMinimalTuple(MinimalTuple mtup, heap_deform_tuple(&htup, slot->tts_tupleDescriptor, slot->tts_values, slot->tts_isnull); ExecStoreVirtualTuple(slot); + + if (shouldFree) + { + ExecMaterializeSlot(slot); + pfree(mtup); + } } } diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index 47161afbd4..d01fc4f52e 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -1806,7 +1806,7 @@ agg_retrieve_direct(AggState *aggstate) * cleared from the slot. */ ExecForceStoreHeapTuple(aggstate->grp_firstTuple, - firstSlot); + firstSlot, true); aggstate->grp_firstTuple = NULL; /* don't keep two pointers */ /* set up for first advance_aggregates call */ diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c index 7711728495..8fd52e9c80 100644 --- a/src/backend/executor/nodeIndexonlyscan.c +++ b/src/backend/executor/nodeIndexonlyscan.c @@ -205,7 +205,7 @@ IndexOnlyNext(IndexOnlyScanState *node) */ Assert(slot->tts_tupleDescriptor->natts == scandesc->xs_hitupdesc->natts); - ExecForceStoreHeapTuple(scandesc->xs_hitup, slot); + ExecForceStoreHeapTuple(scandesc->xs_hitup, slot, false); } else if (scandesc->xs_itup) StoreIndexTuple(slot, scandesc->xs_itup, scandesc->xs_itupdesc); diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 8c0a2c4bac..444c0c0574 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -317,7 +317,12 @@ ExecComputeStoredGenerated(EState *estate, TupleTableSlot *slot) oldtuple = ExecFetchSlotHeapTuple(slot, true, &should_free); newtuple = heap_modify_tuple(oldtuple, tupdesc, values, nulls, replaces); - ExecForceStoreHeapTuple(newtuple, slot); + /* + * The tuple will be freed by way of the memory context - the slot might + * only be cleared after the context is reset, and we'd thus potentially + * double free. + */ + ExecForceStoreHeapTuple(newtuple, slot, false); if (should_free) heap_freetuple(oldtuple); @@ -979,7 +984,7 @@ ldelete:; slot = ExecGetReturningSlot(estate, resultRelInfo); if (oldtuple != NULL) { - ExecForceStoreHeapTuple(oldtuple, slot); + ExecForceStoreHeapTuple(oldtuple, slot, false); } else { diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h index b0561ebe29..63c2cd14f0 100644 --- a/src/include/executor/tuptable.h +++ b/src/include/executor/tuptable.h @@ -306,7 +306,9 @@ extern void ExecSetSlotDescriptor(TupleTableSlot *slot, TupleDesc tupdesc); extern TupleTableSlot *ExecStoreHeapTuple(HeapTuple tuple, TupleTableSlot *slot, bool shouldFree); -extern void ExecForceStoreHeapTuple(HeapTuple tuple, TupleTableSlot *slot); +extern void ExecForceStoreHeapTuple(HeapTuple tuple, + TupleTableSlot *slot, + bool shouldFree); extern TupleTableSlot *ExecStoreBufferHeapTuple(HeapTuple tuple, TupleTableSlot *slot, Buffer buffer);