Merge pull request #1931 from DevOpsOfChaos/sidequest/validate-troop-limit-command

Validate troop limit commands

Author: Flamefire

libs/s25main/GameCommands.h

@@ -17,6 +17,7 @@
 #include "gameTypes/PactTypes.h"
 #include "gameTypes/SettingsTypes.h"
 #include "gameTypes/TempleProductionMode.h"
+#include "gameData/MilitaryConsts.h"
 #include "s25util/Serializer.h"
 #include <cstdint>
 #include <utility>
@@ -25,6 +26,16 @@ class GameWorld;
 
 namespace gc {
 
+namespace detail {
+    inline uint8_t popSoldierRank(Serializer& ser)
+    {
+        const auto rank = ser.PopUnsignedChar();
+        if(rank > MAX_MILITARY_RANK)
+            throw helpers::makeOutOfRange(rank, MAX_MILITARY_RANK);
+        return rank;
+    }
+} // namespace detail
+
 /// Basisklasse für sämtliche GameCommands mit Koordinaten
 class Coords : public GameCommand
 {
@@ -220,7 +231,7 @@ class SetTroopLimit : public Coords
         : Coords(GCType::SetTroopLimit, pt), rank(rank), count(count)
     {}
     SetTroopLimit(Serializer& ser)
-        : Coords(GCType::SetTroopLimit, ser), rank(ser.PopUnsignedChar()), count(ser.PopUnsignedInt())
+        : Coords(GCType::SetTroopLimit, ser), rank(detail::popSoldierRank(ser)), count(ser.PopUnsignedInt())
     {}
 
 public:
@@ -529,7 +540,7 @@ class ChangeReserve : public Coords
         : Coords(GCType::ChangeReserve, pt), rank(rank), count(count)
     {}
     ChangeReserve(Serializer& ser)
-        : Coords(GCType::ChangeReserve, ser), rank(ser.PopUnsignedChar()), count(ser.PopUnsignedInt())
+        : Coords(GCType::ChangeReserve, ser), rank(detail::popSoldierRank(ser)), count(ser.PopUnsignedInt())
     {}
 
 public:

libs/s25main/buildings/nobMilitary.cpp

@@ -696,6 +696,7 @@ unsigned nobMilitary::CalcRequiredNumTroops(FrontierDistance assumedFrontierDist
 
 void nobMilitary::SetTroopLimit(const unsigned rank, const unsigned limit)
 {
+    RTTR_Assert(rank < troop_limits.size());
     troop_limits[rank] = limit;
     RegulateTroops();
 }

tests/s25Main/integration/testAttacking.cpp

@@ -418,6 +418,28 @@ BOOST_FIXTURE_TEST_CASE(StartAttack, AttackFixture<>)
     TestFailingAttack(gwv, milBld1Pos, attackSrc, 1u);
 }
 
+BOOST_FIXTURE_TEST_CASE(TroopLimitCommandRejectsInvalidSerializedRank, AttackFixture<>)
+{
+    std::array<unsigned, NUM_SOLDIER_RANKS> expectedLimits{};
+    const auto firstLimit = rttr::test::randomValue(1u, 20u);
+    const auto limitStep = rttr::test::randomValue(1u, 5u);
+    // Use unique values to detect overwriting
+    for(unsigned rank = 0; rank < expectedLimits.size(); ++rank)
+    {
+        expectedLimits[rank] = firstLimit + rank * limitStep;
+        this->SetTroopLimit(milBld0Pos, rank, expectedLimits[rank]);
+    }
+
+    for(unsigned rank = 0; rank < expectedLimits.size(); ++rank)
+        BOOST_TEST_REQUIRE(milBld0->GetTroopLimit(rank) == expectedLimits[rank]);
+
+    BOOST_REQUIRE_THROW(this->SetTroopLimit(milBld0Pos, NUM_SOLDIER_RANKS, expectedLimits.back() + limitStep),
+                        std::range_error);
+
+    for(unsigned rank = 0; rank < expectedLimits.size(); ++rank)
+        BOOST_TEST_REQUIRE(milBld0->GetTroopLimit(rank) == expectedLimits[rank]);
+}
+
 BOOST_FIXTURE_TEST_CASE(ConquerBld, AttackFixture<>)
 {
     AddSoldiers(milBld0Pos, 1, 5);

tests/s25Main/integration/testGameCommands.cpp

@@ -2,6 +2,7 @@
 //
 // SPDX-License-Identifier: GPL-2.0-or-later
 
+#include "GameCommands.h"
 #include "GamePlayer.h"
 #include "PointOutput.h"
 #include "RttrForeachPt.h"
@@ -12,6 +13,8 @@
 #include "enum_cast.hpp"
 #include "factories/BuildingFactory.h"
 #include "figures/nofPassiveSoldier.h"
+#include "helpers/serializeEnums.h"
+#include "helpers/serializePoint.h"
 #include "postSystem/PostBox.h"
 #include "worldFixtures/WorldWithGCExecution.h"
 #include "worldFixtures/initGameRNG.hpp"
@@ -53,8 +56,41 @@ static void dummySuppressUnused(std::ostream& out)
 }
 // LCOV_EXCL_STOP
 
+namespace {
+// Expose the protected constructors only to serialize the actual command format in this test,
+// rather than duplicating the production Serialize() implementation.
+class TestSetTroopLimit : public gc::SetTroopLimit
+{
+public:
+    TestSetTroopLimit(const MapPoint pt, const uint8_t rank, const uint32_t count) : gc::SetTroopLimit(pt, rank, count)
+    {}
+};
+
+class TestChangeReserve : public gc::ChangeReserve
+{
+public:
+    TestChangeReserve(const MapPoint pt, const uint8_t rank, const uint32_t count) : gc::ChangeReserve(pt, rank, count)
+    {}
+};
+} // namespace
+
 BOOST_AUTO_TEST_SUITE(GameCommandSuite)
 
+BOOST_AUTO_TEST_CASE(SoldierRankCommandsRejectInvalidSerializedRank)
+{
+    constexpr auto invalidRank = static_cast<uint8_t>(MAX_MILITARY_RANK + 1u);
+    const auto pt = rttr::test::randomPoint<MapPoint>();
+    const auto count = rttr::test::randomValue<unsigned>();
+
+    gc::Deserializer troopLimitCommand;
+    TestSetTroopLimit(pt, invalidRank, count).Serialize(troopLimitCommand);
+    BOOST_REQUIRE_THROW(gc::GameCommand::Deserialize(troopLimitCommand), std::range_error);
+
+    gc::Deserializer reserveCommand;
+    TestChangeReserve(pt, invalidRank, count).Serialize(reserveCommand);
+    BOOST_REQUIRE_THROW(gc::GameCommand::Deserialize(reserveCommand), std::range_error);
+}
+
 BOOST_FIXTURE_TEST_CASE(PlaceFlagTest, WorldWithGCExecution2P)
 {
     (void)&dummySuppressUnused;