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

@@ -154,30 +154,23 @@ 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
+	if (!SetState(csDestroying))
-	bool WasAddedToWorld = false;
-	{
-		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;
-	}
 
 	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);
+		return;
-		m_State = csDestroyed;  // Tick thread is allowed to call destructor async at any time after this
 	}
 
-	if (player != nullptr)
-	{
 	// Atomically decrement player count (in world or server thread)
 	cRoot::Get()->GetServer()->PlayerDestroyed();
 
@@ -187,10 +180,10 @@ void cClientHandle::Destroy(void)
 		player->StopEveryoneFromTargetingMe();
 		player->SetIsTicking(false);
 
-			if (WasAddedToWorld)
+		if (!m_PlayerPtr)
 		{
-				// If ownership was transferred, our own smart pointer should be unset
+			// If our own smart pointer is unset, player has been transferred to world
-				ASSERT(!m_PlayerPtr);
+			ASSERT(world->IsPlayerReferencedInWorldOrChunk(*player));
 
 			m_PlayerPtr = world->RemovePlayer(*player);
 
@@ -200,13 +193,11 @@ void cClientHandle::Destroy(void)
 		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));
 		}
 	}
 	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);
+	SetState(csQueuedForDestruction);
-	m_State = 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:

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);