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.

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index fcc42fa0d8d68630de033477448881d13d4ace9d..73b6ebd5ec240f916a4766b3ca53ea80ae09ad15 100644 (file)
--- 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 0f6595fcb1b63f0ffdc2a2d9c0eb9083fa12b3f2..433533e6117f9561356484bce352356c1cacc12a 100644 (file)
--- 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;
+ <waiting ...>
+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 876e5470dba5dfeb109f37cc567aa10572e3da9b..6fb24322863dc2ff7a81e8c027b6b5aafbe55c94 100644 (file)
--- 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"