REL_10_STABLE-fix-double-free-tuplesort.patch

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