REL9_5_STABLE-fix-double-free-tuplesort.patch

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