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

[Flamefire]

I'd say this is clearer:

Suggested change

	RTTR_Assert(rank <= MAX_MILITARY_RANK);
	RTTR_Assert(rank <= troop_limits.size());

[Flamefire]

Are there other places where this can be used?

And a more generic name makes sense. It isn't a "troop limit" rank but simply a "soldier rank"

Suggested change

	inline uint8_t popTroopLimitRank(Serializer& ser)
	inline uint8_t popSoldierRank(Serializer& ser)