Core/Networking: Fixed unsafe access to _worldSession member in WorldSocket

(cherry picked from commit bed88e0dd4)

Core/Networking: Fixed deadlock in HandlePing if the client is about to be kicked for overspeed pings

(cherry picked from commit 3da0f7e409)

Core/Networking: Cleanup CloseSocket calls from read failures in WorldSocket

(cherry picked from commit 18343a7309)

Conflicts:
	src/server/game/Server/WorldSocket.cpp

Ref #14474

src/server/game/Server/WorldSession.cpp

@@ -225,6 +225,7 @@ void WorldSession::SendPacket(WorldPacket* packet)
 
     sScriptMgr->OnPacketSend(this, *packet);
 
+    TC_LOG_TRACE("network.opcode", "S->C: %s %s", GetPlayerInfo().c_str(), GetOpcodeNameForLogging(packet->GetOpcode()).c_str());
     m_Socket->SendPacket(*packet);
 }
 

src/server/game/Server/WorldSocket.cpp

@@ -28,7 +28,7 @@
 using boost::asio::ip::tcp;
 
 WorldSocket::WorldSocket(tcp::socket&& socket)
-    : Socket(std::move(socket)), _authSeed(rand32()), _OverSpeedPings(0), _worldSession(nullptr)
+    : Socket(std::move(socket)), _authSeed(rand32()), _OverSpeedPings(0), _worldSession(nullptr), _authed(false)
 {
     _headerBuffer.Resize(sizeof(ClientPktHeader));
 }
@@ -53,7 +53,15 @@ void WorldSocket::HandleSendAuthSession()
     seed2.SetRand(16 * 8);
     packet.append(seed2.AsByteArray(16).get(), 16);               // new encryption seeds
 
-    SendPacket(packet);
+    SendPacketAndLogOpcode(packet);
+}
+
+void WorldSocket::OnClose()
+{
+    {
+        std::lock_guard<std::mutex> sessionGuard(_worldSessionLock);
+        _worldSession = nullptr;
+    }
 }
 
 void WorldSocket::ReadHandler()
@@ -80,7 +88,10 @@ void WorldSocket::ReadHandler()
 
             // We just received nice new header
             if (!ReadHeaderHandler())
+            {
+                CloseSocket();
                 return;
+            }
         }
 
         // We have full read header, now check the data payload
@@ -101,7 +112,10 @@ void WorldSocket::ReadHandler()
 
         // just received fresh new payload
         if (!ReadDataHandler())
+        {
+            CloseSocket();
             return;
+        }
 
         _headerBuffer.Reset();
     }
@@ -121,17 +135,8 @@ bool WorldSocket::ReadHeaderHandler()
 
     if (!header->IsValidSize() || !header->IsValidOpcode())
     {
-        if (_worldSession)
+        TC_LOG_ERROR("network", "WorldSocket::ReadHeaderHandler(): client %s sent malformed packet (size: %hu, cmd: %u)",
-        {
+            GetRemoteIpAddress().to_string().c_str(), header->size, header->cmd);
-            Player* player = _worldSession->GetPlayer();
-            TC_LOG_ERROR("network", "WorldSocket::ReadHeaderHandler(): client (account: %u, char [GUID: %u, name: %s]) sent malformed packet (size: %hu, cmd: %u)",
-                _worldSession->GetAccountId(), player ? player->GetGUIDLow() : 0, player ? player->GetName().c_str() : "<none>", header->size, header->cmd);
-        }
-        else
-            TC_LOG_ERROR("network", "WorldSocket::ReadHeaderHandler(): client %s sent malformed packet (size: %hu, cmd: %u)",
-                GetRemoteIpAddress().to_string().c_str(), header->size, header->cmd);
-
-        CloseSocket();
         return false;
     }
 
@@ -151,28 +156,32 @@ bool WorldSocket::ReadDataHandler()
     if (sPacketLog->CanLogPacket())
         sPacketLog->LogPacket(packet, CLIENT_TO_SERVER, GetRemoteIpAddress(), GetRemotePort());
 
-    TC_LOG_TRACE("network.opcode", "C->S: %s %s", (_worldSession ? _worldSession->GetPlayerInfo() : GetRemoteIpAddress().to_string()).c_str(), GetOpcodeNameForLogging(opcode).c_str());
+    std::unique_lock<std::mutex> sessionGuard(_worldSessionLock, std::defer_lock);
 
     switch (opcode)
     {
         case CMSG_PING:
-            HandlePing(packet);
+            LogOpcodeText(opcode, sessionGuard);
-            break;
+            return HandlePing(packet);
         case CMSG_AUTH_SESSION:
-            if (_worldSession)
+            LogOpcodeText(opcode, sessionGuard);
+            if (_authed)
             {
-                TC_LOG_ERROR("network", "WorldSocket::ProcessIncoming: received duplicate CMSG_AUTH_SESSION from %s", _worldSession->GetPlayerInfo().c_str());
+                // locking just to safely log offending user is probably overkill but we are disconnecting him anyway
-                break;
+                if (sessionGuard.try_lock())
+                    TC_LOG_ERROR("network", "WorldSocket::ProcessIncoming: received duplicate CMSG_AUTH_SESSION from %s", _worldSession->GetPlayerInfo().c_str());
+                return false;
             }
 
             HandleAuthSession(packet);
             break;
         case CMSG_KEEP_ALIVE:
-            TC_LOG_DEBUG("network", "%s", GetOpcodeNameForLogging(opcode).c_str());
+            LogOpcodeText(opcode, sessionGuard);
-            sScriptMgr->OnPacketReceive(_worldSession, packet);
             break;
         default:
         {
+            sessionGuard.lock();
+            LogOpcodeText(opcode, sessionGuard);
             if (!_worldSession)
             {
                 TC_LOG_ERROR("network.opcode", "ProcessIncoming: Client not authed opcode = %u", uint32(opcode));
@@ -193,7 +202,26 @@ bool WorldSocket::ReadDataHandler()
     return true;
 }
 
-void WorldSocket::SendPacket(WorldPacket& packet)
+void WorldSocket::LogOpcodeText(uint16 opcode, std::unique_lock<std::mutex> const& guard) const
+{
+    if (!guard)
+    {
+        TC_LOG_TRACE("network.opcode", "C->S: %s %s", GetRemoteIpAddress().to_string().c_str(), GetOpcodeNameForLogging(opcode).c_str());
+    }
+    else
+    {
+        TC_LOG_TRACE("network.opcode", "C->S: %s %s", (_worldSession ? _worldSession->GetPlayerInfo() : GetRemoteIpAddress().to_string()).c_str(),
+            GetOpcodeNameForLogging(opcode).c_str());
+    }
+}
+
+void WorldSocket::SendPacketAndLogOpcode(WorldPacket const& packet)
+{
+    TC_LOG_TRACE("network.opcode", "S->C: %s %s", GetRemoteIpAddress().to_string().c_str(), GetOpcodeNameForLogging(packet.GetOpcode()).c_str());
+    SendPacket(packet);
+}
+
+void WorldSocket::SendPacket(WorldPacket const& packet)
 {
     if (!IsOpen())
         return;
@@ -201,8 +229,6 @@ void WorldSocket::SendPacket(WorldPacket& packet)
     if (sPacketLog->CanLogPacket())
         sPacketLog->LogPacket(packet, SERVER_TO_CLIENT, GetRemoteIpAddress(), GetRemotePort());
 
-    TC_LOG_TRACE("network.opcode", "S->C: %s %s", (_worldSession ? _worldSession->GetPlayerInfo() : GetRemoteIpAddress().to_string()).c_str(), GetOpcodeNameForLogging(packet.GetOpcode()).c_str());
-
     ServerPktHeader header(packet.size() + 2, packet.GetOpcode());
 
     std::unique_lock<std::mutex> guard(_writeLock);
@@ -459,6 +485,7 @@ void WorldSocket::HandleAuthSession(WorldPacket& recvPacket)
     // At this point, we can safely hook a successful login
     sScriptMgr->OnAccountLogin(id);
 
+    _authed = true;
     _worldSession = new WorldSession(id, shared_from_this(), AccountTypes(security), expansion, mutetime, locale, recruiter, isRecruiter);
     _worldSession->LoadGlobalAccountData();
     _worldSession->LoadTutorialsData();
@@ -477,10 +504,10 @@ void WorldSocket::SendAuthResponseError(uint8 code)
     WorldPacket packet(SMSG_AUTH_RESPONSE, 1);
     packet << uint8(code);
 
-    SendPacket(packet);
+    SendPacketAndLogOpcode(packet);
 }
 
-void WorldSocket::HandlePing(WorldPacket& recvPacket)
+bool WorldSocket::HandlePing(WorldPacket& recvPacket)
 {
     uint32 ping;
     uint32 latency;
@@ -509,13 +536,14 @@ void WorldSocket::HandlePing(WorldPacket& recvPacket)
 
             if (maxAllowed && _OverSpeedPings > maxAllowed)
             {
+                std::unique_lock<std::mutex> sessionGuard(_worldSessionLock);
+
                 if (_worldSession && !_worldSession->HasPermission(rbac::RBAC_PERM_SKIP_CHECK_OVERSPEED_PING))
                 {
                     TC_LOG_ERROR("network", "WorldSocket::HandlePing: %s kicked for over-speed pings (address: %s)",
                         _worldSession->GetPlayerInfo().c_str(), GetRemoteIpAddress().to_string().c_str());
 
-                    CloseSocket();
+                    return false;
-                    return;
                 }
             }
         }
@@ -523,20 +551,23 @@ void WorldSocket::HandlePing(WorldPacket& recvPacket)
             _OverSpeedPings = 0;
     }
 
-    if (_worldSession)
     {
-        _worldSession->SetLatency(latency);
+        std::lock_guard<std::mutex> sessionGuard(_worldSessionLock);
-        _worldSession->ResetClientTimeDelay();
-    }
-    else
-    {
-        TC_LOG_ERROR("network", "WorldSocket::HandlePing: peer sent CMSG_PING, but is not authenticated or got recently kicked, address = %s", GetRemoteIpAddress().to_string().c_str());
 
-        CloseSocket();
+        if (_worldSession)
-        return;
+        {
+            _worldSession->SetLatency(latency);
+            _worldSession->ResetClientTimeDelay();
+        }
+        else
+        {
+            TC_LOG_ERROR("network", "WorldSocket::HandlePing: peer sent CMSG_PING, but is not authenticated or got recently kicked, address = %s", GetRemoteIpAddress().to_string().c_str());
+            return false;
+        }
     }
 
     WorldPacket packet(SMSG_PONG, 4);
     packet << ping;
-    return SendPacket(packet);
+    SendPacketAndLogOpcode(packet);
+    return true;
 }

src/server/game/Server/WorldSocket.h

@@ -55,19 +55,25 @@ public:
 
     void Start() override;
 
-    void SendPacket(WorldPacket& packet);
+    void SendPacket(WorldPacket const& packet);
 
 protected:
+    void OnClose() override;
     void ReadHandler() override;
     bool ReadHeaderHandler();
     bool ReadDataHandler();
 
 private:
+    /// writes network.opcode log
+    /// accessing WorldSession is not threadsafe, only do it when holding _worldSessionLock
+    void LogOpcodeText(uint16 opcode, std::unique_lock<std::mutex> const& guard) const;
+    /// sends and logs network.opcode without accessing WorldSession
+    void SendPacketAndLogOpcode(WorldPacket const& packet);
     void HandleSendAuthSession();
     void HandleAuthSession(WorldPacket& recvPacket);
     void SendAuthResponseError(uint8 code);
 
-    void HandlePing(WorldPacket& recvPacket);
+    bool HandlePing(WorldPacket& recvPacket);
 
     uint32 _authSeed;
     AuthCrypt _authCrypt;
@@ -75,7 +81,9 @@ private:
     std::chrono::steady_clock::time_point _LastPingTime;
     uint32 _OverSpeedPings;
 
+    std::mutex _worldSessionLock;
     WorldSession* _worldSession;
+    bool _authed;
 
     MessageBuffer _headerBuffer;
     MessageBuffer _packetBuffer;

src/server/shared/Networking/Socket.h

@@ -62,7 +62,7 @@ public:
             return false;
 
 #ifndef TC_SOCKET_USE_IOCP
-        std::unique_lock<std::mutex> guard(_writeLock, std::try_to_lock);
+        std::unique_lock<std::mutex> guard(_writeLock);
         if (!guard)
             return true;
 
@@ -140,6 +140,8 @@ public:
         if (shutdownError)
             TC_LOG_DEBUG("network", "Socket::CloseSocket: %s errored when shutting down socket: %i (%s)", GetRemoteIpAddress().to_string().c_str(),
                 shutdownError.value(), shutdownError.message().c_str());
+
+        OnClose();
     }
 
     /// Marks the socket for closing after write buffer becomes empty
@@ -148,6 +150,8 @@ public:
     MessageBuffer& GetReadBuffer() { return _readBuffer; }
 
 protected:
+    virtual void OnClose() { }
+
     virtual void ReadHandler() = 0;
 
     bool AsyncProcessQueue(std::unique_lock<std::mutex>&)