Guild bank: tab rename, and item movement with the destination on the right side

src/game/Object/Guild.h

@@ -1654,8 +1654,30 @@ class Guild
         /// guild_bank_item row whose TabId no reload can match, and
         /// LoadGuildBankFromDB drops exactly those rows -- the item would be gone
         /// for good. Only a reload of the guild clears the flag.
+        /// True when the named bank slot still holds the item entry the client
+        /// said it did -- zero meaning "empty". A guild bank is shared, so a
+        /// request can arrive after another member has changed the slot under
+        /// it; applying it anyway acts on whatever is there now. Slot 0xFF is
+        /// the client's "anywhere in this tab" and names nothing to compare.
+        bool   BankSlotHoldsEntry(uint8 tabId, uint8 slotId, uint32 expectedEntry);
+
+        /// True when the named bank slot still holds the stack SIZE the client
+        /// said it did. Checking the entry alone is not enough for a request
+        /// that asks for a whole stack: the entry can match while the stack has
+        /// grown underneath it. Slot 0xFF names nothing to compare.
+        bool   BankSlotStackCountIs(uint8 tabId, uint8 slotId, uint32 expectedCount);
+
         bool   IsBankStateTrusted() const { return m_bankStateTrusted; }
         void   MarkBankStateUntrusted() { m_bankStateTrusted = false; }
+
+        /// Commits an item mutation SYNCHRONOUSLY and reports whether the
+        /// database accepted it -- CommitTransactionDirect, never the queuing
+        /// CommitTransaction, which returns true before MySQL has seen anything.
+        /// A false return means memory and the durable rows may now disagree: it
+        /// marks the bank untrusted AND quarantines the player's session, and
+        /// every caller must abandon the operation without broadcasting. See the
+        /// commentary on the definition in GuildBank.cpp.
+        bool   CommitBankMutation(Player* pl, char const* context);
         // per days
         bool   MemberItemWithdraw(uint8 TabId, uint32 LowGuid);
         uint32 GetMemberSlotWithdrawRem(uint32 LowGuid, uint8 TabId);

src/game/Object/GuildBank.cpp

@@ -227,6 +227,83 @@ void Guild::DisplayGuildBankContentUpdate(uint8 TabId, GuildItemPosCountVec cons
     DEBUG_LOG("WORLD: Sent (SMSG_GUILD_BANK_LIST)");
 }
 
+/// Every item mutation below moves the item in memory first and writes the rows
+/// inside a transaction. All nine of those commits used to throw their result
+/// away, and a failure here is not benign: the item has already moved in memory
+/// and its Item state is already back to unchanged, so a rollback leaves the
+/// bank holding one arrangement and the database another, with nothing to say so.
+///
+/// It must be CommitTransactionDirect. The plain CommitTransaction only queues
+/// once the world has loaded -- AllowAsyncTransactions is on from Master.cpp --
+/// handing the statements to the delay thread and returning true before MySQL
+/// has seen them, and the delay thread discards the result. A first version of
+/// this helper called it and claimed to report whether the write landed; it
+/// could not have, and would have reported success for every failure there is.
+///
+/// A false return is ambiguous in the usual way: the statements may have rolled
+/// back, or the COMMIT may have applied with only its result unreadable. Unlike
+/// the money paths there is no small set of balances to re-read -- and as the
+/// tab-purchase path already reasons, correcting item state in memory is the
+/// part that cannot be done safely, since undoing a mutation whose commit
+/// actually landed creates the inverse phantom. So this does not guess.
+///
+/// Marking the bank untrusted is necessary but NOT sufficient on its own. For a
+/// withdrawal the player is already holding the item in memory, and that flag
+/// only stops further BANK packets -- it does nothing about using, trading or
+/// mailing what they now hold. So the session is quarantined the way
+/// WorldSession::SuppressCharacterSave documents: refuse to persist the
+/// character's in-memory state, then disconnect, so the reconnect loads whatever
+/// the database actually holds. Losing unsaved progress is the cheaper error.
+bool Guild::CommitBankMutation(Player* pl, char const* context)
+{
+    if (CharacterDatabase.CommitTransactionDirect())
+    {
+        return true;
+    }
+
+    MarkBankStateUntrusted();
+
+    if (WorldSession* session = pl ? pl->GetSession() : NULL)
+    {
+        session->SuppressCharacterSave();
+        sLog.outError("Guild::%s: commit could not be confirmed for player %u and guild %u. "
+            "Items may have moved in memory without reaching the database, so the bank is "
+            "untrusted until reload and this character's state is discarded rather than saved.",
+            context, pl->GetGUIDLow(), m_Id);
+        session->KickPlayer();
+    }
+    else
+    {
+        sLog.outError("Guild::%s: commit could not be confirmed for guild %u and no session "
+            "was available to quarantine; the bank is untrusted until it is reloaded.",
+            context, m_Id);
+    }
+
+    return false;
+}
+
+bool Guild::BankSlotHoldsEntry(uint8 tabId, uint8 slotId, uint32 expectedEntry)
+{
+    if (slotId == 0xFF)                                     // "anywhere in this tab"
+    {
+        return true;
+    }
+
+    Item const* item = GetItem(tabId, slotId);
+    return uint32(item ? item->GetEntry() : 0) == expectedEntry;
+}
+
+bool Guild::BankSlotStackCountIs(uint8 tabId, uint8 slotId, uint32 expectedCount)
+{
+    if (slotId == 0xFF)                                     // "anywhere in this tab"
+    {
+        return true;
+    }
+
+    Item const* item = GetItem(tabId, slotId);
+    return uint32(item ? item->GetCount() : 0) == expectedCount;
+}
+
 Item* Guild::GetItem(uint8 TabId, uint8 SlotId)
 {
     if (TabId >= GetPurchasedTabs() || SlotId >= GUILD_BANK_MAX_SLOTS)
@@ -269,6 +346,19 @@ void Guild::CreateNewBankTab()
 
 void Guild::SetGuildBankTabInfo(uint8 TabId, std::string Name, std::string Icon)
 {
+    // TabId reaches here straight off the wire (CMSG_GUILD_BANK_UPDATE_TAB), and
+    // the only thing standing between it and this operator[] is the caller's
+    // range check. Guard in-function too, as GetBankRights below does (it bounds
+    // against the GUILD_BANK_MAX_TABS constant rather than a container size, so
+    // the parallel is the practice, not the bound). An out-of-range TabId is not
+    // a bad name: operator[] past the end is undefined behaviour, and the code
+    // below both dereferences the pointer it yields and assigns Name and Icon
+    // through it -- so the consequence is unbounded, not a mere stray read.
+    if (TabId >= m_TabListMap.size())
+    {
+        return;
+    }
+
     if (m_TabListMap[TabId]->Name == Name && m_TabListMap[TabId]->Icon == Icon)
     {
         return;
@@ -1270,21 +1360,40 @@ void Guild::SwapItems(Player* pl, uint8 BankTab, uint8 BankTabSlot, uint8 BankTa
 
     Item* pItemDst = GetItem(BankTabDst, BankTabSlotDst);
 
-    if (BankTab != BankTabDst)
+    // Every slot the destination side actually touches. A merge is not confined
+    // to the slot the client named -- see the refresh at the end of this
+    // function -- so the branches below record what they really wrote.
+    GuildItemPosCountVec destSlots;
+
+    // Rights are checked on EVERY move, not only on one that crosses tabs. Both
+    // checks below used to sit behind `BankTab != BankTabDst`, which left a
+    // same-tab rearrange with no permission check whatsoever -- and same-tab is
+    // the ordinary case rather than a corner one: every bank-to-bank body decoded
+    // from the corpus at build 18414 moves within a single tab. A forged client could
+    // therefore merge, split and reorder items in any purchased tab, including a
+    // tab its rank cannot so much as view. The real client never sends that,
+    // which is exactly why the gap survived: its producer checks the destination
+    // tab's deposit permission before it will build the packet at all.
+    if (!IsMemberHaveRights(pl->GetGUIDLow(), BankTabDst, GUILD_BANK_RIGHT_DEPOSIT_ITEM))
     {
-        // check dest pos rights (if different tabs)
-        if (!IsMemberHaveRights(pl->GetGUIDLow(), BankTabDst, GUILD_BANK_RIGHT_DEPOSIT_ITEM))
-        {
-            return;
-        }
+        return;
+    }
 
-        // check source pos rights (if different tabs)
-        uint32 remRight = GetMemberSlotWithdrawRem(pl->GetGUIDLow(), BankTab);
-        if (remRight <= 0)
+    // The source side is a withdrawal only when the item leaves its tab. Within
+    // one tab nothing leaves the guild, so require sight of the tab but do not
+    // spend the member's daily allowance on tidying it -- otherwise a member who
+    // had used up their withdrawals could not reorder a tab they can see.
+    if (BankTab != BankTabDst)
+    {
+        if (GetMemberSlotWithdrawRem(pl->GetGUIDLow(), BankTab) == 0)
         {
             return;
         }
     }
+    else if (!IsMemberHaveRights(pl->GetGUIDLow(), BankTab, GUILD_BANK_RIGHT_VIEW_TAB))
+    {
+        return;
+    }
 
     if (SplitedAmount)
     {
@@ -1311,12 +1420,33 @@ void Guild::SwapItems(Player* pl, uint8 BankTab, uint8 BankTabSlot, uint8 BankTa
             LogBankEvent(GUILD_BANK_LOG_MOVE_ITEM, BankTab, pl->GetGUIDLow(), pItemSrc->GetEntry(), SplitedAmount, BankTabDst);
         }
 
-        pl->ItemRemovedQuestCheck(pItemSrc->GetEntry(), SplitedAmount);
+        // No ItemRemovedQuestCheck here. Both slots are in the guild bank, so
+        // nothing left the player's inventory -- but that call walks the quest
+        // log and decrements every DELIVER objective matching the entry, so
+        // splitting a bank stack of a quest item used to eat the player's own
+        // progress and could take a completed quest back to incomplete. The
+        // player never held these items; only the bank's arrangement changed.
         pItemSrc->SetCount(pItemSrc->GetCount() - SplitedAmount);
         pItemSrc->FSetState(ITEM_CHANGED);
         pItemSrc->SaveToDB();                               // not in inventory and can be save standalone
         StoreItem(BankTabDst, dest, pNewItem);
-        CharacterDatabase.CommitTransaction();
+        destSlots = dest;
+
+        // Spend the allowance the check above tested. It used to be tested and
+        // never spent, so a member with a single withdrawal left could relay any
+        // number of items out of a restricted tab into one with looser rights and
+        // draw them from there -- the source tab's configured daily limit only
+        // ever had to be non-zero, never sufficient. MoveFromBankToChar has
+        // always consumed it this way; this path simply did not.
+        if (BankTab != BankTabDst)
+        {
+            MemberItemWithdraw(BankTab, pl->GetGUIDLow());
+        }
+
+        if (!CommitBankMutation(pl, "SwapItems"))
+        {
+            return;
+        }
     }
     else                                                    // non split
     {
@@ -1333,7 +1463,17 @@ void Guild::SwapItems(Player* pl, uint8 BankTab, uint8 BankTabSlot, uint8 BankTa
 
             RemoveItem(BankTab, BankTabSlot);
             StoreItem(BankTabDst, gDest, pItemSrc);
-            CharacterDatabase.CommitTransaction();
+            destSlots = gDest;
+
+            if (BankTab != BankTabDst)                      // see the split branch
+            {
+                MemberItemWithdraw(BankTab, pl->GetGUIDLow());
+            }
+
+            if (!CommitBankMutation(pl, "SwapItems"))
+            {
+                return;
+            }
         }
         else                                                // swap
         {
@@ -1381,11 +1521,60 @@ void Guild::SwapItems(Player* pl, uint8 BankTab, uint8 BankTabSlot, uint8 BankTa
             RemoveItem(BankTabDst, BankTabSlotDst);
             StoreItem(BankTab, gSrc, pItemDst);
             StoreItem(BankTabDst, gDest, pItemSrc);
-            CharacterDatabase.CommitTransaction();
+            destSlots = gDest;
+
+            // A cross-tab swap takes an item OUT of both tabs, and the checks
+            // above already tested both allowances, so spend both. Same tab, and
+            // nothing has left the guild at all.
+            if (BankTab != BankTabDst)
+            {
+                MemberItemWithdraw(BankTab, pl->GetGUIDLow());
+                MemberItemWithdraw(BankTabDst, pl->GetGUIDLow());
+            }
+
+            if (!CommitBankMutation(pl, "SwapItems"))
+            {
+                return;
+            }
         }
     }
-    DisplayGuildBankContentUpdate(BankTab, BankTabSlot, BankTab == BankTabDst ? BankTabSlotDst : -1);
-    if (BankTab != BankTabDst)
+    // Refresh what actually changed, which is not always the two slots the
+    // client named. When the destination stack can merge, Guild::CanStoreItem
+    // hands _CanStoreItem_InTab the whole tab and it fills `dest` with EVERY
+    // partial stack of that entry it topped up. Broadcasting only the named
+    // destination left those other slots stale on every viewer -- items sitting
+    // in the bank but drawn as missing until something forced a full refresh.
+    // The character-to-bank paths already broadcast their whole set; this one
+    // did not.
+    //
+    // Source and destination still travel together when they share a tab, so
+    // the common same-tab move stays a single packet.
+    GuildItemPosCountVec touched = destSlots;
+    if (BankTab == BankTabDst)
+    {
+        bool alreadyListed = false;
+        for (GuildItemPosCount const& slot : touched)
+        {
+            if (slot.Slot == BankTabSlot)
+            {
+                alreadyListed = true;
+                break;
+            }
+        }
+        if (!alreadyListed)
+        {
+            touched.push_back(GuildItemPosCount(BankTabSlot, 0));
+        }
+        DisplayGuildBankContentUpdate(BankTab, touched);
+        return;
+    }
+
+    DisplayGuildBankContentUpdate(BankTab, BankTabSlot);
+    if (!touched.empty())
+    {
+        DisplayGuildBankContentUpdate(BankTabDst, touched);
+    }
+    else
     {
         DisplayGuildBankContentUpdate(BankTabDst, BankTabSlotDst);
     }
@@ -1448,7 +1637,10 @@ void Guild::MoveFromBankToChar(Player* pl, uint8 BankTab, uint8 BankTabSlot, uin
         pl->SaveInventoryAndGoldToDB();
 
         MemberItemWithdraw(BankTab, pl->GetGUIDLow());
-        CharacterDatabase.CommitTransaction();
+        if (!CommitBankMutation(pl, "MoveFromBankToChar"))
+        {
+            return;
+        }
     }
     else                                                    // Bank -> Char swap with slot (move)
     {
@@ -1471,7 +1663,10 @@ void Guild::MoveFromBankToChar(Player* pl, uint8 BankTab, uint8 BankTabSlot, uin
             pl->SaveInventoryAndGoldToDB();
 
             MemberItemWithdraw(BankTab, pl->GetGUIDLow());
-            CharacterDatabase.CommitTransaction();
+            if (!CommitBankMutation(pl, "MoveFromBankToChar"))
+            {
+                return;
+            }
         }
         else                                                // Bank <-> Char swap items
         {
@@ -1547,7 +1742,10 @@ void Guild::MoveFromBankToChar(Player* pl, uint8 BankTab, uint8 BankTabSlot, uin
             pl->SaveInventoryAndGoldToDB();
 
             MemberItemWithdraw(BankTab, pl->GetGUIDLow());
-            CharacterDatabase.CommitTransaction();
+            if (!CommitBankMutation(pl, "MoveFromBankToChar"))
+            {
+                return;
+            }
         }
     }
     DisplayGuildBankContentUpdate(BankTab, BankTabSlot);
@@ -1619,7 +1817,10 @@ void Guild::MoveFromCharToBank(Player* pl, uint8 PlayerBag, uint8 PlayerSlot, ui
         pItemChar->SetState(ITEM_CHANGED);
         pl->SaveInventoryAndGoldToDB();
         StoreItem(BankTab, dest, pNewItem);
-        CharacterDatabase.CommitTransaction();
+        if (!CommitBankMutation(pl, "MoveFromCharToBank"))
+        {
+            return;
+        }
 
         DisplayGuildBankContentUpdate(BankTab, dest);
     }
@@ -1646,7 +1847,10 @@ void Guild::MoveFromCharToBank(Player* pl, uint8 PlayerBag, uint8 PlayerSlot, ui
 
             StoreItem(BankTab, dest, pItemChar);
             pl->SaveInventoryAndGoldToDB();
-            CharacterDatabase.CommitTransaction();
+            if (!CommitBankMutation(pl, "MoveFromCharToBank"))
+            {
+                return;
+            }
 
             DisplayGuildBankContentUpdate(BankTab, dest);
         }
@@ -1714,7 +1918,10 @@ void Guild::MoveFromCharToBank(Player* pl, uint8 PlayerBag, uint8 PlayerSlot, ui
             {
                 MemberItemWithdraw(BankTab, pl->GetGUIDLow());
             }
-            CharacterDatabase.CommitTransaction();
+            if (!CommitBankMutation(pl, "MoveFromCharToBank"))
+            {
+                return;
+            }
 
             DisplayGuildBankContentUpdate(BankTab, gDest);
         }

src/game/Object/GuildRank.cpp

@@ -25,6 +25,7 @@
 
 #include "Guild.h"
 #include "Database/DatabaseEnv.h"
+#include "ObjectMgr.h"
 #include "Log.h"
 
 /**