Hi,
I looked again at one of the potential issues Ranier Vilela's static
analysis found and after looking more at it I still think this one is a
real bug. But my original patch was incorrect and introduced a use after
free bug.
The code for resetting the hash tables of the SubPlanState node in
buildSubPlanHash() looks like it can never run, and additionally it
would be broken if it would ever run. This issue was introduced in
commit 356687bd825e5ca7230d43c1bffe7a59ad2e77bd.
As far as I gather the following is true:
1. It sets node->hashtable and node->hashnulls to NULL a few lines
before checking if they are not NULL which means the code for resetting
them cannot ever be reached.
2. But if we changed to code so that the ResetTupleHashTable() calls are
reachable we would hit a typo. It resets node->hashtable twice and never
resets node->hashnulls which would cause non-obvious bugs.
3. Additionally since the memory context used by the hash tables is
reset in buildSubPlanHash() if we start resetting hash tables we will
get a use after free bug.
I have attached a patch which makes sure the code for resetting the hash
tables is actually run while also fixing the code for resetting them.
Since the current behavior of the code in HEAD is not actually broken,
it is just an optimization which is not used, this fix does not have to
be backpatched.
Andreas
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index ff95317879..4d7306ff06 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -494,9 +494,6 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
* If it's not necessary to distinguish FALSE and UNKNOWN, then we don't
* need to store subplan output rows that contain NULL.
*/
- MemoryContextReset(node->hashtablecxt);
- node->hashtable = NULL;
- node->hashnulls = NULL;
node->havehashrows = false;
node->havenullrows = false;
@@ -533,7 +530,7 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
}
if (node->hashnulls)
- ResetTupleHashTable(node->hashtable);
+ ResetTupleHashTable(node->hashnulls);
else
node->hashnulls = BuildTupleHashTableExt(node->parent,
node->descRight,
@@ -549,6 +546,8 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
node->hashtempcxt,
false);
}
+ else
+ node->hashnulls = NULL;
/*
* We are probably in a short-lived expression-evaluation context. Switch
