From 820119c85926f4a94e30f61383603ca1fcd5e555 Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Thu, 2 Feb 2023 10:06:18 +0100 Subject: [PATCH] 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 --- src/backend/distributed/connection/connection_management.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/backend/distributed/connection/connection_management.c b/src/backend/distributed/connection/connection_management.c index c5b300fd4..8ab35ca42 100644 --- a/src/backend/distributed/connection/connection_management.c +++ b/src/backend/distributed/connection/connection_management.c @@ -1454,6 +1454,9 @@ AfterXactHostConnectionHandling(ConnectionHashEntry *entry, bool isCommit) { ShutdownConnection(connection); + /* remove from transactionlist before free-ing */ + ResetRemoteTransaction(connection); + /* unlink from list */ dlist_delete(iter.cur);