Skip to content

[ZH] Prevent hang in network lobby with long player names #1119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
+132 −22
Open

[ZH] Prevent hang in network lobby with long player names #1119

Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5efb9b8
Add estimates for required name truncation to prevent game info strin…
slurmlord Jun 20, 2025
ca773e0
Rename index var to si to avoid colliding with i.
slurmlord Jun 20, 2025
b7eafeb
Move name truncation to separate method and make 2 serialization pass…
slurmlord Jun 20, 2025
d5a4150
Split Player name list creation and truncation into separate functions.
slurmlord Jun 20, 2025
1ae9876
Add removeLastNChars method to AsciiString.
slurmlord Jul 1, 2025
cdbf4b7
Ensure game options are null terminated.
slurmlord Jul 1, 2025
aab7f02
Ensure we accept the maximum length game options.
slurmlord Jul 1, 2025
f8600c6
Fairer truncation algo, rewarding short names.
slurmlord Jul 1, 2025
ca15e22
VC6 compat.
slurmlord Jul 1, 2025
60d1b56
Rename removeLastNChars to truncate.
slurmlord Jul 2, 2025
c83bae0
Merge remote-tracking branch 'origin/main' into fix-lobby-hang
slurmlord Jul 2, 2025
87b552e
Improve truncation target length selection and move away from std::pair.
slurmlord Jul 2, 2025
fad4c92
Merge remote-tracking branch 'origin/main' into fix-lobby-hang
slurmlord Jul 14, 2025
d1ec6c7
Simplify truncation algorithm, ensure unique truncated names.
slurmlord Jul 14, 2025
d1b8e9f
Inadvertent whitespace change.
slurmlord Jul 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 81 additions & 21 deletions GeneralsMD/Code/GameEngine/Source/GameNetwork/GameInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,7 +897,49 @@ Bool GameInfo::isSandbox(void)

static const char slotListID = 'S';

AsciiString GameInfoToAsciiString( const GameInfo *game )
static void BuildPlayerNames(AsciiStringVec& playerNames, const GameInfo& game)
{
for (Int i = 0; i < MAX_SLOTS; ++i)
{
const GameSlot* slot = game.getConstSlot(i);
if (slot && slot->isHuman())
{
playerNames[i] = WideCharStringToMultiByte(slot->getName().str()).c_str();
}
else
{
playerNames[i] = AsciiString::TheEmptyString;
}
}
}

static Bool TruncatePlayerNames(AsciiStringVec& playerNames, UnsignedInt truncateAmount)
{
while (truncateAmount > 0)
{
Bool didTruncate = false;
for (Int i = 0; i < playerNames.size() && truncateAmount > 0; ++i)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is not fair to players that have short names :-)

Player names will be truncated like so

Start

TheDude
ThePrince
HolySmokesLongAssWastefulName

First iteration NOW

TheDud
ThePrinc
HolySmokesLongAssWastefulNam

First iteration MORE FAIR

TheDude
ThePrince
HolySmokesLongAssWastefulN

I think this should be implemented in a way that it aggressively starts truncating the long names first.

To do that, you can take the max length of all player names, and then use that as the cap to start truncating on the iterations. However that is a wasteful algorithm.

A more efficient way to implement it is to sort the names first (use a local array with indices to the vector) and then start truncating from longest to smallest names. Maybe it can be implemented without excessive loops even. Note that removeLastChar is also quite a wasteful operation.

Copy link

@xezon xezon Jun 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more consideration here is: Will a new implementation for the truncate be compatible with Retail game clients? To test this, compile with RTS_MULTI_INSTANCE and launch one client before this change and one after, then matchmake in LAN.

Edit: On the other hand, the original implemention will hang up, so perhaps it does not matter at all.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The game already limits the number of characters a Lan Name can have to 11 characters, even though it should be 12+1 based on g_lanPlayerNameLength + 1 being the size of the character array for the player name

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that the truncation only happens on the host, which then sends out the updated game options when some change happens in the game (like someone joining, selecting team/colors etc.). The clients only receive the ascii string serialization of the game options, and should not require any updated logic.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The newest iteration now "punishes" the players who join with long names, as they are truncated first.
As an example, if player A had 10 chars name and player B had 7 and the amount of truncation required was 6; player A would end up with 5 chars and B with 6.

{
// we won't truncate any names to shorter than 2 characters
if (playerNames[i].getLength() > 2)
{
playerNames[i].removeLastChar();
truncateAmount--;
didTruncate = true;
}
}

if (!didTruncate)
{
// iterated through all names without finding any to truncate.
return false;
}
}

return true;
}

AsciiString GameInfoToAsciiString(const GameInfo *game, const AsciiStringVec& playerNames)
{
if (!game)
return AsciiString::TheEmptyString;
Expand All @@ -923,7 +965,7 @@ AsciiString GameInfoToAsciiString( const GameInfo *game )
newMapName.concat(token);
mapName.nextToken(&token, "\\/");
}
DEBUG_LOG(("Map name is %s\n", mapName.str()));
DEBUG_LOG(("Map name is %s\n", newMapName.str()));
}

AsciiString optionsString;
Expand All @@ -941,23 +983,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())
{
Expand Down Expand Up @@ -989,13 +1021,41 @@ 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!\n",
optionsString.getLength(), m_lanMaxOptionsLength));

return optionsString;
}

AsciiString GameInfoToAsciiString(const GameInfo* game)
{
if (!game)
{
return AsciiString::TheEmptyString;
}

AsciiStringVec playerNames(MAX_SLOTS);
BuildPlayerNames(playerNames, *game);
AsciiString infoString = GameInfoToAsciiString(game, playerNames);

// TheSuperHackers @bugfix Safely truncate the game info string by
// scrapping characters off of player names if the overall length is too large.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be "scrapping" or "scraping"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried looking around for opinions, and ended up with "stripping".

if (infoString.getLength() > m_lanMaxOptionsLength)
{
const UnsignedInt truncateAmount = infoString.getLength() - m_lanMaxOptionsLength;
if (!TruncatePlayerNames(playerNames, truncateAmount))
{
DEBUG_LOG(("GameInfoToAsciiString - unable to truncate player names by %u characters.\n", truncateAmount));
return AsciiString::TheEmptyString;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So originally it would have returned the long string if it was unable to (theoretically) truncate. What happens if we return an empty string or a string that is longer than the cap?

}

infoString = GameInfoToAsciiString(game, playerNames);
}

DEBUG_ASSERTCRASH(!TheLAN || (infoString.getLength() < m_lanMaxOptionsLength),
("WARNING: options string is longer than expected! Length is %d, but max is %d!\n",
infoString.getLength(), m_lanMaxOptionsLength));

return infoString;
}

static Int grabHexInt(const char *s)
{
char tmp[5] = "0xff";
Expand Down
Loading