PostgreSQL crashes with SIGSEGV

From ed4909ff9ee88b6b2f66034f043e544dfcfda3bd Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Sat, 16 Dec 2017 13:48:01 -0800
Subject: [PATCH] Prevent double-free in ExecEndPlan().

This fixes a crash that can occur on at least 9.5 and 9.6, where some
grouping sets queries may call tuplesort_end() at node shutdown time,
and free tuple memory that the executor imagines it is responsible for,
and must free within ExecClearTuple().  This fix simply prevents the
memory from being freed by the tts_shouldFree, which is safe but is also
rather a kludge.  It may be better than a more invasive fix in
tuplesort.c, since no fix is really needed for v10+, because v10 changed
the contract that callers have with tuple-fetch tuplesort routines to
something more robust (it would be even more invasive to backpatch the
v10 tuplesort.enhancements changes that make this a non-issue).

In passing, change the memory context switch point within
tuplesort_getdatum() to matchtuplesort_gettupleslot().  It's not clear
that this can cause a crash, but it's still wrong for pass-by-value
Datum tuplesorts.  Note that this tuplesort_getdatum() fix *is* required
for v10.

Author: Peter Geoghegan
Reported-By: Bernd Helmle
Analyzed-By: Peter Geoghegan, Andreas Seltenreich, Bernd Helmle
Discussion: https://www.postgresql.org/message-id/1512661638.9720.34.camel@oopsware.de
---
 src/backend/commands/copy.c        |  2 +-
 src/backend/executor/execMain.c    |  4 ++--
 src/backend/executor/execTuples.c  | 34 +++++++++++++++-------------------
 src/backend/utils/sort/tuplesort.c |  4 ++--
 src/include/executor/tuptable.h    |  2 +-
 5 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index ca72dd1..0a636b9 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2601,7 +2601,7 @@ CopyFrom(CopyState cstate)
 	pfree(values);
 	pfree(nulls);
 
-	ExecResetTupleTable(estate->es_tupleTable, false);
+	ExecResetTupleTable(estate->es_tupleTable);
 
 	ExecCloseIndices(resultRelInfo);
 
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 10ccf59..d657644 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1466,7 +1466,7 @@ ExecEndPlan(PlanState *planstate, EState *estate)
 	 * the TupleTableSlots, since the containing memory context is about to go
 	 * away anyway.
 	 */
-	ExecResetTupleTable(estate->es_tupleTable, false);
+	ExecResetTupleTable(estate->es_tupleTable);
 
 	/*
 	 * close the result relation(s) if any, but hold locks until xact commit.
@@ -2903,7 +2903,7 @@ EvalPlanQualEnd(EPQState *epqstate)
 	}
 
 	/* throw away the per-estate tuple table */
-	ExecResetTupleTable(estate->es_tupleTable, false);
+	ExecResetTupleTable(estate->es_tupleTable);
 
 	/* close any trigger target relations attached to this EState */
 	foreach(l, estate->es_trig_target_relations)
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index 533050d..d84966f 100644
--- a/src/backend/executor/execTuples.c
+++ b/src/backend/executor/execTuples.c
@@ -147,14 +147,13 @@ ExecAllocTableSlot(List **tupleTable)
  *		ExecResetTupleTable
  *
  *		This releases any resources (buffer pins, tupdesc refcounts)
- *		held by the tuple table, and optionally releases the memory
- *		occupied by the tuple table data structure.
- *		It is expected that this routine be called by EndPlan().
+ *		held by the tuple table.  It never releases the memory
+ *		occupied by the tuple table data structure.  It is expected
+ *		that this routine be called by EndPlan().
  * --------------------------------
  */
 void
-ExecResetTupleTable(List *tupleTable,	/* tuple table */
-					bool shouldFree)	/* true if we should free memory */
+ExecResetTupleTable(List *tupleTable)	/* tuple table */
 {
 	ListCell   *lc;
 
@@ -165,6 +164,17 @@ ExecResetTupleTable(List *tupleTable,	/* tuple table */
 		/* Sanity checks */
 		Assert(IsA(slot, TupleTableSlot));
 
+		/*
+		 * Kludge:  force slot shouldFree policy to match caller's.
+		 *
+		 * This can prevent problems when executor nodes were not sufficiently
+		 * careful about tuple lifetime, for example when tuple comes from a
+		 * utility like a tuplesort that doesn't heed tts_shouldFree
+		 * conventions in managing underlying memory.
+		 */
+		slot->tts_shouldFree = false;
+		slot->tts_shouldFreeMin = false;
+
 		/* Always release resources and reset the slot to empty */
 		ExecClearTuple(slot);
 		if (slot->tts_tupleDescriptor)
@@ -172,21 +182,7 @@ ExecResetTupleTable(List *tupleTable,	/* tuple table */
 			ReleaseTupleDesc(slot->tts_tupleDescriptor);
 			slot->tts_tupleDescriptor = NULL;
 		}
-
-		/* If shouldFree, release memory occupied by the slot itself */
-		if (shouldFree)
-		{
-			if (slot->tts_values)
-				pfree(slot->tts_values);
-			if (slot->tts_isnull)
-				pfree(slot->tts_isnull);
-			pfree(slot);
-		}
 	}
-
-	/* If shouldFree, release the list structure */
-	if (shouldFree)
-		list_free(tupleTable);
 }
 
 /* --------------------------------
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 4dd5407..a6c8de4 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -2155,6 +2155,8 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 		return false;
 	}
 
+	MemoryContextSwitchTo(oldcontext);
+
 	/* Record abbreviated key for caller */
 	if (state->sortKeys->abbrev_converter && abbrev)
 		*abbrev = stup.datum1;
@@ -2175,8 +2177,6 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 		*isNull = false;
 	}
 
-	MemoryContextSwitchTo(oldcontext);
-
 	return true;
 }
 
diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h
index 5ac0b6a..39a678a 100644
--- a/src/include/executor/tuptable.h
+++ b/src/include/executor/tuptable.h
@@ -141,7 +141,7 @@ typedef struct TupleTableSlot
 /* in executor/execTuples.c */
 extern TupleTableSlot *MakeTupleTableSlot(void);
 extern TupleTableSlot *ExecAllocTableSlot(List **tupleTable);
-extern void ExecResetTupleTable(List *tupleTable, bool shouldFree);
+extern void ExecResetTupleTable(List *tupleTable);
 extern TupleTableSlot *MakeSingleTupleTableSlot(TupleDesc tupdesc);
 extern void ExecDropSingleTupleTableSlot(TupleTableSlot *slot);
 extern void ExecSetSlotDescriptor(TupleTableSlot *slot, TupleDesc tupdesc);
-- 
2.7.4

From 8743b443d91b51371fe5df17795a88210aa0264a Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Thu, 14 Dec 2017 21:22:41 -0800
Subject: [PATCH] Fix double free of tuple with grouping sets/tuplesort.c.

Have tuplesort_gettupleslot() copy the contents of its current table
slot into caller's memory context if and when caller is obligated to
free memory itself.  Otherwise, continue to allocate the memory in
tuplesort-owned context, and never tell the caller to free it
themselves.  Repeat this for all "fetch tuple" routines, to be
consistent.

This fixes a crash that can occur on at least 9.5 and 9.6, where some
grouping sets queries may call tuplesort_end() at node shutdown time,
and free tuple memory that the executor imagines it is responsible for,
and must free within ExecClearTuple().

In passing, change the memory context switch point within
tuplesort_getdatum() to matchtuplesort_gettupleslot().  It's not clear
that this can cause a crash, but it's still wrong for pass-by-value
Datum tuplesorts.

Only the tuplesort_getdatum() fix is required for v10, because v10
changed the contract that callers have with tuple-fetch tuplesort
routines to something more robust.  Fetch tuple routines create full
copies of tuples iff callers explicitly opt in on v10, accepting the
risk that it won't be safe to reference the tuple in the very next call
to the fetch routine.

Author: Peter Geoghegan
Reported-By: Bernd Helmle
Analyzed-By: Peter Geoghegan, Andreas Seltenreich, Bernd Helmle
Discussion: https://www.postgresql.org/message-id/1512661638.9720.34.camel@oopsware.de
---
 src/backend/utils/sort/tuplesort.c | 191 ++++++++++++++++++++++++-------------
 1 file changed, 124 insertions(+), 67 deletions(-)

diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 4dd5407..f7f8b83 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -264,6 +264,7 @@ struct Tuplesortstate
 	int			tapeRange;		/* maxTapes-1 (Knuth's P) */
 	MemoryContext sortcontext;	/* memory context holding most sort data */
 	MemoryContext tuplecontext; /* sub-context of sortcontext for tuple data */
+	MemoryContext callercontext; /* tuplesort caller's own context */
 	LogicalTapeSet *tapeset;	/* logtape.c object for tapes in a temp file */
 
 	/*
@@ -390,10 +391,11 @@ struct Tuplesortstate
 	 * sorted order).
 	 */
 	int64		spacePerTape;	/* Space (memory) for tuples (not slots) */
+	int64	   *mergeosize;		/* "overflow" allocation size */
 	char	  **mergetuples;	/* Each tape's memory allocation */
 	char	  **mergecurrent;	/* Current offset into each tape's memory */
 	char	  **mergetail;		/* Last item's start point for each tape */
-	char	  **mergeoverflow;	/* Retail palloc() "overflow" for each tape */
+	char	  **mergeoverflow;	/* "overflow" scratch buffer for each tape */
 
 	/*
 	 * Variables for Algorithm D.  Note that destTape is a "logical" tape
@@ -534,7 +536,11 @@ struct Tuplesortstate
  * Note that this places a responsibility on readtup and copytup routines
  * to use the right memory context for these tuples (and to not use the
  * reset context for anything whose lifetime needs to span multiple
- * external sort runs).
+ * external sort runs).  Note also that readtup routines need to make sure
+ * that memory that caller must free is in fact allocated in caller
+ * context, not in either of the two tuplesort memory contexts.  Callers
+ * must either have full ownership of memory for returned tuples, or
+ * leave it all entirely up to us.
  */
 
 /* When using this macro, beware of double evaluation of len */
@@ -691,6 +697,7 @@ tuplesort_begin_common(int workMem, bool randomAccess)
 	state->availMem = state->allowedMem;
 	state->sortcontext = sortcontext;
 	state->tuplecontext = tuplecontext;
+	state->callercontext = NULL;
 	state->tapeset = NULL;
 
 	state->memtupcount = 0;
@@ -721,6 +728,12 @@ tuplesort_begin_common(int workMem, bool randomAccess)
 
 	state->result_tape = -1;	/* flag that result tape has not been formed */
 
+	/*
+	 * Let caller initialize state->callercontext.  The usage pattern for
+	 * state->callercontext is that it must be set and used in externally
+	 * callable functions.  Functions with local linkage do a conventional
+	 * switch-and-unswitch, to avoid clobbering state->callercontext.
+	 */
 	MemoryContextSwitchTo(oldcontext);
 
 	return state;
@@ -733,11 +746,13 @@ tuplesort_begin_heap(TupleDesc tupDesc,
 					 bool *nullsFirstFlags,
 					 int workMem, bool randomAccess)
 {
-	Tuplesortstate *state = tuplesort_begin_common(workMem, randomAccess);
-	MemoryContext oldcontext;
+	Tuplesortstate *state;
+	MemoryContext oldcontext = CurrentMemoryContext;
 	int			i;
 
-	oldcontext = MemoryContextSwitchTo(state->sortcontext);
+	state = tuplesort_begin_common(workMem, randomAccess);
+	MemoryContextSwitchTo(state->sortcontext);
+	state->callercontext = oldcontext;
 
 	AssertArg(nkeys > 0);
 
@@ -804,14 +819,16 @@ tuplesort_begin_cluster(TupleDesc tupDesc,
 						Relation indexRel,
 						int workMem, bool randomAccess)
 {
-	Tuplesortstate *state = tuplesort_begin_common(workMem, randomAccess);
+	Tuplesortstate *state;
 	ScanKey		indexScanKey;
-	MemoryContext oldcontext;
+	MemoryContext oldcontext = CurrentMemoryContext;
 	int			i;
 
 	Assert(indexRel->rd_rel->relam == BTREE_AM_OID);
 
-	oldcontext = MemoryContextSwitchTo(state->sortcontext);
+	state = tuplesort_begin_common(workMem, randomAccess);
+	MemoryContextSwitchTo(state->sortcontext);
+	state->callercontext = oldcontext;
 
 #ifdef TRACE_SORT
 	if (trace_sort)
@@ -898,12 +915,14 @@ tuplesort_begin_index_btree(Relation heapRel,
 							bool enforceUnique,
 							int workMem, bool randomAccess)
 {
-	Tuplesortstate *state = tuplesort_begin_common(workMem, randomAccess);
+	Tuplesortstate *state;
 	ScanKey		indexScanKey;
-	MemoryContext oldcontext;
+	MemoryContext oldcontext = CurrentMemoryContext;
 	int			i;
 
-	oldcontext = MemoryContextSwitchTo(state->sortcontext);
+	state = tuplesort_begin_common(workMem, randomAccess);
+	MemoryContextSwitchTo(state->sortcontext);
+	state->callercontext = oldcontext;
 
 #ifdef TRACE_SORT
 	if (trace_sort)
@@ -974,10 +993,12 @@ tuplesort_begin_index_hash(Relation heapRel,
 						   uint32 hash_mask,
 						   int workMem, bool randomAccess)
 {
-	Tuplesortstate *state = tuplesort_begin_common(workMem, randomAccess);
-	MemoryContext oldcontext;
+	Tuplesortstate *state;
+	MemoryContext oldcontext = CurrentMemoryContext;
 
-	oldcontext = MemoryContextSwitchTo(state->sortcontext);
+	state = tuplesort_begin_common(workMem, randomAccess);
+	MemoryContextSwitchTo(state->sortcontext);
+	state->callercontext = oldcontext;
 
 #ifdef TRACE_SORT
 	if (trace_sort)
@@ -1010,12 +1031,14 @@ tuplesort_begin_datum(Oid datumType, Oid sortOperator, Oid sortCollation,
 					  bool nullsFirstFlag,
 					  int workMem, bool randomAccess)
 {
-	Tuplesortstate *state = tuplesort_begin_common(workMem, randomAccess);
-	MemoryContext oldcontext;
+	Tuplesortstate *state;
+	MemoryContext oldcontext = CurrentMemoryContext;
 	int16		typlen;
 	bool		typbyval;
 
-	oldcontext = MemoryContextSwitchTo(state->sortcontext);
+	state = tuplesort_begin_common(workMem, randomAccess);
+	MemoryContextSwitchTo(state->sortcontext);
+	state->callercontext = oldcontext;
 
 #ifdef TRACE_SORT
 	if (trace_sort)
@@ -1193,7 +1216,8 @@ tuplesort_end(Tuplesortstate *state)
 
 	/*
 	 * Free the per-sort memory context, thereby releasing all working memory,
-	 * including the Tuplesortstate struct itself.
+	 * including the Tuplesortstate struct itself.  Caller tuples where
+	 * should_free was set to TRUE remain allocated, however.
 	 */
 	MemoryContextDelete(state->sortcontext);
 }
@@ -1335,9 +1359,10 @@ noalloc:
 void
 tuplesort_puttupleslot(Tuplesortstate *state, TupleTableSlot *slot)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
 	SortTuple	stup;
 
+	state->callercontext = MemoryContextSwitchTo(state->sortcontext);
+
 	/*
 	 * Copy the given tuple into memory we control, and decrease availMem.
 	 * Then call the common code.
@@ -1346,7 +1371,7 @@ tuplesort_puttupleslot(Tuplesortstate *state, TupleTableSlot *slot)
 
 	puttuple_common(state, &stup);
 
-	MemoryContextSwitchTo(oldcontext);
+	MemoryContextSwitchTo(state->callercontext);
 }
 
 /*
@@ -1357,18 +1382,18 @@ tuplesort_puttupleslot(Tuplesortstate *state, TupleTableSlot *slot)
 void
 tuplesort_putheaptuple(Tuplesortstate *state, HeapTuple tup)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
 	SortTuple	stup;
 
 	/*
 	 * Copy the given tuple into memory we control, and decrease availMem.
 	 * Then call the common code.
 	 */
+	state->callercontext = MemoryContextSwitchTo(state->sortcontext);
 	COPYTUP(state, &stup, (void *) tup);
 
 	puttuple_common(state, &stup);
 
-	MemoryContextSwitchTo(oldcontext);
+	MemoryContextSwitchTo(state->callercontext);
 }
 
 /*
@@ -1380,11 +1405,11 @@ tuplesort_putindextuplevalues(Tuplesortstate *state, Relation rel,
 							  ItemPointer self, Datum *values,
 							  bool *isnull)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->tuplecontext);
 	SortTuple	stup;
 	Datum		original;
 	IndexTuple	tuple;
 
+	state->callercontext = MemoryContextSwitchTo(state->tuplecontext);
 	stup.tuple = index_form_tuple(RelationGetDescr(rel), values, isnull);
 	tuple = ((IndexTuple) stup.tuple);
 	tuple->t_tid = *self;
@@ -1445,7 +1470,7 @@ tuplesort_putindextuplevalues(Tuplesortstate *state, Relation rel,
 
 	puttuple_common(state, &stup);
 
-	MemoryContextSwitchTo(oldcontext);
+	MemoryContextSwitchTo(state->callercontext);
 }
 
 /*
@@ -1456,7 +1481,6 @@ tuplesort_putindextuplevalues(Tuplesortstate *state, Relation rel,
 void
 tuplesort_putdatum(Tuplesortstate *state, Datum val, bool isNull)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->tuplecontext);
 	SortTuple	stup;
 
 	/*
@@ -1470,6 +1494,7 @@ tuplesort_putdatum(Tuplesortstate *state, Datum val, bool isNull)
 	 * abbreviated value if abbreviation is happening, otherwise it's
 	 * identical to stup.tuple.
 	 */
+	state->callercontext = MemoryContextSwitchTo(state->tuplecontext);
 
 	if (isNull || !state->tuples)
 	{
@@ -1529,7 +1554,7 @@ tuplesort_putdatum(Tuplesortstate *state, Datum val, bool isNull)
 
 	puttuple_common(state, &stup);
 
-	MemoryContextSwitchTo(oldcontext);
+	MemoryContextSwitchTo(state->callercontext);
 }
 
 /*
@@ -1743,7 +1768,7 @@ consider_abort_common(Tuplesortstate *state)
 void
 tuplesort_performsort(Tuplesortstate *state)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
+	state->callercontext = MemoryContextSwitchTo(state->sortcontext);
 
 #ifdef TRACE_SORT
 	if (trace_sort)
@@ -1816,7 +1841,7 @@ tuplesort_performsort(Tuplesortstate *state)
 	}
 #endif
 
-	MemoryContextSwitchTo(oldcontext);
+	MemoryContextSwitchTo(state->callercontext);
 }
 
 /*
@@ -1824,6 +1849,9 @@ tuplesort_performsort(Tuplesortstate *state)
  * direction into *stup.  Returns FALSE if no more tuples.
  * If *should_free is set, the caller must pfree stup.tuple when done with it.
  * Otherwise, caller should not use tuple following next call here.
+ *
+ * Note:  Caller can expect tuple to be allocated in their own memory context
+ * when should_free is TRUE.
  */
 static bool
 tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
@@ -2046,22 +2074,24 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
  * NULL value in leading attribute will set abbreviated value to zeroed
  * representation, which caller may rely on in abbreviated inequality check.
  *
- * The slot receives a copied tuple (sometimes allocated in caller memory
- * context) that will stay valid regardless of future manipulations of the
- * tuplesort's state.
+ * The slot receives a copied tuple that will stay valid regardless of future
+ * manipulations of the tuplesort's state.  This includes calls to
+ * tuplesort_end() -- tuple will be allocated in caller's own context to make
+ * this work.
  */
 bool
 tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
 					   TupleTableSlot *slot, Datum *abbrev)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
 	SortTuple	stup;
 	bool		should_free;
 
+	state->callercontext = MemoryContextSwitchTo(state->sortcontext);
+
 	if (!tuplesort_gettuple_common(state, forward, &stup, &should_free))
 		stup.tuple = NULL;
 
-	MemoryContextSwitchTo(oldcontext);
+	MemoryContextSwitchTo(state->callercontext);
 
 	if (stup.tuple)
 	{
@@ -2089,18 +2119,20 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
  * Returns NULL if no more tuples.  If *should_free is set, the
  * caller must pfree the returned tuple when done with it.
  * If it is not set, caller should not use tuple following next
- * call here.
+ * call here, and can expect tuple to be in their own memory
+ * context.
  */
 HeapTuple
 tuplesort_getheaptuple(Tuplesortstate *state, bool forward, bool *should_free)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
 	SortTuple	stup;
 
+	state->callercontext = MemoryContextSwitchTo(state->sortcontext);
+
 	if (!tuplesort_gettuple_common(state, forward, &stup, should_free))
 		stup.tuple = NULL;
 
-	MemoryContextSwitchTo(oldcontext);
+	MemoryContextSwitchTo(state->callercontext);
 
 	return stup.tuple;
 }
@@ -2116,13 +2148,14 @@ IndexTuple
 tuplesort_getindextuple(Tuplesortstate *state, bool forward,
 						bool *should_free)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
 	SortTuple	stup;
 
+	state->callercontext = MemoryContextSwitchTo(state->sortcontext);
+
 	if (!tuplesort_gettuple_common(state, forward, &stup, should_free))
 		stup.tuple = NULL;
 
-	MemoryContextSwitchTo(oldcontext);
+	MemoryContextSwitchTo(state->callercontext);
 
 	return (IndexTuple) stup.tuple;
 }
@@ -2132,7 +2165,7 @@ tuplesort_getindextuple(Tuplesortstate *state, bool forward,
  * Returns FALSE if no more datums.
  *
  * If the Datum is pass-by-ref type, the returned value is freshly palloc'd
- * and is now owned by the caller.
+ * in caller's context, and is now owned by the caller.
  *
  * Caller may optionally be passed back abbreviated value (on TRUE return
  * value) when abbreviation was used, which can be used to cheaply avoid
@@ -2145,16 +2178,19 @@ bool
 tuplesort_getdatum(Tuplesortstate *state, bool forward,
 				   Datum *val, bool *isNull, Datum *abbrev)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
 	SortTuple	stup;
 	bool		should_free;
 
+	state->callercontext = MemoryContextSwitchTo(state->sortcontext);
+
 	if (!tuplesort_gettuple_common(state, forward, &stup, &should_free))
 	{
-		MemoryContextSwitchTo(oldcontext);
+		MemoryContextSwitchTo(state->callercontext);
 		return false;
 	}
 
+	MemoryContextSwitchTo(state->callercontext);
+
 	/* Record abbreviated key for caller */
 	if (state->sortKeys->abbrev_converter && abbrev)
 		*abbrev = stup.datum1;
@@ -2175,8 +2211,6 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 		*isNull = false;
 	}
 
-	MemoryContextSwitchTo(oldcontext);
-
 	return true;
 }
 
@@ -2188,8 +2222,6 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 bool
 tuplesort_skiptuples(Tuplesortstate *state, int64 ntuples, bool forward)
 {
-	MemoryContext oldcontext;
-
 	/*
 	 * We don't actually support backwards skip yet, because no callers need
 	 * it.  The API is designed to allow for that later, though.
@@ -2225,7 +2257,7 @@ tuplesort_skiptuples(Tuplesortstate *state, int64 ntuples, bool forward)
 			 * We could probably optimize these cases better, but for now it's
 			 * not worth the trouble.
 			 */
-			oldcontext = MemoryContextSwitchTo(state->sortcontext);
+			state->callercontext = MemoryContextSwitchTo(state->sortcontext);
 			while (ntuples-- > 0)
 			{
 				SortTuple	stup;
@@ -2234,14 +2266,14 @@ tuplesort_skiptuples(Tuplesortstate *state, int64 ntuples, bool forward)
 				if (!tuplesort_gettuple_common(state, forward,
 											   &stup, &should_free))
 				{
-					MemoryContextSwitchTo(oldcontext);
+					MemoryContextSwitchTo(state->callercontext);
 					return false;
 				}
 				if (should_free && stup.tuple)
 					pfree(stup.tuple);
 				CHECK_FOR_INTERRUPTS();
 			}
-			MemoryContextSwitchTo(oldcontext);
+			MemoryContextSwitchTo(state->callercontext);
 			return true;
 
 		default:
@@ -2363,6 +2395,7 @@ inittapes(Tuplesortstate *state)
 	state->mergelast = (int *) palloc0(maxTapes * sizeof(int));
 	state->mergeavailslots = (int *) palloc0(maxTapes * sizeof(int));
 	state->mergeavailmem = (int64 *) palloc0(maxTapes * sizeof(int64));
+	state->mergeosize = (int64 *) palloc0(maxTapes * sizeof(int64 *));
 	state->mergetuples = (char **) palloc0(maxTapes * sizeof(char *));
 	state->mergecurrent = (char **) palloc0(maxTapes * sizeof(char *));
 	state->mergetail = (char **) palloc0(maxTapes * sizeof(char *));
@@ -2978,6 +3011,7 @@ mergebatch(Tuplesortstate *state, int64 spacePerTape)
 		state->mergetuples[srcTape] = mergetuples;
 		state->mergecurrent[srcTape] = mergetuples;
 		state->mergetail[srcTape] = mergetuples;
+		state->mergeosize[srcTape] = 0;
 		state->mergeoverflow[srcTape] = NULL;
 	}
 
@@ -2993,7 +3027,8 @@ mergebatch(Tuplesortstate *state, int64 spacePerTape)
  * reset to indicate that all memory may be reused.
  *
  * This routine must deal with fixing up the tuple that is about to be returned
- * to the client, due to "overflow" allocations.
+ * to the client, due to "overflow" allocations, while making sure that tuple
+ * memory ends up in caller's own context.
  */
 static void
 mergebatchone(Tuplesortstate *state, int srcTape, SortTuple *rtup,
@@ -3033,17 +3068,28 @@ mergebatchone(Tuplesortstate *state, int srcTape, SortTuple *rtup,
 		/*
 		 * Handle an "overflow" retail palloc.
 		 *
-		 * This is needed when we run out of tuple memory for the tape.
+		 * This is needed when we run out of tuple memory for the tape.  Note
+		 * that we actually free the memory used for the overflow allocation
+		 * directly, and provide a copy allocated in caller's context to
+		 * caller.
 		 */
 		state->mergecurrent[srcTape] = state->mergetuples[srcTape];
 		state->mergetail[srcTape] = state->mergetuples[srcTape];
 
 		if (rtup->tuple)
 		{
+			Assert(state->mergeosize[srcTape] > 0);
 			Assert(rtup->tuple == (void *) state->mergeoverflow[srcTape]);
+			rtup->tuple = MemoryContextAlloc(state->callercontext,
+											 state->mergeosize[srcTape]);
+			MOVETUP(rtup->tuple, state->mergeoverflow[srcTape],
+					state->mergeosize[srcTape]);
 			/* Caller should free palloc'd tuple */
 			*should_free = true;
+			/* Free scratch buffer */
+			pfree(state->mergeoverflow[srcTape]);
 		}
+		state->mergeosize[srcTape] = 0;
 		state->mergeoverflow[srcTape] = NULL;
 	}
 }
@@ -3064,7 +3110,7 @@ mergebatchfreetape(Tuplesortstate *state, int srcTape, SortTuple *rtup,
 	Assert(state->status == TSS_FINALMERGE);
 
 	/*
-	 * Tuple may or may not already be an overflow allocation from
+	 * Tuple may or may not already be caller-owned allocation from
 	 * mergebatchone()
 	 */
 	if (!*should_free && rtup->tuple)
@@ -3072,17 +3118,15 @@ mergebatchfreetape(Tuplesortstate *state, int srcTape, SortTuple *rtup,
 		/*
 		 * Final tuple still in tape's batch allocation.
 		 *
-		 * Return palloc()'d copy to caller, and have it freed in a similar
-		 * manner to overflow allocation.  Otherwise, we'd free batch memory
-		 * and pass back a pointer to garbage.  Note that we deliberately
-		 * allocate this in the parent tuplesort context, to be on the safe
-		 * side.
+		 * Return palloc()'d copy to caller, in caller's own memory context.
+		 * Otherwise, we'd free batch memory and pass back a pointer to
+		 * garbage.
 		 */
 		Size		tuplen;
 		void	   *oldTuple = rtup->tuple;
 
 		tuplen = state->mergecurrent[srcTape] - state->mergetail[srcTape];
-		rtup->tuple = MemoryContextAlloc(state->sortcontext, tuplen);
+		rtup->tuple = MemoryContextAlloc(state->callercontext, tuplen);
 		MOVETUP(rtup->tuple, oldTuple, tuplen);
 		*should_free = true;
 	}
@@ -3138,11 +3182,14 @@ mergebatchalloc(Tuplesortstate *state, int tapenum, Size tuplen)
 		 * will be detected quickly, in a similar fashion to a LACKMEM()
 		 * condition, and should not happen again before a new round of
 		 * preloading for caller's tape.  Note that we deliberately allocate
-		 * this in the parent tuplesort context, to be on the safe side.
+		 * this in the parent tuplesort context, to be on the safe side.  Note
+		 * also that tuplesort caller will ultimately get their own copy of
+		 * this, in their own memory context.
 		 *
 		 * Sometimes, this does not happen because merging runs out of slots
 		 * before running out of memory.
 		 */
+		state->mergeosize[tapenum] = tuplen;
 		ret = state->mergeoverflow[tapenum] =
 			MemoryContextAlloc(state->sortcontext, tuplen);
 	}
@@ -3460,7 +3507,7 @@ dumpbatch(Tuplesortstate *state, bool alltuples)
 void
 tuplesort_rescan(Tuplesortstate *state)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
+	state->callercontext = MemoryContextSwitchTo(state->sortcontext);
 
 	Assert(state->randomAccess);
 
@@ -3486,7 +3533,7 @@ tuplesort_rescan(Tuplesortstate *state)
 			break;
 	}
 
-	MemoryContextSwitchTo(oldcontext);
+	MemoryContextSwitchTo(state->callercontext);
 }
 
 /*
@@ -3495,7 +3542,7 @@ tuplesort_rescan(Tuplesortstate *state)
 void
 tuplesort_markpos(Tuplesortstate *state)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
+	state->callercontext = MemoryContextSwitchTo(state->sortcontext);
 
 	Assert(state->randomAccess);
 
@@ -3517,7 +3564,7 @@ tuplesort_markpos(Tuplesortstate *state)
 			break;
 	}
 
-	MemoryContextSwitchTo(oldcontext);
+	MemoryContextSwitchTo(state->callercontext);
 }
 
 /*
@@ -3527,7 +3574,7 @@ tuplesort_markpos(Tuplesortstate *state)
 void
 tuplesort_restorepos(Tuplesortstate *state)
 {
-	MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
+	state->callercontext = MemoryContextSwitchTo(state->sortcontext);
 
 	Assert(state->randomAccess);
 
@@ -3550,7 +3597,7 @@ tuplesort_restorepos(Tuplesortstate *state)
 			break;
 	}
 
-	MemoryContextSwitchTo(oldcontext);
+	MemoryContextSwitchTo(state->callercontext);
 }
 
 /*
@@ -3887,7 +3934,8 @@ markrunend(Tuplesortstate *state, int tapenum)
  * from tape's batch allocation.  Otherwise, callers must pfree() or
  * reset tuple child memory context, and account for that with a
  * FREEMEM().  Currently, this only ever needs to happen in WRITETUP()
- * routines.
+ * routines, or when memory is allocated in caller's own context, and
+ * so must be freed on caller's own terms.
  */
 static void *
 readtup_alloc(Tuplesortstate *state, int tapenum, Size tuplen)
@@ -3903,15 +3951,24 @@ readtup_alloc(Tuplesortstate *state, int tapenum, Size tuplen)
 		 */
 		return mergebatchalloc(state, tapenum, tuplen);
 	}
-	else
+	else if (state->status == TSS_BUILDRUNS)
 	{
 		char	   *ret;
 
-		/* Batch allocation yet to be performed */
+		/*
+		 * Batch allocation yet to be performed (non-final merge).  Use
+		 * tuplecontext to avoid fragmentation, much like tuplesort_putXXX and
+		 * copytup_XXX routines during initial run generation.
+		 */
 		ret = MemoryContextAlloc(state->tuplecontext, tuplen);
 		USEMEM(state, GetMemoryChunkSpace(ret));
 		return ret;
 	}
+	else
+	{
+		/* Must be allocated within tuplesort caller's context */
+		return MemoryContextAlloc(state->callercontext, tuplen);
+	}
 }
 
 
-- 
2.7.4

From 083b4f26446b51b4a3d839ca44c58144aecadc34 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Fri, 9 Feb 2018 15:02:08 -0800
Subject: [PATCH] Fix double free of tuple with grouping sets/tuplesort.c.

Have tuplesort_gettupleslot() copy tuple contents for slot into caller's
memory context in all cases.  This necessitates an extra copy on 9.5 and
9.6, though only for cases where an explicit copy was not already
required (mostly just TSS_SORTEDONTAPE fetches, though all
TSS_FINALMERGE fetches on 9.5).  Do the same for tuplesort_getdatum().

This fixes a crash that can occur on at least 9.5 and 9.6, where some
grouping sets queries may call tuplesort_end() at node shutdown time,
and free tuple memory that the executor/a tuple slot imagines it is
responsible for.

In the case of all other fetch routines, continue to allocate the memory
in the tuplesort-owned context (only change comments).  This is a little
inconsistent with what we now do for tuplesort_gettupleslot() and
tuplesort_getdatum(), but that's preferable to adding new copy overhead
in the backbranches where it's clearly unnecessary.  These other fetch
routines provide the weakest possible guarantees about tuple memory
lifespan from v10 on, anyway, so this actually seems more consistent
overall.

In passing, change the memory context switch point within
tuplesort_getdatum() to matchtuplesort_gettupleslot(), so that we
actually allocate memory in the right context there (caller's context),
which seems like an independent bug.  It's not clear that this can cause
a crash, but it's still wrong.

Only the latter, less important fix is required on v10 and master,
because in v10 there is already no question of not having to make a copy
in caller's context anyway (see commits e94568ec and 3856cf96).

Author: Peter Geoghegan
Reported-By: Bernd Helmle
Analyzed-By: Peter Geoghegan, Andreas Seltenreich, Bernd Helmle
Discussion: https://www.postgresql.org/message-id/1512661638.9720.34.camel@oopsware.de
Backpatch: 9.5-, where the issue is known to cause a crash.
---
 src/backend/utils/adt/orderedsetaggs.c | 21 +++++------
 src/backend/utils/sort/tuplesort.c     | 68 +++++++++++++++++++++++++++++-----
 2 files changed, 67 insertions(+), 22 deletions(-)

diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c
index 39ed85b..1a10202 100644
--- a/src/backend/utils/adt/orderedsetaggs.c
+++ b/src/backend/utils/adt/orderedsetaggs.c
@@ -457,10 +457,9 @@ percentile_disc_final(PG_FUNCTION_ARGS)
 		elog(ERROR, "missing row in percentile_disc");
 
 	/*
-	 * Note: we *cannot* clean up the tuplesort object here, because the value
-	 * to be returned is allocated inside its sortcontext.  We could use
-	 * datumCopy to copy it out of there, but it doesn't seem worth the
-	 * trouble, since the cleanup callback will clear the tuplesort later.
+	 * Note: we could clean up the tuplesort object here, but it doesn't seem
+	 * worth the trouble, since the cleanup callback will clear the tuplesort
+	 * later.
 	 */
 
 	/* We shouldn't have stored any nulls, but do the right thing anyway */
@@ -575,10 +574,9 @@ percentile_cont_final_common(FunctionCallInfo fcinfo,
 	}
 
 	/*
-	 * Note: we *cannot* clean up the tuplesort object here, because the value
-	 * to be returned may be allocated inside its sortcontext.  We could use
-	 * datumCopy to copy it out of there, but it doesn't seem worth the
-	 * trouble, since the cleanup callback will clear the tuplesort later.
+	 * Note: we could clean up the tuplesort object here, but it doesn't seem
+	 * worth the trouble, since the cleanup callback will clear the tuplesort
+	 * later.
 	 */
 
 	PG_RETURN_DATUM(val);
@@ -1089,10 +1087,9 @@ mode_final(PG_FUNCTION_ARGS)
 		pfree(DatumGetPointer(last_val));
 
 	/*
-	 * Note: we *cannot* clean up the tuplesort object here, because the value
-	 * to be returned is allocated inside its sortcontext.  We could use
-	 * datumCopy to copy it out of there, but it doesn't seem worth the
-	 * trouble, since the cleanup callback will clear the tuplesort later.
+	 * Note: we could clean up the tuplesort object here, but it doesn't seem
+	 * worth the trouble, since the cleanup callback will clear the tuplesort
+	 * later.
 	 */
 
 	if (mode_freq)
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 5676c07..294f2bd 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -1664,6 +1664,12 @@ tuplesort_performsort(Tuplesortstate *state)
  * Internal routine to fetch the next tuple in either forward or back
  * direction into *stup.  Returns FALSE if no more tuples.
  * If *should_free is set, the caller must pfree stup.tuple when done with it.
+ * Otherwise, caller should not use tuple following next call here.
+ *
+ * Note:  Public tuplesort fetch routine callers cannot rely on tuple being
+ * allocated in their own memory context when should_free is TRUE.  It may be
+ * necessary to create a new copy of the tuple to meet the requirements of
+ * public fetch routine callers.
  */
 static bool
 tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
@@ -1865,6 +1871,12 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
  * Fetch the next tuple in either forward or back direction.
  * If successful, put tuple in slot and return TRUE; else, clear the slot
  * and return FALSE.
+ *
+ * The slot receives a copied tuple that will stay valid regardless of future
+ * manipulations of the tuplesort's state.  This includes calls to
+ * tuplesort_end() -- tuple will be allocated in caller's own context to make
+ * this work (this differs from similar routines for other types of
+ * tuplesorts).
  */
 bool
 tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
@@ -1881,7 +1893,26 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
 
 	if (stup.tuple)
 	{
-		ExecStoreMinimalTuple((MinimalTuple) stup.tuple, slot, should_free);
+		void   *tuple = stup.tuple;
+
+		/*
+		 * Callers rely on memory being in their own memory context, which is
+		 * not guaranteed by tuplesort_gettuple_common(), even when should_free
+		 * is set to TRUE.  We must always copy here, since our interface does
+		 * not allow callers to opt into arrangement where tuple memory can go
+		 * away on the next call here, or after tuplesort_end() is called.
+		 */
+		stup.tuple = heap_copy_minimal_tuple((MinimalTuple) tuple);
+
+		/*
+		 * Free local copy eagerly.  It would be very invasive to get
+		 * tuplesort_gettuple_common() to allocate tuple in caller's context
+		 * for us, so we just do this instead.
+		 */
+		if (should_free)
+			pfree(tuple);
+
+		ExecStoreMinimalTuple((MinimalTuple) stup.tuple, slot, true);
 		return true;
 	}
 	else
@@ -1895,6 +1926,8 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
  * Fetch the next tuple in either forward or back direction.
  * Returns NULL if no more tuples.  If *should_free is set, the
  * caller must pfree the returned tuple when done with it.
+ * If it is not set, caller should not use tuple following next
+ * call here.  It's never okay to use it after tuplesort_end().
  */
 HeapTuple
 tuplesort_getheaptuple(Tuplesortstate *state, bool forward, bool *should_free)
@@ -1914,6 +1947,8 @@ tuplesort_getheaptuple(Tuplesortstate *state, bool forward, bool *should_free)
  * Fetch the next index tuple in either forward or back direction.
  * Returns NULL if no more tuples.  If *should_free is set, the
  * caller must pfree the returned tuple when done with it.
+ * If it is not set, caller should not use tuple following next
+ * call here.  It's never okay to use it after tuplesort_end().
  */
 IndexTuple
 tuplesort_getindextuple(Tuplesortstate *state, bool forward,
@@ -1935,7 +1970,8 @@ tuplesort_getindextuple(Tuplesortstate *state, bool forward,
  * Returns FALSE if no more datums.
  *
  * If the Datum is pass-by-ref type, the returned value is freshly palloc'd
- * and is now owned by the caller.
+ * in caller's context, and is now owned by the caller (this differs from
+ * similar routines for other types of tuplesorts).
  */
 bool
 tuplesort_getdatum(Tuplesortstate *state, bool forward,
@@ -1950,6 +1986,8 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 		MemoryContextSwitchTo(oldcontext);
 		return false;
 	}
+	/* Copy into caller's memory context */
+	MemoryContextSwitchTo(oldcontext);
 
 	if (stup.isnull1 || state->datumTypeByVal)
 	{
@@ -1958,16 +1996,26 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 	}
 	else
 	{
-		/* use stup.tuple because stup.datum1 may be an abbreviation */
-
-		if (should_free)
-			*val = PointerGetDatum(stup.tuple);
-		else
-			*val = datumCopy(PointerGetDatum(stup.tuple), false, state->datumTypeLen);
+		/*
+		 * Callers rely on memory being in their own memory context, which is
+		 * not guaranteed by tuplesort_gettuple_common(), even when should_free
+		 * is set to TRUE.  We must always copy here, since our interface does
+		 * not allow callers to opt into arrangement where tuple memory can go
+		 * away on the next call here, or after tuplesort_end() is called.
+		 *
+		 * Use stup.tuple because stup.datum1 may be an abbreviation.
+		 */
+		*val = datumCopy(PointerGetDatum(stup.tuple), false, state->datumTypeLen);
 		*isNull = false;
-	}
 
-	MemoryContextSwitchTo(oldcontext);
+		/*
+		 * Free local copy eagerly.  It would be very invasive to get
+		 * tuplesort_gettuple_common() to allocate tuple in caller's context
+		 * for us, so we just do this instead.
+		 */
+		if (should_free)
+			pfree(stup.tuple);
+	}
 
 	return true;
 }
-- 
2.7.4

From 61ee080f3763e5a4112e9507f81d60fb8e889e99 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Fri, 9 Feb 2018 15:40:44 -0800
Subject: [PATCH] Fix double free of tuple with grouping sets/tuplesort.c.

Have tuplesort_gettupleslot() copy tuple contents for slot into caller's
memory context in all cases.  This necessitates an extra copy on 9.5 and
9.6, though only for cases where an explicit copy was not already
required (mostly just TSS_SORTEDONTAPE fetches, though all
TSS_FINALMERGE fetches on 9.5).  Do the same for tuplesort_getdatum().

This fixes a crash that can occur on at least 9.5 and 9.6, where some
grouping sets queries may call tuplesort_end() at node shutdown time,
and free tuple memory that the executor/a tuple slot imagines it is
responsible for.

In the case of all other fetch routines, continue to allocate the memory
in the tuplesort-owned context (only change comments).  This is a little
inconsistent with what we now do for tuplesort_gettupleslot() and
tuplesort_getdatum(), but that's preferable to adding new copy overhead
in the backbranches where it's clearly unnecessary.  These other fetch
routines provide the weakest possible guarantees about tuple memory
lifespan from v10 on, anyway, so this actually seems more consistent
overall.

In passing, change the memory context switch point within
tuplesort_getdatum() to matchtuplesort_gettupleslot(), so that we
actually allocate memory in the right context there (caller's context),
which seems like an independent bug.  It's not clear that this can cause
a crash, but it's still wrong.

Only the latter, less important fix is required on v10 and master,
because in v10 there is already no question of not having to make a copy
in caller's context anyway (see commits e94568ec and 3856cf96).

Author: Peter Geoghegan
Reported-By: Bernd Helmle
Analyzed-By: Peter Geoghegan, Andreas Seltenreich, Bernd Helmle
Discussion: https://www.postgresql.org/message-id/1512661638.9720.34.camel@oopsware.de
Backpatch: 9.5-, where the issue is known to cause a crash.
---
 src/backend/utils/adt/orderedsetaggs.c |  5 +----
 src/backend/utils/sort/tuplesort.c     | 19 ++++++++++---------
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c
index 50b34fc..ed36851 100644
--- a/src/backend/utils/adt/orderedsetaggs.c
+++ b/src/backend/utils/adt/orderedsetaggs.c
@@ -329,10 +329,7 @@ ordered_set_startup(FunctionCallInfo fcinfo, bool use_tuples)
  *
  * In the case where we're not expecting multiple finalfn calls, we could
  * arguably rely on the finalfn to clean up; but it's easier and more testable
- * if we just do it the same way in either case.  Note that many of the
- * finalfns could *not* free the tuplesort object, at least not without extra
- * data copying, because what they return is a pointer to a datum inside the
- * tuplesort object.
+ * if we just do it the same way in either case.
  */
 static void
 ordered_set_shutdown(Datum arg)
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 041bdc2..7fec18e 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -2148,11 +2148,12 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
  * representation, which caller may rely on in abbreviated inequality check.
  *
  * If copy is true, the slot receives a copied tuple that will stay valid
- * regardless of future manipulations of the tuplesort's state.  Memory is
- * owned by the caller.  If copy is false, the slot will just receive a
- * pointer to a tuple held within the tuplesort, which is more efficient, but
- * only safe for callers that are prepared to have any subsequent manipulation
- * of the tuplesort's state invalidate slot contents.
+ * regardless of future manipulations of the tuplesort's state.  This includes
+ * calls to tuplesort_end() -- tuple will be allocated in caller's own context
+ * to make this work.  If copy is false, the slot will just receive a pointer
+ * to a tuple held within the tuplesort, which is more efficient, but only safe
+ * for callers that are prepared to have any subsequent manipulation of the
+ * tuplesort's state invalidate slot contents.
  */
 bool
 tuplesort_gettupleslot(Tuplesortstate *state, bool forward, bool copy,
@@ -2230,8 +2231,8 @@ tuplesort_getindextuple(Tuplesortstate *state, bool forward)
  * Returns false if no more datums.
  *
  * If the Datum is pass-by-ref type, the returned value is freshly palloc'd
- * and is now owned by the caller (this differs from similar routines for
- * other types of tuplesorts).
+ * in caller's context, and is now owned by the caller (this differs from
+ * similar routines for other types of tuplesorts).
  *
  * Caller may optionally be passed back abbreviated value (on true return
  * value) when abbreviation was used, which can be used to cheaply avoid
@@ -2252,6 +2253,8 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 		MemoryContextSwitchTo(oldcontext);
 		return false;
 	}
+	/* Copy into caller's memory context */
+	MemoryContextSwitchTo(oldcontext);
 
 	/* Record abbreviated key for caller */
 	if (state->sortKeys->abbrev_converter && abbrev)
@@ -2269,8 +2272,6 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 		*isNull = false;
 	}
 
-	MemoryContextSwitchTo(oldcontext);
-
 	return true;
 }
 
-- 
2.7.4

From fcbdb2e4fb76a75a0c56ca258f99df888b174dc3 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Thu, 14 Dec 2017 21:22:41 -0800
Subject: [PATCH] Fix double free of tuple with grouping sets/tuplesort.c.

Have tuplesort_gettupleslot() copy tuple contents for slot into caller's
memory context in all cases.  This necessitates an extra copy on 9.5 and
9.6, though only for cases where an explicit copy was not already
required (mostly just TSS_SORTEDONTAPE fetches, though all
TSS_FINALMERGE fetches on 9.5).  Do the same for tuplesort_getdatum().

This fixes a crash that can occur on at least 9.5 and 9.6, where some
grouping sets queries may call tuplesort_end() at node shutdown time,
and free tuple memory that the executor/a tuple slot imagines it is
responsible for.

In the case of all other fetch routines, continue to allocate the memory
in the tuplesort-owned context (only change comments).  This is a little
inconsistent with what we now do for tuplesort_gettupleslot() and
tuplesort_getdatum(), but that's preferable to adding new copy overhead
in the backbranches where it's clearly unnecessary.  These other fetch
routines provide the weakest possible guarantees about tuple memory
lifespan from v10 on, anyway, so this actually seems more consistent
overall.

In passing, change the memory context switch point within
tuplesort_getdatum() to matchtuplesort_gettupleslot(), so that we
actually allocate memory in the right context there (caller's context),
which seems like an independent bug.  It's not clear that this can cause
a crash, but it's still wrong.

Only the latter, less important fix is required on v10 and master,
because in v10 there is already no question of not having to make a copy
in caller's context anyway (see commits e94568ec and 3856cf96).

Author: Peter Geoghegan
Reported-By: Bernd Helmle
Analyzed-By: Peter Geoghegan, Andreas Seltenreich, Bernd Helmle
Discussion: https://www.postgresql.org/message-id/1512661638.9720.34.camel@oopsware.de
Backpatch: 9.5-, where the issue is known to cause a crash.
---
 src/backend/utils/adt/orderedsetaggs.c | 21 +++++-----
 src/backend/utils/sort/tuplesort.c     | 74 +++++++++++++++++++++++++---------
 2 files changed, 63 insertions(+), 32 deletions(-)

diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c
index fe44d56..4d1e201 100644
--- a/src/backend/utils/adt/orderedsetaggs.c
+++ b/src/backend/utils/adt/orderedsetaggs.c
@@ -457,10 +457,9 @@ percentile_disc_final(PG_FUNCTION_ARGS)
 		elog(ERROR, "missing row in percentile_disc");
 
 	/*
-	 * Note: we *cannot* clean up the tuplesort object here, because the value
-	 * to be returned is allocated inside its sortcontext.  We could use
-	 * datumCopy to copy it out of there, but it doesn't seem worth the
-	 * trouble, since the cleanup callback will clear the tuplesort later.
+	 * Note: we could clean up the tuplesort object here, but it doesn't seem
+	 * worth the trouble, since the cleanup callback will clear the tuplesort
+	 * later.
 	 */
 
 	/* We shouldn't have stored any nulls, but do the right thing anyway */
@@ -575,10 +574,9 @@ percentile_cont_final_common(FunctionCallInfo fcinfo,
 	}
 
 	/*
-	 * Note: we *cannot* clean up the tuplesort object here, because the value
-	 * to be returned may be allocated inside its sortcontext.  We could use
-	 * datumCopy to copy it out of there, but it doesn't seem worth the
-	 * trouble, since the cleanup callback will clear the tuplesort later.
+	 * Note: we could clean up the tuplesort object here, but it doesn't seem
+	 * worth the trouble, since the cleanup callback will clear the tuplesort
+	 * later.
 	 */
 
 	PG_RETURN_DATUM(val);
@@ -1097,10 +1095,9 @@ mode_final(PG_FUNCTION_ARGS)
 		pfree(DatumGetPointer(last_val));
 
 	/*
-	 * Note: we *cannot* clean up the tuplesort object here, because the value
-	 * to be returned is allocated inside its sortcontext.  We could use
-	 * datumCopy to copy it out of there, but it doesn't seem worth the
-	 * trouble, since the cleanup callback will clear the tuplesort later.
+	 * Note: we could clean up the tuplesort object here, but it doesn't seem
+	 * worth the trouble, since the cleanup callback will clear the tuplesort
+	 * later.
 	 */
 
 	if (mode_freq)
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 4dd5407..544f06d 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -1824,6 +1824,11 @@ tuplesort_performsort(Tuplesortstate *state)
  * direction into *stup.  Returns FALSE if no more tuples.
  * If *should_free is set, the caller must pfree stup.tuple when done with it.
  * Otherwise, caller should not use tuple following next call here.
+ *
+ * Note:  Public tuplesort fetch routine callers cannot rely on tuple being
+ * allocated in their own memory context when should_free is TRUE.  It may be
+ * necessary to create a new copy of the tuple to meet the requirements of
+ * public fetch routine callers.
  */
 static bool
 tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
@@ -2046,9 +2051,11 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
  * NULL value in leading attribute will set abbreviated value to zeroed
  * representation, which caller may rely on in abbreviated inequality check.
  *
- * The slot receives a copied tuple (sometimes allocated in caller memory
- * context) that will stay valid regardless of future manipulations of the
- * tuplesort's state.
+ * The slot receives a copied tuple that will stay valid regardless of future
+ * manipulations of the tuplesort's state.  This includes calls to
+ * tuplesort_end() -- tuple will be allocated in caller's own context to make
+ * this work (this differs from similar routines for other types of
+ * tuplesorts).
  */
 bool
 tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
@@ -2065,16 +2072,30 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
 
 	if (stup.tuple)
 	{
+		void   *tuple = stup.tuple;
+
 		/* Record abbreviated key for caller */
 		if (state->sortKeys->abbrev_converter && abbrev)
 			*abbrev = stup.datum1;
 
-		if (!should_free)
-		{
-			stup.tuple = heap_copy_minimal_tuple((MinimalTuple) stup.tuple);
-			should_free = true;
-		}
-		ExecStoreMinimalTuple((MinimalTuple) stup.tuple, slot, should_free);
+		/*
+		 * Callers rely on memory being in their own memory context, which is
+		 * not guaranteed by tuplesort_gettuple_common(), even when should_free
+		 * is set to TRUE.  We must always copy here, since our interface does
+		 * not allow callers to opt into arrangement where tuple memory can go
+		 * away on the next call here, or after tuplesort_end() is called.
+		 */
+		stup.tuple = heap_copy_minimal_tuple((MinimalTuple) tuple);
+
+		/*
+		 * Free local copy eagerly.  It would be very invasive to get
+		 * tuplesort_gettuple_common() to allocate tuple in caller's context
+		 * for us, so we just do this instead.
+		 */
+		if (should_free)
+			pfree(tuple);
+
+		ExecStoreMinimalTuple((MinimalTuple) stup.tuple, slot, true);
 		return true;
 	}
 	else
@@ -2089,7 +2110,7 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
  * Returns NULL if no more tuples.  If *should_free is set, the
  * caller must pfree the returned tuple when done with it.
  * If it is not set, caller should not use tuple following next
- * call here.
+ * call here.  It's never okay to use it after tuplesort_end().
  */
 HeapTuple
 tuplesort_getheaptuple(Tuplesortstate *state, bool forward, bool *should_free)
@@ -2110,7 +2131,7 @@ tuplesort_getheaptuple(Tuplesortstate *state, bool forward, bool *should_free)
  * Returns NULL if no more tuples.  If *should_free is set, the
  * caller must pfree the returned tuple when done with it.
  * If it is not set, caller should not use tuple following next
- * call here.
+ * call here.  It's never okay to use it after tuplesort_end().
  */
 IndexTuple
 tuplesort_getindextuple(Tuplesortstate *state, bool forward,
@@ -2132,7 +2153,8 @@ tuplesort_getindextuple(Tuplesortstate *state, bool forward,
  * Returns FALSE if no more datums.
  *
  * If the Datum is pass-by-ref type, the returned value is freshly palloc'd
- * and is now owned by the caller.
+ * in caller's context, and is now owned by the caller (this differs from
+ * similar routines for other types of tuplesorts).
  *
  * Caller may optionally be passed back abbreviated value (on TRUE return
  * value) when abbreviation was used, which can be used to cheaply avoid
@@ -2154,6 +2176,8 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 		MemoryContextSwitchTo(oldcontext);
 		return false;
 	}
+	/* Copy into caller's memory context */
+	MemoryContextSwitchTo(oldcontext);
 
 	/* Record abbreviated key for caller */
 	if (state->sortKeys->abbrev_converter && abbrev)
@@ -2166,16 +2190,26 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 	}
 	else
 	{
-		/* use stup.tuple because stup.datum1 may be an abbreviation */
-
-		if (should_free)
-			*val = PointerGetDatum(stup.tuple);
-		else
-			*val = datumCopy(PointerGetDatum(stup.tuple), false, state->datumTypeLen);
+		/*
+		 * Callers rely on memory being in their own memory context, which is
+		 * not guaranteed by tuplesort_gettuple_common(), even when should_free
+		 * is set to TRUE.  We must always copy here, since our interface does
+		 * not allow callers to opt into arrangement where tuple memory can go
+		 * away on the next call here, or after tuplesort_end() is called.
+		 *
+		 * Use stup.tuple because stup.datum1 may be an abbreviation.
+		 */
+		*val = datumCopy(PointerGetDatum(stup.tuple), false, state->datumTypeLen);
 		*isNull = false;
-	}
 
-	MemoryContextSwitchTo(oldcontext);
+		/*
+		 * Free local copy eagerly.  It would be very invasive to get
+		 * tuplesort_gettuple_common() to allocate tuple in caller's context
+		 * for us, so we just do this instead.
+		 */
+		if (should_free)
+			pfree(stup.tuple);
+	}
 
 	return true;
 }
-- 
2.7.4

From f7a480807127e7b2fa5ee356981bbfc23ae0cd60 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Fri, 9 Feb 2018 14:58:08 -0800
Subject: [PATCH] Fix double free of tuple with grouping sets/tuplesort.c.

Have tuplesort_gettupleslot() copy tuple contents for slot into caller's
memory context in all cases.  This necessitates an extra copy on 9.5 and
9.6, though only for cases where an explicit copy was not already
required (mostly just TSS_SORTEDONTAPE fetches, though all
TSS_FINALMERGE fetches on 9.5).  Do the same for tuplesort_getdatum().

This fixes a crash that can occur on at least 9.5 and 9.6, where some
grouping sets queries may call tuplesort_end() at node shutdown time,
and free tuple memory that the executor/a tuple slot imagines it is
responsible for.

In the case of all other fetch routines, continue to allocate the memory
in the tuplesort-owned context (only change comments).  This is a little
inconsistent with what we now do for tuplesort_gettupleslot() and
tuplesort_getdatum(), but that's preferable to adding new copy overhead
in the backbranches where it's clearly unnecessary.  These other fetch
routines provide the weakest possible guarantees about tuple memory
lifespan from v10 on, anyway, so this actually seems more consistent
overall.

In passing, change the memory context switch point within
tuplesort_getdatum() to matchtuplesort_gettupleslot(), so that we
actually allocate memory in the right context there (caller's context),
which seems like an independent bug.  It's not clear that this can cause
a crash, but it's still wrong.

Only the latter, less important fix is required on v10 and master,
because in v10 there is already no question of not having to make a copy
in caller's context anyway (see commits e94568ec and 3856cf96).

Author: Peter Geoghegan
Reported-By: Bernd Helmle
Analyzed-By: Peter Geoghegan, Andreas Seltenreich, Bernd Helmle
Discussion: https://www.postgresql.org/message-id/1512661638.9720.34.camel@oopsware.de
Backpatch: 9.5-, where the issue is known to cause a crash.
---
 src/backend/utils/adt/orderedsetaggs.c | 21 +++++++++------------
 src/backend/utils/sort/tuplesort.c     | 19 ++++++++++---------
 2 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c
index 8502fcf..c1d50eb 100644
--- a/src/backend/utils/adt/orderedsetaggs.c
+++ b/src/backend/utils/adt/orderedsetaggs.c
@@ -458,10 +458,9 @@ percentile_disc_final(PG_FUNCTION_ARGS)
 		elog(ERROR, "missing row in percentile_disc");
 
 	/*
-	 * Note: we *cannot* clean up the tuplesort object here, because the value
-	 * to be returned is allocated inside its sortcontext.  We could use
-	 * datumCopy to copy it out of there, but it doesn't seem worth the
-	 * trouble, since the cleanup callback will clear the tuplesort later.
+	 * Note: we could clean up the tuplesort object here, but it doesn't seem
+	 * worth the trouble, since the cleanup callback will clear the tuplesort
+	 * later.
 	 */
 
 	/* We shouldn't have stored any nulls, but do the right thing anyway */
@@ -576,10 +575,9 @@ percentile_cont_final_common(FunctionCallInfo fcinfo,
 	}
 
 	/*
-	 * Note: we *cannot* clean up the tuplesort object here, because the value
-	 * to be returned may be allocated inside its sortcontext.  We could use
-	 * datumCopy to copy it out of there, but it doesn't seem worth the
-	 * trouble, since the cleanup callback will clear the tuplesort later.
+	 * Note: we could clean up the tuplesort object here, but it doesn't seem
+	 * worth the trouble, since the cleanup callback will clear the tuplesort
+	 * later.
 	 */
 
 	PG_RETURN_DATUM(val);
@@ -1098,10 +1096,9 @@ mode_final(PG_FUNCTION_ARGS)
 		pfree(DatumGetPointer(last_val));
 
 	/*
-	 * Note: we *cannot* clean up the tuplesort object here, because the value
-	 * to be returned is allocated inside its sortcontext.  We could use
-	 * datumCopy to copy it out of there, but it doesn't seem worth the
-	 * trouble, since the cleanup callback will clear the tuplesort later.
+	 * Note: we could clean up the tuplesort object here, but it doesn't seem
+	 * worth the trouble, since the cleanup callback will clear the tuplesort
+	 * later.
 	 */
 
 	if (mode_freq)
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 59cd28e..1dd568c 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -2103,11 +2103,12 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
  * representation, which caller may rely on in abbreviated inequality check.
  *
  * If copy is true, the slot receives a copied tuple that will stay valid
- * regardless of future manipulations of the tuplesort's state.  Memory is
- * owned by the caller.  If copy is false, the slot will just receive a
- * pointer to a tuple held within the tuplesort, which is more efficient, but
- * only safe for callers that are prepared to have any subsequent manipulation
- * of the tuplesort's state invalidate slot contents.
+ * regardless of future manipulations of the tuplesort's state.  This includes
+ * calls to tuplesort_end() -- tuple will be allocated in caller's own context
+ * to make this work.  If copy is false, the slot will just receive a pointer
+ * to a tuple held within the tuplesort, which is more efficient, but only safe
+ * for callers that are prepared to have any subsequent manipulation of the
+ * tuplesort's state invalidate slot contents.
  */
 bool
 tuplesort_gettupleslot(Tuplesortstate *state, bool forward, bool copy,
@@ -2185,8 +2186,8 @@ tuplesort_getindextuple(Tuplesortstate *state, bool forward)
  * Returns FALSE if no more datums.
  *
  * If the Datum is pass-by-ref type, the returned value is freshly palloc'd
- * and is now owned by the caller (this differs from similar routines for
- * other types of tuplesorts).
+ * in caller's context, and is now owned by the caller (this differs from
+ * similar routines for other types of tuplesorts).
  *
  * Caller may optionally be passed back abbreviated value (on TRUE return
  * value) when abbreviation was used, which can be used to cheaply avoid
@@ -2207,6 +2208,8 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 		MemoryContextSwitchTo(oldcontext);
 		return false;
 	}
+	/* Copy into caller's memory context */
+	MemoryContextSwitchTo(oldcontext);
 
 	/* Record abbreviated key for caller */
 	if (state->sortKeys->abbrev_converter && abbrev)
@@ -2224,8 +2227,6 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 		*isNull = false;
 	}
 
-	MemoryContextSwitchTo(oldcontext);
-
 	return true;
 }
 
-- 
2.7.4