From 1bc24055d595a136d6c6d8026a58b17706dc1543 Mon Sep 17 00:00:00 2001
From: peterbell10 <peterbell10@live.co.uk>
Date: Sat, 28 Mar 2020 10:44:44 +0000
Subject: cClientHandle: Only allow m_State to increase (#4533)

* cClientHandle: Only allow m_State to increase

* WasAddedToWorld was incorrect if kicked

* Rewrite cClient::Destroy with a guard clause
---
 src/ClientHandle.cpp | 85 +++++++++++++++++++++++++++-------------------------
 src/ClientHandle.h   |  4 +++
 2 files changed, 49 insertions(+), 40 deletions(-)

diff --git a/src/ClientHandle.cpp b/src/ClientHandle.cpp
index f0c033107..3a0eb1edd 100644
--- a/src/ClientHandle.cpp
+++ b/src/ClientHandle.cpp
@@ -154,58 +154,49 @@ void cClientHandle::Destroy(void)
 		m_Link.reset();
 	}
 
-	// Temporary (#3115-will-fix): variable to keep track of whether the client authenticated and had the opportunity to have ownership transferred to the world
-	bool WasAddedToWorld = false;
+	if (!SetState(csDestroying))
 	{
-		cCSLock Lock(m_CSState);
-		WasAddedToWorld = (m_State >= csAuthenticated);
-		if (m_State >= csDestroying)
-		{
-			// Already called
-			LOGD("%s: client %p, \"%s\" already destroyed, bailing out", __FUNCTION__, static_cast<void *>(this), m_Username.c_str());
-			return;
-		}
-		m_State = csDestroying;
+		// Already called
+		LOGD("%s: client %p, \"%s\" already destroyed, bailing out", __FUNCTION__, static_cast<void *>(this), m_Username.c_str());
+		return;
 	}
 
 	LOGD("%s: destroying client %p, \"%s\" @ %s", __FUNCTION__, static_cast<void *>(this), m_Username.c_str(), m_IPString.c_str());
 	auto player = m_Player;
 	m_Self.reset();
+	SetState(csDestroyed);  // Tick thread is allowed to call destructor async at any time after this
+
+	if (player == nullptr)
 	{
-		cCSLock lock(m_CSState);
-		m_State = csDestroyed;  // Tick thread is allowed to call destructor async at any time after this
+		return;
 	}
 
-	if (player != nullptr)
+	// Atomically decrement player count (in world or server thread)
+	cRoot::Get()->GetServer()->PlayerDestroyed();
+
+	auto world = player->GetWorld();
+	if (world != nullptr)
 	{
-		// Atomically decrement player count (in world or server thread)
-		cRoot::Get()->GetServer()->PlayerDestroyed();
+		player->StopEveryoneFromTargetingMe();
+		player->SetIsTicking(false);
 
-		auto world = player->GetWorld();
-		if (world != nullptr)
+		if (!m_PlayerPtr)
 		{
-			player->StopEveryoneFromTargetingMe();
-			player->SetIsTicking(false);
-
-			if (WasAddedToWorld)
-			{
-				// If ownership was transferred, our own smart pointer should be unset
-				ASSERT(!m_PlayerPtr);
+			// If our own smart pointer is unset, player has been transferred to world
+			ASSERT(world->IsPlayerReferencedInWorldOrChunk(*player));
 
-				m_PlayerPtr = world->RemovePlayer(*player);
+			m_PlayerPtr = world->RemovePlayer(*player);
 
-				// And RemovePlayer should have returned a valid smart pointer
-				ASSERT(m_PlayerPtr);
-			}
-			else
-			{
-				// If ownership was not transferred, our own smart pointer should be valid and RemovePlayer's should not
-				ASSERT(m_PlayerPtr);
-				ASSERT(!world->IsPlayerReferencedInWorldOrChunk(*player));
-			}
+			// And RemovePlayer should have returned a valid smart pointer
+			ASSERT(m_PlayerPtr);
+		}
+		else
+		{
+			// If ownership was not transferred, our own smart pointer should be valid and RemovePlayer's should not
+			ASSERT(!world->IsPlayerReferencedInWorldOrChunk(*player));
 		}
-		player->RemoveClientHandle();
 	}
+	player->RemoveClientHandle();
 }
 
 
@@ -433,7 +424,7 @@ void cClientHandle::FinishAuthenticate(const AString & a_Name, const cUUID & a_U
 		cRoot::Get()->BroadcastPlayerListsAddPlayer(*m_Player);
 		cRoot::Get()->SendPlayerLists(m_Player);
 
-		m_State = csAuthenticated;
+		SetState(csAuthenticated);
 	}
 
 	// Query player team
@@ -2542,7 +2533,7 @@ void cClientHandle::SendDisconnect(const AString & a_Reason)
 		// csKicked means m_Link will be shut down on the next tick. The
 		// disconnect packet data is sent in the tick thread so the connection
 		// is closed there after the data is sent.
-		m_State = csKicked;
+		SetState(csKicked);
 	}
 }
 
@@ -3339,8 +3330,7 @@ void cClientHandle::SocketClosed(void)
 	}
 
 	// Queue self for destruction:
-	cCSLock lock(m_CSState);
-	m_State = csQueuedForDestruction;
+	SetState(csQueuedForDestruction);
 }
 
 
@@ -3357,6 +3347,21 @@ void cClientHandle::SetSelf(cClientHandlePtr a_Self)
 
 
 
+bool cClientHandle::SetState(eState a_NewState)
+{
+	cCSLock Lock(m_CSState);
+	if (a_NewState < m_State)
+	{
+		return false;  // Can only advance the state machine
+	}
+	m_State = a_NewState;
+	return true;
+}
+
+
+
+
+
 void cClientHandle::ProcessProtocolInOut(void)
 {
 	// Process received network data:
diff --git a/src/ClientHandle.h b/src/ClientHandle.h
index 6dcf71a59..00f318191 100644
--- a/src/ClientHandle.h
+++ b/src/ClientHandle.h
@@ -591,6 +591,10 @@ private:
 	/** Called right after the instance is created to store its SharedPtr inside. */
 	void SetSelf(cClientHandlePtr a_Self);
 
+	/** Called to update m_State.
+	Only succeeds if a_NewState > m_State, otherwise returns false. */
+	bool SetState(eState a_NewState);
+
 	/** Processes the data in the network input and output buffers.
 	Called by both Tick() and ServerTick(). */
 	void ProcessProtocolInOut(void);
-- 
cgit v1.2.3