diff --git a/cockatrice/src/game/player/player_actions.cpp b/cockatrice/src/game/player/player_actions.cpp index c9e8628f4..1c469a42a 100644 --- a/cockatrice/src/game/player/player_actions.cpp +++ b/cockatrice/src/game/player/player_actions.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -1530,12 +1531,15 @@ void PlayerActions::offsetCardCounter(QList selectedCards, int count QList commandList; for (auto card : selectedCards) { int oldValue = card->getCounters().value(counterId, 0); - int newValue = oldValue + offset; - // 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) { + // Overflow-safe clamp to the server-enforced range [0, MAX_COUNTER_VALUE]; + // a result differing from oldValue also corrects an out-of-range cached value. + // Callers only ever pass offset == ±1 (actAddCardCounter / actRemoveCardCounter). + // This client-side clamp is a defense-in-depth UX check, consistent with + // actSetCardCounter and actIncrementAllCardCounters; the server remains the + // authoritative enforcer of the bounds. + int newValue = addClamped(oldValue, offset, 0, MAX_COUNTER_VALUE); + if (newValue != oldValue) { auto *cmd = new Command_SetCardCounter; cmd->set_zone(card->getZone()->getName().toStdString()); cmd->set_card_id(card->getId()); @@ -1568,7 +1572,7 @@ void PlayerActions::actSetCardCounter(QList selectedCards, int count Expression exp(oldValue); double parsed = exp.parse(counterValue); // Clamp in double precision first to avoid UB, then cast - int number = static_cast(qBound(0.0, parsed, static_cast(MAX_COUNTERS_ON_CARD))); + int number = static_cast(qBound(0.0, parsed, static_cast(MAX_COUNTER_VALUE))); auto *cmd = new Command_SetCardCounter; cmd->set_zone(card->getZone()->getName().toStdString()); @@ -1598,7 +1602,7 @@ void PlayerActions::actIncrementAllCardCounters(QList cardsToUpdate) counterIterator.next(); int counterId = counterIterator.key(); int currentValue = counterIterator.value(); - if (currentValue >= MAX_COUNTERS_ON_CARD) { + if (currentValue >= MAX_COUNTER_VALUE) { continue; } 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 b858314c0..4ff0cfb5b 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 #include @@ -114,8 +115,8 @@ 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); + // Clamp to valid card counter range [0, MAX_COUNTER_VALUE] + value = qBound(0, value, MAX_COUNTER_VALUE); const int oldValue = counters.value(_id, 0); if (value == oldValue) { @@ -139,10 +140,8 @@ bool Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event) bool Server_Card::incrementCounter(int counterId, int delta, Event_SetCardCounter *event) { const int oldValue = counters.value(counterId, 0); - const auto result = static_cast(oldValue) + static_cast(delta); - // 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))); + // Clamp to [0, MAX_COUNTER_VALUE] for card counters + const int newValue = addClamped(oldValue, delta, 0, MAX_COUNTER_VALUE); 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 3d7e649b9..a2698ad61 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h @@ -156,7 +156,7 @@ public: /** * @brief Sets a card counter to an exact value with clamping. * @param _id The counter ID. - * @param value The desired value (clamped to [0, MAX_COUNTERS_ON_CARD]; 0 removes the counter). + * @param value The desired value (clamped to [0, MAX_COUNTER_VALUE]; 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. - * @note Clamps result to [0, MAX_COUNTERS_ON_CARD]. + * @note Clamps result to [0, MAX_COUNTER_VALUE]. */ [[nodiscard]] bool incrementCounter(int counterId, int delta, Event_SetCardCounter *event = nullptr); void setTapped(bool _tapped) diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.cpp index e65205cbb..b18e11c2b 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.cpp @@ -1,24 +1,12 @@ #include "server_counter.h" #include -#include Server_Counter::Server_Counter(int _id, const QString &_name, const color &_counterColor, int _radius, int _count) : id(_id), name(_name), counterColor(_counterColor), radius(_radius), count(_count) { } -//! \todo Extract overflow-safe arithmetic into shared helper. -//! Duplicated in Server_Card::incrementCounter() - keep in sync if modified. -bool Server_Counter::incrementCount(int delta) -{ - const int oldCount = count; - const auto result = static_cast(count) + static_cast(delta); - count = static_cast(qBound(static_cast(std::numeric_limits::min()), result, - static_cast(std::numeric_limits::max()))); - return count != oldCount; -} - void Server_Counter::getInfo(ServerInfo_Counter *info) { info->set_id(id); diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h index 8226e663f..ca093b7cf 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h @@ -22,6 +22,8 @@ #include #include +#include +#include class ServerInfo_Counter; @@ -92,7 +94,12 @@ public: * @return true if the value changed, false otherwise. * @note Clamps result to [INT_MIN, INT_MAX] to prevent overflow. */ - [[nodiscard]] bool incrementCount(int delta); + [[nodiscard]] bool incrementCount(int delta) + { + const int oldCount = count; + count = addClamped(count, delta, std::numeric_limits::min(), std::numeric_limits::max()); + return count != oldCount; + } /** * @brief Populates info with this counter's current state for network serialization. diff --git a/libcockatrice_utility/CMakeLists.txt b/libcockatrice_utility/CMakeLists.txt index df29d6c9f..13f13bb56 100644 --- a/libcockatrice_utility/CMakeLists.txt +++ b/libcockatrice_utility/CMakeLists.txt @@ -16,6 +16,7 @@ set(UTILITY_HEADERS libcockatrice/utility/macros.h libcockatrice/utility/passwordhasher.h libcockatrice/utility/trice_limits.h + libcockatrice/utility/clamped_arithmetic.h libcockatrice/utility/zone_names.h libcockatrice/utility/days_years_between.h ) diff --git a/libcockatrice_utility/libcockatrice/utility/clamped_arithmetic.h b/libcockatrice_utility/libcockatrice/utility/clamped_arithmetic.h new file mode 100644 index 000000000..1afac758c --- /dev/null +++ b/libcockatrice_utility/libcockatrice/utility/clamped_arithmetic.h @@ -0,0 +1,22 @@ +#ifndef CLAMPED_ARITHMETIC_H +#define CLAMPED_ARITHMETIC_H + +#include +#include + +/** + * @brief Overflow-safe clamped addition: returns value + delta bounded to [minValue, maxValue]. + * + * Uses a 64-bit intermediate so the addition cannot overflow int. Shared by the bounded + * counter arithmetic in both the client and the server. + * + * @note Requires minValue <= maxValue. Bounds come from trusted compile-time call sites; + * qBound() asserts this internally in debug builds. + */ +inline int addClamped(int value, int delta, int minValue, int maxValue) +{ + const auto result = static_cast(value) + static_cast(delta); + return static_cast(qBound(static_cast(minValue), result, static_cast(maxValue))); +} + +#endif // CLAMPED_ARITHMETIC_H diff --git a/libcockatrice_utility/libcockatrice/utility/trice_limits.h b/libcockatrice_utility/libcockatrice/utility/trice_limits.h index 833ce1b98..23227eb3c 100644 --- a/libcockatrice_utility/libcockatrice/utility/trice_limits.h +++ b/libcockatrice_utility/libcockatrice/utility/trice_limits.h @@ -1,6 +1,9 @@ #ifndef TRICE_LIMITS_H #define TRICE_LIMITS_H +//! \todo Split trice_limits.h into focused single-purpose headers: string_limits.h, +//! dice_limits.h, counter_limits.h. + #include // max size for short strings, like names and things that are generally a single phrase @@ -15,11 +18,18 @@ 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; +/** + * @brief Upper bound for a bounded counter's value: [0, MAX_COUNTER_VALUE]. + * + * Caps an individual counter's VALUE (e.g. a +1/+1 counter at 999), not how many counters + * something holds. Applies to counters that are constrained to a non-negative display range, + * such as card counters and commander tax. Unbounded counters (e.g. a player's life total) + * do not use this limit and may go negative, saturating only at the int range. + * + * The max of 999 is a display constraint (3-digit rendering) and a reasonable gameplay limit. + * The server enforces these bounds; the client may also check them for UX optimization. + */ +constexpr int MAX_COUNTER_VALUE = 999; // optimized functions to get qstrings that are at most that long static inline QString nameFromStdString(const std::string &_string) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 04ac7fcee..a179a3603 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -4,6 +4,7 @@ enable_testing() add_test(NAME dummy_test COMMAND dummy_test) add_test(NAME expression_test COMMAND expression_test) +add_test(NAME clamped_arithmetic_test COMMAND clamped_arithmetic_test) add_test(NAME test_age_formatting COMMAND test_age_formatting) add_test(NAME password_hash_test COMMAND password_hash_test) add_test(NAME server_card_counter_test COMMAND server_card_counter_test) @@ -16,6 +17,7 @@ set_tests_properties(deck_hash_performance_test PROPERTIES TIMEOUT 5) add_executable(dummy_test dummy_test.cpp) add_executable(expression_test expression_test.cpp) +add_executable(clamped_arithmetic_test clamped_arithmetic_test.cpp) add_executable(test_age_formatting test_age_formatting.cpp) add_executable(password_hash_test password_hash_test.cpp) add_executable(deck_hash_performance_test deck_hash_performance_test.cpp) @@ -49,6 +51,7 @@ if(NOT GTEST_FOUND) set(GTEST_BOTH_LIBRARIES gtest) add_dependencies(dummy_test gtest) add_dependencies(expression_test gtest) + add_dependencies(clamped_arithmetic_test gtest) add_dependencies(test_age_formatting gtest) add_dependencies(password_hash_test gtest) add_dependencies(deck_hash_performance_test gtest) @@ -59,6 +62,9 @@ endif() include_directories(${GTEST_INCLUDE_DIRS}) target_link_libraries(dummy_test Threads::Threads ${GTEST_BOTH_LIBRARIES}) target_link_libraries(expression_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) +target_link_libraries( + clamped_arithmetic_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +) target_link_libraries( test_age_formatting libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} ) diff --git a/tests/clamped_arithmetic_test.cpp b/tests/clamped_arithmetic_test.cpp new file mode 100644 index 000000000..2471d5870 --- /dev/null +++ b/tests/clamped_arithmetic_test.cpp @@ -0,0 +1,44 @@ +/** @file clamped_arithmetic_test.cpp + * @brief Tests for shared helpers in clamped_arithmetic.h. + * @ingroup Tests + */ + +#include +#include +#include + +TEST(AddClamped, AddsWithinBounds) +{ + EXPECT_EQ(addClamped(5, 3, 0, 100), 8); + EXPECT_EQ(addClamped(10, -3, 0, 100), 7); +} + +TEST(AddClamped, ClampsToUpperAndLowerBound) +{ + EXPECT_EQ(addClamped(99, 5, 0, 100), 100); // saturates at max + EXPECT_EQ(addClamped(2, -10, 0, 100), 0); // saturates at min + EXPECT_EQ(addClamped(999, 1, 0, 999), 999); // crossing the counter cap holds at the bound +} + +TEST(AddClamped, IntOverflowDoesNotWrap) +{ + // The 64-bit intermediate must prevent signed-int overflow UB. + constexpr int intMax = std::numeric_limits::max(); + constexpr int intMin = std::numeric_limits::min(); + EXPECT_EQ(addClamped(intMax, 1, intMin, intMax), intMax); + EXPECT_EQ(addClamped(intMax, intMax, intMin, intMax), intMax); +} + +TEST(AddClamped, IntUnderflowDoesNotWrap) +{ + constexpr int intMax = std::numeric_limits::max(); + constexpr int intMin = std::numeric_limits::min(); + EXPECT_EQ(addClamped(intMin, -1, intMin, intMax), intMin); + EXPECT_EQ(addClamped(intMin, intMin, intMin, intMax), intMin); +} + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tests/server_card_counter_test.cpp b/tests/server_card_counter_test.cpp index ff906b906..b6aacc31b 100644 --- a/tests/server_card_counter_test.cpp +++ b/tests/server_card_counter_test.cpp @@ -28,9 +28,9 @@ TEST(ServerCardCounter, IncrementExistingCounter) TEST(ServerCardCounter, IncrementOverflowProtection) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); - ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD)); + ASSERT_TRUE(card.setCounter(1, MAX_COUNTER_VALUE)); EXPECT_FALSE(card.incrementCounter(1, 1)); - EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD); + EXPECT_EQ(card.getCounter(1), MAX_COUNTER_VALUE); } TEST(ServerCardCounter, DecrementUnderflowProtection) @@ -113,13 +113,13 @@ TEST(ServerCardCounter, IncrementCounterPopulatesEvent) TEST(ServerCardCounter, IncrementCounterEventReflectsClampedValue) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); - ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD - 5)); + ASSERT_TRUE(card.setCounter(1, MAX_COUNTER_VALUE - 5)); Event_SetCardCounter event; EXPECT_TRUE(card.incrementCounter(1, 10, &event)); EXPECT_EQ(event.counter_id(), 1); - EXPECT_EQ(event.counter_value(), MAX_COUNTERS_ON_CARD); + EXPECT_EQ(event.counter_value(), MAX_COUNTER_VALUE); } TEST(ServerCardCounter, IncrementCounterNoEventWhenNullptr) @@ -133,7 +133,7 @@ TEST(ServerCardCounter, IncrementCounterNoEventWhenNullptr) TEST(ServerCardCounter, IncrementCounterEventNotPopulatedWhenUnchanged) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); - ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD)); + ASSERT_TRUE(card.setCounter(1, MAX_COUNTER_VALUE)); Event_SetCardCounter event; event.set_counter_id(999); @@ -156,7 +156,7 @@ 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); + EXPECT_EQ(card.getCounter(1), MAX_COUNTER_VALUE); } TEST(ServerCardCounter, IncrementDoesNotGoBelowZero) @@ -171,9 +171,9 @@ TEST(ServerCardCounter, IncrementDoesNotGoBelowZero) TEST(ServerCardCounter, IncrementDoesNotExceedMax) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); - ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD - 5)); + ASSERT_TRUE(card.setCounter(1, MAX_COUNTER_VALUE - 5)); EXPECT_TRUE(card.incrementCounter(1, 10)); - EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD); + EXPECT_EQ(card.getCounter(1), MAX_COUNTER_VALUE); } int main(int argc, char **argv)