Also reset transactions at connection shutdown

In #6314 I refactored the connection cleanup to be simpler to
understand and use. However, by doing so I introduced a use-after-free
possibility (that valgrind luckily picked up):

In the `ShouldShutdownConnection` path of `AfterXactHostConnectionHandling`
we free connections without removing the `transactionNode` from the
dlist that it might be part of. Before the refactoring this wasn't a
problem, because the dlist would be completely reset quickly after in
`ResetGlobalVariables` (without reading or writing the dlist entries).

The refactoring changed this by moving the `dlist_delete` call to
`ResetRemoteTransaction`, which in turn was called in the
`!ShouldShutdownConnection` path of `AfterXactHostConnectionHandling`.
Thus this `!ShouldShutdownConnection` path would now delete from the
`dlist`, but the `ShouldShutdownConnection` path would not. Thus to
remove itself the deleting path would sometimes update nodes in the list
that were freed right before.

There's two ways of fixing this:
1. Call `dlist_delete` from **both** of paths.
2. Call `dlist_delete` from **neither** of the paths.

This commit implements the second approach, and #6684 implements the
first. We need to choose which approach we prefer.

Sidenote: This bug already existed in 11.1, but valgrind didn't catch it
back then. My guess is that this happened because #6314 was merged after
the initial release branch was cut.

Fixes #6638
fix-valgrind-problem-v2-test
Jelte Fennema 2023-02-02 10:06:18 +01:00
parent 8a9bb272e4
commit 820119c859
1 changed files with 3 additions and 0 deletions

View File

@ -1454,6 +1454,9 @@ AfterXactHostConnectionHandling(ConnectionHashEntry *entry, bool isCommit)
{ {
ShutdownConnection(connection); ShutdownConnection(connection);
/* remove from transactionlist before free-ing */
ResetRemoteTransaction(connection);
/* unlink from list */ /* unlink from list */
dlist_delete(iter.cur); dlist_delete(iter.cur);