Fix actual and potential double-frees around tuplesort usage.

tuplesort_gettupleslot() passed back tuples allocated in the tuplesort's
own memory context, even when the caller was responsible to free them.
This created a double-free hazard, because some callers might destroy
the tuplesort object (via tuplesort_end) before trying to clean up the
last returned tuple.  To avoid this, change the API to specify that the
tuple is allocated in the caller's memory context.  v10 and HEAD already
did things that way, but in 9.5 and 9.6 this is a live bug that can
demonstrably cause crashes with some grouping-set usages.

In 9.5 and 9.6, this requires doing an extra tuple copy in some cases,
which is unfortunate.  But the amount of refactoring needed to avoid it
seems excessive for a back-patched change, especially since the cases
where an extra copy happens are less performance-critical.

Likewise change tuplesort_getdatum() to return pass-by-reference Datums
in the caller's context not the tuplesort's context.  There seem to be
no live bugs among its callers, but clearly the same sort of situation
could happen in future.

For other tuplesort fetch routines, continue to allocate the memory in
the tuplesort's context.  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 back branches 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.

Adjust relevant comments to reflect these API redefinitions.

Arguably, we should change the pre-9.5 branches as well, but since
there are no known failure cases there, it seems not worth the risk.

Peter Geoghegan, per report from Bernd Helmle.  Reviewed by Kyotaro
Horiguchi; thanks also to Andreas Seltenreich for extracting a
self-contained test case.

Discussion: https://postgr.es/m/1512661638.9720.34.camel@oopsware.de

diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c
index 50b34fcbc68b58e1dd20d6cee72dd79fe68b566d..ed36851fddf433430852a6edb203d78c44a1d40c 100644 (file)
--- 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 041bdc2fa7e0b85f531a1502dcb9e6c1dac426ba..e433faad86af1cb428bcb7e401c64bf9e1c1810c 100644 (file)
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -2147,12 +2147,13 @@ 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.
  *
- * 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.
+ * If copy is true, the slot receives a tuple that's been copied into the
+ * caller's memory context, so that it will stay valid regardless of future
+ * manipulations of the tuplesort's state (up to and including deleting the
+ * tuplesort).  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
@@ -2253,6 +2254,9 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
                return false;
        }
 
+       /* Ensure we copy into caller's memory context */
+       MemoryContextSwitchTo(oldcontext);
+
        /* Record abbreviated key for caller */
        if (state->sortKeys->abbrev_converter && abbrev)
                *abbrev = stup.datum1;
@@ -2269,8 +2273,6 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
                *isNull = false;
        }
 
-       MemoryContextSwitchTo(oldcontext);
-
        return true;
 }