jeirmeister/Cockatrice
1
0
Fork 0
mirror of https://github.com/Cockatrice/Cockatrice.git synced 2026-09-29 09:22:18 -07:00
Code Issues Projects Releases Packages Wiki Activity Actions

Harden offsetCardCounter() against signed-int overflow

Browse source 
Replace the raw oldValue + offset sum with addClamped(), clamping to [0, MAX_COUNTER_VALUE] without overflow.
This commit is contained in:
DawnFire42 2026-06-26 11:49:52 -04:00
parent 3aa94b4f31
commit 2a1b505913
No known key found for this signature in database
GPG key ID: 24BB855EE2911B33
2 changed files with 11 additions and 7 deletions
Download patch file Download diff file Expand all files Collapse all files

14
cockatrice/src/game/player/player_actions.cpp
View file

@ -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());

4
libcockatrice_utility/libcockatrice/utility/clamped_arithmetic.h
View file

@ -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.