From f656b0ae966052327a6341be8dee65cfd2f395b8 Mon Sep 17 00:00:00 2001
From: Patric Stout <truebrain@openttd.org>
Date: Sun, 5 Sep 2021 18:17:39 +0200
Subject: [PATCH] Fix: use-after-free after
 ClientNetworkCoordinatorSocketHandler::CloseAllConnections() (#9534)

The function clears all stun-handlers. This causes all of those
objects to be destroyed.
A handler can have a pending connecter, which was only killed in
case CloseConnection() was called. This is never the case when
the object is destroyed. In result, the connecter could finish
and cause a use-after-free by calling into the (now deleted)
handler.
---
 src/network/network_stun.cpp | 12 +++++++++---
 src/network/network_stun.h   |  1 +
 src/network/network_turn.cpp | 12 +++++++++---
 src/network/network_turn.h   |  1 +
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/src/network/network_stun.cpp b/src/network/network_stun.cpp
index 4af9b1e9d4..8964972250 100644
--- a/src/network/network_stun.cpp
+++ b/src/network/network_stun.cpp
@@ -100,9 +100,7 @@ NetworkRecvStatus ClientNetworkStunSocketHandler::CloseConnection(bool error)
 {
 	NetworkStunSocketHandler::CloseConnection(error);
 
-	/* If our connecter is still pending, shut it down too. Otherwise the
-	 * callback of the connecter can call into us, and our object is most
-	 * likely about to be destroyed. */
+	/* Also make sure any pending connecter is killed ASAP. */
 	if (this->connecter != nullptr) {
 		this->connecter->Kill();
 		this->connecter = nullptr;
@@ -111,6 +109,14 @@ NetworkRecvStatus ClientNetworkStunSocketHandler::CloseConnection(bool error)
 	return NETWORK_RECV_STATUS_OKAY;
 }
 
+ClientNetworkStunSocketHandler::~ClientNetworkStunSocketHandler()
+{
+	if (this->connecter != nullptr) {
+		this->connecter->Kill();
+		this->connecter = nullptr;
+	}
+}
+
 /**
  * Check whether we received/can send some data from/to the STUN server and
  * when that's the case handle it appropriately.
diff --git a/src/network/network_stun.h b/src/network/network_stun.h
index 8ffbff5002..d896c991be 100644
--- a/src/network/network_stun.h
+++ b/src/network/network_stun.h
@@ -24,6 +24,7 @@ public:
 	NetworkAddress local_addr;         ///< Local addresses of the socket.
 
 	NetworkRecvStatus CloseConnection(bool error = true) override;
+	~ClientNetworkStunSocketHandler() override;
 	void SendReceive();
 
 	void Connect(const std::string &token, uint8 family);
diff --git a/src/network/network_turn.cpp b/src/network/network_turn.cpp
index e04bec47ca..ae82f3094d 100644
--- a/src/network/network_turn.cpp
+++ b/src/network/network_turn.cpp
@@ -108,9 +108,7 @@ NetworkRecvStatus ClientNetworkTurnSocketHandler::CloseConnection(bool error)
 {
 	NetworkTurnSocketHandler::CloseConnection(error);
 
-	/* If our connecter is still pending, shut it down too. Otherwise the
-	 * callback of the connecter can call into us, and our object is most
-	 * likely about to be destroyed. */
+	/* Also make sure any pending connecter is killed ASAP. */
 	if (this->connecter != nullptr) {
 		this->connecter->Kill();
 		this->connecter = nullptr;
@@ -119,6 +117,14 @@ NetworkRecvStatus ClientNetworkTurnSocketHandler::CloseConnection(bool error)
 	return NETWORK_RECV_STATUS_OKAY;
 }
 
+ClientNetworkTurnSocketHandler::~ClientNetworkTurnSocketHandler()
+{
+	if (this->connecter != nullptr) {
+		this->connecter->Kill();
+		this->connecter = nullptr;
+	}
+}
+
 /**
  * Check whether we received/can send some data from/to the TURN server and
  * when that's the case handle it appropriately
diff --git a/src/network/network_turn.h b/src/network/network_turn.h
index cc569a977d..fba25447aa 100644
--- a/src/network/network_turn.h
+++ b/src/network/network_turn.h
@@ -30,6 +30,7 @@ public:
 	ClientNetworkTurnSocketHandler(const std::string &token, uint8 tracking_number, const std::string &connection_string) : token(token), tracking_number(tracking_number), connection_string(connection_string) {}
 
 	NetworkRecvStatus CloseConnection(bool error = true) override;
+	~ClientNetworkTurnSocketHandler() override;
 	void SendReceive();
 
 	void Connect();