Skip to content

bugfix(memory): Harden memory manager edge cases#2744

Open
IbrahimAlzaidi wants to merge 1 commit into
TheSuperHackers:mainfrom
IbrahimAlzaidi:bugfix/memory_manager_hardening
Open

bugfix(memory): Harden memory manager edge cases#2744
IbrahimAlzaidi wants to merge 1 commit into
TheSuperHackers:mainfrom
IbrahimAlzaidi:bugfix/memory_manager_hardening

Conversation

@IbrahimAlzaidi
Copy link
Copy Markdown

Prevent MemoryPool::releaseEmpties() from freeing the final blob so fixed-size pools remain allocatable after cleanup.

Parse MemoryPools.ini override lines with bounded token handling so malformed or overlong pool names cannot overflow the local buffer or alter defaults accidentally.

Add sized scalar and array delete overloads for both real and null memory manager builds so VS2022/C++20 sized deallocation paths free through the same allocator routes as existing unsized deletes.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 24, 2026

Greptile Summary

This PR hardens three distinct memory-manager edge cases: prevents releaseEmpties() from freeing the sole remaining blob, replaces unbounded sscanf pool-override parsing with a length-bounded strtol-based parser, and adds sized-deallocation operator overloads for both the real and null allocator builds to satisfy VS2022/C++20 sized-delete dispatch.

  • releaseEmpties fix (GameMemory.cpp): a pre-loop countBlobsInPool() establishes a running count; the blobCount > 1 guard skips the final blob while --blobCount keeps the count accurate as blobs are freed.
  • Parser hardening (GameMemoryInit.cpp): parseMemoryPoolOverrideLine() enforces a 255-character name cap, detects blank/comment lines early, and uses strtol with ERANGE checking; the old sscanf "%s" had no length bound on poolName[256].
  • Sized-delete overloads (GameMemory.cpp, GameMemoryNull.cpp): operator delete(void*, size_t) and operator delete[](void*, size_t) now route through the same allocator paths as their unsized counterparts, preventing the C++14 sized-deallocation mechanism from bypassing the custom allocator.

Confidence Score: 5/5

All three changes are narrowly scoped fixes with no new allocator paths, no data structure changes, and no behaviour regressions detectable from static analysis.

The releaseEmpties guard is correct and thread-safe under the existing critical section. The new parser returns false for every malformed input rather than writing partial data. The sized-delete overloads are faithful mirrors of the unsized versions. No existing call sites are affected by the additions.

No files require special attention; GameMemoryInit.cpp has one minor comment-detection ordering nit noted inline.

Important Files Changed

Filename Overview
Core/GameEngine/Source/Common/System/GameMemory.cpp Adds blobCount > 1 guard in releaseEmpties() to preserve the last blob, and adds sized operator delete/operator delete[] overloads routing through TheDynamicMemoryAllocator.
Core/GameEngine/Source/Common/System/GameMemoryInit.cpp Replaces unbounded sscanf with a hardened parseMemoryPoolOverrideLine() that enforces a name-length cap, strtol-based numeric parsing with ERANGE checks, and test-hook gating under MEMORY_MANAGER_HARDENING_TESTS.
Core/GameEngine/Source/Common/System/GameMemoryNull.cpp Adds sized operator delete(void*, size_t) and operator delete[](void*, size_t) stubs that discard the size and forward to free(), matching the real allocator additions.
Core/GameEngine/Include/Common/GameMemory.h Adds userMemoryManagerParsePoolOverrideLineForTest declaration behind MEMORY_MANAGER_HARDENING_TESTS guard for white-box unit testing of the new parser.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
Core/GameEngine/Source/Common/System/GameMemoryInit.cpp:113-121
The `; comment` fast-path check runs before leading whitespace is stripped, so a line like `"  ; this is a comment"` will not be caught here. Instead, it falls through to the name-token loop where `";"` gets parsed as the pool name, and only fails later when `strtol` finds no valid integer. The function still returns `false` (no data is corrupted), but the comment-detection intent is defeated. Moving the check to after the whitespace-skip makes the behavior match the documented purpose.

```suggestion
	const char* cursor = line;
	while (isspace(static_cast<unsigned char>(*cursor)))
		++cursor;

	if (*cursor == '\0' || *cursor == ';')
		return false;
```

Reviews (2): Last reviewed commit: "bugfix(memory): Harden memory manager ed..." | Re-trigger Greptile

Comment thread Core/GameEngine/Source/Common/System/GameMemoryInit.cpp Outdated
Prevent MemoryPool::releaseEmpties() from freeing the final blob so fixed-size pools remain allocatable after cleanup.

Parse MemoryPools.ini override lines with bounded token handling so malformed or overlong pool names cannot overflow the local buffer or alter defaults accidentally.

Add sized scalar and array delete overloads for both real and null memory manager builds so VS2022/C++20 sized deallocation paths free through the same allocator routes as existing unsized deletes.

Validation: openspec validate memory-manager-hardening --type change --strict; Release z_gameengine builds for real and null memory-manager configurations.
@IbrahimAlzaidi IbrahimAlzaidi force-pushed the bugfix/memory_manager_hardening branch from 53f566d to 2bce9ef Compare May 24, 2026 15:29
Copy link
Copy Markdown

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these are 3 isolated fixes/improvements, then they need to go into individual pulls.

// SYSTEM INCLUDES
#include <ctype.h>
#include <errno.h>
#include <stdlib.h>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ctype.h and stdlib.h already come with PreRTS.h

errno.h is maybe already included as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants