From 2a1b505913b2813008d12f35b32a6d6d0cb47e0b Mon Sep 17 00:00:00 2001
From: DawnFire42 <justin.c.durocher@gmail.com>
Date: Fri, 26 Jun 2026 11:49:52 -0400
Subject: [PATCH] Harden offsetCardCounter() against signed-int overflow

Replace the raw oldValue + offset sum with addClamped(), clamping to [0, MAX_COUNTER_VALUE] without overflow.
---
 cockatrice/src/game/player/player_actions.cpp      | 14 +++++++++-----
 .../libcockatrice/utility/clamped_arithmetic.h     |  4 ++--
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/cockatrice/src/game/player/player_actions.cpp b/cockatrice/src/game/player/player_actions.cpp
index a9c2dbf18..1c469a42a 100644
--- a/cockatrice/src/game/player/player_actions.cpp
+++ b/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_COUNTER_VALUE].
-        // Compare clamped value to allow recovery from invalid states.
-        int clampedValue = qBound(0, newValue, MAX_COUNTER_VALUE);
-        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());
diff --git a/libcockatrice_utility/libcockatrice/utility/clamped_arithmetic.h b/libcockatrice_utility/libcockatrice/utility/clamped_arithmetic.h
index 5391cdbd8..54ad79c37 100644
--- a/libcockatrice_utility/libcockatrice/utility/clamped_arithmetic.h
+++ b/libcockatrice_utility/libcockatrice/utility/clamped_arithmetic.h
@@ -7,8 +7,8 @@
 /**
  * @brief Overflow-safe clamped addition: returns value + delta bounded to [minValue, maxValue].
  *
- * Uses a 64-bit intermediate so the addition itself cannot overflow int. Shared by the
- * counter arithmetic in Server_Card and Server_Counter so both stay in sync.
+ * Uses a 64-bit intermediate so the addition cannot overflow int. Shared by the counter
+ * arithmetic in Server_Card, Server_Counter, and PlayerActions.
  *
  * @note Requires minValue <= maxValue. Bounds come from trusted compile-time call sites;
  *       qBound() asserts this internally in debug builds.