Rework user limit and checks around join process

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

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

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

src/server/clientiface.cpp

@@ -659,7 +659,7 @@ std::vector<session_t> ClientInterface::getClientIDs(ClientState min_state)
 {
 	std::vector<session_t> 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<v3s16> &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);
-}

src/server/clientiface.h

@@ -445,7 +445,7 @@ public:
 	/* mark blocks as not sent on all active clients */
 	void markBlocksNotSent(const std::vector<v3s16> &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<con::IConnection> m_con;
 	std::recursive_mutex m_clients_mutex;
-	// Connected clients (behind the con mutex)
+	// Connected clients (behind the mutex)
 	RemoteClientMap m_clients;
-	std::vector<std::string> m_clients_names; //for announcing masterserver
+	std::vector<std::string> m_clients_names; // for announcing to server list
 
 	// Environment
 	ServerEnvironment *m_env;