bugfix(mapcache): Close files and free buffers when a map preview copy or a map load 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 throws 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 swallows 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.

loadMap in the map cache scan allocated the waypoint map and threw on a corrupt file without releasing it. A half-transferred map from a LAN game or a broken download in the maps folder leaked the previous map's data on every scan, and its tech and supply positions carried over into the next map's cache entry.

Every failure path now closes the files and frees the buffers, and a corrupt map resets the scan state before throwing.

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

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 12 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 2e805042-1d8f-4a10-a03f-b48f96c3a4ed

📥 Commits

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

📒 Files selected for processing (1)

Core/GameEngine/Source/GameClient/MapUtil.cpp

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
Title check		The title clearly identifies the main change: fixing file-handle and buffer leaks when map preview copying or map loading fails.
Description check		The description directly explains the affected functions, failure paths, resource leaks, and state-reset behavior covered by the changes.
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.

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: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues remain.

Summary

The PR closes files and frees preview buffers when copying fails. It also clears temporary map data when parsing fails.

A failed map load clears its working data before the error continues.
A failed preview copy releases its open files and buffer.

_{Reviews (3) · Last reviewed commit: "bugfix(mapcache): Close files and free b..." · 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

…y or a map load fails

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.

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

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


why should this throw anything?

Skyaero42 · 2026-10-13T07:35:54Z

 	if( file->read( buffer, fileSize ) < fileSize )
 	{
 		DEBUG_CRASH(( "copyFromBigToDir - Error reading from file '%s'", infile.str() ));
+		file->close();


Shouldn't this be called before debug_crash? Otherwise in debug the file isn't closed.

Skyaero42 · 2026-10-13T07:36:20Z

 		DEBUG_CRASH(( "copyFromBigToDir - Error writing to file '%s'", outfile.str() ));
+		if( filenew )
+			filenew->close();
+		delete [] buffer;


nit: whiteline before delete [] buffer; for readablity

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

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

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

bugfix(mapcache): Close files and free buffers when a map preview cop…

7371b20

…y or a map load fails

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

Navigation Menu

Repository navigation

bugfix(mapcache): Close files and free buffers when a map preview copy or a map load fails - #3485

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

tintinhamans commented Oct 12, 2026

Uh oh!

coderabbitai Bot commented Oct 12, 2026 •

edited

Loading

Review limit reached

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!

Skyaero42 Oct 13, 2026

Uh oh!

Skyaero42 Oct 13, 2026

Uh oh!

Skyaero42 Oct 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

tintinhamans commented Oct 12, 2026

Uh oh!

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

Uh oh!

Review limit reached

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: 5/5

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!

Skyaero42 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!

Skyaero42 Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Oct 12, 2026 •

edited

Loading

greptile-apps Bot commented Oct 12, 2026 •

edited

Loading