From 20229fc07bc65a90955fd52d7cd0ac8f7405a91c Mon Sep 17 00:00:00 2001 From: itsRevela Date: Fri, 10 Apr 2026 01:12:59 -0500 Subject: [PATCH] fix: dedicated server thread safety, disconnect deadlock, and console freeze - Protect PlayerList and ServerConnection players vectors with critical sections; all iterations use copy-on-read snapshots to prevent iterator invalidation during concurrent join/leave - Add null check on player bounding box in movement validation to prevent crash when player is removed mid-tick - Re-validate socket player pointer immediately before SendData to narrow the TOCTOU race window on disconnect - Replace inline disconnect cleanup with a queued system drained on the main tick thread, eliminating the done_cs -> m_playersCS lock inversion that caused deadlocks under load - Disable Windows QuickEdit mode at server startup to prevent console input selection from freezing the process - Move chunk priority sort behind ServerConnection::sortPlayersByChunkPriority() to keep the players vector lock-protected --- Minecraft.Client/MinecraftServer.cpp | 31 +--- Minecraft.Client/PlayerConnection.cpp | 59 ++---- Minecraft.Client/PlayerList.cpp | 208 +++++++++++++++++----- Minecraft.Client/PlayerList.h | 16 ++ Minecraft.Client/ServerConnection.cpp | 100 +++++++++-- Minecraft.Client/ServerConnection.h | 3 + Minecraft.Server/Windows64/ServerMain.cpp | 13 ++ Minecraft.World/Socket.cpp | 29 +-- README.md | 8 + 9 files changed, 309 insertions(+), 158 deletions(-) diff --git a/Minecraft.Client/MinecraftServer.cpp b/Minecraft.Client/MinecraftServer.cpp index 8594ee75..4406cdab 100644 --- a/Minecraft.Client/MinecraftServer.cpp +++ b/Minecraft.Client/MinecraftServer.cpp @@ -2401,36 +2401,7 @@ void MinecraftServer::chunkPacketManagement_PreTick() s_tickStartTime = System::currentTimeMillis(); s_sentTo.clear(); - vector< shared_ptr > *players = connection->getPlayers(); - - if( players->size() ) - { - vector< shared_ptr > playersOrig = *players; - players->clear(); - - do - { - int longestTime = 0; - auto playerConnectionBest = playersOrig.begin(); - for( auto it = playersOrig.begin(); it != playersOrig.end(); it++) - { - int thisTime = 0; - INetworkPlayer *np = (*it)->getNetworkPlayer(); - if( np ) - { - thisTime = np->GetTimeSinceLastChunkPacket_ms(); - } - - if( thisTime > longestTime ) - { - playerConnectionBest = it; - longestTime = thisTime; - } - } - players->push_back(*playerConnectionBest); - playersOrig.erase(playerConnectionBest); - } while ( playersOrig.size() > 0 ); - } + connection->sortPlayersByChunkPriority(); } void MinecraftServer::chunkPacketManagement_PostTick() diff --git a/Minecraft.Client/PlayerConnection.cpp b/Minecraft.Client/PlayerConnection.cpp index a436c187..bbe9e97d 100644 --- a/Minecraft.Client/PlayerConnection.cpp +++ b/Minecraft.Client/PlayerConnection.cpp @@ -205,31 +205,14 @@ void PlayerConnection::disconnect(DisconnectPacket::eDisconnectReason reason) app.DebugPrintf("PlayerConnection disconect reason: %d\n", reason ); player->disconnect(); - // 4J Stu - Need to remove the player from the receiving list before their socket is NULLed so that we can find another player on their system - server->getPlayers()->removePlayerFromReceiving( player ); - send(std::make_shared(reason)); - connection->sendAndQuit(); - // 4J-PB - removed, since it needs to be localised in the language the client is in - //server->players->broadcastAll( shared_ptr( new ChatPacket(L"�e" + player->name + L" left the game.") ) ); - if (!kickLeaveMessage.empty()) - { - server->getPlayers()->broadcastAll(std::make_shared(kickLeaveMessage)); - } - else if (!fourKitHandledQuit) - { - if(getWasKicked()) - { - server->getPlayers()->broadcastAll(std::make_shared(player->name, ChatPacket::e_ChatPlayerKickedFromGame)); - } - else - { - server->getPlayers()->broadcastAll(std::make_shared(player->name, ChatPacket::e_ChatPlayerLeftGame)); - } - } - - server->getPlayers()->remove(player); + // Mark done and release the lock BEFORE the heavy PlayerList work. + // The actual removal, broadcast, and socket teardown are queued for + // the next tick, which processes them without holding done_cs. done = true; LeaveCriticalSection(&done_cs); + + server->getPlayers()->queueDisconnect(player, static_cast(reason), + kickLeaveMessage, getWasKicked(), fourKitHandledQuit); } void PlayerConnection::handlePlayerInput(shared_ptr packet) @@ -404,6 +387,7 @@ void PlayerConnection::handleMovePlayer(shared_ptr packet) #endif float r = 1 / 16.0f; + if (player->bb == nullptr) return; bool oldOk = level->getCubes(player, player->bb->copy()->shrink(r, r, r))->empty(); if (player->onGround && !packet->onGround && yDist > 0) @@ -451,13 +435,19 @@ void PlayerConnection::handleMovePlayer(shared_ptr packet) } player->absMoveTo(xt, yt, zt, yRotT, xRotT); - bool newOk = level->getCubes(player, player->bb->copy()->shrink(r, r, r))->empty(); + AABB *playerBB = player->bb; + if (playerBB == nullptr) + { + teleport(xLastOk, yLastOk, zLastOk, yRotT, xRotT); + return; + } + bool newOk = level->getCubes(player, playerBB->copy()->shrink(r, r, r))->empty(); if (oldOk && (fail || !newOk) && !player->isSleeping()) { teleport(xLastOk, yLastOk, zLastOk, yRotT, xRotT); return; } - AABB *testBox = player->bb->copy()->grow(r, r, r)->expand(0, -0.55, 0); + AABB *testBox = playerBB->copy()->grow(r, r, r)->expand(0, -0.55, 0); // && server.level.getCubes(player, testBox).size() == 0 if (!server->isFlightAllowed() && !player->gameMode->isCreative() && !level->containsAnyBlocks(testBox) && !player->isAllowedToFly() ) { @@ -843,23 +833,12 @@ void PlayerConnection::onDisconnect(DisconnectPacket::eDisconnectReason reason, false); fourKitHandledQuit = FourKitBridge::FirePlayerQuit(player->entityId); #endif - // logger.info(player.name + " lost connection: " + reason); - // 4J-PB - removed, since it needs to be localised in the language the client is in - //server->players->broadcastAll( shared_ptr( new ChatPacket(L"�e" + player->name + L" left the game.") ) ); - if (!fourKitHandledQuit) - { - if(getWasKicked()) - { - server->getPlayers()->broadcastAll(std::make_shared(player->name, ChatPacket::e_ChatPlayerKickedFromGame)); - } - else - { - server->getPlayers()->broadcastAll(std::make_shared(player->name, ChatPacket::e_ChatPlayerLeftGame)); - } - } - server->getPlayers()->remove(player); + done = true; LeaveCriticalSection(&done_cs); + + server->getPlayers()->queueDisconnect(player, static_cast(reason), + L"", getWasKicked(), fourKitHandledQuit); } void PlayerConnection::openSecurityGate() diff --git a/Minecraft.Client/PlayerList.cpp b/Minecraft.Client/PlayerList.cpp index e1ac879c..c79c0cfe 100644 --- a/Minecraft.Client/PlayerList.cpp +++ b/Minecraft.Client/PlayerList.cpp @@ -77,6 +77,8 @@ PlayerList::PlayerList(MinecraftServer *server) int rawMax = server->settings->getInt(L"max-players", 8); maxPlayers = static_cast(Mth::clamp(rawMax, 1, MINECRAFT_NET_MAX_PLAYERS)); doWhiteList = false; + InitializeCriticalSection(&m_playersCS); + InitializeCriticalSection(&m_disconnectCS); InitializeCriticalSection(&m_banCS); InitializeCriticalSection(&m_kickPlayersCS); InitializeCriticalSection(&m_closePlayersCS); @@ -91,11 +93,34 @@ PlayerList::~PlayerList() player->gameMode = nullptr; } + DeleteCriticalSection(&m_playersCS); + DeleteCriticalSection(&m_disconnectCS); DeleteCriticalSection(&m_banCS); DeleteCriticalSection(&m_kickPlayersCS); DeleteCriticalSection(&m_closePlayersCS); } +vector > PlayerList::getPlayersSnapshot() +{ + EnterCriticalSection(&m_playersCS); + vector > snapshot = players; + LeaveCriticalSection(&m_playersCS); + return snapshot; +} + +void PlayerList::queueDisconnect(shared_ptr player, int reason, const wstring& kickMessage, bool wasKicked, bool fourKitHandledQuit) +{ + PendingDisconnect pd; + pd.player = player; + pd.reason = reason; + pd.kickMessage = kickMessage; + pd.wasKicked = wasKicked; + pd.fourKitHandledQuit = fourKitHandledQuit; + EnterCriticalSection(&m_disconnectCS); + m_pendingDisconnects.push_back(pd); + LeaveCriticalSection(&m_disconnectCS); +} + bool PlayerList::placeNewPlayer(Connection *connection, shared_ptr player, shared_ptr packet) { CompoundTag *playerTag = load(player); @@ -531,7 +556,9 @@ void PlayerList::add(shared_ptr player) broadcastAll(std::make_shared(player)); } + EnterCriticalSection(&m_playersCS); players.push_back(player); + LeaveCriticalSection(&m_playersCS); // 4J Added addPlayerToReceiving(player); @@ -548,13 +575,16 @@ void PlayerList::add(shared_ptr player) changeDimension(player, nullptr); level->addEntity(player); - for (size_t i = 0; i < players.size(); i++) { - shared_ptr op = players.at(i); - //player->connection->send(shared_ptr( new PlayerInfoPacket(op->name, true, op->latency) ) ); - if( op->connection->getNetworkPlayer() ) + vector > snapshot = getPlayersSnapshot(); + for (size_t i = 0; i < snapshot.size(); i++) { - player->connection->send(std::make_shared(op)); + shared_ptr op = snapshot.at(i); + //player->connection->send(shared_ptr( new PlayerInfoPacket(op->name, true, op->latency) ) ); + if( op->connection->getNetworkPlayer() ) + { + player->connection->send(std::make_shared(op)); + } } } @@ -580,9 +610,10 @@ void PlayerList::add(shared_ptr player) broadcastAll(std::make_shared(player, xuid, onlineXuid, xp, yp, zp, yRotp, xRotp, yHeadRotp)); // Send all existing players to the new player - for (size_t i = 0; i < players.size(); i++) + vector > snapshot = getPlayersSnapshot(); + for (size_t i = 0; i < snapshot.size(); i++) { - shared_ptr op = players.at(i); + shared_ptr op = snapshot.at(i); if (op != player && op->connection->getNetworkPlayer()) { PlayerUID opXuid = INVALID_XUID; @@ -605,9 +636,10 @@ void PlayerList::add(shared_ptr player) if(level->isAtLeastOnePlayerSleeping()) { shared_ptr firstSleepingPlayer = nullptr; - for (unsigned int i = 0; i < players.size(); i++) + vector > snapshot = getPlayersSnapshot(); + for (unsigned int i = 0; i < snapshot.size(); i++) { - shared_ptr thisPlayer = players[i]; + shared_ptr thisPlayer = snapshot[i]; if(thisPlayer->isSleeping()) { if(firstSleepingPlayer == nullptr) firstSleepingPlayer = thisPlayer; @@ -655,11 +687,13 @@ if (player->riding != nullptr) level->getTracker()->removeEntity(player); level->removeEntity(player); level->getChunkMap()->remove(player); + EnterCriticalSection(&m_playersCS); auto it = find(players.begin(), players.end(), player); if( it != players.end() ) { players.erase(it); } + LeaveCriticalSection(&m_playersCS); // Notify fork clients that this player has left the server so they can // clean up IQNet/Tab list entries. Uses a custom payload channel so the // wire format of existing packets is unchanged (upstream clients simply @@ -781,11 +815,13 @@ shared_ptr PlayerList::respawn(shared_ptr serverPlay } serverPlayer->getLevel()->getChunkMap()->remove(serverPlayer); + EnterCriticalSection(&m_playersCS); auto it = find(players.begin(), players.end(), serverPlayer); if( it != players.end() ) { players.erase(it); } + LeaveCriticalSection(&m_playersCS); server->getLevel(serverPlayer->dimension)->removeEntityImmediately(serverPlayer); Pos *bedPosition = serverPlayer->getRespawnPosition(); @@ -899,7 +935,9 @@ shared_ptr PlayerList::respawn(shared_ptr serverPlay level->getChunkMap()->add(player); level->addEntity(player); + EnterCriticalSection(&m_playersCS); players.push_back(player); + LeaveCriticalSection(&m_playersCS); player->initMenu(); player->setHealth(player->getHealth()); @@ -1135,10 +1173,11 @@ void PlayerList::tick() } // Report cipher completion and open security gate for all completed handshakes + vector > tickSnapshot = getPlayersSnapshot(); for (unsigned char smallId : completed) { // Open the security gate -- flush buffered game packets now that cipher is active - for (auto &p : players) + for (auto &p : tickSnapshot) { if (p == nullptr || p->connection == nullptr) continue; INetworkPlayer *np = p->connection->getNetworkPlayer(); @@ -1168,7 +1207,7 @@ void PlayerList::tick() for (unsigned char smallId : completed) { // Find the player by smallId - for (auto &p : players) + for (auto &p : tickSnapshot) { if (p == nullptr || p->connection == nullptr) continue; INetworkPlayer *np = p->connection->getNetworkPlayer(); @@ -1212,7 +1251,7 @@ void PlayerList::tick() } // Enforce identity token response timeout - for (auto &p : players) + for (auto &p : tickSnapshot) { if (p == nullptr || p->connection == nullptr) continue; int challengeTick = p->connection->getIdentityChallengeTick(); @@ -1244,27 +1283,78 @@ void PlayerList::tick() sendAllPlayerInfoIn = 0; } - if (sendAllPlayerInfoIn < players.size()) { - shared_ptr op = players[sendAllPlayerInfoIn]; + vector > infoSnapshot = getPlayersSnapshot(); + if (sendAllPlayerInfoIn < infoSnapshot.size()) + { + shared_ptr op = infoSnapshot[sendAllPlayerInfoIn]; //broadcastAll(shared_ptr( new PlayerInfoPacket(op->name, true, op->latency) ) ); if( op->connection->getNetworkPlayer() ) { broadcastAll(std::make_shared(op)); } } + } - EnterCriticalSection(&m_closePlayersCS); - while(!m_smallIdsToClose.empty()) + // Drain the pending disconnect queue. disconnect() enqueues here so it + // can release done_cs before the heavy cleanup runs on the tick thread. { - BYTE smallId = m_smallIdsToClose.front(); - m_smallIdsToClose.pop_front(); + std::deque dcCopy; + EnterCriticalSection(&m_disconnectCS); + dcCopy.swap(m_pendingDisconnects); + LeaveCriticalSection(&m_disconnectCS); + + while (!dcCopy.empty()) + { + PendingDisconnect pd = dcCopy.front(); + dcCopy.pop_front(); + + server->getPlayers()->removePlayerFromReceiving(pd.player); + if (pd.player->connection != nullptr) + { + pd.player->connection->send(std::make_shared(static_cast(pd.reason))); + pd.player->connection->connection->sendAndQuit(); + } + + if (!pd.kickMessage.empty()) + { + broadcastAll(std::make_shared(pd.kickMessage)); + } + else if (!pd.fourKitHandledQuit) + { + if (pd.wasKicked) + { + broadcastAll(std::make_shared(pd.player->name, ChatPacket::e_ChatPlayerKickedFromGame)); + } + else + { + broadcastAll(std::make_shared(pd.player->name, ChatPacket::e_ChatPlayerLeftGame)); + } + } + + remove(pd.player); + } + } + + // Drain the close queue: snapshot the deque, then release the CS before + // calling disconnect() which may itself try to acquire other locks. + std::deque closeCopy; + EnterCriticalSection(&m_closePlayersCS); + closeCopy.swap(m_smallIdsToClose); + LeaveCriticalSection(&m_closePlayersCS); + + { + vector > closeSnapshot = getPlayersSnapshot(); + while(!closeCopy.empty()) + { + BYTE smallId = closeCopy.front(); + closeCopy.pop_front(); shared_ptr player = nullptr; - for(unsigned int i = 0; i < players.size(); i++) + for(unsigned int i = 0; i < closeSnapshot.size(); i++) { - shared_ptr p = players.at(i); + shared_ptr p = closeSnapshot.at(i); // 4J Stu - May be being a bit overprotective with all the nullptr checks, but adding late in TU7 so want to be safe if (p != nullptr && p->connection != nullptr && p->connection->connection != nullptr && p->connection->connection->getSocket() != nullptr && p->connection->connection->getSocket()->getSmallId() == smallId ) { @@ -1286,13 +1376,19 @@ void PlayerList::tick() WinsockNetLayer::ClearSocketForSmallId(smallId); #endif } - LeaveCriticalSection(&m_closePlayersCS); + } + std::deque kickCopy; EnterCriticalSection(&m_kickPlayersCS); - while(!m_smallIdsToKick.empty()) + kickCopy.swap(m_smallIdsToKick); + LeaveCriticalSection(&m_kickPlayersCS); + { - BYTE smallId = m_smallIdsToKick.front(); - m_smallIdsToKick.pop_front(); + vector > kickSnapshot = getPlayersSnapshot(); + while(!kickCopy.empty()) + { + BYTE smallId = kickCopy.front(); + kickCopy.pop_front(); INetworkPlayer *selectedPlayer = g_NetworkManager.GetPlayerBySmallId(smallId); if( selectedPlayer != nullptr ) { @@ -1303,9 +1399,9 @@ void PlayerList::tick() // Kick this player from the game shared_ptr player = nullptr; - for(unsigned int i = 0; i < players.size(); i++) + for(unsigned int i = 0; i < kickSnapshot.size(); i++) { - shared_ptr p = players.at(i); + shared_ptr p = kickSnapshot.at(i); PlayerUID playersXuid = p->getOnlineXuid(); if (p != nullptr && ProfileManager.AreXUIDSEqual(playersXuid, xuid ) ) { @@ -1326,7 +1422,7 @@ void PlayerList::tick() } } } - LeaveCriticalSection(&m_kickPlayersCS); + } // Check our receiving players, and if they are dead see if we can replace them for(unsigned int dim = 0; dim < 2; ++dim) @@ -1360,29 +1456,32 @@ void PlayerList::prioritiseTileChanges(int x, int y, int z, int dimension) void PlayerList::broadcastAll(shared_ptr packet) { - for (unsigned int i = 0; i < players.size(); i++) + vector > snapshot = getPlayersSnapshot(); + for (unsigned int i = 0; i < snapshot.size(); i++) { - shared_ptr player = players[i]; + shared_ptr player = snapshot[i]; player->connection->send(packet); } } void PlayerList::broadcastAll(shared_ptr packet, int dimension) { - for (unsigned int i = 0; i < players.size(); i++) + vector > snapshot = getPlayersSnapshot(); + for (unsigned int i = 0; i < snapshot.size(); i++) { - shared_ptr player = players[i]; + shared_ptr player = snapshot[i]; if (player->dimension == dimension) player->connection->send(packet); } } wstring PlayerList::getPlayerNames() { + vector > snapshot = getPlayersSnapshot(); wstring msg; - for (unsigned int i = 0; i < players.size(); i++) + for (unsigned int i = 0; i < snapshot.size(); i++) { if (i > 0) msg += L", "; - msg += players[i]->name; + msg += snapshot[i]->name; } return msg; } @@ -1410,9 +1509,10 @@ bool PlayerList::isOp(shared_ptr player) shared_ptr PlayerList::getPlayer(const wstring& name) { - for (unsigned int i = 0; i < players.size(); i++) + vector > snapshot = getPlayersSnapshot(); + for (unsigned int i = 0; i < snapshot.size(); i++) { - shared_ptr p = players[i]; + shared_ptr p = snapshot[i]; if (p->name == name) // 4J - used to be case insensitive (using equalsIgnoreCase) - imagine we'll be shifting to XUIDs anyway { return p; @@ -1424,9 +1524,10 @@ shared_ptr PlayerList::getPlayer(const wstring& name) // 4J Added shared_ptr PlayerList::getPlayer(PlayerUID uid) { - for (unsigned int i = 0; i < players.size(); i++) + vector > snapshot = getPlayersSnapshot(); + for (unsigned int i = 0; i < snapshot.size(); i++) { - shared_ptr p = players[i]; + shared_ptr p = snapshot[i]; if (p->getXuid() == uid || p->getOnlineXuid() == uid) // 4J - used to be case insensitive (using equalsIgnoreCase) - imagine we'll be shifting to XUIDs anyway { return p; @@ -1437,15 +1538,16 @@ shared_ptr PlayerList::getPlayer(PlayerUID uid) shared_ptr PlayerList::getNearestPlayer(Pos *position, int range) { - if (players.empty()) return nullptr; - if (position == nullptr) return players.at(0); + vector > snapshot = getPlayersSnapshot(); + if (snapshot.empty()) return nullptr; + if (position == nullptr) return snapshot.at(0); shared_ptr current = nullptr; double dist = -1; int rangeSqr = range * range; - for (size_t i = 0; i < players.size(); i++) + for (size_t i = 0; i < snapshot.size(); i++) { - shared_ptr next = players.at(i); + shared_ptr next = snapshot.at(i); double newDist = position->distSqr(next->getCommandSenderWorldPosition()); if ((dist == -1 || newDist < dist) && (range <= 0 || newDist <= rangeSqr)) @@ -1566,9 +1668,10 @@ void PlayerList::broadcast(shared_ptr except, double x, double y, double sentTo.push_back(dynamic_pointer_cast(except)); } - for (unsigned int i = 0; i < players.size(); i++) + vector > snapshot = getPlayersSnapshot(); + for (unsigned int i = 0; i < snapshot.size(); i++) { - shared_ptr p = players[i]; + shared_ptr p = snapshot[i]; if (p == except) continue; if (p->dimension != dimension) continue; @@ -1630,14 +1733,15 @@ void PlayerList::saveAll(ProgressListener *progressListener, bool bDeleteGuestMa if(playerIo) { playerIo->saveAllCachedData(); - for (unsigned int i = 0; i < players.size(); i++) + vector > snapshot = getPlayersSnapshot(); + for (unsigned int i = 0; i < snapshot.size(); i++) { - playerIo->save(players[i]); + playerIo->save(snapshot[i]); //4J Stu - We don't want to save the map data for guests, so when we are sure that the player is gone delete the map - if(bDeleteGuestMaps && players[i]->isGuest()) playerIo->deleteMapFilesForPlayer(players[i]); + if(bDeleteGuestMaps && snapshot[i]->isGuest()) playerIo->deleteMapFilesForPlayer(snapshot[i]); - if(progressListener != nullptr) progressListener->progressStagePercentage((i * 100)/ static_cast(players.size())); + if(progressListener != nullptr) progressListener->progressStagePercentage((i * 100)/ static_cast(snapshot.size())); } playerIo->clearOldPlayerFiles(); playerIo->saveMapIdLookup(); @@ -1794,9 +1898,15 @@ void PlayerList::removePlayerFromReceiving(shared_ptr player, bool } INetworkPlayer *thisPlayer = player->connection->getNetworkPlayer(); + + // Snapshot the players vector to avoid iterator invalidation during concurrent modifications + EnterCriticalSection(&m_playersCS); + vector > playersSnapshot = players; + LeaveCriticalSection(&m_playersCS); + if( thisPlayer && playerRemoved ) { - for(auto& newPlayer : players) + for(auto& newPlayer : playersSnapshot) { INetworkPlayer *otherPlayer = newPlayer->connection->getNetworkPlayer(); @@ -1821,7 +1931,7 @@ void PlayerList::removePlayerFromReceiving(shared_ptr player, bool #endif // 4J Stu - Something went wrong, or possibly the QNet player left before we got here. // Re-check all active players and make sure they have someone on their system to receive all packets - for(auto& newPlayer : players) + for(auto& newPlayer : playersSnapshot) { INetworkPlayer *checkingPlayer = newPlayer->connection->getNetworkPlayer(); diff --git a/Minecraft.Client/PlayerList.h b/Minecraft.Client/PlayerList.h index 2cafe77e..1265ad24 100644 --- a/Minecraft.Client/PlayerList.h +++ b/Minecraft.Client/PlayerList.h @@ -24,11 +24,26 @@ private: // public static Logger logger = Logger.getLogger("Minecraft"); public: vector > players; + CRITICAL_SECTION m_playersCS; // Protects players vector for concurrent access + vector > getPlayersSnapshot(); private: MinecraftServer *server; unsigned int maxPlayers; + // Pending disconnect queue: disconnect() enqueues here, tick() drains it. + // This avoids holding done_cs across PlayerList operations (deadlock fix). + struct PendingDisconnect + { + shared_ptr player; + int reason; + wstring kickMessage; + bool wasKicked; + bool fourKitHandledQuit; + }; + deque m_pendingDisconnects; + CRITICAL_SECTION m_disconnectCS; + // 4J Added vector m_bannedXuids; CRITICAL_SECTION m_banCS; // 4J Added - protects m_bannedXuids for concurrent access @@ -82,6 +97,7 @@ public: void add(shared_ptr player); void move(shared_ptr player); void remove(shared_ptr player); + void queueDisconnect(shared_ptr player, int reason, const wstring& kickMessage, bool wasKicked, bool fourKitHandledQuit); shared_ptr getPlayerForLogin(PendingConnection *pendingConnection, const wstring& userName, PlayerUID xuid, PlayerUID OnlineXuid); shared_ptr respawn(shared_ptr serverPlayer, int targetDimension, bool keepAllPlayerData); void toggleDimension(shared_ptr player, int targetDimension); diff --git a/Minecraft.Client/ServerConnection.cpp b/Minecraft.Client/ServerConnection.cpp index 3131ab5f..7bf05a9b 100644 --- a/Minecraft.Client/ServerConnection.cpp +++ b/Minecraft.Client/ServerConnection.cpp @@ -20,6 +20,7 @@ ServerConnection::ServerConnection(MinecraftServer *server) // 4J - added initialiser connectionCounter = 0; InitializeCriticalSection(&pending_cs); + InitializeCriticalSection(&players_cs); this->server = server; } @@ -27,6 +28,7 @@ ServerConnection::ServerConnection(MinecraftServer *server) ServerConnection::~ServerConnection() { DeleteCriticalSection(&pending_cs); + DeleteCriticalSection(&players_cs); } // 4J - added to handle incoming connections, to replace thread that original used to have @@ -38,7 +40,9 @@ void ServerConnection::NewIncomingSocket(Socket *socket) void ServerConnection::addPlayerConnection(shared_ptr uc) { + EnterCriticalSection(&players_cs); players.push_back(uc); + LeaveCriticalSection(&players_cs); } void ServerConnection::handleConnection(shared_ptr uc) @@ -76,7 +80,9 @@ void ServerConnection::stop() } // Snapshot to avoid iterator invalidation if disconnect modifies the vector. + EnterCriticalSection(&players_cs); std::vector > playerSnapshot = players; + LeaveCriticalSection(&players_cs); for (unsigned int i = 0; i < playerSnapshot.size(); i++) { shared_ptr player = playerSnapshot[i]; @@ -118,26 +124,34 @@ void ServerConnection::tick() } LeaveCriticalSection(&pending_cs); - for (unsigned int i = 0; i < players.size(); i++) { - shared_ptr player = players[i]; - shared_ptr serverPlayer = player->getPlayer(); - if( serverPlayer ) + EnterCriticalSection(&players_cs); + vector< shared_ptr > tempPlayers = players; + LeaveCriticalSection(&players_cs); + + for (unsigned int i = 0; i < tempPlayers.size(); i++) { - serverPlayer->updateFrameTick(); - serverPlayer->doChunkSendingTick(false); + shared_ptr player = tempPlayers[i]; + shared_ptr serverPlayer = player->getPlayer(); + if( serverPlayer ) + { + serverPlayer->updateFrameTick(); + serverPlayer->doChunkSendingTick(false); + } + player->tick(); + if (player->done) + { + EnterCriticalSection(&players_cs); + auto it = find(players.begin(), players.end(), player); + if (it != players.end()) players.erase(it); + LeaveCriticalSection(&players_cs); + } + else + { + player->connection->flush(); + } } - player->tick(); - if (player->done) - { - players.erase(players.begin()+i); - i--; - } - else - { - player->connection->flush(); - } - } + } } @@ -163,7 +177,10 @@ void ServerConnection::handleTextureReceived(const wstring &textureName) { m_pendingTextureRequests.erase(it); } - for (auto& player : players) + EnterCriticalSection(&players_cs); + vector< shared_ptr > tempPlayers = players; + LeaveCriticalSection(&players_cs); + for (auto& player : tempPlayers) { if (!player->done) { @@ -179,7 +196,10 @@ void ServerConnection::handleTextureAndGeometryReceived(const wstring &textureNa { m_pendingTextureRequests.erase(it); } - for (auto& player : players) + EnterCriticalSection(&players_cs); + vector< shared_ptr > tempPlayers = players; + LeaveCriticalSection(&players_cs); + for (auto& player : tempPlayers) { if (!player->done) { @@ -231,4 +251,46 @@ void ServerConnection::handleServerSettingsChanged(shared_ptr > * ServerConnection::getPlayers() { return &players; +} + +vector< shared_ptr > ServerConnection::getPlayersSnapshot() +{ + EnterCriticalSection(&players_cs); + vector< shared_ptr > snapshot = players; + LeaveCriticalSection(&players_cs); + return snapshot; +} + +void ServerConnection::sortPlayersByChunkPriority() +{ + EnterCriticalSection(&players_cs); + if( players.size() ) + { + vector< shared_ptr > playersOrig = players; + players.clear(); + + do + { + int longestTime = 0; + auto playerConnectionBest = playersOrig.begin(); + for( auto it = playersOrig.begin(); it != playersOrig.end(); it++) + { + int thisTime = 0; + INetworkPlayer *np = (*it)->getNetworkPlayer(); + if( np ) + { + thisTime = np->GetTimeSinceLastChunkPacket_ms(); + } + + if( thisTime > longestTime ) + { + playerConnectionBest = it; + longestTime = thisTime; + } + } + players.push_back(*playerConnectionBest); + playersOrig.erase(playerConnectionBest); + } while ( playersOrig.size() > 0 ); + } + LeaveCriticalSection(&players_cs); } \ No newline at end of file diff --git a/Minecraft.Client/ServerConnection.h b/Minecraft.Client/ServerConnection.h index 9ef80973..aa4f20d2 100644 --- a/Minecraft.Client/ServerConnection.h +++ b/Minecraft.Client/ServerConnection.h @@ -20,6 +20,7 @@ private: int connectionCounter; private: CRITICAL_SECTION pending_cs; // 4J added + CRITICAL_SECTION players_cs; // Protects players vector for concurrent access vector< shared_ptr > pending; vector< shared_ptr > players; @@ -47,4 +48,6 @@ public: void handleTextureAndGeometryReceived(const wstring &textureName); void handleServerSettingsChanged(shared_ptr packet); vector< shared_ptr > *getPlayers(); + vector< shared_ptr > getPlayersSnapshot(); + void sortPlayersByChunkPriority(); }; diff --git a/Minecraft.Server/Windows64/ServerMain.cpp b/Minecraft.Server/Windows64/ServerMain.cpp index 206b762a..699cca01 100644 --- a/Minecraft.Server/Windows64/ServerMain.cpp +++ b/Minecraft.Server/Windows64/ServerMain.cpp @@ -371,6 +371,19 @@ int main(int argc, char **argv) config.showHelp = false; SetConsoleCtrlHandler(ConsoleCtrlHandlerProc, TRUE); + + // Disable QuickEdit mode so clicking in the console window doesn't freeze + // the server process. Without this, any accidental click pauses all threads + // that write to stdout until a key is pressed. + { + HANDLE hInput = GetStdHandle(STD_INPUT_HANDLE); + DWORD mode = 0; + GetConsoleMode(hInput, &mode); + mode &= ~ENABLE_QUICK_EDIT_MODE; + mode |= ENABLE_EXTENDED_FLAGS; + SetConsoleMode(hInput, mode); + } + SetExeWorkingDirectory(); // Load base settings from server.properties, then override with CLI values when provided diff --git a/Minecraft.World/Socket.cpp b/Minecraft.World/Socket.cpp index f6a780c3..f07e3b2b 100644 --- a/Minecraft.World/Socket.cpp +++ b/Minecraft.World/Socket.cpp @@ -506,7 +506,6 @@ void Socket::SocketOutputStreamNetwork::writeWithFlags(byteArray b, unsigned int INetworkPlayer *socketPlayer = m_socket->getPlayer(); if(socketPlayer == nullptr) { - app.DebugPrintf("Trying to write to network, but the socketPlayer is nullptr\n"); return; } @@ -518,30 +517,20 @@ void Socket::SocketOutputStreamNetwork::writeWithFlags(byteArray b, unsigned int bool requireAck = ( ( flags & NON_QNET_SENDDATA_ACK_REQUIRED ) == NON_QNET_SENDDATA_ACK_REQUIRED ); #endif + // Re-validate the socket player immediately before use to minimize + // the TOCTOU window where the network layer could remove the player + // between our initial null-check and the SendData call. if( m_queueIdx == SOCKET_SERVER_END ) { - //printf( "Sent %u bytes of data from \"%ls\" to \"%ls\"\n", - //buffer.dwDataSize, - //hostPlayer->GetGamertag(), - //m_socket->networkPlayer->GetGamertag()); - - hostPlayer->SendData(socketPlayer, buffer.pbyData, buffer.dwDataSize, lowPriority, requireAck); - - // DWORD queueSize = hostPlayer->GetSendQueueSize( nullptr, QNET_GETSENDQUEUESIZE_BYTES ); - // if( queueSize > 24000 ) - // { - // //printf("Queue size is: %d, forcing doWork()\n",queueSize); - // g_NetworkManager.DoWork(); - // } + INetworkPlayer *validatedPlayer = m_socket->getPlayer(); + if(validatedPlayer == nullptr) return; + hostPlayer->SendData(validatedPlayer, buffer.pbyData, buffer.dwDataSize, lowPriority, requireAck); } else { - //printf( "Sent %u bytes of data from \"%ls\" to \"%ls\"\n", - //buffer.dwDataSize, - //m_socket->networkPlayer->GetGamertag(), - //hostPlayer->GetGamertag()); - - socketPlayer->SendData(hostPlayer, buffer.pbyData, buffer.dwDataSize, lowPriority, requireAck); + INetworkPlayer *validatedPlayer = m_socket->getPlayer(); + if(validatedPlayer == nullptr) return; + validatedPlayer->SendData(hostPlayer, buffer.pbyData, buffer.dwDataSize, lowPriority, requireAck); } } } diff --git a/README.md b/README.md index 7e0a3188..737dd902 100644 --- a/README.md +++ b/README.md @@ -23,6 +23,14 @@ This project is based on Legacy Console Edition v1.6.0560.0 (TU19) with fixes an ## Latest: +### Dedicated Server Stability Fixes + +- Fixed crashes caused by concurrent access to the player list during multiplayer. The players vector is now protected by a critical section, and all iterations use copy-on-read snapshots to prevent iterator invalidation when players join or leave mid-tick +- Fixed a null pointer crash in the movement validation handler when a player's bounding box was accessed during removal +- Fixed a race condition in network socket writes where a disconnecting player's network reference could become invalid between validation and use +- Fixed a deadlock in the player disconnect path by replacing inline cleanup with a queued disconnect system that is drained safely on the main tick thread +- Disabled Windows QuickEdit mode on the server console to prevent the process from freezing until console input is received + ### Beacon Menu Fixes - Fixed the beacon consuming a payment item (emerald, diamond, iron ingot, or gold ingot) when pressing the submit checkmark without changing any powers