bugfix(pathutil): Handle both separators when extracting filenames by bobtista · Pull Request #3362 · TheSuperHackers/GeneralsGameCode

bobtista · 2026-09-25T17:15:08Z

Related to bugfix(network): Handle both map path separators #3108

Several filename extractions search only for a backslash. Paths using forward slashes can therefore retain directory names, and some callers use an invalid pointer when no backslash is found.

Use getFileName and getLastPathSeparator from PathUtil.h to handle both separators. For example, the save list fallback label now uses foo.map for Maps/foo/foo.map. Existing map labels remain unchanged.

The changes cover save labels and default descriptions, map cache names, lobby and game setup map names, replay lists, generated stats and replay filenames, audio localization, file transfer helpers, and diagnostic output. Applied to Generals and Zero Hour.

getMapLeafAndDirName, which constructs the portable save path, and Windows-only tools such as WorldBuilder are unchanged. The vertex water map comparison continues to include the separator in the compared names.

Verification:

Focused checks for forward-slash, backslash, mixed-separator and bare filenames
Check that existing map labels are preserved
Verify the fallback label in the in-game save list
Build Generals and Zero Hour on Windows

In-game verification covers the save list fallback label. The other changed call sites have not been tested in game.

coderabbitai · 2026-09-25T17:15:28Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

Walkthrough

The changes replace manual path-separator searches with shared PathUtil functions across map handling, menus, networking, audio, and diagnostic output in both game variants.

Changes

Shared path filename extraction

Layer / File(s)	Summary
Map path handling `Generals*/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp`, `Core/GameEngine/Source/GameClient/MapUtil.cpp`, `Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp`, `Core/GameEngine/Source/GameNetwork/FileTransfer.cpp`	Map-state, map-loading, water-grid, and file-transfer code uses `getFileName` or `getLastPathSeparator` instead of local separator searches.
Map and file name display `Core/GameEngine/Source/Common/INI/INIMapCache.cpp`, `Core/GameEngine/Source/GameNetwork/GameSpy/LobbyUtils.cpp`, `Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/`	Fallback map labels, replay-list names, and download filenames use `getFileName`.
Diagnostic and output filenames `Core/GameEngine/Source/Common/Audio/AudioEventRTS.cpp`, `Core/GameEngine/Source/Common/CRCDebug.cpp`, `Core/GameEngineDevice/Source/`, `Generals/Code/GameEngine/Source/Common/{Recorder.cpp,StatsCollector.cpp}`	Audio, CRC, asset-usage, replay, and statistics code uses `getFileName` for path-derived names.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: xezon

Merge Risk: 🔵 Low · up to 10a87

Maps with mixed separator styles can miss their water-grid settings. This is a narrow, pre-existing limitation, so the change is mergeable with owner awareness.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 10a87

The change lets forward-slash map paths resolve to the intended filenames during transfers. Receiving clients retain destination and content checks, and no new security bypass was established. Some uncertainty remains about where all map paths originate.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The relevant exposure is a game host sending map files to peers that lack the map, rather than a new service or deployment privilege. Forward-slash paths can now identify auxiliary files beside that map.

Trust Boundaries and Controls

observed — The receiver rejects failed destination normalization, disallowed extensions, and invalid content before opening the destination for writing. Destination conversion checks containment in the applicable map or save directory.
observed — Auxiliary transfers use fixed sibling filenames, while the original map path was already sent directly. The inspected change does not give the extracted filename control over an arbitrary auxiliary destination.

Resilience and Maintainability Implications

observed — The sequential transfer loop and timeout remain outside the changed extraction helpers; rejected receiver paths return before a write. No new retry or cleanup transition was identified.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly states the main change: path filename extraction now handles both separators. It is concise and specific.
Description check	✅ Passed	The description directly explains the path separator bug, the utility changes, affected areas, scope, and verification performed.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

greptile-apps · 2026-09-26T17:00:12Z

Confidence Score: 5/5

[Medium risk] Refactors path parsing to use centralized utility functions.

The PR appears safe to merge; no new actionable failure was identified.

Summary

The PR replaces backslash-only filename extraction with path helpers that recognize both separators across shared engine code and both game variants. The changes since the previous review simplify file-transfer helpers and diagnostic formatting, retain an owned map path while generating replay filenames, and adjust the terrain name comparison. No new actionable issue was established.

_{Reviews (6) · Last reviewed commit: "refactor(generals): Align filename helpe..."}

xezon

Looks like it can be simplified

xezon

Looks plausible. Can you make a full audit over the code if there are any other places like that and fix them too?

bobtista · 2026-09-29T20:33:34Z

Done. Audited all places that take a file name from a path with only a backslash and switched them to getFileName or getLastPathSeparator. I left the windows-only tools stuff eg WorldBuilder and getMapLeafAndDirName alone

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 822dca59-ce18-4fcc-ae4a-c01b2145c2d2

📥 Commits

Reviewing files that changed from the base of the PR and between 312cf98 and 10a872c.

📒 Files selected for processing (29)

Core/GameEngine/Source/Common/Audio/AudioEventRTS.cpp
Core/GameEngine/Source/Common/CRCDebug.cpp
Core/GameEngine/Source/Common/INI/INIMapCache.cpp
Core/GameEngine/Source/GameClient/MapUtil.cpp
Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp
Core/GameEngine/Source/GameNetwork/FileTransfer.cpp
Core/GameEngine/Source/GameNetwork/GameSpy/LobbyUtils.cpp
Core/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp
Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp
Generals/Code/GameEngine/Source/Common/Recorder.cpp
Generals/Code/GameEngine/Source/Common/StatsCollector.cpp
Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/DownloadMenu.cpp
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/GameInfoWindow.cpp
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/LanGameOptionsMenu.cpp
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/PopupSaveLoad.cpp
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLGameSetupMenu.cpp
Generals/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp
GeneralsMD/Code/GameEngine/Source/Common/Recorder.cpp
GeneralsMD/Code/GameEngine/Source/Common/StatsCollector.cpp
GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/DownloadMenu.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/GameInfoWindow.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/LanGameOptionsMenu.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/PopupSaveLoad.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLGameSetupMenu.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

coderabbitai · 2026-09-29T20:39:03Z


 			// create stripped map name
-			c = strrchr( TheGlobalData->m_mapName.str(), '\\' );
+			c = getLastPathSeparator( TheGlobalData->m_mapName.str() );


🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Compare map filenames without their separators.

When the map path and configured path use different separators, the fallback compares strings such as /CHI01.map and \CHI01.map. The comparison fails, so the water-grid settings are not applied. Advance each separator pointer before setting the stripped name.

Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp#L1149-L1149: Set strippedMapNameOnly from c + 1.

Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp#L1156-L1156: Set strippedCompareMapNameOnly from c + 1.

Generals/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp#L1149-L1149: Set strippedMapNameOnly from c + 1.

Generals/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp#L1156-L1156: Set strippedCompareMapNameOnly from c + 1.

📍 Affects 2 files

Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp#L1149-L1149 (this comment)

Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp#L1156-L1156

Generals/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp#L1149-L1149

Generals/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp#L1156-L1156

…n from a path

xezon

Can see further polishing

xezon · 2026-09-30T16:42:22Z

-				strippedCompareMapNameOnly = TheGlobalData->m_vertexWaterAvailableMaps[ i ];
+			// TheSuperHackers @bugfix bobtista 29/09/2026 Match water settings regardless of path separators.
+			AsciiString strippedMapNameOnly = getFileName( TheGlobalData->m_mapName.str() );
+			AsciiString strippedCompareMapNameOnly = getFileName( TheGlobalData->m_vertexWaterAvailableMaps[ i ].str() );


Can we also make most of these const char* then?

xezon · 2026-09-30T17:33:10Z

-	return path;
-}
-
 AsciiString GetExtensionFromFile( AsciiString fname )


This function appears to be uncalled. Remove?

xezon · 2026-09-30T17:35:44Z


 			// now try this compare
-			if( strippedMapNameOnly.compareNoCase( strippedCompareMapNameOnly.str() ) == 0 )
+			if( _stricmp( strippedMapNameOnly, strippedCompareMapNameOnly ) == 0 )


stricmp is used dominantly

xezon · 2026-09-30T17:41:11Z

-			const char *fname = mapName.reverseFind('\\');
-			if (fname)
-				mapName = fname+1;
+			AsciiString mapName = getFileName(game->getMap().str());


const char* ?

xezon · 2026-09-30T17:44:48Z

While at it, this mumbo jumbo can be simplified by using constructor AsciiString(const char* s, int len)

xezon · 2026-09-30T17:45:32Z

Same mumbo jumbo here.

xezon · 2026-09-30T17:47:05Z

@@ -183,7 +183,7 @@ void outputCRCDumpLines()

 static AsciiString getFname(AsciiString path)


Maybe remove this function too

bobtista self-assigned this Sep 25, 2026

bobtista added Bug Something is not working right, typically is user facing Minor Severity: Minor < Major < Critical < Blocker labels Sep 25, 2026

bobtista marked this pull request as ready for review September 26, 2026 16:58

xezon reviewed Sep 27, 2026

View reviewed changes

Comment thread Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp Outdated

bobtista force-pushed the bobtista/bugfix/save-map-label-separators branch from ffcf008 to 312cf98 Compare September 27, 2026 19:28

xezon approved these changes Sep 29, 2026

View reviewed changes

coderabbitai Bot reviewed Sep 29, 2026

View reviewed changes

bobtista changed the title ~~bugfix(savegame): Handle both separators in fallback map labels~~ bugfix(pathutil): Handle both separators when extracting filenames Sep 29, 2026

bobtista added 8 commits September 29, 2026 18:14

bugfix(savegame): Handle both separators in fallback map labels

74edba2

bugfix(generals): Handle both separators in fallback map labels

7409582

refactor(savegame): Reuse the filename helper for fallback labels

91077cb

refactor(generals): Reuse the filename helper for fallback labels

b257e76

bugfix(savegame): Handle both separators wherever a file name is take…

aed1f54

…n from a path

bugfix(generals): Handle both separators wherever a file name is take…

55a0bca

…n from a path

bugfix(terrain): Compare water map filenames without separators

b894e78

bugfix(generals): Compare water map filenames without separators

a426ff8

bobtista force-pushed the bobtista/bugfix/save-map-label-separators branch from 10a872c to a426ff8 Compare September 29, 2026 22:21

xezon reviewed Sep 30, 2026

View reviewed changes

bobtista added 2 commits September 30, 2026 12:58

refactor(pathutil): Apply filename review cleanup

bdc3801

refactor(generals): Align filename review cleanup

43a93df

xezon reviewed Sep 30, 2026

View reviewed changes

bobtista added 2 commits September 30, 2026 17:57

refactor(pathutil): Simplify filename helpers and avoid string copies

45350f0

refactor(generals): Align filename helper cleanup

80c84b6

		@@ -183,7 +183,7 @@ void outputCRCDumpLines()

		static AsciiString getFname(AsciiString path)

Conversation

bobtista commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Security Architecture Review

Uh oh!

greptile-apps Bot commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

bobtista commented Sep 29, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xezon Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xezon Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bobtista commented Sep 25, 2026 •

edited

Loading

coderabbitai Bot commented Sep 25, 2026 •

edited

Loading

greptile-apps Bot commented Sep 26, 2026 •

edited

Loading