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 4 commits into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
4 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
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)
Copy link

Choose a reason for hiding this comment

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

I suggest build the vector in this class and return it. Because right now this expectes passing the vector with the correct size which is not so nice.

{
for (Int i = 0; i < MAX_SLOTS; ++i)
{
const GameSlot* slot = game.getConstSlot(i);
if (slot && slot->isHuman())
Copy link

Choose a reason for hiding this comment

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

Null test is not necessary. Slot is not expected NULL.

{
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

{
// 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"?

if (infoString.getLength() > m_lanMaxOptionsLength)
Copy link

Choose a reason for hiding this comment

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

Perhaps we can do TheLAN && (infoString.getLength() < m_lanMaxOptionsLength), judging by the DEBUG_ASSERTCRASH below.

{
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),
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 <= instead ?

Copy link

Choose a reason for hiding this comment

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

Perhaps we move this assert to where the new DEBUG_LOG is above?

("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