Validate troop limit commands

[DevOpsOfChaos]

Summary

• validate serialized troop-limit ranks before executing the game command
• reject invalid rank values during command deserialization instead of indexing the troop-limit array
• keep valid troop-limit counts unchanged, including large counts
• add regression coverage for invalid serialized rank values

Motivation

Troop limits are normally changed through the military building UI, where rank values are bounded.

However, the troop-limit game command stores the rank as a raw uint8_t and deserializes it from the command stream before forwarding it to nobMilitary::SetTroopLimit.

Review feedback pointed out that silently ignoring invalid values inside nobMilitary::SetTroopLimit is not the preferred style. This version validates the deserialized command value instead, matching the existing approach used for invalid/out-of-range enum values.

Behavior

• valid troop-limit commands keep their existing behavior
• large valid troop-limit counts are still accepted
• invalid serialized rank values now throw std::range_error via helpers::makeOutOfRange
• nobMilitary::SetTroopLimit keeps only an internal assert

Validation

• Built Test_integration locally with Visual Studio 2022 / Debug
• Ran Test_integration.exe --run_test=GameCommandSuite/SetTroopLimitRejectsInvalidSerializedRank --report_level=short
  • 1 test case passed
  • 1 assertion passed
• Ran Test_integration.exe --run_test=AttackSuite --report_level=short
  • 23 test cases passed
  • 1244 assertions passed
• Ran git diff --check upstream/master...HEAD
  • clean

[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.

[Flamefire]

See my prior comment

[DevOpsOfChaos]

Updated this PR according to the deserialization-boundary feedback.

The invalid rank is no longer silently ignored in nobMilitary::SetTroopLimit. Instead, gc::SetTroopLimit now validates the deserialized rank before Execute() can call into the building code.

Invalid serialized ranks now throw std::range_error via helpers::makeOutOfRange, matching the existing style used for invalid enum deserialization. nobMilitary::SetTroopLimit keeps only an internal assert, and large valid troop-limit counts remain accepted unchanged.

Validation:

• clang-format version 10.0.0
• built Test_integration
• Test_integration.exe --run_test=GameCommandSuite/SetTroopLimitRejectsInvalidSerializedRank --report_level=short
  • 1 test case
  • 1 assertion
• Test_integration.exe --run_test=AttackSuite --report_level=short
  • 23 test cases
  • 1244 assertions
• git diff --check upstream/master...HEAD
  • clean

[DevOpsOfChaos]

Applied the two review suggestions with one correction.

• renamed the helper to popSoldierRank
• kept the validation scoped to SetTroopLimit
• updated the internal assert to rank < troop_limits.size()

I used < instead of the suggested <= because troop_limits[rank] uses 0-based indexing, so rank == troop_limits.size() would already be out of range.

I also noticed ChangeReserve deserializes a soldier rank directly, but I left it unchanged here to avoid broadening this PR beyond the reviewed SetTroopLimit command path.

[Flamefire]

I also noticed ChangeReserve deserializes a soldier rank directly, but I left it unchanged here to avoid broadening this PR beyond the reviewed SetTroopLimit command path.

I'd be fine with using the new function in all command deserializations where it applies, at least for consistency, possibly without (new) tests. In the future we might otherwise wonder why it wasn't used.
Of course just where it is obviously correct, i.e. also had the same rank(ser.PopUnsignedChar())

[Flamefire]

This test seems incomplete: Why test the limit for (only) 0?
Shouldn't the test be:

• Use invalid command
• Assert that nothing changed

And instead of a fixed initial value(s) it could use random values to avoid reliance on some specific one unless testing for a specific corner case.

[Flamefire]

This test seems odd: Why test for essentially the same thing as TroopLimitCommandRejectsInvalidSerializedRank but with +1?

Do we need this test at all or does it provide any additional benefit?