fix(data): Sanitize GlobalData values after reading INI data by Cellcote · Pull Request #2519 · TheSuperHackers/GeneralsGameCode

Cellcote · 2026-04-01T19:30:40Z

Summary

Clamp m_numGlobalLights to [0, MAX_GLOBAL_LIGHTS] to prevent out-of-bounds access on fixed-size terrain lighting arrays
Floor allocation-size fields (m_maxRoadSegments, m_maxVisibleTranslucentObjects, etc.) to 0 to prevent undefined behavior from negative INI values
Floor m_maxTankTrackEdges and network history lengths to 1 since they are used in subtraction and modulo arithmetic

greptile-apps · 2026-04-01T19:34:23Z

Greptile Summary

This PR sanitizes GlobalData values after INI parsing to prevent out-of-bounds array access, negative allocation sizes, and divide-by-zero in modulo/division arithmetic. It also extracts MAX_TANK_TRACK_EDGES and ROAD_BUFFER_PADDING as shared constexpr constants in GlobalData.h so the rendering layer uses the same bounds as the data-sanitization layer. The previously flagged m_networkCushionHistoryLength modulo-by-zero is now addressed in both Generals/ and GeneralsMD/ variants.

Confidence Score: 5/5

Safe to merge — all sanitization is correct and complete, including the previously flagged m_networkCushionHistoryLength in both Generals and GeneralsMD.

No P0 or P1 findings remain. The previously reported divide-by-zero via m_networkCushionHistoryLength is addressed in both code trees. Constants are correctly defined, shared, and validated with a static_assert. The old local #define MAX_TRACK_EDGE_COUNT is fully removed with no remaining references.

No files require special attention.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/Common/GlobalData.cpp	Adds complete sanitization block after INI parsing; all three network history lengths (FPS, latency, cushion) are guarded to ≥1, and road/track fields are clamped with named constants and a static_assert.
GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp	Mirror of the Generals sanitization block; includes the m_networkCushionHistoryLength fix that was flagged in prior review threads.
Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DTerrainTracks.h	Replaces local #define MAX_TRACK_EDGE_COUNT (100) with the shared constexpr MAX_TANK_TRACK_EDGES from GlobalData.h; array declaration is unchanged in size.
Generals/Code/GameEngine/Include/Common/GlobalData.h	Adds MAX_TANK_TRACK_EDGES = 100 and ROAD_BUFFER_PADDING = 4 as constexpr constants alongside MAX_GLOBAL_LIGHTS.
GeneralsMD/Code/GameEngine/Include/Common/GlobalData.h	Identical constant additions to the GeneralsMD header; consistent with the Generals variant.
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DRoadBuffer.cpp	Replaces magic number 4 with ROAD_BUFFER_PADDING in DX8 vertex/index buffer allocations; no logic change.
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DRoadBuffer.cpp	Same ROAD_BUFFER_PADDING substitution as the Generals counterpart; consistent.
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainTracks.cpp	Cosmetic blank-line reorder only; no functional change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[INI File Loaded] --> B[initFromINI fills GlobalData fields]
    B --> C{parseGameDataDefinition sanitization}
    C --> D["m_numGlobalLights → clamp(0, MAX_GLOBAL_LIGHTS)"]
    C --> E["m_maxRoadVertex/Index → clamp(0, 65531)"]
    C --> F["m_maxTankTrackEdges → clamp(1, MAX_TANK_TRACK_EDGES=100)"]
    C --> G["allocation-size fields → max(value, 0)"]
    C --> H["network history lengths → max(value, 1)"]
    D --> I[W3DTerrainTracks\nm_edges MAX_TANK_TRACK_EDGES array\nsafe index access]
    E --> J[W3DRoadBuffer\nDX8 16-bit index buffers\nsafe allocation]
    F --> I
    H --> K[FrameMetrics\nmodulo / division\nno divide-by-zero]

_{Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.
Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.}

_{Reviews (4): Last reviewed commit: "fix(data): Sanitize GlobalData values af..." | Re-trigger Greptile}

Generals/Code/GameEngine/Source/Common/GlobalData.cpp

GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp

xezon · 2026-04-01T21:21:27Z

Generals/Code/GameEngine/Source/Common/GlobalData.cpp

+	TheWritableGlobalData->m_maxVisibleNonOccluderOrOccludeeObjects = std::max(TheWritableGlobalData->m_maxVisibleNonOccluderOrOccludeeObjects, 0);
+	TheWritableGlobalData->m_maxLineBuildObjects = std::max(TheWritableGlobalData->m_maxLineBuildObjects, 0);
+	TheWritableGlobalData->m_maxRoadSegments = std::max(TheWritableGlobalData->m_maxRoadSegments, 0);
+	// Capped to 65531 because DX8 vertex/index buffers use 16-bit indices and allocate size + 4.


Can we static assert that?

Had to create a constant for the padding but added.

xezon · 2026-04-01T21:21:41Z

Generals/Code/GameEngine/Source/Common/GlobalData.cpp

+	TheWritableGlobalData->m_maxRoadVertex = std::clamp(TheWritableGlobalData->m_maxRoadVertex, 0, 65531);
+	TheWritableGlobalData->m_maxRoadIndex = std::clamp(TheWritableGlobalData->m_maxRoadIndex, 0, 65531);
+	TheWritableGlobalData->m_maxRoadTypes = std::max(TheWritableGlobalData->m_maxRoadTypes, 0);
+	// Capped to 100 (MAX_TRACK_EDGE_COUNT) because TerrainTracksRenderObjClass has a fixed-size m_edges array.


Can we use this constant value perhaps?

Clamp m_numGlobalLights to [0, MAX_GLOBAL_LIGHTS] and floor allocation-size fields to prevent out-of-bounds array access and undefined behavior from bad INI values. Cap m_maxRoadVertex and m_maxRoadIndex to 16-bit DX8 buffer limit minus ROAD_BUFFER_PADDING, and m_maxTankTrackEdges to MAX_TANK_TRACK_EDGES. Move MAX_TRACK_EDGE_COUNT to GlobalData.h as MAX_TANK_TRACK_EDGES and replace magic +4 in W3DRoadBuffer with ROAD_BUFFER_PADDING constant.

xezon

It is a bit odd that GlobalData needs to be aware about limits of implementations. Maybe the clamping should be done where these globals are used? Not sure.

xezon · 2026-04-05T19:17:26Z

Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DTerrainTracks.h

 #include "Lib/BaseType.h"
-
-#define MAX_TRACK_EDGE_COUNT	100	//maximum number of edges or divisions in track mark
+#include "Common/GlobalData.h"


Tough to include GlobalData.h for just one define.

xezon · 2026-04-05T19:27:57Z

GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp

+	TheWritableGlobalData->m_maxLineBuildObjects = std::max(TheWritableGlobalData->m_maxLineBuildObjects, 0);
+	TheWritableGlobalData->m_maxRoadSegments = std::max(TheWritableGlobalData->m_maxRoadSegments, 0);
+	// Capped because DX8 vertex/index buffers use 16-bit indices and allocate size + ROAD_BUFFER_PADDING.
+	static constexpr Int MAX_ROAD_BUFFER_SIZE = 65535 - ROAD_BUFFER_PADDING;


constexpr const, because of VC6. No static.

xezon · 2026-04-05T19:29:42Z

GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp

+	TheWritableGlobalData->m_maxRoadSegments = std::max(TheWritableGlobalData->m_maxRoadSegments, 0);
+	// Capped because DX8 vertex/index buffers use 16-bit indices and allocate size + ROAD_BUFFER_PADDING.
+	static constexpr Int MAX_ROAD_BUFFER_SIZE = 65535 - ROAD_BUFFER_PADDING;
+	static_assert(MAX_ROAD_BUFFER_SIZE > 0, "ROAD_BUFFER_PADDING must be less than 65535");


The assert condition does not match the message.

xezon · 2026-04-05T19:33:36Z

Generals/Code/GameEngine/Source/Common/GlobalData.cpp

+	TheWritableGlobalData->m_maxRoadVertex = std::clamp(TheWritableGlobalData->m_maxRoadVertex, 0, MAX_ROAD_BUFFER_SIZE);
+	TheWritableGlobalData->m_maxRoadIndex = std::clamp(TheWritableGlobalData->m_maxRoadIndex, 0, MAX_ROAD_BUFFER_SIZE);
+	TheWritableGlobalData->m_maxRoadTypes = std::max(TheWritableGlobalData->m_maxRoadTypes, 0);
+	// Capped because TerrainTracksRenderObjClass has a fixed-size m_edges[MAX_TANK_TRACK_EDGES] array.


Better write m_edges[MAX_TANK_TRACK_EDGES] a different way, to not establish named code references in comments.

xezon · 2026-04-05T19:34:21Z

Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainTracks.cpp

 #include "WW3D2/dx8wrapper.h"
 #include "WW3D2/scene.h"
 #include "GameLogic/TerrainLogic.h"
+


Superfluous blank line

greptile-apps bot reviewed Apr 1, 2026

View reviewed changes

Generals/Code/GameEngine/Source/Common/GlobalData.cpp Show resolved Hide resolved

GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp Outdated Show resolved Hide resolved

Cellcote force-pushed the fix/sanitize-globaldata-ini-values branch 2 times, most recently from 93b4a9c to 2729baa Compare April 1, 2026 19:43

xezon reviewed Apr 1, 2026

View reviewed changes

Cellcote force-pushed the fix/sanitize-globaldata-ini-values branch from 2729baa to a401e81 Compare April 4, 2026 16:29

Cellcote requested a review from xezon April 4, 2026 21:27

xezon reviewed Apr 5, 2026

View reviewed changes

Conversation

Cellcote commented Apr 1, 2026

Summary

Uh oh!

greptile-apps bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greptile-apps bot commented Apr 1, 2026 •

edited

Loading