chore(garrisoncontain): Add assert for condition index in GarrisonContain::findClosestFreeGarrisonPointIndex() by CryoTheRenegade · Pull Request #3357 · TheSuperHackers/GeneralsGameCode

CryoTheRenegade · 2026-09-25T04:50:01Z

findClosestFreeGarrisonPointIndex() previously used conditionIndex to access the garrison point array without checking its bounds.

My solution is to return GARRISON_INDEX_INVALID when the index is negative or at least MAX_GARRISON_POINT_CONDITIONS. This prevents out-of-bounds reads and lets callers use their existing handling for unavailable garrison points.

Found by clang-tidy

Note

Fix out-of-bounds access in `GarrisonContain.findClosestFreeGarrisonPointIndex` for invalid condition indices

Adds bounds checking for conditionIndex in GarrisonContain.cpp, rejecting values below zero or at/above the maximum supported condition count. The same fix applies to the GeneralsMD copy at GarrisonContain.cpp. When the index is invalid, the method returns GARRISON_INDEX_INVALID without scanning garrison points.

📊 Macroscope summarized 968a1cf. 2 files reviewed, 2 issues evaluated, 1 issue filtered, 1 comment posted

🗂️ Filtered Issues

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/GarrisonContain.cpp — 0 comments posted, 1 evaluated, 1 filtered

line 99: The new check is only a DEBUG_ASSERTCRASH; in release builds DEBUG_ASSERTCRASH expands to ((void)0), so an invalid conditionIndex still reaches m_garrisonPoint[conditionIndex][i] at line 117 and performs the out-of-bounds read this change is intended to prevent. The condition must be handled with a real runtime guard that returns GARRISON_INDEX_INVALID. [ Cross-file consolidated ]

…rrison and return `GARRISON_INDEX_INVALID`

coderabbitai · 2026-09-25T04:50:19Z

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

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: a5c5dc89-e336-422e-83ca-570a1e5b6ab8

📥 Commits

Reviewing files that changed from the base of the PR and between 968a1cf and cac6d36.

📒 Files selected for processing (2)

Generals/Code/GameEngine/Source/GameLogic/Object/Contain/GarrisonContain.cpp
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/GarrisonContain.cpp

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Walkthrough

Both game variants now compile the conditionIndex validity assertion only under the existing debug or non-retail-compatible CRC condition.

Changes

Garrison assertion build guard

Layer / File(s)	Summary
Guard the condition-index assertion `Generals/Code/GameEngine/Source/GameLogic/Object/Contain/GarrisonContain.cpp`, `GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/GarrisonContain.cpp`	Both implementations place the `conditionIndex` validity assertion inside the existing conditional-compilation block.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Suggested reviewers: xezon

Merge Risk: ⚪ Minimal · up to cac6d

The change adds a guarded diagnostic without altering supported gameplay behavior. No actionable merge-blocking risk is established.

Architecture Summary

Architecture risk: 🔵 Low · up to cac6d

The change affects 2 systems.

Changed systems: Generals, GeneralsMD

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

observed — Generals (service) was modified; 1 changed file maps to changed impact.
observed — GeneralsMD (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

observed — Modified behavior in Generals/Code/GameEngine/Source/GameLogic/Object/Contain/GarrisonContain.cpp: The conditionIndex validity assertion is now included in the existing RTS_DEBUG || !RETAIL_COMPATIBLE_CRC block; it is no longer compiled for other configurations.
observed — Modified behavior in GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/GarrisonContain.cpp: The conditionIndex validity assertion is now within the RTS_DEBUG or non-retail-compatible CRC conditional block; it is no longer compiled outside that condition.

🚥 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 identifies the addition of a condition-index assertion in `GarrisonContain::findClosestFreeGarrisonPointIndex`, which matches the changeset.
Description check	✅ Passed	The description directly addresses condition-index validation and out-of-bounds access in the modified function, so it is related to the changeset.

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

greptile-apps · 2026-09-25T04:51:49Z

Confidence Score: 5/5

[Low risk] Adds debug assertions to garrison condition validation.

No new blocking issue was established by the changes since the previous review.

Summary

The PR adds a condition-index assertion to findClosestFreeGarrisonPointIndex() in both game variants. Since the previous review, the assertion has been moved inside the existing debug-or-non-retail-CRC guard.

_{Reviews (3) · Last reviewed commit: "fix(GarrisonContain): Preserve replay co..."}

macroscopeapp · 2026-09-25T04:52:44Z

Approvability

Verdict: Approved at b699282

Macroscope's review found this PR approvable — This is a narrowly scoped defensive fix in the Generals and GeneralsMD garrison implementations. It prevents invalid condition indices from reaching a fixed-size array while preserving existing behavior for valid callers.

^{You can add or adjust custom eligibility rules. Learn more.}

Skyaero42 · 2026-09-25T06:31:04Z


 	// sanity
-	if( targetPos == nullptr || m_garrisonPointsInUse == MAX_GARRISON_POINTS )
+	if (targetPos == nullptr || m_garrisonPointsInUse == MAX_GARRISON_POINTS ||


An assert would be better here.

conditionIndex is determined by GarrisonContain::findConditionIndex(). and only has an out of bound value due to a coding issue, not a runtime issue.

xezon · 2026-09-25T06:32:32Z

 	// sanity
-	if( targetPos == nullptr || m_garrisonPointsInUse == MAX_GARRISON_POINTS )
+	if (targetPos == nullptr || m_garrisonPointsInUse == MAX_GARRISON_POINTS ||
+		conditionIndex < 0 || conditionIndex >= MAX_GARRISON_POINT_CONDITIONS)


This can never be false on runtime. The function is protected and all callers pass an already validated conditionIndex. Assert.

Of course the review bots are happy anyway...

Skyaero42

Bad bot

macroscopeapp · 2026-09-25T17:27:31Z

+	DEBUG_ASSERTCRASH(conditionIndex >= 0 && conditionIndex < MAX_GARRISON_POINT_CONDITIONS,
+		("GarrisonContain::findClosestFreeGarrisonPointIndex - Invalid condition index '%d'", conditionIndex));


🟠 High Contain/GarrisonContain.cpp:98

In release builds, an invalid conditionIndex still reaches m_garrisonPoint[conditionIndex][i], causing an out-of-bounds read because DEBUG_ASSERTCRASH expands to a no-op. Add a runtime guard that returns GARRISON_INDEX_INVALID after the assertion.

Suggested change

DEBUG_ASSERTCRASH(conditionIndex >= 0 && conditionIndex < MAX_GARRISON_POINT_CONDITIONS,

("GarrisonContain::findClosestFreeGarrisonPointIndex - Invalid condition index '%d'", conditionIndex));

DEBUG_ASSERTCRASH(conditionIndex >= 0 && conditionIndex < MAX_GARRISON_POINT_CONDITIONS,

("GarrisonContain::findClosestFreeGarrisonPointIndex - Invalid condition index '%d'", conditionIndex));

if( conditionIndex < 0 || conditionIndex >= MAX_GARRISON_POINT_CONDITIONS )

return GARRISON_INDEX_INVALID;

Also found in 1 other location(s)

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/GarrisonContain.cpp:99

The new check is only a DEBUG_ASSERTCRASH; in release builds DEBUG_ASSERTCRASH expands to ((void)0), so an invalid conditionIndex still reaches m_garrisonPoint[conditionIndex][i] at line 117 and performs the out-of-bounds read this change is intended to prevent. The condition must be handled with a real runtime guard that returns GARRISON_INDEX_INVALID.

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

In file @Generals/Code/GameEngine/Source/GameLogic/Object/Contain/GarrisonContain.cpp around lines 98-99: In release builds, an invalid `conditionIndex` still reaches `m_garrisonPoint[conditionIndex][i]`, causing an out-of-bounds read because `DEBUG_ASSERTCRASH` expands to a no-op. Add a runtime guard that returns `GARRISON_INDEX_INVALID` after the assertion. Also found in 1 other location(s): - GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/GarrisonContain.cpp:99 -- The new check is only a `DEBUG_ASSERTCRASH`; in release builds `DEBUG_ASSERTCRASH` expands to `((void)0)`, so an invalid `conditionIndex` still reaches `m_garrisonPoint[conditionIndex][i]` at line 117 and performs the out-of-bounds read this change is intended to prevent. The condition must be handled with a real runtime guard that returns `GARRISON_INDEX_INVALID`.

The runtime guard was deliberately replaced with an assertion following xezon’s and Skyaero42’s reviews. They confirmed that callers supply valid condition indices and that an invalid index indicates a programming error. The assertion documents that invariant.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

The assertion does document the invariant, but it compiles out in retail. findConditionIndex() can return GARRISON_INDEX_INVALID for an unrecognized BodyDamageType, so that path still indexes m_garrisonPoint out of bounds. The runtime guard remains necessary.

xezon · 2026-09-25T18:13:24Z

 #endif

+	// TheSuperHackers @bugfix CryoTheRenegade 06/09/2026 Assert that the condition index supplied by findConditionIndex is valid.
+	DEBUG_ASSERTCRASH(conditionIndex >= 0 && conditionIndex < MAX_GARRISON_POINT_CONDITIONS,


I think you need to move this into the define block above, because

9/10 Simulating Replay "18-13-02_3v3_Supremac_Loonen_JB_HardAI_HardAI_HardAI.rep"
CRC Mismatch in Frame 6910

…assertion

xezon · 2026-09-26T08:06:10Z

 #if defined(RTS_DEBUG) || !RETAIL_COMPATIBLE_CRC
 	DEBUG_ASSERTCRASH(m_garrisonPointsInitialized, ("garrisonPoints are not inited"));
+
+	// TheSuperHackers @bugfix CryoTheRenegade 06/09/2026 Assert that the condition index supplied by findConditionIndex is valid.


Superfluous comment. Is not a bugfix either.

fix(GarrisonContain): Treat an invalid condition index like a full ga…

b699282

…rrison and return `GARRISON_INDEX_INVALID`

macroscopeapp Bot approved these changes Sep 25, 2026

View reviewed changes

Skyaero42 reviewed Sep 25, 2026

View reviewed changes

xezon requested changes Sep 25, 2026

View reviewed changes

Skyaero42 requested changes Sep 25, 2026

View reviewed changes

fix(GarrisonContain): Assert valid condition indices

968a1cf

macroscopeapp Bot reviewed Sep 25, 2026

View reviewed changes

greptile-apps Bot reviewed Sep 25, 2026

View reviewed changes

Comment thread Generals/Code/GameEngine/Source/GameLogic/Object/Contain/GarrisonContain.cpp

CryoTheRenegade changed the title ~~fix(GarrisonContain): Prevent out-of-bounds access for invalid condition indices~~ fix(GarrisonContain): Assert valid garrison condition indices Sep 25, 2026

xezon reviewed Sep 25, 2026

View reviewed changes

fix(GarrisonContain): Preserve replay compatibility around condition …

cac6d36

…assertion

xezon changed the title ~~fix(GarrisonContain): Assert valid garrison condition indices~~ chore(garrisoncontain): Add assert for condition index in GarrisonContain::findClosestFreeGarrisonPointIndex() Sep 26, 2026

xezon reviewed Sep 26, 2026

View reviewed changes

xezon added the Debug Is mostly debug functionality label Sep 26, 2026

		DEBUG_ASSERTCRASH(conditionIndex >= 0 && conditionIndex < MAX_GARRISON_POINT_CONDITIONS,
		("GarrisonContain::findClosestFreeGarrisonPointIndex - Invalid condition index '%d'", conditionIndex));

Conversation

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

Uh oh!

Fix out-of-bounds access in GarrisonContain.findClosestFreeGarrisonPointIndex for invalid condition indices

🗂️ Filtered Issues

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

Architecture Summary

Uh oh!

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

Uh oh!

Confidence Score: 5/5

Uh oh!

macroscopeapp Bot commented Sep 25, 2026

Approvability

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xezon Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

Skyaero42 left a comment

Choose a reason for hiding this comment

Uh oh!

macroscopeapp Bot Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

CryoTheRenegade Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

macroscopeapp Bot Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

macroscopeapp Bot Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xezon Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CryoTheRenegade commented Sep 25, 2026 •

edited by macroscopeapp Bot

Loading

Fix out-of-bounds access in `GarrisonContain.findClosestFreeGarrisonPointIndex` for invalid condition indices

coderabbitai Bot commented Sep 25, 2026 •

edited

Loading

greptile-apps Bot commented Sep 25, 2026 •

edited

Loading

Skyaero42 Sep 25, 2026 •

edited

Loading