fix(gamespy): run availability check, fix async DNS hostname lifetime

[sokie]

Hello! I'm the creator of the open source gamespy server https://github.com/sokie/kirov-server-emulator/tree/main
Users reported not being able to connect to Kirov on this build, after investigating found 2 issues stopping online to work:

• asyncGethostbyname() passes its argument to CreateThread and returns immediately, so the stack-local hostname at both call sites is dead before the lookup thread reads it; make it static.
• The backend availability check was never run, leaving __GSIACResult at GSIACWaiting, which makes peerInitialize() return null; release builds then dereference it in peerSetRoomWatchKeys() and report the fault as DISCONNECT_LOSTCON. Run the check as a fifth pre-online check and handle a null peer.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix GameSpy online init: run availability check and stabilize async DNS hostname

🐞 Bug fix🕐 20-40 Minutes

AI Description

• Run GameSpy backend availability check before starting GameSpy threads.
• Prevent async DNS lookup thread from reading freed stack memory.
• Handle null peerInitialize() in release builds to avoid crash/misreported disconnect.

Diagram

graph TD
UI["Main menu: Online"] --> Patch["reallyStartPatchCheck()"] --> AvStart["GSIStartAvailableCheck()"] --> Think["HTTPThinkWrapper()"] --> AvThink["GSIAvailableCheckThink()"] --> Gate{"Available?"}
Gate -->|"Yes"| Online["startOnline()"] --> Peer["PeerThread: peerInitialize()"]
Gate -->|"No"| Error["Show GS disconnect reason"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make `asyncGethostbyname()` copy the hostname

• ➕ Eliminates lifetime hazards at the API boundary (no reliance on caller storage duration).
• ➕ Avoids hidden global/static state and is safe for multiple concurrent lookups.
• ➖ May require changing or wrapping third-party/legacy SDK code.
• ➖ Potentially larger refactor if many call sites exist.

2. Heap-allocate hostname per request and free after completion

• ➕ Fixes lifetime without introducing static storage.
• ➕ Keeps asyncGethostbyname() signature unchanged.
• ➖ Requires explicit ownership/cleanup synchronization with the lookup thread.
• ➖ Easy to leak or double-free if cancellation/error paths aren’t unified.

Recommendation: The PR’s approach is appropriate for a minimal, low-touch fix: gating online startup on the availability check addresses the root cause of peerInitialize() returning null, and adding a release-build null guard prevents a hard fault. The static hostname workaround is acceptable given the apparent single-host, repeated lookup usage; if the async DNS helper grows or becomes reusable, prefer an API-level fix where the lookup thread owns a copied hostname buffer.

Files changed (2) +66 / -3

Bug fix (2) +66 / -3

MainMenuUtils.cppAdd availability-check gate and fix async DNS hostname lifetime+56/-3

Add availability-check gate and fix async DNS hostname lifetime

• Adds a GameSpy backend availability check as an additional pre-online step and blocks 'startOnline()' unless the check completes successfully. Fixes a thread-lifetime bug by making the DNS hostname buffer static where passed to 'asyncGethostbyname()'. Updates the pre-online counter from 4 to 5 and ensures prior availability checks are canceled before retrying.

Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp

PeerThread.cppHandle null 'peerInitialize()' in release builds with a clean disconnect+10/-0

Handle null 'peerInitialize()' in release builds with a clean disconnect

• Keeps the debug assertion but adds a runtime null check for 'peerInitialize()' to prevent release-build crashes. On null peer creation, enqueues a disconnect response with 'DISCONNECT_COULDNOTCONNECT' instead of allowing a later dereference and misclassification as 'DISCONNECT_LOSTCON'.

Core/GameEngine/Source/GameNetwork/GameSpy/Thread/PeerThread.cpp

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (4)📘 Rule violations (0)📜 Skill insights (0)

1. Cancel causes counter underflow 🐞 Bug☼ Reliability

Description

CancelPatchCheckCallback() sets checksLeftBeforeOnline=0 but does not cancel/reset the in-flight
availability check, so when it completes HTTPThinkWrapper() decrements the counter to -1. This
breaks subsequent online attempts (assert in debug; in release the negative value prevents the
normal “when checks reach 0, startOnline()” flow).

Code

Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[R818-821]

+ if (availableCheckResult != GSIACWaiting)+ {+ --checksLeftBeforeOnline;+ DEBUG_ASSERTCRASH(checksLeftBeforeOnline>=0, ("Too many callbacks"));

Evidence

The new availability completion path always decrements checksLeftBeforeOnline when the result
becomes non-waiting, but the cancel path does not cancel the availability check or reset
availableCheckResult, so completion after cancel will still execute the decrement against a zeroed
counter.

Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[795-833]
Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[572-596]
Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[894-904]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
`CancelPatchCheckCallback()` resets the UI/callback counters but leaves the new GameSpy availability check running. When the availability check finishes, `HTTPThinkWrapper()` decrements `checksLeftBeforeOnline` even though the operation was cancelled, driving the counter negative and corrupting later online flows.
### Issue Context
- Availability completion path decrements `checksLeftBeforeOnline`.
- Cancel path resets `checksLeftBeforeOnline` and destroys the cancel window, but does not call `GSICancelAvailableCheck()` or otherwise prevent the completion path from applying.
### Fix Focus Areas
- Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[795-833]
- Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[572-596]
- Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[894-904]
Suggested direction: on cancel, explicitly cancel the availability check and move `availableCheckResult` to a non-waiting state (or introduce an `availabilityCheckInProgress` flag / generation token) so the completion path cannot decrement after cancellation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Availability dialog repeats forever 🐞 Bug≡ Correctness

Description

On availability failure, startOnline() displays a MessageBoxOk() that invokes
noPatchBeforeOnlineCallback(), but that callback calls startOnline() again when
mustDownloadPatch/cantConnectBeforeOnline are false. This makes dismissing the dialog re-open the
same dialog indefinitely, trapping the user.

Code

Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[R233-239]

+	if (availableCheckResult != GSIACAvailable)+	{+ // GSIACUnavailable / GSIACTemporarilyUnavailable: every SDK would fail anyway.+ MessageBoxOk(TheGameText->fetch("GUI:GSErrorTitle"),+ TheGameText->fetch("GUI:GSDisconReason4"),+ noPatchBeforeOnlineCallback);+ return;

Evidence

The availability failure branch in startOnline() wires the OK callback to
noPatchBeforeOnlineCallback(), and that callback calls startOnline() again whenever both
mustDownloadPatch and cantConnectBeforeOnline are false—creating a repeatable modal loop for
availability failures.

Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[190-240]
Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[141-155]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
The new availability failure handling in `startOnline()` uses `noPatchBeforeOnlineCallback` as the OK callback, but that callback re-enters `startOnline()` under the same conditions, causing an endless modal loop.
### Issue Context
`noPatchBeforeOnlineCallback()` only avoids calling `startOnline()` when `mustDownloadPatch` or `cantConnectBeforeOnline` are true. Availability failure sets neither of those flags.
### Fix Focus Areas
- Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[141-155]
- Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[190-245]
Suggested direction: use a dedicated callback for availability failure that returns to the menu / closes state without calling `startOnline()`, or set a flag that makes `noPatchBeforeOnlineCallback()` choose the non-retry path for availability failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. getGameSpyGameName lacks fallback 🐞 Bug≡ Correctness

Description

getGameSpyGameName() can fall off the end without returning a value when neither RTS_GENERALS nor
RTS_ZEROHOUR is defined, yielding undefined behavior if that configuration is built. The returned
value is passed directly into GSIStartAvailableCheck().

Code

Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[R72-75]

+#elif RTS_ZEROHOUR+	return "ccgenzh";+#endif+}

Evidence

The function body only returns inside the two conditional compilation branches and is used as an
argument to GSIStartAvailableCheck() with no runtime validation.

Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[64-75]
Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[894-904]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
`getGameSpyGameName()` is a non-void function with no `#else` / default return path. In builds where neither `RTS_GENERALS` nor `RTS_ZEROHOUR` is defined, this is undefined behavior and can pass an indeterminate pointer into `GSIStartAvailableCheck()`.
### Issue Context
Even if current build configurations always define one of the macros, this is fragile and can break new targets or misconfigured builds.
### Fix Focus Areas
- Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[64-75]
- Core/GameEngine/Source/GameNetwork/GameSpy/MainMenuUtils.cpp[894-904]
Suggested direction: add a `#else` branch that either returns a safe default, returns `nullptr` with a caller-side guard, or emits a compile-time error (`#error`) so unsupported configurations fail fast.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Peer init failure keeps stale flags 🐞 Bug☼ Reliability

Description

The new peerInitialize() failure branch returns before resetting m_isConnecting/m_isConnected,
which can leave stale connection state if the same PeerThreadClass instance is re-executed. This can
confuse UI/state machines that query isConnecting()/isConnected() after a failed retry.

Code

Core/GameEngine/Source/GameNetwork/GameSpy/Thread/PeerThread.cpp[R1199-1202]

+ PeerResponse resp;+ resp.peerResponseType = PeerResponse::PEERRESPONSE_DISCONNECT;+ resp.discon.reason = DISCONNECT_COULDNOTCONNECT;+ TheGameSpyPeerMessageQueue->addResponse(resp);

Evidence

The early return executes before the existing m_isConnected = m_isConnecting = false; line, while
the message queue can re-run an existing thread object via Execute() without reconstructing it;
separately, m_isConnecting is set true during login handling, demonstrating the flags can hold
non-default values.

Core/GameEngine/Source/GameNetwork/GameSpy/Thread/PeerThread.cpp[1153-1206]
Core/GameEngine/Source/GameNetwork/GameSpy/Thread/PeerThread.cpp[543-557]
Core/GameEngine/Source/GameNetwork/GameSpy/Thread/PeerThread.cpp[1359-1398]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
On `peerInitialize()` failure, the thread enqueues a disconnect response and returns before clearing connection flags. Because `GameSpyPeerMessageQueue::startThread()` can re-`Execute()` an existing `PeerThreadClass` instance, any previously set flags can persist into/through this failure path.
### Issue Context
`m_isConnecting` is set to true during login handling; if the object is reused across attempts, clearing should happen on initialization failure too.
### Fix Focus Areas
- Core/GameEngine/Source/GameNetwork/GameSpy/Thread/PeerThread.cpp[543-557]
- Core/GameEngine/Source/GameNetwork/GameSpy/Thread/PeerThread.cpp[1153-1206]
- Core/GameEngine/Source/GameNetwork/GameSpy/Thread/PeerThread.cpp[1374-1398]
Suggested direction: set `m_isConnecting = m_isConnected = false` (or call `markAsDisconnected()`) before returning in the null-peer branch (and/or right before attempting initialization). Optionally consider recreating the thread object on restart to avoid stale per-thread state.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[Skyaero42]

Were these issues introduced by TheSuperHackers or were they always present?
If introduced by us, any idea which commit/PR caused the issue?

[sokie]

Were these issues introduced by TheSuperHackers or were they always present? If introduced by us, any idea which commit/PR caused the issue?

issue1: async DNS stuff, in #426, "[ZH] Fix constness errors for Zero Hour build" (1647f86, xezon, 2025-03-15).

• -> before the original EA code passed a string literal, which has static storage duration.

issue2: GS availabilty doesn't seem to be anything introduced by you guys, might be just an old mismatch on Gamespy SDK tbh, wouldn't know.

[xezon]

This change has a lot of new comments but nothing that explains the fix at one place. I do not quite understand this change.

[xezon]

I do not understand how this here works. I am unable to review this logic.

[sokie]

@xezon replied below!

[sokie]

and some comments I added as I was debugging this and left in, apologies.
Went through all comments and cleaned them up or tidied them up.

[sokie]

This change has a lot of new comments but nothing that explains the fix at one place. I do not quite understand this change.

@xezon fair, and this is more of a gamespy SDK gap than your guys stuff.
But let me try to explain since I've worked with gamespy more I guess:

Every GameSpy SDK entry point opens with the same guard: if (__GSIACResult != GSIACAvailable) return null;

__GSIACResult starts at GSIACWaiting (gsavailable.c:11) and nothing in the game ever moves it, there is not one reference to GSIStartAvailableCheck, GSIAvailableCheckThink or __GSIACResult anywhere in the tree.
So peerInitialize() returns NULL on every call. DEBUG_ASSERTCRASH compiles out of release, so the null flows into peerSetRoomWatchKeys(), the peer thread faults, and the thread-level catch(...) reports DISCONNECT_LOSTCON and the player sees "Lost connection to C&C Generals Zero Hour Online" for a connection that was never attempted.

startOnline() is what starts the GameSpy threads, so the check has to finish first; it's wired into the existing checksLeftBeforeOnline counter as a fifth prerequisite alongside the four HTTP fetches, so it runs concurrently with them rather than blocking the UI thread for up to two retry timeouts.

This is EA-era code meeting a post-EA SDK: the availability check was added to the SDK on 1.10.36, 10-29-2003 (its own changelog), after Zero Hour shipped, so the game legitimately never called an API that didn't exist.
Since the current SDK linked is more recent, it was now a gap in the init process.

So this adds proper init support now.

[Mauller]

Is this what the game uses to lookup the gamespy server?

If so it would probably be better to make it configurable.

[sokie]

my library and most other patches redirect DNS anyway, but for long term I agree all gamespy DNS records should be configurable so game can be pointed to other services.
I think that is out of scope for this PR.

[bobtista]

This is looking good to me. Could you add test results to the PR description eg:

• a successful connection
• cancelling while the availability check is pending and then retrying
• unavailable-backend response returning to the menu without reopening the dialog?

Whatever new paths that CI can't test