10000 Fix actual and potential double-frees around tuplesort usage. · linzhihui/postgres@e4ff711 · GitHub
[go: up one dir, main page]
Skip to content

Commit e4ff711

Browse files
tglsfdctglsfdc
committed
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
1 parent d2b9c02 commit e4ff711

File tree

2 files changed

+64
-21
lines changed

2 files changed

+64
-21
lines changed

‎src/backend/utils/adt/orderedsetaggs.c

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -457,10 +457,9 @@ percentile_disc_final(PG_FUNCTION_ARGS)
457457
elog(ERROR, "missing row in percentile_disc");
458458

459459
/*
460-
* Note: we *cannot* clean up the tuplesort object here, because the value
461-
* to be returned is allocated inside its sortcontext. We could use
462-
* datumCopy to copy it out of there, but it doesn't seem worth the
463-
* trouble, since the cleanup callback will clear the tuplesort later.
460+
* Note: we could clean up the tuplesort object here, but it doesn't seem
461+
* worth the trouble, since the cleanup callback will clear the tuplesort
462+
* later.
464463
*/
465464

466465
/* We shouldn't have stored any nulls, but do the right thing anyway */
@@ -575,10 +574,9 @@ percentile_cont_final_common(FunctionCallInfo fcinfo,
575574
}
576575

577576
/*
578-
* Note: we *cannot* clean up the tuplesort object here, because the value
579-
* to be returned may be allocated inside its sortcontext. We could use
580-
* datumCopy to copy it out of there, but it doesn't seem worth the
581-
* trouble, since the cleanup callback will clear the tuplesort later.
577+
* Note: we could clean up the tuplesort object here, but it doesn't seem
578+
* worth the trouble, since the cleanup callback will clear the tuplesort
579+
* later.
582580
*/
583581

584582
PG_RETURN_DATUM(val);
@@ -1089,10 +1087,9 @@ mode_final(PG_FUNCTION_ARGS)
10891087
pfree(DatumGetPointer(last_val));
10901088

10911089
/*
1092-
* Note: we *cannot* clean up the tuplesort object here, because the value
1093-
* to be returned is allocated inside its sortcontext. We could use
1094-
* datumCopy to copy it out of there, but it doesn't seem worth the
1095-
* trouble, since the cleanup callback will clear the tuplesort later.
1090+
* Note: we could clean up the tuplesort object here, but it doesn't seem
1091+
* worth the trouble, since the cleanup callback will clear the tuplesort
1092+
* later.
10961093
*/
10971094

10981095
if (mode_freq)

‎src/backend/utils/sort/tuplesort.c

Lines changed: 55 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1664,6 +1664,12 @@ tuplesort_performsort(Tuplesortstate *state)
16641664
* Internal routine to fetch the next tuple in either forward or back
16651665
* direction into *stup. Returns FALSE if no more tuples.
16661666
* If *should_free is set, the caller must pfree stup.tuple when done with it.
1667+
* Otherwise, caller should not use tuple following next call here.
1668+
*
1669+
* Note: Public tuplesort fetch routine callers cannot rely on tuple being
1670+
* allocated in their own memory context when should_free is TRUE. It may be
1671+
* necessary to create a new copy of the tuple to meet the requirements of
1672+
* public fetch routine callers.
16671673
*/
16681674
static bool
16691675
tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
@@ -1865,6 +1871,11 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
18651871
* Fetch the next tuple in either forward or back direction.
18661872
* If successful, put tuple in slot and return TRUE; else, clear the slot
18671873
* and return FALSE.
1874+
*
1875+
* The slot receives a tuple that's been copied into the caller's memory
1876+
* context, so that it will stay valid regardless of future manipulations of
1877+
* the tuplesort's state (up to and including deleting the tuplesort).
1878+
* This differs from similar routines for other types of tuplesorts.
18681879
*/
18691880
bool
18701881
tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
@@ -1881,7 +1892,24 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
18811892

18821893
if (stup.tuple)
18831894
{
1884-
ExecStoreMinimalTuple((MinimalTuple) stup.tuple, slot, should_free);
1895+
/*
1896+
* Callers rely on tuple being in their own memory context, which is
1897+
* not guaranteed by tuplesort_gettuple_common(), even when should_free
1898+
* is set to TRUE. We must always copy here, since our interface does
1899+
* not allow callers to opt into arrangement where tuple memory can go
1900+
* away on the next call here, or after tuplesort_end() is called.
1901+
*/
1902+
ExecStoreMinimalTuple(heap_copy_minimal_tuple((MinimalTuple) stup.tuple),
1903+
slot, true);
1904+
1905+
/*
1906+
* Free local copy if needed. It would be very invasive to get
1907+
* tuplesort_gettuple_common() to allocate tuple in caller's context
1908+
* for us, so we just do this instead.
1909+
*/
1910+
if (should_free)
1911+
pfree(stup.tuple);
1912+
18851913
return true;
18861914
}
18871915
else
@@ -1895,6 +1923,8 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
18951923
* Fetch the next tuple in either forward or back direction.
18961924
* Returns NULL if no more tuples. If *should_free is set, the
18971925
* caller must pfree the returned tuple when done with it.
1926+
* If it is not set, caller should not use tuple following next
1927+
* call here. It's never okay to use it after tuplesort_end().
18981928
*/
18991929
HeapTuple
19001930
tuplesort_getheaptuple(Tuplesortstate *state, bool forward, bool *should_free)
@@ -1914,6 +1944,8 @@ tuplesort_getheaptuple(Tuplesortstate *state, bool forward, bool *should_free)
19141944
* Fetch the next index tuple in either forward or back direction.
19151945
* Returns NULL if no more tuples. If *should_free is set, the
19161946
* caller must pfree the returned tuple when done with it.
1947+
* If it is not set, caller should not use tuple following next
1948+
* call here. It's never okay to use it after tuplesort_end().
19171949
*/
19181950
IndexTuple
19191951
tuplesort_getindextuple(Tuplesortstate *state, bool forward,
@@ -1935,7 +1967,8 @@ tuplesort_getindextuple(Tuplesortstate *state, bool forward,
19351967
* Returns FALSE if no more datums.
19361968
*
19371969
* If the Datum is pass-by-ref type, the returned value is freshly palloc'd
1938-
* and is now owned by the caller.
1970+
* in caller's context, and is now owned by the caller (this differs from
1971+
* similar routines for other types of tuplesorts).
19391972
*/
19401973
bool
19411974
tuplesort_getdatum(Tuplesortstate *state, bool forward,
@@ -1951,24 +1984,37 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
19511984
return false;
19521985
}
19531986

1987+
/* Ensure we copy into caller's memory context */
1988+
MemoryContextSwitchTo(oldcontext);
1989+
19541990
if (stup.isnull1 || state->datumTypeByVal)
19551991
{
19561992
*val = stup.datum1;
19571993
*isNull = stup.isnull1;
19581994
}
19591995
else
19601996
{
1961-
/* use stup.tuple because stup.datum1 may be an abbreviation */
1997+
/*
1998+
* Callers rely on datum being in their own memory context, which is
1999+
* not guaranteed by tuplesort_gettuple_common(), even when should_free
2000+
* is set to TRUE. We must always copy here, since our interface does
2001+
* not allow callers to opt into arrangement where tuple memory can go
2002+
* away on the next call here, or after tuplesort_end() is called.
2003+
*
2004+
* Use stup.tuple because stup.datum1 may be an abbreviation.
2005+
*/
2006+
*val = datumCopy(PointerGetDatum(stup.tuple), false, state->datumTypeLen);
2007+
*isNull = false;
19622008

2009+
/*
2010+
* Free local copy if needed. It would be very invasive to get
2011+
* tuplesort_gettuple_common() to allocate tuple in caller's context
2012+
* for us, so we just do this instead.
2013+
*/
19632014
if (should_free)
1964-
*val = PointerGetDatum(stup.tuple);
1965-
else
1966-
*val = datumCopy(PointerGetDatum(stup.tuple), false, state->datumTypeLen);
1967-
*isNull = false;
2015+
pfree(stup.tuple);
19682016
}
19692017

1970-
MemoryContextSwitchTo(oldcontext);
1971-
19722018
return true;
19732019
}
19742020

0 commit comments

Comments
 (0)
0