From eca3d75e1273248ca75df2df3a51b7da83b32a33 Mon Sep 17 00:00:00 2001 From: DawnFire42 Date: Tue, 12 May 2026 22:40:04 -0400 Subject: [PATCH] Centralize MAX_COUNTERS_ON_CARD and enforce [0, 999] bounds on server - Move MAX_COUNTERS_ON_CARD to trice_limits.h - Server clamps values in setCounter() and incrementCounter() - Client uses clamped comparison to allow recovery from invalid states - Add tests for clamping behavior --- cockatrice/src/game/board/card_item.h | 2 +- cockatrice/src/game/player/player_actions.cpp | 9 +++- .../server/remote/game/server_card.cpp | 11 ++-- .../network/server/remote/game/server_card.h | 6 +-- .../libcockatrice/utility/trice_limits.h | 6 +++ tests/server_card_counter_test.cpp | 50 ++++++++++++++++--- 6 files changed, 66 insertions(+), 18 deletions(-) diff --git a/cockatrice/src/game/board/card_item.h b/cockatrice/src/game/board/card_item.h index 408bb03a7..451fed0c6 100644 --- a/cockatrice/src/game/board/card_item.h +++ b/cockatrice/src/game/board/card_item.h @@ -12,6 +12,7 @@ #include "card_state.h" #include +#include class CardDatabase; class CardDragItem; @@ -21,7 +22,6 @@ class PlayerLogic; class QAction; class QColor; -const int MAX_COUNTERS_ON_CARD = 999; const int ROTATION_DEGREES_PER_FRAME = 10; class CardItem : public AbstractCardItem diff --git a/cockatrice/src/game/player/player_actions.cpp b/cockatrice/src/game/player/player_actions.cpp index 341769899..c2b9140af 100644 --- a/cockatrice/src/game/player/player_actions.cpp +++ b/cockatrice/src/game/player/player_actions.cpp @@ -1501,7 +1501,10 @@ void PlayerActions::offsetCardCounter(int counterId, int offset) int oldValue = card->getCounters().value(counterId, 0); int newValue = oldValue + offset; - if (newValue >= 0 && newValue <= MAX_COUNTERS_ON_CARD) { + // Early exit optimization: server enforces [0, MAX_COUNTERS_ON_CARD]. + // Compare clamped value to allow recovery from invalid states. + int clampedValue = qBound(0, newValue, MAX_COUNTERS_ON_CARD); + if (clampedValue != oldValue) { auto *cmd = new Command_SetCardCounter; cmd->set_zone(card->getZone()->getName().toStdString()); cmd->set_card_id(card->getId()); @@ -1541,7 +1544,9 @@ void PlayerActions::actSetCardCounter(int counterId) for (auto card : sel) { int oldValue = card->getCounters().value(counterId, 0); Expression exp(oldValue); - int number = static_cast(exp.parse(dialog.textValue())); + double parsed = exp.parse(dialog.textValue()); + // Clamp in double precision first to avoid UB, then cast + int number = static_cast(qBound(0.0, parsed, static_cast(MAX_COUNTERS_ON_CARD))); auto *cmd = new Command_SetCardCounter; cmd->set_zone(card->getZone()->getName().toStdString()); diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp index 3532abb0b..b858314c0 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include Server_Card::Server_Card(const CardRef &cardRef, int _id, int _coord_x, int _coord_y, Server_CardZone *_zone) @@ -113,6 +114,9 @@ QString Server_Card::setAttribute(CardAttribute attribute, const QString &avalue bool Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event) { + // Clamp to valid card counter range [0, MAX_COUNTERS_ON_CARD] + value = qBound(0, value, MAX_COUNTERS_ON_CARD); + const int oldValue = counters.value(_id, 0); if (value == oldValue) { return false; @@ -134,12 +138,11 @@ bool Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event) bool Server_Card::incrementCounter(int counterId, int delta, Event_SetCardCounter *event) { - // TODO: Extract overflow-safe arithmetic into shared helper. - // Duplicated in Server_Counter::incrementCount() - keep in sync if modified. const int oldValue = counters.value(counterId, 0); const auto result = static_cast(oldValue) + static_cast(delta); - const int newValue = static_cast(qBound(static_cast(std::numeric_limits::min()), result, - static_cast(std::numeric_limits::max()))); + // Clamp to [0, MAX_COUNTERS_ON_CARD] for card counters + const int newValue = + static_cast(qBound(static_cast(0), result, static_cast(MAX_COUNTERS_ON_CARD))); if (newValue == oldValue) { return false; diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h index 41304ef99..3d7e649b9 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h @@ -154,9 +154,9 @@ public: cardRef = _cardRef; } /** - * @brief Sets a card counter to an exact value. + * @brief Sets a card counter to an exact value with clamping. * @param _id The counter ID. - * @param value The new value (0 removes the counter). + * @param value The desired value (clamped to [0, MAX_COUNTERS_ON_CARD]; 0 removes the counter). * @param event Optional event to populate with counter state. * @return true if the value changed, false otherwise. */ @@ -168,7 +168,7 @@ public: * @param event Optional event to populate with counter state. * @return true if the value changed, false otherwise. * @note If counter does not exist, starts from 0. Counter is removed if result is 0. - * @warning For direct assignment without clamping, use setCounter(). + * @note Clamps result to [0, MAX_COUNTERS_ON_CARD]. */ [[nodiscard]] bool incrementCounter(int counterId, int delta, Event_SetCardCounter *event = nullptr); void setTapped(bool _tapped) diff --git a/libcockatrice_utility/libcockatrice/utility/trice_limits.h b/libcockatrice_utility/libcockatrice/utility/trice_limits.h index fa7ce7489..833ce1b98 100644 --- a/libcockatrice_utility/libcockatrice/utility/trice_limits.h +++ b/libcockatrice_utility/libcockatrice/utility/trice_limits.h @@ -15,6 +15,12 @@ constexpr uint MAXIMUM_DIE_SIDES = 1000000; constexpr uint MINIMUM_DICE_TO_ROLL = 1; constexpr uint MAXIMUM_DICE_TO_ROLL = 100; +// Card counter value bounds [0, MAX_COUNTERS_ON_CARD]. +// Counters on cards (e.g., +1/+1 counters, charge counters) are non-negative physical game objects. +// The max of 999 is a display constraint (3-digit rendering) and reasonable gameplay limit. +// Server enforces these bounds; client may also check for UX optimization. +constexpr int MAX_COUNTERS_ON_CARD = 999; + // optimized functions to get qstrings that are at most that long static inline QString nameFromStdString(const std::string &_string) { diff --git a/tests/server_card_counter_test.cpp b/tests/server_card_counter_test.cpp index 67794d26e..ff906b906 100644 --- a/tests/server_card_counter_test.cpp +++ b/tests/server_card_counter_test.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include TEST(ServerCardCounter, IncrementNewCounter) @@ -27,17 +28,18 @@ TEST(ServerCardCounter, IncrementExistingCounter) TEST(ServerCardCounter, IncrementOverflowProtection) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); - ASSERT_TRUE(card.setCounter(1, std::numeric_limits::max())); + ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD)); EXPECT_FALSE(card.incrementCounter(1, 1)); - EXPECT_EQ(card.getCounter(1), std::numeric_limits::max()); + EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD); } TEST(ServerCardCounter, DecrementUnderflowProtection) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); - ASSERT_TRUE(card.setCounter(1, std::numeric_limits::min())); - EXPECT_FALSE(card.incrementCounter(1, -1)); - EXPECT_EQ(card.getCounter(1), std::numeric_limits::min()); + ASSERT_TRUE(card.setCounter(1, 5)); + EXPECT_TRUE(card.incrementCounter(1, -10)); + EXPECT_EQ(card.getCounter(1), 0); + EXPECT_FALSE(card.getCounters().contains(1)); } TEST(ServerCardCounter, ReturnsFalseWhenUnchanged) @@ -111,13 +113,13 @@ TEST(ServerCardCounter, IncrementCounterPopulatesEvent) TEST(ServerCardCounter, IncrementCounterEventReflectsClampedValue) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); - ASSERT_TRUE(card.setCounter(1, std::numeric_limits::max() - 5)); + ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD - 5)); Event_SetCardCounter event; EXPECT_TRUE(card.incrementCounter(1, 10, &event)); EXPECT_EQ(event.counter_id(), 1); - EXPECT_EQ(event.counter_value(), std::numeric_limits::max()); + EXPECT_EQ(event.counter_value(), MAX_COUNTERS_ON_CARD); } TEST(ServerCardCounter, IncrementCounterNoEventWhenNullptr) @@ -131,7 +133,7 @@ TEST(ServerCardCounter, IncrementCounterNoEventWhenNullptr) TEST(ServerCardCounter, IncrementCounterEventNotPopulatedWhenUnchanged) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); - ASSERT_TRUE(card.setCounter(1, std::numeric_limits::max())); + ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD)); Event_SetCardCounter event; event.set_counter_id(999); @@ -142,6 +144,38 @@ TEST(ServerCardCounter, IncrementCounterEventNotPopulatedWhenUnchanged) EXPECT_EQ(event.counter_value(), 999); } +TEST(ServerCardCounter, SetCounterClampsNegativeToZero) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + EXPECT_FALSE(card.setCounter(1, -5)); + EXPECT_EQ(card.getCounter(1), 0); + EXPECT_FALSE(card.getCounters().contains(1)); +} + +TEST(ServerCardCounter, SetCounterClampsAboveMaxToMax) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + EXPECT_TRUE(card.setCounter(1, 1500)); + EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD); +} + +TEST(ServerCardCounter, IncrementDoesNotGoBelowZero) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, 5)); + EXPECT_TRUE(card.incrementCounter(1, -10)); + EXPECT_EQ(card.getCounter(1), 0); + EXPECT_FALSE(card.getCounters().contains(1)); +} + +TEST(ServerCardCounter, IncrementDoesNotExceedMax) +{ + Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); + ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD - 5)); + EXPECT_TRUE(card.incrementCounter(1, 10)); + EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD); +} + int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv);