bugfix(mapcache): Close the file and free the buffer when a map preview copy fails by tintinhamans · Pull Request #3485 · TheSuperHackers/GeneralsGameCode

tintinhamans · 2026-10-12T01:26:12Z

copyFromBigToDir extracts a map's preview image into the user folder and threw on failure. Every throw left the source file open, and the read and write failures also left the 50 to 70 KB buffer behind. getMapPreviewImage swallowed the throw and never caches the result, so the leak repeats on every preview request for that map.

This bites whenever writing to the user folder fails: a Documents folder that OneDrive is syncing (#458), a read-only or full disk, or a preview locked by an antivirus scan. Each map browsed in the skirmish or LAN picker then costs a file handle and a buffer, again on every refresh.

copyFromBigToDir now returns a Bool instead of throwing, so the file and the buffer are released in one place on every path.

coderabbitai · 2026-10-12T01:26:41Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 5f775637-39be-484b-813c-a681c7075210

📥 Commits

Reviewing files that changed from the base of the PR and between 5a61292 and 5a20064.

📒 Files selected for processing (1)

Core/GameEngine/Source/GameClient/MapUtil.cpp

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

Walkthrough

loadMap resets map state when parsing fails or throws. copyFromBigToDir closes files and frees its buffer on failure.

Changes

Map failure-path cleanup

Layer / File(s)	Summary
Map parsing failure cleanup `Core/GameEngine/Source/GameClient/MapUtil.cpp`	`loadMap` resets map state when parsing fails or throws. A false parse result still produces `ERROR_CORRUPT_FILE_FORMAT`.
File-copy failure cleanup `Core/GameEngine/Source/GameClient/MapUtil.cpp`	`copyFromBigToDir` closes the source file if buffer allocation throws. On short reads, output-open failures, or short writes, it closes opened files and frees the buffer before throwing `SC_INVALID_DATA`.

Priority: ➖ Normal

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 5a200

Map parsing and preview-copy failure paths now clean up the reported state and resources. The remaining unchanged exception path adds no new merge risk in this PR.

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 preview-copy fix: closing the file and freeing the buffer after a failure. It does not mention the separate map-load cleanup, but it remains accurate and specific.
Description check		The description directly explains the resource leaks in `copyFromBigToDir`, the failure conditions, and the intended cleanup behavior. It is related to the changeset.

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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-10-12T01:29:45Z

Confidence Score: 4/5

[Medium impact] Changes error handling in map file copy operation.

Restore the preview failure catch before merging so allocation errors do not interrupt map selection.

Findings

Preview errors interrupt map selection ▶

Summary

The PR makes copyFromBigToDir return success or failure and closes files and frees the buffer after ordinary read or write failures.

Map preview copies release their files and buffer when they fail.

_{Reviews (4) · Last reviewed commit: "bugfix(mapcache): Close the file and fre..." · Reviewed by Greptile}

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 7e468575-3b0d-4752-acb2-eca8a9b6c528

📥 Commits

Reviewing files that changed from the base of the PR and between 0e6a06c and 5a61292.

📒 Files selected for processing (1)

Core/GameEngine/Source/GameClient/MapUtil.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.

macroscopeapp · 2026-10-12T20:44:43Z

+	}
+	catch (...)
+	{
+		resetMap();


🟡 Medium GameClient/MapUtil.cpp:245

A corrupt map that parses WorldInfo before failing leaves worldDict and scalar map state populated, so a later scan can attribute stale metadata such as m_nameLookupTag to another map without valid world info. resetMap() does not clear that parsed state; clear it on failure along with the other map state.

🤖 Copy this AI Prompt to have your agent fix this:

In file @Core/GameEngine/Source/GameClient/MapUtil.cpp around line 245: A corrupt map that parses `WorldInfo` before failing leaves `worldDict` and scalar map state populated, so a later scan can attribute stale metadata such as `m_nameLookupTag` to another map without valid world info. `resetMap()` does not clear that parsed state; clear it on failure along with the other map state.

resetMap now clears worldDict

Skyaero42 · 2026-10-13T07:32:03Z

+		if (!file.parse(nullptr))
+			throw ERROR_CORRUPT_FILE_FORMAT;
+	}
+	catch (...)


throwing and catching in the same method is imho not a very good design.

Took the loadMap change out of this PR. That throw ends up in the engine’s uncaught exception handler anyway.

Skyaero42 · 2026-10-13T07:34:48Z

+	char *buffer = nullptr;
+	try
+	{
+		buffer = NEW char[ fileSize ];


why should this throw anything?

Function returns a Bool instead of throwing now.

…ew copy fails

tintinhamans force-pushed the arctic/fix/mapcache-error-leaks branch from 3b29348 to 5a61292 Compare October 12, 2026 01:26

greptile-apps Bot reviewed Oct 12, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameClient/MapUtil.cpp Outdated

coderabbitai Bot reviewed Oct 12, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameClient/MapUtil.cpp Outdated

Comment thread Core/GameEngine/Source/GameClient/MapUtil.cpp Outdated

tintinhamans marked this pull request as draft October 12, 2026 12:11

tintinhamans force-pushed the arctic/fix/mapcache-error-leaks branch from 5a61292 to 5a20064 Compare October 12, 2026 20:41

tintinhamans marked this pull request as ready for review October 12, 2026 20:43

macroscopeapp Bot reviewed Oct 12, 2026

View reviewed changes

tintinhamans force-pushed the arctic/fix/mapcache-error-leaks branch from 5a20064 to 7371b20 Compare October 12, 2026 20:50

tintinhamans added Gen Relates to Generals ZH Relates to Zero Hour Memory Is memory related Minor Severity: Minor < Major < Critical < Blocker Fix Is fixing something, but is not user facing labels Oct 12, 2026

Skyaero42 reviewed Oct 13, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameClient/MapUtil.cpp Outdated

Skyaero42 reviewed Oct 13, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameClient/MapUtil.cpp Outdated

bugfix(mapcache): Close the file and free the buffer when a map previ…

1b06706

…ew copy fails

tintinhamans force-pushed the arctic/fix/mapcache-error-leaks branch from 7371b20 to 1b06706 Compare October 13, 2026 20:20

tintinhamans changed the title ~~bugfix(mapcache): Close files and free buffers when a map preview copy or a map load fails~~ bugfix(mapcache): Close the file and free the buffer when a map preview copy fails Oct 13, 2026

greptile-apps Bot reviewed Oct 13, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameClient/MapUtil.cpp

Navigation Menu

Repository navigation

bugfix(mapcache): Close the file and free the buffer when a map preview copy fails - #3485

bugfix(mapcache): Close the file and free the buffer when a map preview copy fails#3485
tintinhamans wants to merge 1 commit into
TheSuperHackers:mainfrom
tintinhamans:arctic/fix/mapcache-error-leaks

tintinhamans commented Oct 12, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 12, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Oct 12, 2026 •

edited

Loading

Summary

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

macroscopeapp Bot Oct 12, 2026

Uh oh!

tintinhamans Oct 12, 2026

Uh oh!

Skyaero42 Oct 13, 2026

Uh oh!

tintinhamans Oct 13, 2026

Uh oh!

Skyaero42 Oct 13, 2026

Uh oh!

tintinhamans Oct 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

tintinhamans commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Walkthrough

Changes

Uh oh!

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

Uh oh!

Confidence Score: 4/5

Findings

Summary

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

macroscopeapp Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

tintinhamans Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

Skyaero42 Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

tintinhamans Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

Skyaero42 Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

tintinhamans Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tintinhamans commented Oct 12, 2026 •

edited

Loading

coderabbitai Bot commented Oct 12, 2026 •

edited

Loading

greptile-apps Bot commented Oct 12, 2026 •

edited

Loading