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

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index db62caf6d9f22ba14bf03b8f53a6e878fd2087de..25e219dd82c56b9f5616f34ea3dc7148feefa498 100644 (file)
--- 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 c81ccc8fcffc9108594aa40530107595cecb606f..2beb37814507d1da399144eecefa29c51cf3cafa 100644 (file)
--- 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 546db02cad0936aa85f984cab69d5a9c61b593e9..55d1669db099ac8b3b4ffa6d5a3c70b84ea6fd94 100644 (file)
--- 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 47161afbd42da1752cf32761c76be944ee52baec..d01fc4f52e1e3171e6f61bf9a931f5879399ffc0 100644 (file)
--- 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 7711728495c0bf71c1e2d64be3ea7041a3899ff2..8fd52e9c8038e52d5c50e308a43d02a442850780 100644 (file)
--- 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 8c0a2c4bac538fdb1b25f93ad18fc71f08336cb1..444c0c057463b2cda33f4bd999add7b4ebca41b6 100644 (file)
--- 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 b0561ebe2917b3743fc9589bea8a49f0a1c93a18..63c2cd14f0112b4a9189a948d28ffa74023575b9 100644 (file)
--- 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);