From ed4909ff9ee88b6b2f66034f043e544dfcfda3bd Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Sat, 16 Dec 2017 13:48:01 -0800 Subject: [PATCH] Prevent double-free in ExecEndPlan(). 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 imagines it is responsible for, and must free within ExecClearTuple(). This fix simply prevents the memory from being freed by the tts_shouldFree, which is safe but is also rather a kludge. It may be better than a more invasive fix in tuplesort.c, since no fix is really needed for v10+, because v10 changed the contract that callers have with tuple-fetch tuplesort routines to something more robust (it would be even more invasive to backpatch the v10 tuplesort.enhancements changes that make this a non-issue). In passing, change the memory context switch point within tuplesort_getdatum() to matchtuplesort_gettupleslot(). It's not clear that this can cause a crash, but it's still wrong for pass-by-value Datum tuplesorts. Note that this tuplesort_getdatum() fix *is* required for v10. 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 --- src/backend/commands/copy.c | 2 +- src/backend/executor/execMain.c | 4 ++-- src/backend/executor/execTuples.c | 34 +++++++++++++++------------------- src/backend/utils/sort/tuplesort.c | 4 ++-- src/include/executor/tuptable.h | 2 +- 5 files changed, 21 insertions(+), 25 deletions(-) diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index ca72dd1..0a636b9 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -2601,7 +2601,7 @@ CopyFrom(CopyState cstate) pfree(values); pfree(nulls); - ExecResetTupleTable(estate->es_tupleTable, false); + ExecResetTupleTable(estate->es_tupleTable); ExecCloseIndices(resultRelInfo); diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 10ccf59..d657644 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1466,7 +1466,7 @@ ExecEndPlan(PlanState *planstate, EState *estate) * the TupleTableSlots, since the containing memory context is about to go * away anyway. */ - ExecResetTupleTable(estate->es_tupleTable, false); + ExecResetTupleTable(estate->es_tupleTable); /* * close the result relation(s) if any, but hold locks until xact commit. @@ -2903,7 +2903,7 @@ EvalPlanQualEnd(EPQState *epqstate) } /* throw away the per-estate tuple table */ - ExecResetTupleTable(estate->es_tupleTable, false); + ExecResetTupleTable(estate->es_tupleTable); /* close any trigger target relations attached to this EState */ foreach(l, estate->es_trig_target_relations) diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c index 533050d..d84966f 100644 --- a/src/backend/executor/execTuples.c +++ b/src/backend/executor/execTuples.c @@ -147,14 +147,13 @@ ExecAllocTableSlot(List **tupleTable) * ExecResetTupleTable * * This releases any resources (buffer pins, tupdesc refcounts) - * held by the tuple table, and optionally releases the memory - * occupied by the tuple table data structure. - * It is expected that this routine be called by EndPlan(). + * held by the tuple table. It never releases the memory + * occupied by the tuple table data structure. It is expected + * that this routine be called by EndPlan(). * -------------------------------- */ void -ExecResetTupleTable(List *tupleTable, /* tuple table */ - bool shouldFree) /* true if we should free memory */ +ExecResetTupleTable(List *tupleTable) /* tuple table */ { ListCell *lc; @@ -165,6 +164,17 @@ ExecResetTupleTable(List *tupleTable, /* tuple table */ /* Sanity checks */ Assert(IsA(slot, TupleTableSlot)); + /* + * Kludge: force slot shouldFree policy to match caller's. + * + * This can prevent problems when executor nodes were not sufficiently + * careful about tuple lifetime, for example when tuple comes from a + * utility like a tuplesort that doesn't heed tts_shouldFree + * conventions in managing underlying memory. + */ + slot->tts_shouldFree = false; + slot->tts_shouldFreeMin = false; + /* Always release resources and reset the slot to empty */ ExecClearTuple(slot); if (slot->tts_tupleDescriptor) @@ -172,21 +182,7 @@ ExecResetTupleTable(List *tupleTable, /* tuple table */ ReleaseTupleDesc(slot->tts_tupleDescriptor); slot->tts_tupleDescriptor = NULL; } - - /* If shouldFree, release memory occupied by the slot itself */ - if (shouldFree) - { - if (slot->tts_values) - pfree(slot->tts_values); - if (slot->tts_isnull) - pfree(slot->tts_isnull); - pfree(slot); - } } - - /* If shouldFree, release the list structure */ - if (shouldFree) - list_free(tupleTable); } /* -------------------------------- diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c index 4dd5407..a6c8de4 100644 --- a/src/backend/utils/sort/tuplesort.c +++ b/src/backend/utils/sort/tuplesort.c @@ -2155,6 +2155,8 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward, return false; } + MemoryContextSwitchTo(oldcontext); + /* Record abbreviated key for caller */ if (state->sortKeys->abbrev_converter && abbrev) *abbrev = stup.datum1; @@ -2175,8 +2177,6 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward, *isNull = false; } - MemoryContextSwitchTo(oldcontext); - return true; } diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h index 5ac0b6a..39a678a 100644 --- a/src/include/executor/tuptable.h +++ b/src/include/executor/tuptable.h @@ -141,7 +141,7 @@ typedef struct TupleTableSlot /* in executor/execTuples.c */ extern TupleTableSlot *MakeTupleTableSlot(void); extern TupleTableSlot *ExecAllocTableSlot(List **tupleTable); -extern void ExecResetTupleTable(List *tupleTable, bool shouldFree); +extern void ExecResetTupleTable(List *tupleTable); extern TupleTableSlot *MakeSingleTupleTableSlot(TupleDesc tupdesc); extern void ExecDropSingleTupleTableSlot(TupleTableSlot *slot); extern void ExecSetSlotDescriptor(TupleTableSlot *slot, TupleDesc tupdesc); -- 2.7.4