fix(gamespy): run availability check, fix async DNS hostname lifetime by sokie · Pull Request #3166 · TheSuperHackers/GeneralsGameCode

sokie · 2026-08-18T09:41:43Z

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 · 2026-08-18T09:42:31Z

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

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 · 2026-08-18T09:46:11Z

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 · 2026-08-18T10:05:05Z

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

sokie · 2026-08-18T10:14:33Z

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 · 2026-08-18T10:34:12Z

 	s_asyncDNSLookupInProgress = TRUE;
-	Char hostname[] = "servserv.generals.ea.com";
+	// static: see HTTPThinkWrapper().
+	static Char hostname[] = "servserv.generals.ea.com";


Hmm right I did not pay attention to this back in the day.

Instead, can we perhaps make asyncGethostbyname take const char* so that we can pass the string pointer again? But that requires a const_cast in it perhaps.

@xezon since this is the first time I'm touching this codebase I just wanted to keep the PR self-contained ( and this basically restores behavior to the previous EA one ).
~~Happy to expand this if you prefer.~~

@xezon done, the blast radius was smaller than I thought!

asyncGethostbyname() passes its argument to CreateThread and returns immediately, so the stack-local hostname introduced at both call sites in TheSuperHackers#426 was dead before the lookup thread read it; take const char* instead and pass the string literal directly, as the original code did. 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 Bot reviewed Aug 18, 2026

View reviewed changes

sokie force-pushed the fix/gamespy-online-init branch from 8f276c3 to a9804d3 Compare August 18, 2026 09:54

sokie force-pushed the fix/gamespy-online-init branch from a9804d3 to 08d348e Compare August 18, 2026 10:19

xezon reviewed Aug 18, 2026

View reviewed changes

sokie force-pushed the fix/gamespy-online-init branch from 08d348e to 1af5e89 Compare August 18, 2026 10:44

Skyaero42 added ThisProject The issue was introduced by this project, or this task is specific to this project Fix Is fixing something, but is not user facing labels Aug 18, 2026

Navigation Menu

Uh oh!

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

fix(gamespy): run availability check, fix async DNS hostname lifetime#3166
sokie wants to merge 1 commit into
TheSuperHackers:mainfrom
sokie:fix/gamespy-online-init

sokie commented Aug 18, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 18, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 18, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Skyaero42 commented Aug 18, 2026

Uh oh!

sokie commented Aug 18, 2026 •

edited

Loading

Uh oh!

xezon Aug 18, 2026

Uh oh!

sokie Aug 18, 2026 •

edited

Loading

Uh oh!

sokie Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

sokie commented Aug 18, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 18, 2026

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 18, 2026

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Skyaero42 commented Aug 18, 2026

Uh oh!

sokie commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xezon Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

sokie Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sokie Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sokie commented Aug 18, 2026 •

edited

Loading

sokie Aug 18, 2026 •

edited

Loading