diff --git a/libs/s25main/GameCommands.h b/libs/s25main/GameCommands.h index db8c094f63..0243830829 100644 --- a/libs/s25main/GameCommands.h +++ b/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 #include @@ -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: diff --git a/libs/s25main/buildings/nobMilitary.cpp b/libs/s25main/buildings/nobMilitary.cpp index 9b180a1975..47c8a7b05e 100644 --- a/libs/s25main/buildings/nobMilitary.cpp +++ b/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(); } diff --git a/tests/s25Main/integration/testAttacking.cpp b/tests/s25Main/integration/testAttacking.cpp index 42db686490..3dcb6320f0 100644 --- a/tests/s25Main/integration/testAttacking.cpp +++ b/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 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); diff --git a/tests/s25Main/integration/testGameCommands.cpp b/tests/s25Main/integration/testGameCommands.cpp index be9876d878..22ba315bde 100644 --- a/tests/s25Main/integration/testGameCommands.cpp +++ b/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(MAX_MILITARY_RANK + 1u); + const auto pt = rttr::test::randomPoint(); + const auto count = rttr::test::randomValue(); + + 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;