From 76fa87c63ef2659d45dc2533e5c3f46269c5429b Mon Sep 17 00:00:00 2001
From: Basile Clement <Elarnon@users.noreply.github.com>
Date: Fri, 21 Mar 2025 01:30:46 +0100
Subject: [PATCH] Fix StackZone crash when divideCardSpaceInZone overflows
 (#5751)

The divideCardSpaceInZone function introduced in #4930 is buggy and
sometimes returns an index that is too large for the current zone, which
causes us to call `cards.at(index)` with an `index` that's bigger than
the amount of cards.

This is the bug that #5609 intended to fix but was improperly diagnosed.
Remove part of #5609 as the cases it is guarding against (e.g. null card
pointer) cannot actually happen.
---
 cockatrice/resources/config/qtlogging.ini |  4 +---
 cockatrice/src/game/zones/stack_zone.cpp  | 27 +++++++++++------------
 cockatrice/src/game/zones/stack_zone.h    |  2 --
 3 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/cockatrice/resources/config/qtlogging.ini b/cockatrice/resources/config/qtlogging.ini
index 378b962b7..60a123d5e 100644
--- a/cockatrice/resources/config/qtlogging.ini
+++ b/cockatrice/resources/config/qtlogging.ini
@@ -47,12 +47,10 @@
 # card_info = false
 # card_list = false
 
-# stack_zone = false
-
 flow_layout.debug = false
 flow_widget.debug = false
 flow_widget.size.debug = false
 
 # pixel_map_generator = false
 
-# filter_string = false
\ No newline at end of file
+# filter_string = false
diff --git a/cockatrice/src/game/zones/stack_zone.cpp b/cockatrice/src/game/zones/stack_zone.cpp
index 2f307085c..b597a9e81 100644
--- a/cockatrice/src/game/zones/stack_zone.cpp
+++ b/cockatrice/src/game/zones/stack_zone.cpp
@@ -66,26 +66,25 @@ void StackZone::handleDropEvent(const QList<CardDragItem *> &dragItems, CardZone
     cmd.set_target_zone(getName().toStdString());
 
     int index = 0;
-    if (cards.isEmpty()) {
-        index = 0;
-    } else {
+
+    if (!cards.isEmpty()) {
         const auto cardCount = static_cast<int>(cards.size());
         const auto &card = cards.at(0);
 
-        if (card == nullptr) {
-            qCWarning(StackZoneLog) << "Attempted to move card from" << startZone->getName() << ", but was null";
-            return;
-        }
-
         index = qRound(divideCardSpaceInZone(dropPoint.y(), cardCount, boundingRect().height(),
                                              card->boundingRect().height(), true));
-    }
 
-    if (startZone == this) {
-        const auto &dragItem = dragItems.at(0);
-        const auto &card = cards.at(index);
-        if (card != nullptr && dragItem != nullptr && card->getId() == dragItem->getId()) {
-            return;
+        // divideCardSpaceInZone is not guaranteed to return a valid index
+        // currently, so clamp it to avoid crashes.
+        index = qBound(0, index, cardCount - 1);
+
+        if (startZone == this) {
+            const auto &dragItem = dragItems.at(0);
+            const auto &card = cards.at(index);
+
+            if (card->getId() == dragItem->getId()) {
+                return;
+            }
         }
     }
 
diff --git a/cockatrice/src/game/zones/stack_zone.h b/cockatrice/src/game/zones/stack_zone.h
index 4ef06ea9a..38dee0aca 100644
--- a/cockatrice/src/game/zones/stack_zone.h
+++ b/cockatrice/src/game/zones/stack_zone.h
@@ -3,8 +3,6 @@
 
 #include "select_zone.h"
 
-inline Q_LOGGING_CATEGORY(StackZoneLog, "stack_zone");
-
 class StackZone : public SelectZone
 {
     Q_OBJECT