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
- fix-subplan-hash-reset-v2.patch (text/x-patch) patch v2
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
-
Correctly re-use hash tables in buildSubPlanHash().
- 9eb6b652a46d 11.8 landed
- 96d783ae55b9 12.3 landed
- 58c47ccfff20 13.0 landed
-
Reset, not recreate, execGrouping.c style hashtables.
- 356687bd825e 12.0 cited