Broken resetting of subplan hash tables

Andreas Karlsson <andreas@proxel.se>

From: Andreas Karlsson <andreas@proxel.se>
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Andres Freund <andres@anarazel.de>, Ranier Vilela <ranier.vf@gmail.com>
Date: 2020-02-29T08:58:46Z
Lists: pgsql-hackers

Attachments

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

Commits

  1. Correctly re-use hash tables in buildSubPlanHash().

  2. Reset, not recreate, execGrouping.c style hashtables.