From 5db778396cac183a30f6979da004e2d1032232da Mon Sep 17 00:00:00 2001 From: sfan5 Date: Tue, 27 May 2025 12:52:59 +0200 Subject: [PATCH] Rework user limit and checks around join process --- src/network/serverpackethandler.cpp | 88 +++++++++++++++-------------- src/server.cpp | 10 +++- src/server.h | 5 ++ src/server/clientiface.cpp | 35 +----------- src/server/clientiface.h | 13 +---- 5 files changed, 67 insertions(+), 84 deletions(-) diff --git a/src/network/serverpackethandler.cpp b/src/network/serverpackethandler.cpp index e16494652..e9012d351 100644 --- a/src/network/serverpackethandler.cpp +++ b/src/network/serverpackethandler.cpp @@ -40,10 +40,6 @@ void Server::handleCommand_Deprecated(NetworkPacket* pkt) void Server::handleCommand_Init(NetworkPacket* pkt) { - - if(pkt->getSize() < 1) - return; - session_t peer_id = pkt->getPeerId(); RemoteClient *client = getClient(peer_id, CS_Created); @@ -75,15 +71,6 @@ void Server::handleCommand_Init(NetworkPacket* pkt) verbosestream << "Server: Got TOSERVER_INIT from " << addr_s << " (peer_id=" << peer_id << ")" << std::endl; - // Do not allow multiple players in simple singleplayer mode. - // This isn't a perfect way to do it, but will suffice for now - if (m_simple_singleplayer_mode && !m_clients.getClientIDs().empty()) { - infostream << "Server: Not allowing another client (" << addr_s << - ") to connect in simple singleplayer mode" << std::endl; - DenyAccess(peer_id, SERVER_ACCESSDENIED_SINGLEPLAYER); - return; - } - if (denyIfBanned(peer_id)) return; @@ -161,18 +148,14 @@ void Server::handleCommand_Init(NetworkPacket* pkt) return; } - RemotePlayer *player = m_env->getPlayer(playername, true); - - // If player is already connected, cancel - if (player && player->getPeerId() != PEER_ID_INEXISTENT) { - actionstream << "Server: Player with name \"" << playername << - "\" tried to connect, but player with same name is already connected" << std::endl; - DenyAccess(peer_id, SERVER_ACCESSDENIED_ALREADY_CONNECTED); + // Do not allow multiple players in simple singleplayer mode + if (isSingleplayer() && !m_clients.getClientIDs(CS_HelloSent).empty()) { + infostream << "Server: Not allowing another client (" << addr_s << + ") to connect in simple singleplayer mode" << std::endl; + DenyAccess(peer_id, SERVER_ACCESSDENIED_SINGLEPLAYER); return; } - - m_clients.setPlayerName(peer_id, playername); - + // Or the "singleplayer" name to be used on regular servers if (!isSingleplayer() && strcasecmp(playername, "singleplayer") == 0) { actionstream << "Server: Player with the name \"singleplayer\" tried " "to connect from " << addr_s << std::endl; @@ -180,12 +163,25 @@ void Server::handleCommand_Init(NetworkPacket* pkt) return; } + { + RemotePlayer *player = m_env->getPlayer(playername, true); + // If player is already connected, cancel + if (player && player->getPeerId() != PEER_ID_INEXISTENT) { + actionstream << "Server: Player with name \"" << playername << + "\" tried to connect, but player with same name is already connected" << std::endl; + DenyAccess(peer_id, SERVER_ACCESSDENIED_ALREADY_CONNECTED); + return; + } + } + + client->setName(playerName); + { std::string reason; if (m_script->on_prejoinplayer(playername, addr_s, &reason)) { actionstream << "Server: Player with the name \"" << playerName << "\" tried to connect from " << addr_s << - " but it was disallowed for the following reason: " << reason << + " but was disallowed for the following reason: " << reason << std::endl; DenyAccess(peer_id, SERVER_ACCESSDENIED_CUSTOM_STRING, reason); return; @@ -195,14 +191,11 @@ void Server::handleCommand_Init(NetworkPacket* pkt) infostream << "Server: New connection: \"" << playerName << "\" from " << addr_s << " (peer_id=" << peer_id << ")" << std::endl; - // Enforce user limit. - // Don't enforce for users that have some admin right or mod permits it. - if (m_clients.isUserLimitReached() && - playername != g_settings->get("name") && - !m_script->can_bypass_userlimit(playername, addr_s)) { + // Early check for user limit, so the client doesn't need to run + // through the join process only to be denied. + if (checkUserLimit(playerName, addr_s)) { actionstream << "Server: " << playername << " tried to join from " << - addr_s << ", but there are already max_users=" << - g_settings->getU16("max_users") << " players." << std::endl; + addr_s << ", but the user limit was reached." << std::endl; DenyAccess(peer_id, SERVER_ACCESSDENIED_TOO_MANY_USERS); return; } @@ -355,6 +348,8 @@ void Server::handleCommand_RequestMedia(NetworkPacket* pkt) void Server::handleCommand_ClientReady(NetworkPacket* pkt) { session_t peer_id = pkt->getPeerId(); + RemoteClient *client = getClient(peer_id, CS_Created); + assert(client); // decode all information first u8 major_ver, minor_ver, patch_ver, reserved; @@ -365,8 +360,17 @@ void Server::handleCommand_ClientReady(NetworkPacket* pkt) if (pkt->getRemainingBytes() >= 2) *pkt >> formspec_ver; - m_clients.setClientVersion(peer_id, major_ver, minor_ver, patch_ver, - full_ver); + client->setVersionInfo(major_ver, minor_ver, patch_ver, full_ver); + + // Since only active clients count for the user limit, two could race the + // join process so we have to do a final check for the user limit here. + std::string addr_s = client->getAddress().serializeString(); + if (checkUserLimit(client->getName(), addr_s)) { + actionstream << "Server: " << client->getName() << " tried to join from " << + addr_s << ", but the user limit was reached (late)." << std::endl; + DenyAccess(peer_id, SERVER_ACCESSDENIED_TOO_MANY_USERS); + return; + } // Emerge player PlayerSAO* playersao = StageTwoClientInit(peer_id); @@ -1427,7 +1431,7 @@ void Server::handleCommand_FirstSrp(NetworkPacket* pkt) std::string salt, verification_key; - std::string addr_s = getPeerAddress(peer_id).serializeString(); + std::string addr_s = client->getAddress().serializeString(); u8 is_empty; *pkt >> salt >> verification_key >> is_empty; @@ -1513,9 +1517,11 @@ void Server::handleCommand_SrpBytesA(NetworkPacket* pkt) RemoteClient *client = getClient(peer_id, CS_Invalid); ClientState cstate = client->getState(); + std::string addr_s = client->getAddress().serializeString(); + if (!((cstate == CS_HelloSent) || (cstate == CS_Active))) { actionstream << "Server: got SRP _A packet in wrong state " << cstate << - " from " << getPeerAddress(peer_id).serializeString() << + " from " << addr_s << ". Ignoring." << std::endl; return; } @@ -1525,7 +1531,7 @@ void Server::handleCommand_SrpBytesA(NetworkPacket* pkt) if (client->chosen_mech != AUTH_MECHANISM_NONE) { actionstream << "Server: got SRP _A packet, while auth is already " "going on with mech " << client->chosen_mech << " from " << - getPeerAddress(peer_id).serializeString() << + addr_s << " (wantSudo=" << wantSudo << "). Ignoring." << std::endl; if (wantSudo) { DenySudoAccess(peer_id); @@ -1542,7 +1548,7 @@ void Server::handleCommand_SrpBytesA(NetworkPacket* pkt) infostream << "Server: TOSERVER_SRP_BYTES_A received with " << "based_on=" << int(based_on) << " and len_A=" - << bytes_A.length() << "." << std::endl; + << bytes_A.length() << std::endl; AuthMechanism chosen = (based_on == 0) ? AUTH_MECHANISM_LEGACY_PASSWORD : AUTH_MECHANISM_SRP; @@ -1551,17 +1557,17 @@ void Server::handleCommand_SrpBytesA(NetworkPacket* pkt) // Right now, the auth mechs don't change between login and sudo mode. if (!client->isMechAllowed(chosen)) { actionstream << "Server: Player \"" << client->getName() << - "\" at " << getPeerAddress(peer_id).serializeString() << + "\" from " << addr_s << " tried to change password using unallowed mech " << chosen << - "." << std::endl; + std::endl; DenySudoAccess(peer_id); return; } } else { if (!client->isMechAllowed(chosen)) { actionstream << "Server: Client tried to authenticate from " << - getPeerAddress(peer_id).serializeString() << - " using unallowed mech " << chosen << "." << std::endl; + addr_s << + " using unallowed mech " << chosen << std::endl; DenyAccess(peer_id, SERVER_ACCESSDENIED_UNEXPECTED_DATA); return; } diff --git a/src/server.cpp b/src/server.cpp index efe74119c..a1263ef1d 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -3356,6 +3356,15 @@ bool Server::denyIfBanned(session_t peer_id) return false; } +bool Server::checkUserLimit(const std::string &player_name, const std::string &addr_s) +{ + if (!m_clients.isUserLimitReached()) + return false; + if (player_name == g_settings->get("name")) // admin can always join + return false; + return !m_script->can_bypass_userlimit(player_name, addr_s); +} + void Server::notifyPlayer(const char *name, const std::wstring &msg) { // m_env will be NULL if the server is initializing @@ -3489,7 +3498,6 @@ void Server::hudSetHotbarSelectedImage(RemotePlayer *player, const std::string & Address Server::getPeerAddress(session_t peer_id) { - // Note that this is only set after Init was received in Server::handleCommand_Init return getClient(peer_id, CS_Invalid)->getAddress(); } diff --git a/src/server.h b/src/server.h index c9869e1dd..ae5b10682 100644 --- a/src/server.h +++ b/src/server.h @@ -368,6 +368,7 @@ public: void hudSetHotbarImage(RemotePlayer *player, const std::string &name); void hudSetHotbarSelectedImage(RemotePlayer *player, const std::string &name); + /// @note this is only available for client state >= CS_HelloSent Address getPeerAddress(session_t peer_id); void setLocalPlayerAnimations(RemotePlayer *player, v2f animation_frames[4], @@ -611,6 +612,10 @@ private: void handleChatInterfaceEvent(ChatEvent *evt); + /// @brief Checks if user limit allows a potential client to join + /// @return true if the client can NOT join + bool checkUserLimit(const std::string &player_name, const std::string &addr_s); + // This returns the answer to the sender of wmessage, or "" if there is none std::wstring handleChat(const std::string &name, std::wstring wmessage_input, bool check_shout_priv = false, RemotePlayer *player = nullptr); diff --git a/src/server/clientiface.cpp b/src/server/clientiface.cpp index 2f6abbf85..bbcd4720d 100644 --- a/src/server/clientiface.cpp +++ b/src/server/clientiface.cpp @@ -659,7 +659,7 @@ std::vector ClientInterface::getClientIDs(ClientState min_state) { std::vector reply; RecursiveMutexAutoLock clientslock(m_clients_mutex); - + reply.reserve(m_clients.size()); for (const auto &m_client : m_clients) { if (m_client.second->getState() >= min_state) reply.push_back(m_client.second->peer_id); @@ -677,14 +677,10 @@ void ClientInterface::markBlocksNotSent(const std::vector &positions, boo } } -/** - * Verify if user limit was reached. - * User limit count all clients from HelloSent state (MT protocol user) to Active state - * @return true if user limit was reached - */ bool ClientInterface::isUserLimitReached() { - return getClientIDs(CS_HelloSent).size() >= g_settings->getU16("max_users"); + // Note that this only counts clients that have fully joined + return getClientIDs().size() >= g_settings->getU16("max_users"); } void ClientInterface::step(float dtime) @@ -812,16 +808,6 @@ ClientState ClientInterface::getClientState(session_t peer_id) return n->second->getState(); } -void ClientInterface::setPlayerName(session_t peer_id, const std::string &name) -{ - RecursiveMutexAutoLock clientslock(m_clients_mutex); - RemoteClientMap::iterator n = m_clients.find(peer_id); - // The client may not exist; clients are immediately removed if their - // access is denied, and this event occurs later then. - if (n != m_clients.end()) - n->second->setName(name); -} - void ClientInterface::DeleteClient(session_t peer_id) { RecursiveMutexAutoLock conlock(m_clients_mutex); @@ -902,18 +888,3 @@ u16 ClientInterface::getProtocolVersion(session_t peer_id) return n->second->net_proto_version; } - -void ClientInterface::setClientVersion(session_t peer_id, u8 major, u8 minor, u8 patch, - const std::string &full) -{ - RecursiveMutexAutoLock conlock(m_clients_mutex); - - // Error check - RemoteClientMap::iterator n = m_clients.find(peer_id); - - // No client to set versions - if (n == m_clients.end()) - return; - - n->second->setVersionInfo(major, minor, patch, full); -} diff --git a/src/server/clientiface.h b/src/server/clientiface.h index d2194774c..d0e91dcca 100644 --- a/src/server/clientiface.h +++ b/src/server/clientiface.h @@ -445,7 +445,7 @@ public: /* mark blocks as not sent on all active clients */ void markBlocksNotSent(const std::vector &positions, bool low_priority = false); - /* verify is server user limit was reached */ + /* verify if server user limit was reached */ bool isUserLimitReached(); /* get list of client player names */ @@ -475,16 +475,9 @@ public: /* get state of client by id*/ ClientState getClientState(session_t peer_id); - /* set client playername */ - void setPlayerName(session_t peer_id, const std::string &name); - /* get protocol version of client */ u16 getProtocolVersion(session_t peer_id); - /* set client version */ - void setClientVersion(session_t peer_id, u8 major, u8 minor, u8 patch, - const std::string &full); - /* event to update client state */ void event(session_t peer_id, ClientStateEvent event); @@ -515,9 +508,9 @@ private: // Connection std::shared_ptr m_con; std::recursive_mutex m_clients_mutex; - // Connected clients (behind the con mutex) + // Connected clients (behind the mutex) RemoteClientMap m_clients; - std::vector m_clients_names; //for announcing masterserver + std::vector m_clients_names; // for announcing to server list // Environment ServerEnvironment *m_env;