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

src/game/Object/Guild.h

@@ -1656,6 +1656,15 @@ class Guild
         /// for good. Only a reload of the guild clears the flag.
         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,61 @@ 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;
+}
+
 Item* Guild::GetItem(uint8 TabId, uint8 SlotId)
 {
     if (TabId >= GetPurchasedTabs() || SlotId >= GUILD_BANK_MAX_SLOTS)
@@ -269,6 +324,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 +1338,35 @@ void Guild::SwapItems(Player* pl, uint8 BankTab, uint8 BankTabSlot, uint8 BankTa
 
     Item* pItemDst = GetItem(BankTabDst, BankTabSlotDst);
 
-    if (BankTab != BankTabDst)
+    // 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)
     {
@@ -1316,7 +1398,22 @@ void Guild::SwapItems(Player* pl, uint8 BankTab, uint8 BankTabSlot, uint8 BankTa
         pItemSrc->FSetState(ITEM_CHANGED);
         pItemSrc->SaveToDB();                               // not in inventory and can be save standalone
         StoreItem(BankTabDst, dest, pNewItem);
-        CharacterDatabase.CommitTransaction();
+
+        // 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 +1430,16 @@ void Guild::SwapItems(Player* pl, uint8 BankTab, uint8 BankTabSlot, uint8 BankTa
 
             RemoveItem(BankTab, BankTabSlot);
             StoreItem(BankTabDst, gDest, pItemSrc);
-            CharacterDatabase.CommitTransaction();
+
+            if (BankTab != BankTabDst)                      // see the split branch
+            {
+                MemberItemWithdraw(BankTab, pl->GetGUIDLow());
+            }
+
+            if (!CommitBankMutation(pl, "SwapItems"))
+            {
+                return;
+            }
         }
         else                                                // swap
         {
@@ -1381,7 +1487,20 @@ void Guild::SwapItems(Player* pl, uint8 BankTab, uint8 BankTabSlot, uint8 BankTa
             RemoveItem(BankTabDst, BankTabSlotDst);
             StoreItem(BankTab, gSrc, pItemDst);
             StoreItem(BankTabDst, gDest, pItemSrc);
-            CharacterDatabase.CommitTransaction();
+
+            // 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);
@@ -1448,7 +1567,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 +1593,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 +1672,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 +1747,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 +1777,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 +1848,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/MopGuildBankPackets.h

@@ -19,7 +19,28 @@ namespace MopGuildBankPackets
     static size_t const MAX_ITEM_COUNT = 98;
     static size_t const MAX_SOCKET_ENCHANT_COUNT = 3;
     static size_t const MAX_TAB_NAME_BYTES = 64;
-    static size_t const MAX_TAB_ICON_BYTES = 255;
+    // A hard client-buffer bound, not merely a copy limit. In the inbound parser's
+    // record sub_6A224B puts the icon at +0x14 and the name at +0x115, so the icon
+    // field is exactly 0x101 = 256 bytes plus its terminator -- while the 9-bit
+    // length that precedes it could carry 511. The copy loops agree: 0x96F05F
+    // outbound and the one in sub_96ED66 both count down from 0x100 and write the
+    // terminator wherever the pointer stopped, so 256 bytes survive intact.
+    //
+    // This was 255, one below the real bound and one away from the reader in
+    // MopCompactPackets. That difference IS observable, and not only through the
+    // rename path this constant was added for: Guild::DisplayGuildBankContent
+    // truncates every tab icon against it before BuildListBody ever sees one, so
+    // it governs every bank list the server sends. An icon longer than 255 bytes
+    // can reach that truncation without ever passing the 100-byte handler guard,
+    // because LoadGuildBankFromDB reads TabIcon straight out of a utf8 varchar(100)
+    // -- 100 characters, so as much as 300 bytes -- with no length check at all.
+    // At 255 TruncateUtf8 cut such a value on the way to every client -- by one
+    // byte for the ASCII macro filenames the stock UI sends, by more where the cut
+    // landed inside a multi-byte sequence and it backed up to the lead byte.
+    //
+    // Keep this in step with MopCompactPackets::ReadGuildBankUpdateTab if either
+    // ever moves; they mirror the same client limit and cannot be edited apart.
+    static size_t const MAX_TAB_ICON_BYTES = 256;
     static size_t const MAX_POST_CRYPT_PAYLOAD_BYTES = 0x7FFFF;
 
     struct SocketEnchant
@@ -212,6 +233,51 @@ namespace MopGuildBankPackets
         out << uint64(bankMoney);
     }
 
+    /// SMSG_GUILD_EVENT_BANK_TAB_MODIFIED (0x0BF1): one tab's new name and icon.
+    ///
+    /// Unusually for this campaign, this body comes from the client's INBOUND
+    /// parser rather than from a corpus capture -- there is no observation of
+    /// this opcode at 18414 in generation 2BE10C89...88752. The client's own
+    /// reader is the better oracle here anyway, and it is unambiguous:
+    ///
+    ///   sub_6A224B reads a 9-bit length (8 bits via sub_66529C, then 1 bit,
+    ///   combined as (hi << 1) | lo), then a 7-bit length (sub_6650D3) -- 16 bits
+    ///   exactly, so nothing is padded -- then the NAME bytes, then a uint32 tab
+    ///   id, then the ICON bytes. Both strings are raw; the parser NUL-terminates
+    ///   them itself after reading, so none is sent.
+    ///
+    /// Which length belongs to which string is fixed by where the parser puts
+    /// them: the 7-bit length reads into the record at +0x115 and the 9-bit one
+    /// into +0x14, and sub_96ED66 then takes those two, sanitises the +0x115
+    /// string through the SAME 16-character limiter the outbound
+    /// SetGuildBankTabInfo path uses (sub_CBCC7F with 0x10/0x41) and copies the
+    /// +0x14 string under the same 256-byte limit, into the client's tab cache at
+    /// 0x11F4140 with stride 0x2148, before raising event 0x1AF. So the 7-bit
+    /// field is the name and the 9-bit field is the icon, exactly as in the
+    /// request -- see MopCompactPackets::ReadGuildBankUpdateTab.
+    ///
+    /// Note the order differs from the request: here the NAME bytes come first
+    /// and the tab id sits BETWEEN the two strings.
+    inline bool BuildGuildBankTabModified(ByteBuffer& out, uint32 tabId,
+        std::string const& name, std::string const& icon)
+    {
+        if (tabId >= MAX_TAB_COUNT ||
+                name.size() > MAX_TAB_NAME_BYTES ||
+                icon.size() > MAX_TAB_ICON_BYTES)
+        {
+            return false;
+        }
+
+        out.WriteBits(uint32(icon.size()), 9);
+        out.WriteBits(uint32(name.size()), 7);
+        out.FlushBits();                                    // 16 bits: adds nothing
+
+        out.append(name.data(), name.size());
+        out << uint32(tabId);
+        out.append(icon.data(), icon.size());
+        return true;
+    }
+
 }
 
 #endif