Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitc98f218

Browse files
committed
Fix actual and potential double-frees around tuplesort usage.
tuplesort_gettupleslot() passed back tuples allocated in the tuplesort'sown memory context, even when the caller was responsible to free them.This created a double-free hazard, because some callers might destroythe tuplesort object (via tuplesort_end) before trying to clean up thelast returned tuple. To avoid this, change the API to specify that thetuple is allocated in the caller's memory context. v10 and HEAD alreadydid things that way, but in 9.5 and 9.6 this is a live bug that candemonstrably 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 itseems excessive for a back-patched change, especially since the caseswhere an extra copy happens are less performance-critical.Likewise change tuplesort_getdatum() to return pass-by-reference Datumsin the caller's context not the tuplesort's context. There seem to beno live bugs among its callers, but clearly the same sort of situationcould happen in future.For other tuplesort fetch routines, continue to allocate the memory inthe tuplesort's context. This is a little inconsistent with what we nowdo for tuplesort_gettupleslot() and tuplesort_getdatum(), but that'spreferable to adding new copy overhead in the back branches where it'sclearly unnecessary. These other fetch routines provide the weakestpossible 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 sincethere are no known failure cases there, it seems not worth the risk.Peter Geoghegan, per report from Bernd Helmle. Reviewed by KyotaroHoriguchi; thanks also to Andreas Seltenreich for extracting aself-contained test case.Discussion:https://postgr.es/m/1512661638.9720.34.camel@oopsware.de
1 parentb69df6f commitc98f218

File tree

2 files changed

+21
-22
lines changed

2 files changed

+21
-22
lines changed

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

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

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

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

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

585583
PG_RETURN_DATUM(val);
@@ -1098,10 +1096,9 @@ mode_final(PG_FUNCTION_ARGS)
10981096
pfree(DatumGetPointer(last_val));
10991097

11001098
/*
1101-
* Note: we *cannot* clean up the tuplesort object here, because the value
1102-
* to be returned is allocated inside its sortcontext. We could use
1103-
* datumCopy to copy it out of there, but it doesn't seem worth the
1104-
* trouble, since the cleanup callback will clear the tuplesort later.
1099+
* Note: we could clean up the tuplesort object here, but it doesn't seem
1100+
* worth the trouble, since the cleanup callback will clear the tuplesort
1101+
* later.
11051102
*/
11061103

11071104
if (mode_freq)

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

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2102,12 +2102,13 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
21022102
* NULL value in leading attribute will set abbreviated value to zeroed
21032103
* representation, which caller may rely on in abbreviated inequality check.
21042104
*
2105-
* If copy is true, the slot receives a copied tuple that will stay valid
2106-
* regardless of future manipulations of the tuplesort's state. Memory is
2107-
* owned by the caller. If copy is false, the slot will just receive a
2108-
* pointer to a tuple held within the tuplesort, which is more efficient, but
2109-
* only safe for callers that are prepared to have any subsequent manipulation
2110-
* of the tuplesort's state invalidate slot contents.
2105+
* If copy is true, the slot receives a tuple that's been copied into the
2106+
* caller's memory context, so that it will stay valid regardless of future
2107+
* manipulations of the tuplesort's state (up to and including deleting the
2108+
* tuplesort). If copy is false, the slot will just receive a pointer to a
2109+
* tuple held within the tuplesort, which is more efficient, but only safe for
2110+
* callers that are prepared to have any subsequent manipulation of the
2111+
* tuplesort's state invalidate slot contents.
21112112
*/
21122113
bool
21132114
tuplesort_gettupleslot(Tuplesortstate*state,boolforward,boolcopy,
@@ -2185,8 +2186,8 @@ tuplesort_getindextuple(Tuplesortstate *state, bool forward)
21852186
* Returns FALSE if no more datums.
21862187
*
21872188
* If the Datum is pass-by-ref type, the returned value is freshly palloc'd
2188-
* and is now owned by the caller (this differs from similar routines for
2189-
* other types of tuplesorts).
2189+
*in caller's context,and is now owned by the caller (this differs from
2190+
*similar routines forother types of tuplesorts).
21902191
*
21912192
* Caller may optionally be passed back abbreviated value (on TRUE return
21922193
* value) when abbreviation was used, which can be used to cheaply avoid
@@ -2208,6 +2209,9 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
22082209
return false;
22092210
}
22102211

2212+
/* Ensure we copy into caller's memory context */
2213+
MemoryContextSwitchTo(oldcontext);
2214+
22112215
/* Record abbreviated key for caller */
22122216
if (state->sortKeys->abbrev_converter&&abbrev)
22132217
*abbrev=stup.datum1;
@@ -2224,8 +2228,6 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
22242228
*isNull= false;
22252229
}
22262230

2227-
MemoryContextSwitchTo(oldcontext);
2228-
22292231
return true;
22302232
}
22312233

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp