Skip to content

bugfix(network): Prevent LAN lobby hang with long player names - #3039

Open
bobtista wants to merge 2 commits into
TheSuperHackers:mainfrom
bobtista:bobtista/bugfix/lan-lobby-long-name-hang
Open

bugfix(network): Prevent LAN lobby hang with long player names#3039
bobtista wants to merge 2 commits into
TheSuperHackers:mainfrom
bobtista:bobtista/bugfix/lan-lobby-long-name-hang

Conversation

@bobtista

@bobtista bobtista commented Aug 1, 2026

Copy link
Copy Markdown

GameInfoToAsciiString serializes the LAN lobby state into a string with a 400-byte limit. The existing code truncates each player name while appending its slot:

int lenRem = m_lanMaxOptionsLength - lenCur;   // can go negative
int lenMax = lenRem / (MAX_SLOTS-i);           // can go negative
while( name.getLength() > lenMax )
    name.removeLastChar();

Once the fixed portion of the options string consumes the remaining budget, lenMax becomes negative. The loop removes the entire name, after which AsciiString::removeLastChar becomes a no-op. Because 0 > lenMax remains true, the host spins forever.

Now the options string is built once with the full player names. Only when that string exceeds the limit is an exceptional second pass performed:

  1. Determine how many serialized bytes must be removed.
  2. Distribute that reduction across player names, taking more from longer names so shorter names are penalized less.
  3. Preserve at least one complete UTF-8 character per non-empty name and truncate only at character boundaries.
  4. Make collisions introduced by truncation deterministic by replacing the final complete character with the player's slot index, then trying 0 through 9 if necessary.
  5. Rebuild the options string with the adjusted names.

If the names cannot provide enough space while retaining one complete character each, serialization returns an empty string. Receivers reject that payload through the existing ParseAsciiStringToGameInfo failure path instead of leaving the host in an infinite loop.

Truncation affects only the serialized LAN payload. The host retains the full player names, while remote clients may display their truncated forms.

Supporting changes add a compile-time check that the GameOptions.options buffer exceeds m_lanMaxOptionsLength and allow a payload of exactly 400 bytes, which fits in the 401-byte null-terminated buffer.

Follow-up: cache each converted player name in GameSlot so it is not rebuilt on every room refresh, as suggested in #1119.

Todo:

  • Resolve the outstanding review points inherited from [ZH] Prevent hang in network lobby with long player names #1119
  • Preserve UTF-8 validity during truncation and collision handling
  • Replicate to Generals — N/A, the implementation is shared through Core
  • Retest a full LAN lobby with eight long names, including multibyte names and truncation collisions
  • Verify the exactly-400-byte boundary and the impossible-to-fit failure path in-game

Verification

  • A model of the truncation and collision algorithm passed 500,000 randomized UTF-8 cases covering size reduction, UTF-8 validity, uniqueness, and determinism.

@bobtista bobtista self-assigned this Aug 1, 2026
@bobtista bobtista added the Bug Something is not working right, typically is user facing label Aug 1, 2026
@greptile-apps

greptile-apps Bot commented Aug 1, 2026

Copy link
Copy Markdown

Greptile Summary

The PR prevents LAN lobby serialization from hanging when long player names exceed the 400-byte options limit.

  • Builds the complete payload first and performs a bounded second-pass truncation only when necessary.
  • Preserves UTF-8 character boundaries and deterministically resolves names that collide after truncation.
  • Allows exactly 400 payload bytes while retaining space for the null terminator.
  • Returns an empty payload when names cannot provide enough removable bytes.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking issue identified.

The truncation path removes at least the required number of bytes while preserving UTF-8 boundaries and one complete character per non-empty name, and the exactly-400-byte payload safely fits the existing null-terminated buffers.

Important Files Changed

Filename Overview
Core/GameEngine/Source/GameNetwork/GameInfo.cpp Adds bounded UTF-8-aware player-name truncation, deterministic collision handling, and a second serialization pass for oversized LAN payloads.
Core/GameEngine/Source/GameNetwork/LANAPI.cpp Correctly permits a payload of exactly 400 bytes, which fits the existing 401-byte null-terminated destination.
Core/GameEngine/Include/GameNetwork/LANAPI.h Adds a compile-time assertion confirming that the LAN options buffer includes capacity beyond the payload limit.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Build UTF-8 player names] --> B[Serialize full GameInfo]
    B --> C{Payload exceeds 400 bytes?}
    C -- No --> D[Return payload]
    C -- Yes --> E[Calculate required byte reduction]
    E --> F{Enough removable name bytes?}
    F -- No --> G[Return empty string]
    F -- Yes --> H[Truncate names at UTF-8 boundaries]
    H --> I[Resolve truncated-name collisions]
    I --> J[Rebuild serialized GameInfo]
    J --> D
Loading

Reviews (1): Last reviewed commit: "bugfix(network): Prevent LAN lobby hang ..." | Re-trigger Greptile

@Skyaero42 Skyaero42 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This feels very complicated for what is eventually just a hack. FIxing the 400 byte gameinfo byte limit should be the true goal.

Something as simple as: count the number of available bytes for player names and divide that by the number of players - this gives the number of bytes each player name can have. Yes, it is not exact (if there a players with shorter names, that would also allow players with longer names than the threshold).

In general, there is a lot of stuff added in GameInfo that doesn't belong there. Specific byte counts of characters belong in Asciistring. Such function can probably also be generalized instead of using First and Last in functions.

void SkirmishGameInfo::loadPostProcess()
{
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

removing these lines is out of scope imho. Let clang deal with that.

Int remainingTruncatableByteCount = 0;

// Build truncatable byte count and player index pairs for the player names.
for (Int pi = 0; pi < MAX_SLOTS; ++pi)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

pi could be confused with the number pi. Maybe just use i?

return (static_cast<unsigned char>(c) & 0xC0) == 0x80;
}

static Int GetFirstUtf8CharacterByteLength(const AsciiString& string)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should be an Asciistring function. It does not belong in GameInfo

}
}

static Bool IsUtf8ContinuationByte(Char c)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should be an Asciistring function. It does not belong in GameInfo


static Int DivCeilPositive(Int numerator, Int denominator)
{
return (numerator + denominator - 1) / denominator;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • Requires a division by zero check
  • Doesn't really fit GameInfo. Maybe WWMath?
  • The name doesn't make it clear what it does.

return true;
}

static void ReplaceLastUtf8Character(AsciiString& string, Char replacement)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should not be in GameInfo

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Greptile has paused reviews on this repository — it used its 100 free open-source review credits for this billing period. Reviews resume automatically on August 26. To continue before then, an organization admin can keep reviews running past the free credits — those bill as normal usage.

@bobtista
bobtista force-pushed the bobtista/bugfix/lan-lobby-long-name-hang branch from 7900833 to 85030a1 Compare August 1, 2026 17:10
@bobtista

bobtista commented Aug 1, 2026

Copy link
Copy Markdown
Author

FIxing the 400 byte gameinfo byte limit should be the true goal.

Agreed, but retail still needs something, even if it's hacky. The 400-byte limit is part of the packed retail LAN wire layout: LANMessage is sent by size and cast directly by receivers, so enlarging the options array would change field offsets and break retail compat. Removing that limit requires a versioned or chunked protocol extension and should be a separate change. This PR keeps the existing wire format and fixes the current infinite loop; it would remain necessary as the retail-compatible fallback even after an extended protocol is introduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something is not working right, typically is user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Network Game Room hangs if 8 players with long nicknames join

3 participants