[CryoTheRenegade]

Fixes #3659. A FlightDeckBehavior whose NumSpacesPerRunway is larger than a runway's Runway<N>Spaces bone list, or not positive, reads out of bounds when the deck builds its parking spaces. CodeRabbit found this while reviewing #3356 (comment). It is split out here because the fix changes how spaces are built, not only how a value is parsed.

Summary

buildInfo fills m_spaces row by row, one space per runway in each row. NumSpacesPerRunway comes straight from INI and is never checked against the bone lists.

A new parser clamps a negative NumSpacesPerRunway to 0 when the INI is read. The cap against the bone counts has to stay in buildInfo. Retail INI sets NumSpacesPerRunway before the Runway<N>Spaces lists, and module data has no step that runs after its INI block ends, so buildInfo is the first place that knows both values.

 NumSpacesPerRunway
-  INI::parseInt                                       // negative count: huge reserve
+  parseNumSpacesPerRunway                             // negative count: 0

 buildInfo
-  reserve( NumSpacesPerRunway * NumRunways )
-  for row < NumSpacesPerRunway
+  rows = min( NumSpacesPerRunway, smallest Runway<N>Spaces count )
+  reserve( rows * NumRunways )
+  for row < rows
     for runway < NumRunways
-      it = spaces.begin() + row                      // past end(): dereferenced
+      it = spaces.begin() + row                      // always valid

The front-row loops in reserveRunway, isInPositionToTakeoff and update read m_spaces[i] for i < NumRunways. When no rows can be built, m_spaces is empty. A file-static getFrontSpaceCount now bounds those loops by the number of spaces actually built.

Retail Zero Hour sets NumSpacesPerRunway = 10 with 10 bones per runway on AmericaAircraftCarrier, so retail data builds the same spaces as before.

NumRunways above MAX_RUNWAYS is a separate out-of-bounds read, which #3356 clamps at parse time.

Evidence

• Disassembly against main, Zero Hour FlightDeckBehavior.cpp, ClangCL and VC6 Release:
  • buildInfo: the new row cap adds instructions. Float arithmetic and calls match. The changed ClangCL dtor$N funclets are the same cleanup handlers, renumbered. VC6 inlines testStatus and the RunwayInfo copy differently.
  • reserveRunway, isInPositionToTakeoff, update: only the loop bound changes. Float arithmetic and calls match, except that VC6 no longer inlines isInPositionToTakeoff into update. That moves its getDistanceSquared call and fcomp into the out-of-line function.
• In game, Zero Hour ClangCL Release, mission MD_GLA03 (the USS Reagan carrier), with a loose Data\INI\Object\FactionBuilding.ini override of AmericaAircraftCarrier:
  • NumSpacesPerRunway = 0, main: access violation right after loading, reading address 0x14 in FlightDeckBehavior::update (mov ecx,[eax+ecx*8+14h] with eax = 0, the data pointer of the empty m_spaces). Stack: FlightDeckBehavior::update ← GameLogic::update ← GameEngine::update ← GameMain.
  • NumSpacesPerRunway = 0, this PR: no crash. The deck has no parked jets, and the mission stayed playable for 2:34.
  • NumSpacesPerRunway = 11, main: no visible effect for 2:11, in two runs. The read past end() is silent in Release.
  • NumSpacesPerRunway = 11, this PR: two rows of 10 parked jets, as with retail data.

Implementation was made with AI assistance

[coderabbitai]

Walkthrough

FlightDeckBehavior clamps negative runway-row counts to zero, limits initialized rows to the available space bones, and uses the resulting front-space count for takeoff reservation, takeoff-position checks, and launch-wave processing.

Changes

Flight deck runway-space bounds

Layer / File(s)	Summary
Bound runway-space capacity GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/FlightDeckBehavior.cpp	The parser logs negative NumSpacesPerRunway values and stores zero. buildInfo caps initialized rows at the smallest runway space-bone count. getFrontSpaceCount returns the smaller of the runway count and total initialized spaces.
Use bounded spaces in takeoff and launch GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/FlightDeckBehavior.cpp	Takeoff runway reservation, takeoff-position checks, and launch-wave processing scan no more than the available front-space count.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested labels: Bug, ZH

Suggested reviewers: xezon

Merge Risk: Low · up to 44cb3

The change prevents crashes from oversized or negative runway-space settings. An extreme modded configuration with more than 255 total spaces could still corrupt save files. Retail data is unaffected, so this is a narrow edge case to note before merge.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title identifies the main change: preventing invalid access when NumSpacesPerRunway exceeds the runway space bones.
Linked Issues check		[#3659] The INI parser clamps negative counts to zero. buildInfo limits rows to the smallest runway bone list before reserving or reading spaces. Front-row loops use the built space count. Retail da…
Out of Scope Changes check		[#3659] The changes to FlightDeckBehavior.cpp prevent invalid space construction and access. The parser logging supports the same fix. No unrelated changes appear in the whole-PR summary.
Description check		The description matches the changes to parking-space initialization, negative-value parsing, and front-row loop bounds.

Full details: Linked Issues check

Explanation

[#3659] The INI parser clamps negative counts to zero. buildInfo limits rows to the smallest runway bone list before reserving or reading spaces. Front-row loops use the built space count. Retail data with 10 bones per runway retains 10 rows. The issue does not specify automated tests.

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

• X
• Mastodon
• Reddit
• LinkedIn

[Skyaero42]

I presume the number of rows is a parameter set by the user? Then this is an invalid user input issue that should be handled when the input is read.

[Skyaero42]

Why would the number of rows be smaller than 0?

[CryoTheRenegade]

Only from bad INI. INI::parseInt accepted NumSpacesPerRunway = -1. buildInfo then passed the negative product to reserve, which read it as a request for about 4 billion elements and threw. The parser now clamps the value to 0, so I removed this check.

[CryoTheRenegade]

I presume the number of rows is a parameter set by the user? Then this is an invalid user input issue that should be handled when the input is read.

Yes, it comes from INI. A negative NumSpacesPerRunway is now clamped to 0 when the INI is read.

[coderabbitai]

Actionable comments posted: 1

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: ef657d0d-4e77-44f0-8f1b-4d3078bda8f6

Commits

Reviewing files that changed from the base of the PR and between c412cfc and 44cb378.

Files selected for processing: 1

• GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/FlightDeckBehavior.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.