fix(network): Prevent hang in crowded network lobby caused by long player names

Author: slurmlord

Core/GameEngine/Include/Common/AsciiString.h

@@ -377,6 +377,20 @@ class AsciiString
 	AsciiString& operator=(const AsciiString& stringSrc);	///< the same as set()
 	AsciiString& operator=(const char* s);				///< the same as set()
 
+	const Char& operator[](Int index) const
+	{
+		DEBUG_ASSERTCRASH(index >= 0 && index < getLength(), ("bad index in AsciiString::operator[]"));
+		return peek()[index];
+	}
+
+	Char& operator[](Int index)
+	{
+		Int length = getLength();
+		DEBUG_ASSERTCRASH(index >= 0 && index < length, ("bad index in AsciiString::operator[]"));
+		ensureUniqueBufferOfSize(length + 1, true, NULL, NULL);
+		return peek()[index];
+	}
+
 	void debugIgnoreLeaks();
 
 };

Core/GameEngine/Include/Common/UnicodeString.h

@@ -328,6 +328,20 @@ class UnicodeString
 
 	UnicodeString& operator=(const UnicodeString& stringSrc);	///< the same as set()
 	UnicodeString& operator=(const WideChar* s);				///< the same as set()
+
+	const WideChar& operator[](Int index) const
+	{
+		DEBUG_ASSERTCRASH(index >= 0 && index < getLength(), ("bad index in UnicodeString::operator[]"));
+		return peek()[index];
+	}
+
+	WideChar& operator[](Int index)
+	{
+		Int length = getLength();
+		DEBUG_ASSERTCRASH(index >= 0 && index < length, ("bad index in UnicodeString::operator[]"));
+		ensureUniqueBufferOfSize(length + 1, true, NULL, NULL);
+		return peek()[index];
+	}
 };
 
 

Core/GameEngine/Include/GameNetwork/LANAPI.h

@@ -406,6 +406,7 @@ struct LANMessage
 		{
 			char options[m_lanMaxOptionsLength+1];
 		} GameOptions;
+		static_assert(ARRAY_SIZE(GameOptions.options) > m_lanMaxOptionsLength, "GameOptions.options buffer must be larger than m_lanMaxOptionsLength");
 
 	};
 };

Core/GameEngine/Source/GameNetwork/GameInfo.cpp

@@ -44,6 +44,9 @@
 #include "GameNetwork/LANAPI.h"						// for testing packet size
 #include "GameNetwork/LANAPICallbacks.h"	// for testing packet size
 #include "strtok_r.h"
+#include <algorithm>
+#include <numeric>
+#include <utility>
 
 
 
@@ -892,7 +895,130 @@ Bool GameInfo::isSandbox(void)
 
 static const char slotListID		= 'S';
 
-AsciiString GameInfoToAsciiString( const GameInfo *game )
+struct LengthIndexPair
+{
+	Int Length;
+	size_t Index;
+	friend bool operator<(const LengthIndexPair& lhs, const LengthIndexPair& rhs)
+	{
+		if (lhs.Length == rhs.Length)
+			return lhs.Index < rhs.Index;
+		return lhs.Length < rhs.Length;
+	}
+	friend bool operator>(const LengthIndexPair& lhs, const LengthIndexPair& rhs) { return rhs < lhs; }
+	friend bool operator<=(const LengthIndexPair& lhs, const LengthIndexPair& rhs) { return !(lhs > rhs); }
+	friend bool operator>=(const LengthIndexPair& lhs, const LengthIndexPair& rhs) { return !(lhs < rhs); }
+};
+
+static AsciiStringVec BuildPlayerNames(const GameInfo& game)
+{
+	AsciiStringVec playerNames;
+	playerNames.resize(MAX_SLOTS);
+
+	for (Int i = 0; i < MAX_SLOTS; ++i)
+	{
+		const GameSlot* slot = game.getConstSlot(i);
+		if (slot->isHuman())
+		{
+			playerNames[i] = WideCharStringToMultiByte(slot->getName().str()).c_str();
+		}
+	}
+
+	return playerNames;
+}
+
+static Int SumLength(Int sum, const LengthIndexPair& l)
+{
+	return sum + l.Length;
+}
+
+static Bool TruncatePlayerNames(AsciiStringVec& playerNames, Int truncateAmount)
+{
+	// wont truncate any name to below this length
+	constexpr const Int MinimumNameLength = 1;
+
+	// make length+index pairs for the player names
+	std::vector<LengthIndexPair> lengthIndex;
+	lengthIndex.resize(playerNames.size());
+	for (size_t pi = 0; pi < playerNames.size(); ++pi)
+	{
+		Int playerNameLength = playerNames[pi].getLength();
+		lengthIndex[pi].Length = std::max(0, playerNameLength - MinimumNameLength);
+		lengthIndex[pi].Index = pi;
+	}
+
+	Int remainingNamesLength = std::accumulate(lengthIndex.begin(), lengthIndex.end(), 0, SumLength);
+
+	if (truncateAmount > remainingNamesLength)
+	{
+		DEBUG_LOG(("TruncatePlayerNames - Requested to truncate %u chars from player names, but only %u were available for truncation.",
+			truncateAmount, remainingNamesLength));
+		return false;
+	}
+
+	// sort based on length in ascending order
+	std::sort(lengthIndex.begin(), lengthIndex.end());
+
+	Int truncateNameAmount = 0;
+	for (size_t i = 0; i < lengthIndex.size(); ++i)
+	{
+		// round avg name length up, which will penalize the final entry (longest name) as it will have to account for the roundings
+		Int avgNameLength = ((remainingNamesLength - truncateAmount) + (playerNames.size() - i - 1)) / (playerNames.size() - i);
+		remainingNamesLength -= lengthIndex[i].Length;
+		if (lengthIndex[i].Length <= avgNameLength)
+		{
+			continue;
+		}
+
+		// ensure a longer name is not truncated less than a previous, shorter name
+		truncateNameAmount = std::max(truncateNameAmount, lengthIndex[i].Length - avgNameLength);
+		if (i == lengthIndex.size() - 1)
+		{
+			// ensure we account for rounding errors when truncating the last, longest entry
+			truncateNameAmount = std::max(truncateAmount, truncateNameAmount);
+		}
+
+		// as the name is UTF-8, make sure we don't truncate part of a multibyte character
+		while (lengthIndex[i].Length - truncateNameAmount >=  MinimumNameLength
+			&& (playerNames[lengthIndex[i].Index][lengthIndex[i].Length - truncateNameAmount + 1] & 0xC0) == 0x80)
+		{
+			++truncateNameAmount; // move back to the start of the multibyte character
+		}
+
+		playerNames[lengthIndex[i].Index].truncateBy(truncateNameAmount);
+		truncateAmount -= truncateNameAmount;
+	}
+
+	return true;
+}
+
+static void EnsureUniqueNames(AsciiStringVec& playerNames)
+{
+	// ensure there are no duplicates in the list of player names
+	std::set<AsciiString> uniqueNames;
+	for (size_t i = 0; i < playerNames.size(); ++i)
+	{
+		static_assert(MAX_SLOTS < 10, "Name collision avoidance assumes less than 10 players in the game.");
+		AsciiString& playerName = playerNames[i];
+
+		if (playerName.isEmpty())
+			continue;
+
+		Int charOffset = -1;
+		Int nameLength = playerName.getLength();
+		while (uniqueNames.insert(playerName).second == false)
+		{
+			// The name already exists, so change the last char to the number index of the player in the game.
+			// If that fails to be unique, iterate through 0-9 and change the last char to ensure differentiation.
+			// Guaranteed to find a unique name as the number of slots is less than 10.
+			char charToTry = '0' + static_cast<char>(charOffset == -1 ? i : charOffset);
+			playerName[nameLength - 1] = charToTry;
+			++charOffset;
+		}
+	}
+}
+
+AsciiString GameInfoToAsciiString(const GameInfo *game, const AsciiStringVec& playerNames)
 {
 	if (!game)
 		return AsciiString::TheEmptyString;
@@ -918,7 +1044,7 @@ AsciiString GameInfoToAsciiString( const GameInfo *game )
 			newMapName.concat(token);
 			mapName.nextToken(&token, "\\/");
 		}
-		DEBUG_LOG(("Map name is %s", mapName.str()));
+		DEBUG_LOG(("Map name is %s", newMapName.str()));
 	}
 
 	AsciiString optionsString;
@@ -941,23 +1067,13 @@ AsciiString GameInfoToAsciiString( const GameInfo *game )
 		AsciiString str;
 		if (slot && slot->isHuman())
 		{
-			AsciiString tmp;  //all this data goes after name
-			tmp.format( ",%X,%d,%c%c,%d,%d,%d,%d,%d:",
-				slot->getIP(), slot->getPort(),
-				(slot->isAccepted()?'T':'F'),
-				(slot->hasMap()?'T':'F'),
+			str.format( "H%s,%X,%d,%c%c,%d,%d,%d,%d,%d:",
+				playerNames[i].str(), slot->getIP(),
+				slot->getPort(), (slot->isAccepted() ? 'T' : 'F'),
+				(slot->hasMap() ? 'T' : 'F'),
 				slot->getColor(), slot->getPlayerTemplate(),
 				slot->getStartPos(), slot->getTeamNumber(),
-				slot->getNATBehavior() );
-			//make sure name doesn't cause overflow of m_lanMaxOptionsLength
-			int lenCur = tmp.getLength() + optionsString.getLength() + 2;  //+2 for H and trailing ;
-			int lenRem = m_lanMaxOptionsLength - lenCur;  //length remaining before overflowing
-			int lenMax = lenRem / (MAX_SLOTS-i);  //share lenRem with all remaining slots
-			AsciiString name = WideCharStringToMultiByte(slot->getName().str()).c_str();
-			while( name.getLength() > lenMax )
-				name.removeLastChar();  //what a horrible way to truncate.  I hate AsciiString.
-
-			str.format( "H%s%s", name.str(), tmp.str() );
+				slot->getNATBehavior());
 		}
 		else if (slot && slot->isAI())
 		{
@@ -989,13 +1105,39 @@ AsciiString GameInfoToAsciiString( const GameInfo *game )
 	}
 	optionsString.concat(';');
 
-	DEBUG_ASSERTCRASH(!TheLAN || (optionsString.getLength() < m_lanMaxOptionsLength),
-		("WARNING: options string is longer than expected!  Length is %d, but max is %d!",
-		optionsString.getLength(), m_lanMaxOptionsLength));
-
 	return optionsString;
 }
 
+AsciiString GameInfoToAsciiString(const GameInfo* game)
+{
+	if (!game)
+	{
+		return AsciiString::TheEmptyString;
+	}
+
+	AsciiStringVec playerNames = BuildPlayerNames(*game);
+	AsciiString infoString = GameInfoToAsciiString(game, playerNames);
+
+	// TheSuperHackers @bugfix Safely truncate the game info string by
+	// stripping characters off of player names if the overall length is too large.
+	if (TheLAN && (infoString.getLength() > m_lanMaxOptionsLength))
+	{
+		const UnsignedInt truncateAmount = infoString.getLength() - m_lanMaxOptionsLength;
+		if (!TruncatePlayerNames(playerNames, truncateAmount))
+		{
+			DEBUG_CRASH(("WARNING: options string is longer than expected! Length is %d, but max is %d. "
+				"Attempted to truncate player names by %u characters, but was unsuccessful!",
+				infoString.getLength(), m_lanMaxOptionsLength, truncateAmount));
+			return AsciiString::TheEmptyString;
+		}
+
+		EnsureUniqueNames(playerNames);
+		infoString = GameInfoToAsciiString(game, playerNames);
+	}
+
+	return infoString;
+}
+
 static Int grabHexInt(const char *s)
 {
 	char tmp[5] = "0xff";

Core/GameEngine/Source/GameNetwork/LANAPI.cpp

@@ -831,7 +831,7 @@ void LANAPI::RequestGameStartTimer( Int seconds )
 
 void LANAPI::RequestGameOptions( AsciiString gameOptions, Bool isPublic, UnsignedInt ip /* = 0 */ )
 {
-	DEBUG_ASSERTCRASH(gameOptions.getLength() < m_lanMaxOptionsLength, ("Game options string is too long!"));
+	DEBUG_ASSERTCRASH(gameOptions.getLength() <= m_lanMaxOptionsLength, ("Game options string is too long!"));
 
 	if (!m_currentGame)
 		return;