Validate troop limit commands

[DevOpsOfChaos]

Summary

• ignore invalid troop-limit ranks before writing to the rank-limit array
• keep valid troop-limit counts unchanged
• add regression coverage for invalid rank values

Motivation

Troop limits are normally changed through the military building UI, but the underlying game command should still avoid indexing the troop-limit array with an invalid rank.

Before this change, nobMilitary::SetTroopLimit() wrote directly into troop_limits[rank]. This made the command rely on callers always passing a valid rank.

This PR keeps the existing behavior for valid ranks, including large troop-limit values. It only adds a defensive guard for invalid rank values.

Validation

• Built Test_integration locally with Visual Studio 2022 / Debug
• Ran Test_integration.exe --run_test=AttackSuite/TroopLimitCommandIgnoresInvalidRank --report_level=short
  • 1 test case passed
  • 21 assertions passed
• Ran Test_integration.exe --run_test=AttackSuite --report_level=short
  • 22 test cases passed
  • 1210 assertions passed
• Ran git diff --check

[Flamefire]

Even though a limit > soldier count might not look right, it doesn't cause any issues: A large limit is just the same as no limit.

So if this doesn't fix an actual wrong behavior resulting from that I'd keep it, especially as it is virtually impossible to trigger/use the wrong count, so nothing will be gained by the added code.

[Flow86]

well, this part is the only thing I would think i useful. Accessing a rank outside the troop_limits range is UB.
but may be I would generally say to use a RTTR_Assert here instead (to "crash" the game if someone manipulates that)

[Flamefire]

Sure this could have an assert like rank < troop_limits.size() but then: Why here and not in one of the 100s other places?

IIRC MSVC already does range checks in debug mode which (partially) covers that.

[DevOpsOfChaos]

I kept this as a runtime guard instead of assert-only because the value comes through the game command path, not just a direct UI-only call.

An RTTR_Assert(rank < NUM_SOLDIER_RANKS) would be fine as an additional debug check, but assert-only would still leave release builds indexing out of range if a malformed command gets through.

So my intention here is not to introduce broad validation everywhere, only to avoid UB at this command boundary. If you prefer the project style to be assert + return here, I can adjust it that way.

[Flamefire]

So my intention here is not to introduce broad validation everywhere, only to avoid UB at this command boundary.

If the intention is not to introduce it everywhere (which I wouldn't do anyway) then the question is, why here? What makes this place special?

So unless there is some actual situation where this is an issue I'd just leave it as-is.

Otherwise: Simply ignoring the caller isn't great either. So if we would use a runtime-check then I'd simply use troop_limits.at(rank) = limit;
But then we'd need to basically do this everywhere just for assuming some issue that never happened before.

[DevOpsOfChaos]

I checked the path again.

The normal UI path seems bounded: the military building window only creates rank values from controls in the 0..NUM_SOLDIER_RANKS - 1 range.

The reason I considered this command boundary special is that SetTroopLimit is also deserialized from the game-command stream: the command stores rank as a raw uint8_t, SetTroopLimit(Serializer& ser) reads it via PopUnsignedChar(), and Execute() forwards it directly to nobMilitary::SetTroopLimit(rank, count).

So the current guard is not meant to handle normal UI-generated input, but to avoid UB if a malformed command stream reaches this boundary.

That said, I do not currently have a reproduced normal-gameplay path where such an invalid rank is generated. If that makes the guard too speculative for this codebase, I’m fine with leaving the current behavior unchanged.

[Flamefire]

IMO there are 3 better paths:

• RTTR_Assert
• .at()
• Validate and throw when deserializing the GC.

The last one is more in line with what we do already: E.g. invalid/out-of-range enum values cause an exception.
It might then make sense to check the other commands if they have similar, critical issues. I.e. not just "might cause odd results", but actual, always-wrong OOB accesses

[DevOpsOfChaos]

Even though a limit > soldier count might not look right, it doesn't cause any issues: A large limit is just the same as no limit.

So if this doesn't fix an actual wrong behavior resulting from that I'd keep it, especially as it is virtually impossible to trigger/use the wrong count, so nothing will be gained by the added code.

Good point. I removed the count clamp.

Large troop-limit values now keep the existing behavior. The PR only keeps the invalid-rank guard before indexing the troop-limit array.