From c480cb9d246cec5e1dd7d72956e792df16e5445d Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 15 Jan 2015 18:52:22 -0500 Subject: [PATCH] Fix use-of-already-freed-memory problem in EvalPlanQual processing. Up to now, the "child" executor state trees generated for EvalPlanQual rechecks have simply shared the ResultRelInfo arrays used for the original execution tree. However, this leads to dangling-pointer problems, because ExecInitModifyTable() is all too willing to scribble on some fields of the ResultRelInfo(s) even when it's being run in one of those child trees. This trashes those fields from the perspective of the parent tree, because even if the generated subtree is logically identical to what was in use in the parent, it's in a memory context that will go away when we're done with the child state tree. We do however want to share information in the direction from the parent down to the children; in particular, fields such as es_instrument *must* be shared or we'll lose the stats arising from execution of the children. So the simplest fix is to make a copy of the parent's ResultRelInfo array, but not copy any fields back at end of child execution. Per report from Manuel Kniep. The added isolation test is based on his example. In an unpatched memory-clobber-enabled build it will reliably fail with "ctid is NULL" errors in all branches back to 9.1, as a consequence of junkfilter->jf_junkAttNo being overwritten with $7f7f. This test cannot be run as-is before that for lack of WITH syntax; but I have no doubt that some variant of this problem can arise in older branches, so apply the code change all the way back. --- src/backend/executor/execMain.c | 24 ++++++++++++-- .../isolation/expected/eval-plan-qual.out | 32 +++++++++++++++++++ src/test/isolation/specs/eval-plan-qual.spec | 15 +++++++-- 3 files changed, 65 insertions(+), 6 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index fcc42fa0d8..73b6ebd5ec 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -2419,6 +2419,14 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree) * the snapshot, rangetable, result-rel info, and external Param info. * They need their own copies of local state, including a tuple table, * es_param_exec_vals, etc. + * + * The ResultRelInfo array management is trickier than it looks. We + * create a fresh array for the child but copy all the content from the + * parent. This is because it's okay for the child to share any + * per-relation state the parent has already created --- but if the child + * sets up any ResultRelInfo fields, such as its own junkfilter, that + * state must *not* propagate back to the parent. (For one thing, the + * pointed-to data is in a memory context that won't last long enough.) */ estate->es_direction = ForwardScanDirection; estate->es_snapshot = parentestate->es_snapshot; @@ -2427,9 +2435,19 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree) estate->es_plannedstmt = parentestate->es_plannedstmt; estate->es_junkFilter = parentestate->es_junkFilter; estate->es_output_cid = parentestate->es_output_cid; - estate->es_result_relations = parentestate->es_result_relations; - estate->es_num_result_relations = parentestate->es_num_result_relations; - estate->es_result_relation_info = parentestate->es_result_relation_info; + if (parentestate->es_num_result_relations > 0) + { + int numResultRelations = parentestate->es_num_result_relations; + ResultRelInfo *resultRelInfos; + + resultRelInfos = (ResultRelInfo *) + palloc(numResultRelations * sizeof(ResultRelInfo)); + memcpy(resultRelInfos, parentestate->es_result_relations, + numResultRelations * sizeof(ResultRelInfo)); + estate->es_result_relations = resultRelInfos; + estate->es_num_result_relations = numResultRelations; + } + /* es_result_relation_info must NOT be copied */ /* es_trig_target_relations must NOT be copied */ estate->es_rowMarks = parentestate->es_rowMarks; estate->es_top_eflags = parentestate->es_top_eflags; diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out index 0f6595fcb1..433533e611 100644 --- a/src/test/isolation/expected/eval-plan-qual.out +++ b/src/test/isolation/expected/eval-plan-qual.out @@ -72,3 +72,35 @@ c2 (0,1) 1 0 0 c3 (0,1) 2 0 0 c3 (0,4) 2 1 0 step c2: COMMIT; + +starting permutation: writep2 returningp1 c1 c2 +step writep2: UPDATE p SET b = -b WHERE a = 1 AND c = 0; +step returningp1: + WITH u AS ( UPDATE p SET b = b WHERE a > 0 RETURNING * ) + SELECT * FROM u; + +step c1: COMMIT; +step returningp1: <... completed> +a b c + +1 0 0 +1 0 1 +1 0 2 +1 -1 0 +1 1 1 +1 1 2 +1 -2 0 +1 2 1 +1 2 2 +1 -3 0 +2 0 0 +2 0 1 +2 0 2 +2 1 0 +2 1 1 +2 1 2 +2 2 0 +2 2 1 +2 2 2 +2 3 0 +step c2: COMMIT; diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec index 876e5470db..6fb2432286 100644 --- a/src/test/isolation/specs/eval-plan-qual.spec +++ b/src/test/isolation/specs/eval-plan-qual.spec @@ -39,11 +39,15 @@ step "upsert1" { INSERT INTO accounts SELECT 'savings', 500 WHERE NOT EXISTS (SELECT 1 FROM upsert); } -# tests with table p check inheritance cases, specifically a bug where -# nodeLockRows did the wrong thing when the first updated tuple was in -# a non-first child table + +# tests with table p check inheritance cases: +# readp1/writep1/readp2 tests a bug where nodeLockRows did the wrong thing +# when the first updated tuple was in a non-first child table. +# writep2/returningp1 tests a memory allocation issue + step "readp1" { SELECT tableoid::regclass, ctid, * FROM p WHERE b IN (0, 1) AND c = 0 FOR UPDATE; } step "writep1" { UPDATE p SET b = -1 WHERE a = 1 AND b = 1 AND c = 0; } +step "writep2" { UPDATE p SET b = -b WHERE a = 1 AND c = 0; } step "c1" { COMMIT; } session "s2" @@ -59,6 +63,10 @@ step "upsert2" { WHERE NOT EXISTS (SELECT 1 FROM upsert); } step "readp2" { SELECT tableoid::regclass, ctid, * FROM p WHERE b IN (0, 1) AND c = 0 FOR UPDATE; } +step "returningp1" { + WITH u AS ( UPDATE p SET b = b WHERE a > 0 RETURNING * ) + SELECT * FROM u; +} step "c2" { COMMIT; } session "s3" @@ -70,3 +78,4 @@ permutation "wx1" "wx2" "c1" "c2" "read" permutation "wy1" "wy2" "c1" "c2" "read" permutation "upsert1" "upsert2" "c1" "c2" "read" permutation "readp1" "writep1" "readp2" "c1" "c2" +permutation "writep2" "returningp1" "c1" "c2" -- 2.40.0