Unify counter clamp arithmetic into shared addClamped() helper (#7009)

* Unify counter clamp arithmetic into shared addClamped() helper

- Add addClamped() in new header clamped_arithmetic.h; uses a 64-bit
  intermediate so the addition cannot overflow int.
- Use it in Server_Card::incrementCounter() (clamps [0, MAX_COUNTERS_ON_CARD])
  and Server_Counter::incrementCount() (clamps [INT_MIN, INT_MAX]), removing
  the duplicated overflow-safe logic and its keep-in-sync TODO.
- Inline incrementCount() into server_counter.h; server_counter.cpp now holds
  only the constructor and getInfo().
- Clarify the card-counter bounds comment in trice_limits.h.

* Rename MAX_COUNTERS_ON_CARD to MAX_COUNTER_VALUE

The constant caps the counter's value, not how many counters can be on the card

* Add direct unit tests for addClamped() helper

* Harden offsetCardCounter() against signed-int overflow

Replace the raw oldValue + offset sum with addClamped(), clamping to [0, MAX_COUNTER_VALUE] without overflow.

* Comment update

* Remove class names from addClamped() docstring

cockatrice/src/game/player/player_actions.cpp

@@ -27,6 +27,7 @@
 #include <libcockatrice/protocol/pb/command_shuffle.pb.h>
 #include <libcockatrice/protocol/pb/command_undo_draw.pb.h>
 #include <libcockatrice/protocol/pb/context_move_card.pb.h>
+#include <libcockatrice/utility/clamped_arithmetic.h>
 #include <libcockatrice/utility/expression.h>
 #include <libcockatrice/utility/trice_limits.h>
 #include <libcockatrice/utility/zone_names.h>
@@ -1530,12 +1531,15 @@ void PlayerActions::offsetCardCounter(QList<CardItem *> selectedCards, int count
     QList<const ::google::protobuf::Message *> 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<CardItem *> 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<int>(qBound(0.0, parsed, static_cast<double>(MAX_COUNTERS_ON_CARD)));
+        int number = static_cast<int>(qBound(0.0, parsed, static_cast<double>(MAX_COUNTER_VALUE)));
 
         auto *cmd = new Command_SetCardCounter;
         cmd->set_zone(card->getZone()->getName().toStdString());
@@ -1598,7 +1602,7 @@ void PlayerActions::actIncrementAllCardCounters(QList<CardItem *> cardsToUpdate)
             counterIterator.next();
             int counterId = counterIterator.key();
             int currentValue = counterIterator.value();
-            if (currentValue >= MAX_COUNTERS_ON_CARD) {
+            if (currentValue >= MAX_COUNTER_VALUE) {
                 continue;
             }
 

libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp

@@ -26,6 +26,7 @@
 #include <libcockatrice/protocol/pb/event_set_card_attr.pb.h>
 #include <libcockatrice/protocol/pb/event_set_card_counter.pb.h>
 #include <libcockatrice/protocol/pb/serverinfo_card.pb.h>
+#include <libcockatrice/utility/clamped_arithmetic.h>
 #include <libcockatrice/utility/trice_limits.h>
 #include <limits>
 
@@ -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<int64_t>(oldValue) + static_cast<int64_t>(delta);
-    // Clamp to [0, MAX_COUNTERS_ON_CARD] for card counters
-    const int newValue =
-        static_cast<int>(qBound(static_cast<int64_t>(0), result, static_cast<int64_t>(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;

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)

libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.cpp

@@ -1,24 +1,12 @@
 #include "server_counter.h"
 
 #include <libcockatrice/protocol/pb/serverinfo_counter.pb.h>
-#include <limits>
 
 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<int64_t>(count) + static_cast<int64_t>(delta);
-    count = static_cast<int>(qBound(static_cast<int64_t>(std::numeric_limits<int>::min()), result,
-                                    static_cast<int64_t>(std::numeric_limits<int>::max())));
-    return count != oldCount;
-}
-
 void Server_Counter::getInfo(ServerInfo_Counter *info)
 {
     info->set_id(id);

libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h

@@ -22,6 +22,8 @@
 
 #include <QString>
 #include <libcockatrice/protocol/pb/color.pb.h>
+#include <libcockatrice/utility/clamped_arithmetic.h>
+#include <limits>
 
 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<int>::min(), std::numeric_limits<int>::max());
+        return count != oldCount;
+    }
 
     /**
      * @brief Populates info with this counter's current state for network serialization.

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
 )

libcockatrice_utility/libcockatrice/utility/clamped_arithmetic.h

@@ -0,0 +1,22 @@
+#ifndef CLAMPED_ARITHMETIC_H
+#define CLAMPED_ARITHMETIC_H
+
+#include <QtGlobal>
+#include <cstdint>
+
+/**
+ * @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<int64_t>(value) + static_cast<int64_t>(delta);
+    return static_cast<int>(qBound(static_cast<int64_t>(minValue), result, static_cast<int64_t>(maxValue)));
+}
+
+#endif // CLAMPED_ARITHMETIC_H

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 <QString>
 
 // 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)

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}
 )

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 <gtest/gtest.h>
+#include <libcockatrice/utility/clamped_arithmetic.h>
+#include <limits>
+
+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<int>::max();
+    constexpr int intMin = std::numeric_limits<int>::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<int>::max();
+    constexpr int intMin = std::numeric_limits<int>::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();
+}

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)