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

4 files changed, 27 insertions, 6 deletions

diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index 546db02cad0..55d1669db09 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 47161afbd42..d01fc4f52e1 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 7711728495c..8fd52e9c803 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 8c0a2c4bac5..444c0c05746 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
 			{