fix: Handle both path separators when parsing paths

[bobtista]

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

Many path checks 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, getLastPathSeparator and isPathSeparator 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 and map preview names, lobby and game setup map names, replay lists, generated stats and replay filenames, audio localization, file transfer helpers, particle and model texture names, the file factory, the download helpers, executable paths, diagnostic output, the debug library, WorldBuilder, W3DView, GUIEdit, ImagePacker, Autorun, Launcher, wdump and max2w3d. Applied to Generals and Zero Hour.

PathUtil.h gains non-const getLastPathSeparator overloads and a wchar_t overload of isPathSeparator.

Unchanged:

• getMapLeafAndDirName, which constructs the portable save path (bugfix(network): Handle both map path separators #3108)
• The window layout loader, because relative layout names such as Menus/MessageBox.wnd rely on / not being a separator
• Backslashes that are not path separators: text escapes, the GameSpy key/value delimiter, registry keys and UNC path checks
• Code that already converts backslashes to forward slashes

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, Zero Hour and all tools on Windows
• Launch Generals and Zero Hour to the main menu

In-game verification covers the save list fallback label. The other changed call sites have not been tested in game, and the tools have not been run. max2w3d was not built because it needs the 3ds Max SDK.

[coderabbitai]

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		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly describes the main change: handling both path separators when parsing paths.
Description check		The description explains the path-separator issue, the changes, and the reported verification. It is directly related to the changeset.

---

---

---

---

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

• X
• Mastodon
• Reddit
• LinkedIn

---

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

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge based on the changes since the previous review.

Summary

The PR updates path parsing across Generals, Zero Hour, shared libraries, and tools to recognize both slash styles. Since the previous review, it added wide-character first-separator support and a shared reverse-separator helper. No new actionable issue was established.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Game and tool paths] --> B[PathUtil separator helpers]
  B --> C[Last-separator search in stringex]
  B --> D[Filename, directory, and extension handling]
  D --> E[Labels, assets, saves, replays, and tools]

Loading

_{Reviews (15) · Last reviewed commit: "refactor(pathutil): Add strrpbrk for get..." · Reviewed by Greptile}

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

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.

[xezon]

Can see further polishing

[xezon]

Very nice improvements. Does this cover all of them or is something left hanging?

[xezon]

Open question is left.

[bobtista]

I found and fixed a couple more directory checks in GameInfo and one in the window script loader. Then there's getMapLeafAndDirName, which I fixed in #3108. I didn't touch Windows-specific path handling, tools and the unused Get_W3D_Name helper

[xezon]

The bot has a complaint.

[bobtista]

The bot was right. Layout names like Menus/MessageBox.wnd lost the Window\ prefix. I reverted the window script loader change.

[xezon]

There are more slashes in CWorldBuilderDoc::OnOpenDocument

And there are a lot more in many places using strrchr

This changes needs more auditing

[greptile-apps]

Comments Outside Diff

These findings could not be posted inline.

•  Full blend table stores invalid index Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp:1197 ▶

  When adding a third terrain blend, the first lookup can use the last available blend entry. The new second lookup then returns -1 because the table is full, but that value is stored as the cell's primary blend index. Subsequent editing can use an invalid index, and the map can be saved with invalid terrain data.

[bobtista]

I audited every '\\' character literal in Core, Generals and Zero Hour, listed with:

git grep -nF "'\\\\'" -- Core Generals GeneralsMD

All path parsing now handles both separators: engine code, Windows API paths, the debug library, WorldBuilder and the other tools, one commit per area. The remaining hits are not file paths: text escapes in GameText and Babylon, the GameSpy key/value delimiter, registry keys, UNC checks, and code that already converts backslashes to slashes. The window layout loader stays as it is, because relative layout names rely on '/' not being a separator.

[xezon]

Incomplete review. A few notes.

[xezon]

There are more strpbrk calls in the code base

[bobtista]

Done. Added getFirstPathSeparator for the forward searches. I also found a few more places that checked for separators by hand and switched them to the PathUtil helpers

[xezon]

Lot's of magical things in this code base.

[xezon]

How about we also use the strpbrk approach for this function? This means we need a reverse strpbrk. It does not exist in C. I asked Chat Gippy and it produced this:

Try add this to stringex.h and then simplify these.

char *strrpbrk(const char *str, const char *accept)
{
    const char *last = nullptr;

    for (; *str; ++str) {
        if (strchr(accept, *str)) {
            last = str;
        }
    }

    return (char *)last;
}

[bobtista]

Done

[xezon]

Should also add a wchar_t version for good measure.

[bobtista]

Done