You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: Validate INI and savegame bounds across game modules - #3356
clang-tidy flagged W3DDebrisDraw for indexing its animation arrays with a state loaded from a savegame, without a range check. In review, xezon asked for an audit of INI parsing and savegame loading for the same kind of bug (comment), so this PR also fixes what that audit found.
Validate INI and savegame values before they control array access or allocation sizes in Generals and Zero Hour.
The changes reject invalid counts, indices, and inconsistent saved array lengths. They preserve valid sentinel values and the savegame format. Debris state is clamped during post-load processing, and particle rate calculations handle completed keyframe sequences before indexing.
INI counts that size fixed arrays (NumberGlobalLights, OuterEffectNumBones, NumRunways and the map cache numPlayers) are clamped by the new INI::parseIntClamped and INI::parseUnsignedIntClamped, which take the maximum in the field's userData. A loaded drawable with an unknown module type count or icon name, or a railed transport with an out-of-range path, now rejects the save.
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.
The changes add clamped INI parsers for selected settings and add bounds or consistency checks for loaded and transferred game state across Core, Generals, and GeneralsMD.
Adds signed and unsigned clamped INI parsers and applies them to selected map, lighting, runway, and particle effect settings.
Validate client and map state Core/GameEngine/Source/Common/System/Radar.cpp, Core/GameEngine/Source/GameClient/System/ParticleSys.cpp, Core/GameEngine/Source/GameLogic/Map/*, Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DDebrisDraw.cpp, Generals*/Code/GameEngine/Source/GameClient/Drawable.cpp
Checks saved radar, particle, polygon, terrain, and drawable values before use. Debris animation state is clamped to its valid range.
Validate gameplay load state Generals*/Code/GameEngine/Source/Common/RTS/Money.cpp, Generals*/Code/GameEngine/Source/GameLogic/AI/{AIPlayer,AISkirmishPlayer}.cpp, Generals*/Code/GameEngine/Source/GameLogic/Object/{Contain,ExperienceTracker.cpp,Update,BoneFXUpdate.cpp,WeaponSet.cpp}
Checks saved gameplay bounds during load post-processing. Skirmish players also call the base AI post-processing method, and station garrison-point queries are capped at buffer capacity.
Validate gameplay transfers and vector sizes Generals*/Code/GameEngine/Source/GameLogic/Object/Update/{AIUpdate,DockUpdate}/..., Generals*/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptEngine.cpp
Checks serialized transport paths and counts, script count consistency, and dock vector bounds and sizes before processing them.
This PR hardens INI and savegame parsing, but a few gaps remain. A map cache entry without a player count can read out of bounds, and some configuration values (parking spaces per runway, out-of-range literals) and oversized dock vector sizes in saves are still not safely bounded. These should be fixed or explicitly accepted before merge.
Security Architecture Review
Security architecture risk:🔵 Low · up to 8b11c
The examined changes reject invalid counts before normal use or constrain loaded state, without showing a new exposure or weakened control. Some security coverage and runtime behavior remain unverified.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The demonstrated input paths affect rendering initialization or savegame loading within the game process. The available caller evidence does not establish remote attacker reachability or a broader privilege boundary.
Trust Boundaries and Controls
observed — The laser check rejects invalid configuration counts before allocation; the AI post-processing check rejects out-of-range saved repair-queue counts before normal queue processing.
Resilience and Maintainability Implications
inferred — Post-processing failure routes the examined save-load path through reset rather than onward into gameplay; repeated-load behavior was not exercised.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: validating INI and savegame values across game modules. The changeset adds bounds checks in many load/xfer paths (Radar, ParticleSys, PolygonTrigger, TerrainLogic, Mon…
Description check
The description states the purpose: clang-tidy flagged W3DDebrisDraw for unchecked savegame-loaded indices, and an audit of INI parsing and savegame loading followed. It lists the kinds of checks adde…
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.
Full details: Title check
Explanation
The title names the main change: validating INI and savegame values across game modules. The changeset adds bounds checks in many load/xfer paths (Radar, ParticleSys, PolygonTrigger, TerrainLogic, Money, AIPlayer, OpenContain, DockUpdate, etc.) and adds INI clamped parsers. The title is specific and concise, and it matches the PR objectives. The word 'fix' is a reasonable conventional-commit prefix for defensive validation of invalid data.
Full details: Description check
Explanation
The description states the purpose: clang-tidy flagged W3DDebrisDraw for unchecked savegame-loaded indices, and an audit of INI parsing and savegame loading followed. It lists the kinds of checks added (invalid counts, indices, array lengths), the new INI::parseIntClamped and INI::parseUnsignedIntClamped helpers, and the affected fields. This matches 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.
Macroscope's review found this PR approvable — This is a focused defensive fix that clamps invalid savegame state before existing fixed-size animation arrays are accessed. Valid states and serialization behavior remain unchanged, with no broader runtime or schema impact.
You can add or adjust custom eligibility rules. Learn more.
The reason will be displayed to describe this comment to others. Learn more.
Looks sloppy. Of course the bots are happy because they have no taste ;)
I expect this belongs into postProcessLoad as a m_state = clamp(INITIAL, m_state, FINAL)
Also, I expect this type of issue to be a concern for MANY modules that relate to INI parse and xfer. Can we make a project wide audit and fix all such findings?
The reason will be displayed to describe this comment to others. Learn more.
Moved the clamp into loadPostProcess() using clamp<Int>. I can audit the INI parsing and save/load code for similar issues and address the findings in a follow-up PR.
The reason will be displayed to describe this comment to others. Learn more.
Yes make an audit please and maybe all of them can be fixed in one go or several batches depending on size. Try to bundle the changes where it makes sense.
This change is very small right now.
CryoTheRenegade
changed the title
fix(W3DDebrisDraw): Clamp loaded debris state to prevent out-of-bounds access
fix: Validate INI and savegame bounds across game modules
Sep 28, 2026
Reject invalid counts, indices and inconsistent saved array sizes in both games. Preserve valid sentinel values and serialized layouts, and handle completed particle key sequences before indexing.
Ignore invalid docking counts and disable unsafe laser visuals instead of throwing INI errors. Use debug logging for the added validation diagnostics while preserving rejection of invalid save data.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
Major · Initialize m_numPlayers before parsing. · INIMapCache.cpp:46
Core/GameEngine/Source/Common/INI/INIMapCache.cpp:46 Major · Stability & Availability · Quick win
Initialize m_numPlayers before parsing.
If a cache entry omits numPlayers, initFromINI does not assign mdr.m_numPlayers. The later copy and waypoint loop can therefore use an indeterminate bound and read beyond mdr.m_waypoints.
Suggested fix
MapMetaDataReader mdr;
MapMetaData md;
+ mdr.m_numPlayers = 0;
// read the name
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 5fa421ec-0917-4c27-8dcf-845b64cc44df
Commits
Reviewing files that changed from the base of the PR and between 3d02290 and 113c46a.
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Major · Stability & Availability · Quick win
Supported by static analysis
Script executed:
grep -n 'm_numRows\|m_numRunways\|reserve\|NumSpacesPerRunway' GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/FlightDeckBehavior.cpp
sed -n '150,230p' GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/FlightDeckBehavior.cpp
Repository: TheSuperHackers/GeneralsGameCode
Length of output: 4034
Validate NumSpacesPerRunway before it sizes or indexes the parking spaces.
NumSpacesPerRunway (m_numRows) is still parsed with INI::parseInt and is not range-checked. buildInfo uses it in m_spaces.reserve( data->m_numRows * data->m_numCols ) and in the row loop. This PR does not change this count.
Two invalid values can fail in this path:
A negative count with at least one runway makes the product negative. Converted to size_t, it becomes a very large size. reserve then throws std::length_error, which this function does not catch. This applies only if the INI parser accepts negative values.
A count greater than the number of SpacesBoneNames in a used runway moves the iterator to end(). The dereference of it at the bone-position call is then undefined behavior.
Reject invalid counts before reserve runs. Require a value that is not negative and does not exceed the number of bone names in each runway that is used.
The reason will be displayed to describe this comment to others. Learn more.
This one is real. The fix is in #3660 (tracked in #3659), separate from this PR, because it changes how the flight deck builds its spaces rather than how a value is parsed. buildInfo now caps the row count at the smallest Runway<N>Spaces list and at zero. The front-row loops are bounded by the number of spaces actually built.
The reason will be displayed to describe this comment to others. Learn more.
@CryoTheRenegade Understood. The changes you describe cover both space construction and front-row access. Keeping that behavior change in #3660, tracked by #3659, is reasonable. No duplicate fix is needed in this PR.
I have not verified #3660, so this finding remains open pending verification there.
If you found this review helpful, would you consider giving us a shout-out on X?
Adds INI::parseIntClamped and INI::parseUnsignedIntClamped, which take the
maximum in userData, and uses them for NumberGlobalLights,
OuterEffectNumBones, NumRunways and the map cache numPlayers. Garrison
station bones are capped at the station buffer size.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
CryoTheRenegade
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
clang-tidy flagged
W3DDebrisDrawfor indexing its animation arrays with a state loaded from a savegame, without a range check. In review, xezon asked for an audit of INI parsing and savegame loading for the same kind of bug (comment), so this PR also fixes what that audit found.Validate INI and savegame values before they control array access or allocation sizes in Generals and Zero Hour.
The changes reject invalid counts, indices, and inconsistent saved array lengths. They preserve valid sentinel values and the savegame format. Debris state is clamped during post-load processing, and particle rate calculations handle completed keyframe sequences before indexing.
INI counts that size fixed arrays (
NumberGlobalLights,OuterEffectNumBones,NumRunwaysand the map cachenumPlayers) are clamped by the newINI::parseIntClampedandINI::parseUnsignedIntClamped, which take the maximum in the field's userData. A loaded drawable with an unknown module type count or icon name, or a railed transport with an out-of-range path, now rejects the save.Implementation was made with AI assistance