diff --git a/src/game/Object/Guild.h b/src/game/Object/Guild.h index 0e8075062..8fc009f4e 100644 --- a/src/game/Object/Guild.h +++ b/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); diff --git a/src/game/Object/GuildBank.cpp b/src/game/Object/GuildBank.cpp index c36b17b8b..ac62efed8 100644 --- a/src/game/Object/GuildBank.cpp +++ b/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); } diff --git a/src/game/Object/MopGuildBankPackets.h b/src/game/Object/MopGuildBankPackets.h index 9ddef7b5a..eaf1d7d25 100644 --- a/src/game/Object/MopGuildBankPackets.h +++ b/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 diff --git a/src/game/Object/Unit.h b/src/game/Object/Unit.h index fde9d81b0..a47385e3e 100644 --- a/src/game/Object/Unit.h +++ b/src/game/Object/Unit.h @@ -1180,6 +1180,321 @@ namespace MopCompactPackets return true; } + /// CMSG_GUILD_BANK_UPDATE_TAB (0x07C2) -- thunk sub_686A1D, vtable 0xD64874 + /// with the 0x00C84A3D signature in slot +12, body writer sub_68B694. + /// + /// The tab id leads as a plain byte. Then a bit stream that interleaves the + /// bank GUID's presence mask with BOTH string lengths, and only afterwards + /// the byte section, where the two strings sit between GUID bytes rather + /// than after them. It comes to exactly 24 bits, so the client's flush adds + /// no padding and the body is byte-aligned from the tab id onwards. + /// + /// Which string is which is NOT decidable from the writer. It emits two + /// variable-length strings and nothing on the wire labels either. The Lua + /// binding settles it: sub_96F6DE is SetGuildBankTabInfo(tab, name, + /// iconFileName), and it calls sub_96EFA2(tab, name, icon), which stores the + /// name at object +0x11 and the icon path at +0x60. The writer gives +0x11 a + /// 7-bit length and +0x60 a 9-bit one, which fits those two fields and would + /// not fit them reversed. + /// + /// The caps below are the client's copy limits, not the bit fields' ranges: + /// 7 bits would allow 127 and 9 would allow 511, but the client cannot emit + /// more than it copies. Both copy loops are the same shape -- count down from + /// a limit, then write the terminator wherever the pointer stopped -- so the + /// limit is the maximum strlen, NOT one less than it: the name loop at + /// 0x96F037 runs from 0x40 and permits 64, and the icon loop at 0x96F05F runs + /// from 0x100 and permits 256. Anything longer did not come from a stock + /// client. (Both are far above what is reachable in practice -- the name is + /// sanitised at 0x96EFB8 before it is even copied -- sub_CBCC7F terminates as + /// its count REACHES 16, so 15 characters survive, which the client's own + /// Blizzard_GuildBankUI.xml confirms with letters="15" -- and the icon is a + /// bare macro filename -- but a reader that refused a body the + /// client can legitimately produce would be the worse error.) + /// + /// The 9-bit length is written as a whole byte (sub_665185) followed by one + /// bit (sub_665157), high part first, so it reassembles as (hi << 1) | lo. + /// + /// No capture of this opcode exists at build 18414 under catalogue + /// generation 2BE10C89...88752, so this layout has never met a real body. + /// The fixture covering it is synthetic and encoded from the writer above, + /// not from this reader. + inline bool ReadGuildBankUpdateTab(WorldPacket& in, uint8& tabId, + std::string& name, std::string& icon, ObjectGuid& bankGuid) + { + size_t const remaining = in.size() - in.rpos(); + if (remaining < 4) // tab id plus 24 mask bits + { + in.rfinish(); + return false; + } + + uint8 parsedTab = 0; + in >> parsedTab; + + // Mask bits and the two lengths share one stream, in the writer's order: + // guid[5], the 9-bit icon length, the remaining seven mask bits, then the + // 7-bit name length. + uint8 guid[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; + uint8 const maskTail[] = { 1, 4, 2, 7, 0, 6, 3 }; + in.ResetBitReader(); + guid[5] = in.ReadBit(); + uint32 const iconHigh = in.ReadBits(8); + uint32 const iconLow = in.ReadBits(1); + uint32 const iconLength = (iconHigh << 1) | iconLow; + for (uint8 index = 0; index < 7; ++index) + { + guid[maskTail[index]] = in.ReadBit(); + } + uint32 const nameLength = in.ReadBits(7); + + // The client's own limits. Deliberately literals rather than + // MopGuildBankPackets::MAX_TAB_*_BYTES: this header cannot include that one + // without dragging its dependencies into everything that includes Unit.h. + // They must stay in step all the same -- they were 255 and 256 for a while, + // and that skew is what a review caught, so change both or neither. + if (iconLength > 256 || nameLength > 64) + { + in.rfinish(); + return false; + } + + uint8 present[8]; + size_t guidByteCount = 0; + for (uint8 index = 0; index < 8; ++index) + { + present[index] = guid[index]; + guidByteCount += guid[index] ? 1 : 0; + } + if (remaining != 4 + guidByteCount + iconLength + nameLength) + { + in.rfinish(); + return false; + } + + // guid[7], guid[4], icon, guid[5], guid[1], guid[0], name, guid[2], + // guid[3], guid[6] -- the strings are not NUL terminated. + uint8 const beforeIcon[] = { 7, 4 }; + uint8 const betweenStrings[] = { 5, 1, 0 }; + uint8 const afterName[] = { 2, 3, 6 }; + for (uint8 index = 0; index < 2; ++index) + { + in.ReadByteSeq(guid[beforeIcon[index]]); + } + // Parsed into locals, not the caller's out-params: two checks remain + // below, and a reader that rejects a body must leave every out-param + // untouched, the way the buy-tab and money readers do. + std::string parsedIcon = in.ReadString(iconLength); + for (uint8 index = 0; index < 3; ++index) + { + in.ReadByteSeq(guid[betweenStrings[index]]); + } + std::string parsedName = in.ReadString(nameLength); + for (uint8 index = 0; index < 3; ++index) + { + in.ReadByteSeq(guid[afterName[index]]); + } + + // Neither string may carry an embedded NUL. The client's writer takes both + // lengths from strlen, so it cannot emit one; and downstream they stop + // being length-delimited -- Guild::SetGuildBankTabInfo stores them in a + // std::string but the reload path hands the column back as a C string, so + // "A\0B" would come back as "A" and memory would disagree with the + // database. It also defeats the handler's own non-empty policy: a name of + // one NUL byte is non-empty to size(), and empty to every downstream + // C-string consumer and to whatever survives a reload. + if (parsedName.find('\0') != std::string::npos || + parsedIcon.find('\0') != std::string::npos) + { + in.rfinish(); + return false; + } + + uint64 raw = 0; + for (uint8 index = 0; index < 8; ++index) + { + // A byte the mask called present cannot decode to zero: the client + // marks a zero byte absent, so 0x01 on the wire (which XORs to 0) is + // a body no stock client produces. Checked per byte rather than by + // scanning the tail, because the two strings sit inside that tail + // and may legitimately contain 0x01. + if (present[index] && guid[index] == 0) + { + in.rfinish(); + return false; + } + raw |= uint64(guid[index]) << (8 * index); + } + if (raw == 0 || in.rpos() != in.size()) + { + in.rfinish(); + return false; + } + + tabId = parsedTab; + name = parsedName; + icon = parsedIcon; + bankGuid = ObjectGuid(raw); + return true; + } + + /// One CMSG_GUILD_BANK_SWAP_ITEMS body, as the 18414 client builds it. + /// + /// Absent fields are left at the sentinels the client's own constructor + /// (sub_686794) uses, so "absent" and "zero" stay distinguishable: srcTab is + /// 0xFF when there is no bank-side source, and the rest are zero. + struct GuildBankSwapItems + { + ObjectGuid bankGuid; // +0x30, packed + uint32 splitAmount; // +0x18, 0 = whole stack + uint32 entryAtBankSlot; // +0x20, 0 = that slot is empty + uint32 srcEntry; // +0x28, entry at (srcTab, srcSlot) + uint32 autoStoreCount; // +0x1c, full stack size + uint8 bankTab; // +0x14 + uint8 bankSlot; // +0x26, 0xFF = anywhere in tab + uint8 toChar; // +0x25, 1 = bank -> player + uint8 playerBag; // +0x10, 0xFF = backpack + uint8 playerSlot; // +0x24 + uint8 srcTab; // +0x12, 0xFF = none + uint8 srcSlot; // +0x13 + bool autoStore; // +0x11 + bool bankToBank; // +0x2c + }; + + /// CMSG_GUILD_BANK_SWAP_ITEMS (0x136A) -- thunk sub_6865DF, vtable 0xD648EC + /// with the 0x00C84A3D signature in slot +12, body writer sub_68A2FD. + /// + /// FOUR different player actions build this one opcode, with different + /// subsets of the fields set, so the server must dispatch on the flags rather + /// than assume a shape. The inherited handler read a raw GUID first and then + /// branched on a plain BankToBank byte; at 18414 neither of those is where it + /// thought, and both flags live in the bit stream. + /// + /// Eleven plain bytes lead, then a 16-bit stream, then the packed GUID and + /// six optional scalars. Six of those sixteen bits are INVERTED presence -- + /// the bit is SET when the field is absent -- because the writer emits + /// `sete` on a comparison against the field's own "none" value. srcTab's + /// none-value is 0xFF; the other five use zero. + /// + /// The dangerous field pair: bankTab/bankSlot is NOT always the source. It is + /// the bank-side slot of the operation, which is the source for a withdrawal + /// but the DESTINATION for a bank-to-bank move, where the source is + /// srcTab/srcSlot. A reference fork naming these "BankTab" and "BankTabDst" + /// has them the other way round for that case. + /// + /// That moves the WRONG item; it does not duplicate one, and an earlier + /// version of this comment said it did. Guild::SwapItems reads its first pair + /// as the source, so with the pairs reversed a move into an empty slot finds + /// nothing there and returns, and a move between two occupied slots still + /// conserves both stacks. What you get is a silent no-op or an item dragged + /// the opposite way -- bad on an item path, but not a dupe. + /// + /// That was settled from the wire, not just the binary: querying the corpus + /// (generation 2BE10C89...88752) for bank-to-bank bodies whose entryAtBankSlot + /// is zero returns twelve packets -- capture-000188 seq 6613 and + /// capture-000192 seq 18440 among them -- in which bankTab/bankSlot names an + /// EMPTY slot while srcTab/srcSlot holds a real item. An empty slot cannot be + /// a source. Ordinary swaps are symmetric and cannot tell the two apart. + inline bool ReadGuildBankSwapItems(WorldPacket& in, GuildBankSwapItems& out) + { + size_t const remaining = in.size() - in.rpos(); + if (remaining < 13) // 11 plain bytes plus 16 mask bits + { + in.rfinish(); + return false; + } + + GuildBankSwapItems parsed; + parsed.srcTab = 0xFF; // the client's own "none" + parsed.srcSlot = 0; + parsed.playerBag = 0; + parsed.playerSlot = 0; + parsed.srcEntry = 0; + parsed.autoStoreCount = 0; + + in >> parsed.splitAmount; // +0x18, written first + in >> parsed.bankSlot; // +0x26 + in >> parsed.toChar; // +0x25 + in >> parsed.entryAtBankSlot; // +0x20 + in >> parsed.bankTab; // +0x14 + + uint8 guid[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; + bool absent[6] = { false, false, false, false, false, false }; + enum { ABS_SRC_ENTRY, ABS_PLAYER_BAG, ABS_PLAYER_SLOT, ABS_SRC_SLOT, ABS_AUTOSTORE, ABS_SRC_TAB }; + + in.ResetBitReader(); + guid[5] = in.ReadBit(); + absent[ABS_SRC_TAB] = in.ReadBit() != 0; + guid[1] = in.ReadBit(); + absent[ABS_PLAYER_BAG] = in.ReadBit() != 0; + parsed.autoStore = in.ReadBit() != 0; + guid[0] = in.ReadBit(); + absent[ABS_SRC_ENTRY] = in.ReadBit() != 0; + absent[ABS_SRC_SLOT] = in.ReadBit() != 0; + guid[2] = in.ReadBit(); + parsed.bankToBank = in.ReadBit() != 0; + guid[4] = in.ReadBit(); + guid[7] = in.ReadBit(); + guid[3] = in.ReadBit(); + absent[ABS_PLAYER_SLOT] = in.ReadBit() != 0; + guid[6] = in.ReadBit(); + absent[ABS_AUTOSTORE] = in.ReadBit() != 0; + + uint8 present[8]; + size_t guidByteCount = 0; + for (uint8 index = 0; index < 8; ++index) + { + present[index] = guid[index]; + guidByteCount += guid[index] ? 1 : 0; + } + + size_t const optionalBytes = + (absent[ABS_SRC_ENTRY] ? 0 : 4) + + (absent[ABS_PLAYER_BAG] ? 0 : 1) + + (absent[ABS_PLAYER_SLOT] ? 0 : 1) + + (absent[ABS_SRC_SLOT] ? 0 : 1) + + (absent[ABS_AUTOSTORE] ? 0 : 4) + + (absent[ABS_SRC_TAB] ? 0 : 1); + + if (remaining != 13 + guidByteCount + optionalBytes) + { + in.rfinish(); + return false; + } + + uint8 const byteOrder[] = { 2, 6, 5, 4, 0, 3, 1, 7 }; + for (uint8 index = 0; index < 8; ++index) + { + in.ReadByteSeq(guid[byteOrder[index]]); + } + + if (!absent[ABS_SRC_ENTRY]) { in >> parsed.srcEntry; } + if (!absent[ABS_PLAYER_BAG]) { in >> parsed.playerBag; } + if (!absent[ABS_PLAYER_SLOT]) { in >> parsed.playerSlot; } + if (!absent[ABS_SRC_SLOT]) { in >> parsed.srcSlot; } + if (!absent[ABS_AUTOSTORE]) { in >> parsed.autoStoreCount; } + if (!absent[ABS_SRC_TAB]) { in >> parsed.srcTab; } + + uint64 raw = 0; + for (uint8 index = 0; index < 8; ++index) + { + if (present[index] && guid[index] == 0) // 0x01 on the wire XORs to 0 + { + in.rfinish(); + return false; + } + raw |= uint64(guid[index]) << (8 * index); + } + if (raw == 0 || in.rpos() != in.size()) + { + in.rfinish(); + return false; + } + + parsed.bankGuid = ObjectGuid(raw); + out = parsed; + return true; + } + /// A plain byte, then one mask byte, then the present bytes of a packed /// eight-byte value. Two unrelated opcodes share this exact shape at 18414 /// and differ only in their orders, so the walk is written once. diff --git a/src/game/Server/Opcodes.cpp b/src/game/Server/Opcodes.cpp index b73ef6d71..fb6326358 100644 --- a/src/game/Server/Opcodes.cpp +++ b/src/game/Server/Opcodes.cpp @@ -1418,6 +1418,45 @@ void InitializeOpcodes() // list, not with the money-changed packet above. DefC(CMSG_GUILD_BANK_BUY_TAB, "CMSG_GUILD_BANK_BUY_TAB", STATUS_LOGGEDIN, PROCESS_THREADUNSAFE, &WorldSession::HandleGuildBankBuyTab); + // Naming a tab. The naming popup is raised by RIGHT-CLICKING a tab button: + // Blizzard_GuildBankUI.lua:584 gates GuildBankPopupFrame:Show() on + // CanEditGuildBankTabInfo(), mouseButton == "RightButton", and the tab not + // being the purchase slot (currentTab ~= GetNumGuildBankTabs() + 1), and that + // is the only place in the UI that shows the frame. Buying a tab + // (StaticPopup.lua:485) calls BuyGuildBankTab() and nothing else. So a bought + // tab stayed permanently unnamed while this was dormant -- but not because the + // purchase raised anything, which is what an earlier version of this comment + // claimed. Its inherited reader was pre-MoP in every field. Derived from writer sub_68B694 + // (thunk sub_686A1D, vtable 0xD64874); which of its two strings is the name is + // fixed by the Lua binding SetGuildBankTabInfo(tab, name, iconFileName), not by + // the writer, which cannot distinguish them. No corpus body exists at 18414. + DefC(CMSG_GUILD_BANK_UPDATE_TAB, "CMSG_GUILD_BANK_UPDATE_TAB", STATUS_LOGGEDIN, PROCESS_THREADUNSAFE, &WorldSession::HandleGuildBankUpdateTab); + + // The reply to a rename, sent to every online member. Its body is derived from + // the client's own inbound parser sub_6A224B rather than from a capture, there + // being none at 18414; sub_96ED66 consumes the parsed record into the tab cache + // and raises event 0x1AF, which is what makes this value's meaning certain + // despite the fork-sourced name. + DefS(SMSG_GUILD_EVENT_BANK_TAB_MODIFIED, "SMSG_GUILD_EVENT_BANK_TAB_MODIFIED"); + + // Moving an item. One opcode carries four different player actions, and at + // 18414 they are four different bodies: 20, 21, 23 and 25 bytes are all + // observed at that build. Derived from writer sub_68A2FD (thunk sub_6865DF, + // vtable 0xD648EC), and checked against decoded corpus bodies of each of the + // four lengths, every one of which the reader consumes exactly. + // + // Two things the inherited reader had wrong are worth naming, because both + // are silent rather than fatal. The BankToBank and AutoStore flags are bits + // in the mask, not plain bytes. And bankTab/bankSlot is the DESTINATION of a + // bank-to-bank move, not its source; twelve captured bodies settle that by + // naming an EMPTY bank slot there while srcTab/srcSlot holds a real item. + // Backwards, that moves the wrong item or silently does nothing at all -- + // it does not duplicate one, which an earlier version of this note claimed. + // + // It answers with the bank list refresh the move functions already send; + // there is no dedicated reply opcode. + DefC(CMSG_GUILD_BANK_SWAP_ITEMS, "CMSG_GUILD_BANK_SWAP_ITEMS", STATUS_LOGGEDIN, PROCESS_THREADUNSAFE, &WorldSession::HandleGuildBankSwapItems); + DefC(CMSG_GUILD_BANKER_ACTIVATE, "CMSG_GUILD_BANKER_ACTIVATE", STATUS_LOGGEDIN, PROCESS_THREADUNSAFE, &WorldSession::HandleGuildBankerActivate); DefC(CMSG_GUILD_BANK_QUERY_TAB, "CMSG_GUILD_BANK_QUERY_TAB", STATUS_LOGGEDIN, PROCESS_THREADUNSAFE, &WorldSession::HandleGuildBankQueryTab); DefS(SMSG_GUILD_BANK_LIST, "SMSG_GUILD_BANK_LIST"); diff --git a/src/game/Server/Opcodes.h b/src/game/Server/Opcodes.h index 52ad1fced..1c2917fb2 100644 --- a/src/game/Server/Opcodes.h +++ b/src/game/Server/Opcodes.h @@ -1394,7 +1394,7 @@ enum OpcodesList SMSG_LF_GUILD_APPLICANT_LIST_UPDATED = 0x0B71, // 5.4.8 18414 (Wow.exe leaf; name fork tables, low confidence) SMSG_GUILD_MOVE_COMPLETE = 0x0BE8, // 5.4.8 18414 (Wow.exe binary) SMSG_GUILD_MEMBERS_FOR_RECIPE = 0x0BF0, // 5.4.8 18414 (Wow.exe leaf; name fork tables, low confidence) - SMSG_GUILD_EVENT_BANK_TAB_MODIFIED = 0x0BF1, // 5.4.8 18414 (Wow.exe leaf; name fork tables, low confidence) + SMSG_GUILD_EVENT_BANK_TAB_MODIFIED = 0x0BF1, // 5.4.8 18414 (Wow.exe binary: sub_68EC4C idx 29 -> lookup_table_68F810[29]=22 -> jump_table_68F708[22]=0x68F667 -> sub_6A39C1 -> parser sub_6A224B -> consumer sub_96ED66 -> event 0x1AF. Routing/body/meaning binary-confirmed 2026-08-23; the LABEL is still the fork tables' -- no such string is in the client. Body and reasoning: MopGuildBankPackets::BuildGuildBankTabModified. Opcodes_reference.h's [low-conf] is the clean-room pass's, not ours to flip; superseded by this) SMSG_GUILD_EVENT_PLAYER_LEFT = 0x0BF8, // 5.4.8 18414 (Wow.exe reader/handler; guild leave/remove UI) SMSG_RESEARCH_COMPLETE = 0x0C0E, // 5.4.8 18414 (Wow.exe leaf; name fork tables) SMSG_MONEY_NOTIFY = 0x0C0F, // 5.4.8 18414 (Wow.exe leaf; name fork tables) diff --git a/src/game/Server/Opcodes_reference.h b/src/game/Server/Opcodes_reference.h index 16b0a8f37..8be3b3f33 100644 --- a/src/game/Server/Opcodes_reference.h +++ b/src/game/Server/Opcodes_reference.h @@ -175,9 +175,9 @@ * TOTAL SMSG rows 925 * * SUBSYSTEM CONFIDENCE: high=365, low=221, medium=182, none=157 - * STATUS TOTALS (excludes 3 shared MSG aliases): ACTIVE=662, DOC=420, DORMANT=435 - * SMSG: ACTIVE=364, DOC=270, DORMANT=290 - * CMSG: ACTIVE=298, DOC=150, DORMANT=145 + * STATUS TOTALS (excludes 3 shared MSG aliases): ACTIVE=669, DOC=420, DORMANT=428 + * SMSG: ACTIVE=366, DOC=270, DORMANT=288 + * CMSG: ACTIVE=303, DOC=150, DORMANT=140 */ // CAVEATS -- read before trusting any single row: @@ -568,7 +568,7 @@ typedef uint16_t uint16; * SMSG_GUILD_MEMBER_UPDATE_NOTE 0x0BE1 DOC [low-conf] * SMSG_UNKNOWN_0x0BE9 0x0BE9 DOC [low-conf] * SMSG_GUILD_MEMBERS_FOR_RECIPE 0x0BF0 DORMANT [low-conf] - * SMSG_GUILD_EVENT_BANK_TAB_MODIFIED 0x0BF1 DORMANT [low-conf] + * SMSG_GUILD_EVENT_BANK_TAB_MODIFIED 0x0BF1 ACTIVE [low-conf] * SMSG_GUILD_EVENT_PLAYER_LEFT 0x0BF8 ACTIVE [low-conf] * SMSG_UNKNOWN_0x0E69 0x0E69 ACTIVE [low-conf] server-binding=SMSG_GUILD_EVENT_NEW_LEADER * SMSG_GUILD_RENAMED 0x0E70 DORMANT [low-conf] @@ -1549,7 +1549,7 @@ typedef uint16_t uint16; * CMSG_QUESTLOG_REMOVE_QUEST 0x0779 ACTIVE * CMSG_GET_MAIL_LIST 0x077A ACTIVE * CMSG_MAIL_QUERY_NEXT_TIME 0x077B ACTIVE - * CMSG_GUILD_BANK_UPDATE_TAB 0x07C2 DORMANT + * CMSG_GUILD_BANK_UPDATE_TAB 0x07C2 ACTIVE * CMSG_QUESTGIVER_CHOOSE_REWARD 0x07CB ACTIVE * CMSG_PET_ABANDON 0x07D0 DORMANT * CMSG_TEXT_EMOTE 0x07E9 ACTIVE @@ -1750,7 +1750,7 @@ typedef uint16_t uint16; * CMSG_SELL_ITEM 0x1358 ACTIVE * CMSG_REQUEST_PET_INFO 0x135B ACTIVE * CMSG_COMPLETE_MOVIE 0x1362 DORMANT - * CMSG_GUILD_BANK_SWAP_ITEMS 0x136A DORMANT + * CMSG_GUILD_BANK_SWAP_ITEMS 0x136A ACTIVE * CMSG_PETITION_SHOW_SIGNATURES 0x136B ACTIVE * CMSG_QUEST_PUSH_RESULT 0x1370 ACTIVE * CMSG_MAIL_TAKE_ITEM 0x1371 ACTIVE diff --git a/src/game/Server/WorldSession.cpp b/src/game/Server/WorldSession.cpp index 41e6272e6..decf51c49 100644 --- a/src/game/Server/WorldSession.cpp +++ b/src/game/Server/WorldSession.cpp @@ -673,6 +673,9 @@ static bool IsEnterWorldConverted(uint16 opcode) // all 143 corpus bodies are that size, value is the new bank total case SMSG_GUILD_BANK_LIST: // MopGuildBankPackets::BuildListBody, byte-exact vs capture-000601 seq 1289646 // (723 bytes, fifteen present items) + case SMSG_GUILD_EVENT_BANK_TAB_MODIFIED: // MopGuildBankPackets::BuildGuildBankTabModified; derived from the + // client's INBOUND parser sub_6A224B, there being no corpus body + // for it -- 9-bit icon length, 7-bit name length, name, tabId, icon case SMSG_BINDER_CONFIRM: // MopBindPackets::BuildBinderConfirm case SMSG_PLAYERBOUND: // MopBindPackets::BuildPlayerBound case SMSG_LFG_PROPOSAL_UPDATE: // MopLfgPackets::BuildProposalUpdate, byte-exact vs capture-000044 seq 1948 and capture-000059 seq 2063424 diff --git a/src/game/Server/tests/mop_compact_packets_test.cpp b/src/game/Server/tests/mop_compact_packets_test.cpp index 087ca536b..c46ba2e58 100644 --- a/src/game/Server/tests/mop_compact_packets_test.cpp +++ b/src/game/Server/tests/mop_compact_packets_test.cpp @@ -33,6 +33,7 @@ #include "InstanceData.h" #include "Opcodes.h" #include "WorldPacket.h" +#include "MopGuildBankPackets.h" #include #include @@ -1076,6 +1077,404 @@ static void test_guild_bank_buy_tab_round_trip() } } +// CMSG_GUILD_BANK_UPDATE_TAB has no capture at build 18414, so these bodies were +// encoded from the client's writer sub_68B694, not produced by the reader they +// test. What they lock is the layout against transposition and regression; only +// the writer can say the layout is right in the first place. +// +// The four presence patterns are chosen so that all eight GUID slots differ from +// one another somewhere in the set, which makes every one of the 28 maskOrder +// transpositions change at least one body -- verified by enumeration, 28/28, not +// assumed. (The buy-tab fixture above began as a single vector, and a single +// vector leaves 16 of the 28 undetectable, because swapping two slots that are +// both present produces an identical body.) Vector 4 has all eight bytes present +// so every slot's position in the byte order is exercised with a distinct value. +static void test_guild_bank_update_tab_round_trip() +{ + struct Vector + { + std::vector body; + uint8 tab; + uint64 guid; + char const* name; + char const* icon; + }; + + std::vector const vectors = { + { { 0x03, 0x03, 0xFA, 0x04, 0x54, 0x49, 0x4E, 0x56, 0x5F, 0x4D, 0x69, 0x73, + 0x63, 0x5F, 0x42, 0x61, 0x67, 0x5F, 0x30, 0x38, 0x23, 0x10, 0x42, 0x61, + 0x6E, 0x6B, 0x32 }, + 3, UINT64_C(0x0000005500332211), "Bank", "INV_Misc_Bag_08" }, + + { { 0x01, 0x84, 0x22, 0x85, 0x49, 0x4E, 0x56, 0x5F, 0x4D, 0x69, 0x73, 0x63, + 0x5F, 0x48, 0x65, 0x72, 0x62, 0x5F, 0x30, 0x31, 0x67, 0x23, 0x10, 0x48, + 0x65, 0x72, 0x62, 0x73, 0x45 }, + 1, UINT64_C(0x0000660044002211), "Herbs", "INV_Misc_Herb_01" }, + + { { 0x00, 0x03, 0x0B, 0x83, 0x49, 0x4E, 0x56, 0x5F, 0x49, 0x6E, 0x67, 0x6F, + 0x74, 0x5F, 0x30, 0x33, 0x10, 0x4F, 0x72, 0x65, 0x32, 0x45, 0x76 }, + 0, UINT64_C(0x0077000044330011), "Ore", "INV_Ingot_03" }, + + { { 0x05, 0x84, 0xBF, 0x85, 0x89, 0x54, 0x49, 0x4E, 0x56, 0x5F, 0x46, 0x61, + 0x62, 0x72, 0x69, 0x63, 0x5F, 0x53, 0x69, 0x6C, 0x6B, 0x5F, 0x30, 0x31, + 0x67, 0x23, 0x10, 0x43, 0x6C, 0x6F, 0x74, 0x68, 0x32, 0x45, 0x76 }, + 5, UINT64_C(0x8877665544332211), "Cloth", "INV_Fabric_Silk_01" }, + }; + + for (Vector const& vector : vectors) + { + WorldPacket packet = InputPacket(CMSG_GUILD_BANK_UPDATE_TAB, vector.body); + uint8 tabId = 0xFF; + std::string name; + std::string icon; + ObjectGuid guid(UINT64_C(0xFFFFFFFFFFFFFFFF)); + CHECK(MopCompactPackets::ReadGuildBankUpdateTab(packet, tabId, name, icon, guid)); + CHECK(tabId == vector.tab); + CHECK(guid.GetRawValue() == vector.guid); + CHECK(name == vector.name); + CHECK(icon == vector.icon); + CHECK(packet.rpos() == packet.size()); + } + + std::vector const& body = vectors[3].body; + std::vector> malformed; + for (size_t size = 0; size < body.size(); ++size) + { + malformed.emplace_back(body.begin(), body.begin() + size); + } + std::vector trailing = body; + trailing.push_back(0x00); + malformed.push_back(trailing); + + // A byte the mask called present but which XORs to zero. + std::vector zeroed = body; + zeroed[5] = 0x01; + malformed.push_back(zeroed); + + // An embedded NUL in either string. The client's writer takes both lengths + // from strlen so it cannot produce these, and downstream they stop being + // length-delimited, so the reader must refuse them. + std::vector nulInName = vectors[3].body; + nulInName[nulInName.size() - 5] = 0x00; // inside "Cloth" + malformed.push_back(nulInName); + std::vector nulInIcon = vectors[3].body; + nulInIcon[10] = 0x00; // inside the icon path + malformed.push_back(nulInIcon); + + // Every mask bit clear, so both lengths are zero too. This one gets PAST the + // size check -- 4 == 4 + 0 + 0 + 0 -- and is refused by the all-zero GUID + // test at the end, which is the only thing standing between a body like this + // and a handler holding ObjectGuid(0). + malformed.push_back({ 0x05, 0x00, 0x00, 0x00 }); + + for (std::vector const& bad : malformed) + { + WorldPacket rejected = InputPacket(CMSG_GUILD_BANK_UPDATE_TAB, bad); + uint8 rejectedTab = 0xFF; + std::string rejectedName = "untouched"; + std::string rejectedIcon = "untouched"; + ObjectGuid rejectedGuid(UINT64_C(0xFFFFFFFFFFFFFFFF)); + CHECK(!MopCompactPackets::ReadGuildBankUpdateTab( + rejected, rejectedTab, rejectedName, rejectedIcon, rejectedGuid)); + CHECK(rejected.rpos() == rejected.size()); + CHECK(rejectedGuid.GetRawValue() == UINT64_C(0xFFFFFFFFFFFFFFFF)); + + // A refused body must leave every out-param alone. Some of these are + // rejected only after the strings have been parsed, so this is a real + // constraint on the reader, not a restatement of the line above. + CHECK(rejectedTab == 0xFF); + CHECK(rejectedName == "untouched"); + CHECK(rejectedIcon == "untouched"); + } +} + +// The reader's own 64/256-byte limits, exercised AT the boundary. +// +// The vectors above are real shapes but short ones -- five name bytes and +// eighteen icon bytes at most -- and the at-limit check further down tests +// BuildGuildBankTabModified, which is the reply BUILDER. So nothing above would +// notice the reader's limits drifting back to 63/255. That is not a hypothetical +// regression: this reader shipped with exactly that skew, which silently refused +// a legitimate 64-byte rename, and the fix is one edit away from being undone. +static std::vector EncodeGuildBankUpdateTab(uint8 tabId, uint64 guid, + std::string const& name, std::string const& icon) +{ + uint8 g[8]; + for (size_t i = 0; i < 8; ++i) + { + g[i] = uint8((guid >> (8 * i)) & 0xFF); + } + + ByteBuffer out; + out << uint8(tabId); + + out.WriteBit(g[5] != 0); + out.WriteBits(uint32(icon.size() >> 1), 8); // 9-bit length, high 8 + out.WriteBit(uint32(icon.size() & 1)); // then its low bit + static uint8 const maskTail[] = { 1, 4, 2, 7, 0, 6, 3 }; + for (size_t i = 0; i < sizeof(maskTail); ++i) + { + out.WriteBit(g[maskTail[i]] != 0); + } + out.WriteBits(uint32(name.size()), 7); + out.FlushBits(); // 24 bits: adds nothing + + static uint8 const byteOrder[] = { 7, 4, 0xFF, 5, 1, 0, 0xFE, 2, 3, 6 }; + for (size_t i = 0; i < sizeof(byteOrder); ++i) + { + if (byteOrder[i] == 0xFF) // the icon sits here + { + if (!icon.empty()) { out.append(icon.data(), icon.size()); } + } + else if (byteOrder[i] == 0xFE) // and the name here + { + if (!name.empty()) { out.append(name.data(), name.size()); } + } + else if (g[byteOrder[i]]) + { + out << uint8(g[byteOrder[i]] ^ 1); // WriteGuidBytes obfuscation + } + } + + return std::vector(out.contents(), out.contents() + out.size()); +} + +static void test_guild_bank_update_tab_length_boundaries() +{ + // Self-check first, so the boundary cases below are not merely this encoder + // agreeing with itself: reproduce a captured-shape vector byte for byte. + std::vector const known = { + 0x03, 0x03, 0xFA, 0x04, 0x54, 0x49, 0x4E, 0x56, 0x5F, 0x4D, 0x69, 0x73, + 0x63, 0x5F, 0x42, 0x61, 0x67, 0x5F, 0x30, 0x38, 0x23, 0x10, 0x42, 0x61, + 0x6E, 0x6B, 0x32 }; + CHECK(EncodeGuildBankUpdateTab(3, UINT64_C(0x0000005500332211), "Bank", + "INV_Misc_Bag_08") == known); + + uint64 const guid = UINT64_C(0x0000660044002211); + std::string const nameAtLimit(64, 'n'); + std::string const iconAtLimit(256, 'i'); + + // 64 and 256 are the client's own copy limits, so both must be ACCEPTED. + WorldPacket accepted = InputPacket(CMSG_GUILD_BANK_UPDATE_TAB, + EncodeGuildBankUpdateTab(5, guid, nameAtLimit, iconAtLimit)); + uint8 tab = 0xFF; + std::string name = "untouched"; + std::string icon = "untouched"; + ObjectGuid parsedGuid(UINT64_C(0xFFFFFFFFFFFFFFFF)); + CHECK(MopCompactPackets::ReadGuildBankUpdateTab(accepted, tab, name, icon, parsedGuid)); + CHECK(tab == 5); + CHECK(name == nameAtLimit); + CHECK(icon == iconAtLimit); + CHECK(parsedGuid.GetRawValue() == guid); + CHECK(accepted.rpos() == accepted.size()); + + // One byte past either limit must be refused, and refused without touching a + // single out-param. The icon limb needs 257 bytes, which is also the only + // case here that spends the ninth length bit. + std::vector> const overLimit = { + EncodeGuildBankUpdateTab(5, guid, std::string(65, 'n'), "Icon"), + EncodeGuildBankUpdateTab(5, guid, "Bank", std::string(257, 'i')), + }; + + for (size_t i = 0; i < overLimit.size(); ++i) + { + WorldPacket rejected = InputPacket(CMSG_GUILD_BANK_UPDATE_TAB, overLimit[i]); + uint8 rejectedTab = 0xFF; + std::string rejectedName = "untouched"; + std::string rejectedIcon = "untouched"; + ObjectGuid rejectedGuid(UINT64_C(0xFFFFFFFFFFFFFFFF)); + CHECK(!MopCompactPackets::ReadGuildBankUpdateTab( + rejected, rejectedTab, rejectedName, rejectedIcon, rejectedGuid)); + CHECK(rejected.rpos() == rejected.size()); + CHECK(rejectedTab == 0xFF); + CHECK(rejectedName == "untouched"); + CHECK(rejectedIcon == "untouched"); + CHECK(rejectedGuid.GetRawValue() == UINT64_C(0xFFFFFFFFFFFFFFFF)); + } +} + +static void test_guild_bank_tab_modified_body() +{ + ByteBuffer body; + CHECK(MopGuildBankPackets::BuildGuildBankTabModified(body, 3, "Bank", "INV_Misc_Bag_08")); + + // icon length 15 in 9 bits = 0 0000 1111, name length 4 in 7 bits = 000 0100. + // MSB-first that is 00000111 10000100 -> 0x07 0x84, and 9 + 7 is a whole + // number of bytes so the flush contributes nothing. + std::vector expected = { 0x07, 0x84 }; + for (char c : std::string("Bank")) { expected.push_back(uint8_t(c)); } + expected.push_back(0x03); expected.push_back(0x00); + expected.push_back(0x00); expected.push_back(0x00); + for (char c : std::string("INV_Misc_Bag_08")) { expected.push_back(uint8_t(c)); } + + CHECK(body.size() == expected.size()); + for (size_t index = 0; index < expected.size(); ++index) + { + CHECK(body.contents()[index] == expected[index]); + } + + // Refused rather than truncated, and the buffer is checked as well as the + // return: "refused" is a claim about what was written, so asserting only the + // false return would leave the interesting half untested. The tab bound is + // load-bearing on the receiving side -- the client's consumer sub_96ED66 does + // NOT range-check the tab before indexing its 8-entry cache at 0x11F4140 with + // stride 0x2148, so this guard is what stops a wild write in every client that + // receives the event. + ByteBuffer rejected; + CHECK(!MopGuildBankPackets::BuildGuildBankTabModified(rejected, 8, "Bank", "Icon")); + CHECK(!MopGuildBankPackets::BuildGuildBankTabModified( + rejected, 0, std::string(MopGuildBankPackets::MAX_TAB_NAME_BYTES + 1, 'x'), "Icon")); + CHECK(!MopGuildBankPackets::BuildGuildBankTabModified( + rejected, 0, "Bank", std::string(MopGuildBankPackets::MAX_TAB_ICON_BYTES + 1, 'x'))); + CHECK(rejected.size() == 0); + + // The client's copy limits exactly, one byte below the refusals above. + ByteBuffer atLimit; + CHECK(MopGuildBankPackets::BuildGuildBankTabModified( + atLimit, 7, std::string(MopGuildBankPackets::MAX_TAB_NAME_BYTES, 'n'), + std::string(MopGuildBankPackets::MAX_TAB_ICON_BYTES, 'i'))); +} + +// CMSG_GUILD_BANK_SWAP_ITEMS, unlike its siblings, has real wire evidence: 125 +// PACKETS at build 18414, spread across many captures, under catalogue generation +// 2BE10C89...88752. Every body below is a genuine retail packet rather than a +// synthetic one, which makes this a stronger check than the update-tab fixture. +// +// It is not an unconditional check on the bit order, though, and an earlier +// version of this comment claimed it was. Exact-size validation only catches a +// mistake that CHANGES the size: permuting the GUID mask bits preserves their +// popcount, and transposing two optional fields of equal width preserves the +// total. What these vectors do pin down is every field's VALUE, which is the +// part that matters here. +// +// The seven cover all four producer shapes and the whole observed size range +// (20-25 bytes), including two bank-to-bank moves into an EMPTY destination. Those +// two are the ones that prove which pair is the source: bankTab/bankSlot names a +// slot holding nothing while srcTab/srcSlot holds a real item, and an empty slot +// cannot be a source. Ordinary swaps are symmetric and cannot distinguish them. +static void test_guild_bank_swap_items_real_captures() +{ + struct Vector + { + std::vector body; + uint64 guid; + uint32 splitAmount; + uint32 entryAtBankSlot; + uint32 srcEntry; + uint32 autoStoreCount; + uint8 bankTab; + uint8 bankSlot; + uint8 toChar; + uint8 playerBag; + uint8 playerSlot; + uint8 srcTab; + uint8 srcSlot; + bool autoStore; + bool bankToBank; + }; + + std::vector const vectors = { + // capture-000067 seq 550753 -- P3 deposit, player -> bank + { { 0x00, 0x00, 0x00, 0x00, 0x61, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xE7, 0x37, 0x12, 0x43, 0x70, 0x11, 0x06, 0xF0, 0x13 }, + UINT64_C(0xF113427100000710), 0, 0, 0, 0, + 2, 97, 0, 19, 0, 255, 0, + false, false }, + + // capture-000033 seq 14266 -- P1 auto-store, bank -> player + { { 0x00, 0x00, 0x00, 0x00, 0x27, 0x01, 0x36, 0x29, 0x01, 0x00, 0x03, 0xFF, 0x36, 0x12, 0x43, 0x70, 0x11, 0x06, 0xF0, 0x05, 0x00, 0x00, 0x00 }, + UINT64_C(0xF113427100000710), 0, 76086, 0, 5, + 3, 39, 1, 0, 0, 255, 0, + true, false }, + + // capture-000059 seq 1695242 -- P2 bank -> player, named bag slot + { { 0x01, 0x00, 0x00, 0x00, 0x08, 0x01, 0x08, 0x56, 0x01, 0x00, 0x02, 0xE7, 0x33, 0x12, 0x43, 0x70, 0x11, 0x06, 0xF0, 0x15, 0x02 }, + UINT64_C(0xF113427100000710), 1, 87560, 0, 0, + 2, 8, 1, 21, 2, 255, 0, + false, false }, + + // capture-000112 seq 465086 -- P4 bank -> bank, source and destination equal + { { 0x03, 0x00, 0x00, 0x00, 0x36, 0x00, 0x89, 0x2B, 0x01, 0x00, 0x01, 0xB4, 0x77, 0x12, 0x43, 0x6F, 0xE1, 0x07, 0xF0, 0x89, 0x2B, 0x01, 0x00, 0x36, 0x01 }, + UINT64_C(0xF113426E000006E0), 3, 76681, 76681, 0, + 1, 54, 0, 0, 0, 1, 54, + false, true }, + + // capture-000067 seq 551446 -- P4 bank -> bank swap of two different items + { { 0x00, 0x00, 0x00, 0x00, 0x53, 0x00, 0x3A, 0x29, 0x01, 0x00, 0x02, 0xB4, 0x77, 0x12, 0x43, 0x70, 0x11, 0x06, 0xF0, 0x3C, 0x29, 0x01, 0x00, 0x50, 0x02 }, + UINT64_C(0xF113427100000710), 0, 76090, 76092, 0, + 2, 83, 0, 0, 0, 2, 80, + false, true }, + + // capture-000188 seq 6613 -- P4 into an EMPTY destination: proves the direction + { { 0x00, 0x00, 0x00, 0x00, 0x5C, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xB4, 0x77, 0x12, 0x43, 0x71, 0xE7, 0x07, 0xF0, 0xE3, 0x23, 0x01, 0x00, 0x03, 0x02 }, + UINT64_C(0xF1134270000006E6), 0, 0, 74723, 0, + 2, 92, 0, 0, 0, 2, 3, + false, true }, + + // capture-000192 seq 18440 -- P4 into an EMPTY destination, second instance + { { 0x00, 0x00, 0x00, 0x00, 0x57, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0xB4, 0x77, 0x12, 0x26, 0x0B, 0x23, 0x05, 0xF0, 0x7C, 0x2B, 0x01, 0x00, 0x06, 0x01 }, + UINT64_C(0xF113270A00000422), 0, 0, 76668, 0, + 1, 87, 0, 0, 0, 1, 6, + false, true }, + }; + + for (Vector const& vector : vectors) + { + WorldPacket packet = InputPacket(CMSG_GUILD_BANK_SWAP_ITEMS, vector.body); + MopCompactPackets::GuildBankSwapItems parsed; + CHECK(MopCompactPackets::ReadGuildBankSwapItems(packet, parsed)); + CHECK(parsed.bankGuid.GetRawValue() == vector.guid); + CHECK(parsed.splitAmount == vector.splitAmount); + CHECK(parsed.entryAtBankSlot == vector.entryAtBankSlot); + CHECK(parsed.srcEntry == vector.srcEntry); + CHECK(parsed.autoStoreCount == vector.autoStoreCount); + CHECK(parsed.bankTab == vector.bankTab); + CHECK(parsed.bankSlot == vector.bankSlot); + CHECK(parsed.toChar == vector.toChar); + CHECK(parsed.playerBag == vector.playerBag); + CHECK(parsed.playerSlot == vector.playerSlot); + CHECK(parsed.srcTab == vector.srcTab); + CHECK(parsed.srcSlot == vector.srcSlot); + CHECK(parsed.autoStore == vector.autoStore); + CHECK(parsed.bankToBank == vector.bankToBank); + CHECK(packet.rpos() == packet.size()); + } + + // The direction claim, asserted directly rather than left implicit in the + // table above: in both empty-destination captures the bank-side pair is empty + // and the source pair is not. + for (size_t index = 5; index <= 6; ++index) + { + CHECK(vectors[index].bankToBank); + CHECK(vectors[index].entryAtBankSlot == 0); + CHECK(vectors[index].srcEntry != 0); + } + + std::vector const& body = vectors[6].body; + std::vector> malformed; + for (size_t size = 0; size < body.size(); ++size) + { + malformed.emplace_back(body.begin(), body.begin() + size); + } + std::vector trailing = body; + trailing.push_back(0x00); + malformed.push_back(trailing); + + // A GUID byte the mask called present but which XORs to zero. + std::vector zeroed = body; + zeroed[13] = 0x01; + malformed.push_back(zeroed); + + for (std::vector const& bad : malformed) + { + WorldPacket rejected = InputPacket(CMSG_GUILD_BANK_SWAP_ITEMS, bad); + MopCompactPackets::GuildBankSwapItems parsed; + parsed.bankGuid = ObjectGuid(UINT64_C(0xFFFFFFFFFFFFFFFF)); + CHECK(!MopCompactPackets::ReadGuildBankSwapItems(rejected, parsed)); + CHECK(rejected.rpos() == rejected.size()); + CHECK(parsed.bankGuid.GetRawValue() == UINT64_C(0xFFFFFFFFFFFFFFFF)); + } +} + static void test_combo_points_packet() { WorldPacket packet; @@ -2334,6 +2733,10 @@ int main(int /*argc*/, char** /*argv*/) test_pre_resurrect_packet(); test_guild_bank_deposit_money_matches_capture(); test_guild_bank_buy_tab_round_trip(); + test_guild_bank_update_tab_round_trip(); + test_guild_bank_update_tab_length_boundaries(); + test_guild_bank_tab_modified_body(); + test_guild_bank_swap_items_real_captures(); test_combo_points_packet(); test_instance_reset_result_bodies(); diff --git a/src/game/WorldHandlers/GuildHandler.cpp b/src/game/WorldHandlers/GuildHandler.cpp index 8359a324b..610a689b8 100644 --- a/src/game/WorldHandlers/GuildHandler.cpp +++ b/src/game/WorldHandlers/GuildHandler.cpp @@ -1600,128 +1600,117 @@ void WorldSession::HandleGuildBankSwapItems(WorldPacket& recv_data) { DEBUG_LOG("WORLD: Received (CMSG_GUILD_BANK_SWAP_ITEMS)"); - // Closed ahead of this opcode being registered, because the consequence is - // permanent. A tab purchase whose commit could not be confirmed can leave a - // tab in memory with no guild_bank_tab row; storing an item into it writes a - // guild_bank_item row with a TabId that LoadGuildBankFromDB then drops, and - // the item is gone for good. Refuse while the bank state is untrusted. - if (Guild* untrustedGuild = sGuildMgr.GetGuildById(GetPlayer()->GetGuildId())) + // FOUR player actions build this one opcode, and at 18414 they are four + // different bodies -- 20, 21, 23 and 25 bytes are all observed at that build. + // The inherited code read a raw GUID first and then branched on a plain + // BankToBank byte; neither is where it thought, and both flags now live in + // the bit stream, so the shape has to come out of the reader. + MopCompactPackets::GuildBankSwapItems req; + if (!MopCompactPackets::ReadGuildBankSwapItems(recv_data, req)) { - if (!untrustedGuild->IsBankStateTrusted()) - { - sLog.outError("CMSG_GUILD_BANK_SWAP_ITEMS: refused for player %u -- guild %u " - "bank state is untrusted after an unrecoverable commit; reload the guild", - GetPlayer()->GetGUIDLow(), GetPlayer()->GetGuildId()); - recv_data.rfinish(); - return; - } + DEBUG_LOG("WORLD: Rejected malformed CMSG_GUILD_BANK_SWAP_ITEMS"); + return; } - ObjectGuid goGuid; - uint8 BankToBank; - - uint8 BankTab, BankTabSlot, AutoStore; - uint8 PlayerSlot = NULL_SLOT; - uint8 PlayerBag = NULL_BAG; - uint8 BankTabDst, BankTabSlotDst, unk2; - uint8 ToChar = 1; - uint32 ItemEntry, unk1; - uint32 AutoStoreCount = 0; - uint32 SplitedAmount = 0; - - recv_data >> goGuid >> BankToBank; - uint32 GuildId = GetPlayer()->GetGuildId(); if (!GuildId) { - recv_data.rfinish(); // prevent additional spam at rejected packet return; } Guild* pGuild = sGuildMgr.GetGuildById(GuildId); if (!pGuild) { - recv_data.rfinish(); // prevent additional spam at rejected packet return; } - if (BankToBank) + // Closed ahead of this opcode being registered, because the consequence is + // permanent. A tab purchase whose commit could not be confirmed can leave a + // tab in memory with no guild_bank_tab row; storing an item into it writes a + // guild_bank_item row with a TabId that LoadGuildBankFromDB then drops, and + // the item is gone for good. Refuse while the bank state is untrusted. + if (!pGuild->IsBankStateTrusted()) { - recv_data >> BankTabDst; - recv_data >> BankTabSlotDst; - recv_data >> unk1; // always 0 - recv_data >> BankTab; - recv_data >> BankTabSlot; - recv_data >> ItemEntry; - recv_data >> unk2; // always 0 - recv_data >> SplitedAmount; - - if (BankTabSlotDst >= GUILD_BANK_MAX_SLOTS || - (BankTabDst == BankTab && BankTabSlotDst == BankTabSlot) || - BankTab >= pGuild->GetPurchasedTabs() || - BankTabDst >= pGuild->GetPurchasedTabs()) - { - recv_data.rfinish(); // prevent additional spam at rejected packet - return; - } + sLog.outError("CMSG_GUILD_BANK_SWAP_ITEMS: refused for player %u -- guild %u " + "bank state is untrusted after an unrecoverable commit; reload the guild", + GetPlayer()->GetGUIDLow(), GuildId); + return; } - else + + if (!GetPlayer()->GetGameObjectIfCanInteractWith(req.bankGuid, GAMEOBJECT_TYPE_GUILD_BANK)) { - recv_data >> BankTab; - recv_data >> BankTabSlot; - recv_data >> ItemEntry; - recv_data >> AutoStore; - if (AutoStore) - { - recv_data >> AutoStoreCount; - recv_data.read_skip(); // ToChar (?), always and expected to be 1 (autostore only triggered in guild->ToChar) - recv_data.read_skip(); // unknown, always 0 - } - else - { - recv_data >> PlayerBag; - recv_data >> PlayerSlot; - recv_data >> ToChar; - recv_data >> SplitedAmount; - } + return; + } - if ((BankTabSlot >= GUILD_BANK_MAX_SLOTS && BankTabSlot != 0xFF) || - BankTab >= pGuild->GetPurchasedTabs()) + // Bank <-> Bank. bankTab/bankSlot is the DESTINATION on this path and + // srcTab/srcSlot the source -- the opposite of what the field names in a + // reference fork suggest. ReadGuildBankSwapItems carries the evidence. + // + // srcTab defaults to the client's own "none" value of 0xFF when the field is + // absent, so a bank-to-bank body that names no source fails the tab bound + // below rather than silently moving out of tab 0. + if (req.bankToBank) + { + if (req.srcTab >= pGuild->GetPurchasedTabs() || + req.bankTab >= pGuild->GetPurchasedTabs() || + req.srcSlot >= GUILD_BANK_MAX_SLOTS || + req.bankSlot >= GUILD_BANK_MAX_SLOTS || + (req.srcTab == req.bankTab && req.srcSlot == req.bankSlot)) { - recv_data.rfinish(); // prevent additional spam at rejected packet return; } - } - if (!GetPlayer()->GetGameObjectIfCanInteractWith(goGuid, GAMEOBJECT_TYPE_GUILD_BANK)) - { + pGuild->SwapItems(_player, req.srcTab, req.srcSlot, req.bankTab, req.bankSlot, req.splitAmount); return; } - // Bank <-> Bank - if (BankToBank) + // Player <-> Bank. 0xFF stays legal for the bank slot here: it is the + // client's "anywhere in this tab" for a deposit. + if (req.bankTab >= pGuild->GetPurchasedTabs() || + (req.bankSlot >= GUILD_BANK_MAX_SLOTS && req.bankSlot != 0xFF)) { - pGuild->SwapItems(_player, BankTab, BankTabSlot, BankTabDst, BankTabSlotDst, SplitedAmount); return; } - // Player <-> Bank + // The auto-store body omits both player-side fields and the reader leaves + // them zero. NULL_BAG is itself 0 so the bag needs no translation, but slot 0 + // is a real slot while NULL_SLOT is 255. + // + // Without this line an auto-store is REFUSED, not misdirected: (NULL_BAG, 0) + // is not an inventory position -- IsInventoryPos accepts bag 0 only with + // NULL_SLOT or a backpack slot index -- so it fails the guard below and the + // player is told the move is impossible. An earlier version of this comment + // said the item would land in the first backpack slot instead; it would not. + // The translation is still required, or auto-store never works at all. + // + // Key this on autoStore and never on the fields being absent. That absence + // bit is INVERTED presence: the client omits playerSlot whenever it is zero, + // so an ordinary deposit out of bag 19 slot 0 -- capture-000067 sequence + // 550753 is one, at 20 bytes -- arrives with playerSlot absent and genuinely + // meaning slot 0. Translating on absence would silently redirect it. + uint8 playerBag = req.playerBag; + uint8 playerSlot = req.playerSlot; + if (req.autoStore) + { + playerBag = NULL_BAG; + playerSlot = NULL_SLOT; + } // allow work with inventory only - if (!Player::IsInventoryPos(PlayerBag, PlayerSlot) && !(PlayerBag == NULL_BAG && PlayerSlot == NULL_SLOT)) + if (!Player::IsInventoryPos(playerBag, playerSlot) && + !(playerBag == NULL_BAG && playerSlot == NULL_SLOT)) { _player->SendEquipError(EQUIP_ERR_NONE, NULL, NULL); return; } - // BankToChar swap or char to bank remaining - if (ToChar) // Bank -> Char cases + if (req.toChar) // Bank -> Char cases { - pGuild->MoveFromBankToChar(_player, BankTab, BankTabSlot, PlayerBag, PlayerSlot, SplitedAmount); + pGuild->MoveFromBankToChar(_player, req.bankTab, req.bankSlot, playerBag, playerSlot, req.splitAmount); } else // Char -> Bank cases { - pGuild->MoveFromCharToBank(_player, PlayerBag, PlayerSlot, BankTab, BankTabSlot, SplitedAmount); + pGuild->MoveFromCharToBank(_player, playerBag, playerSlot, req.bankTab, req.bankSlot, req.splitAmount); } } @@ -1867,15 +1856,21 @@ void WorldSession::HandleGuildBankUpdateTab(WorldPacket& recv_data) { DEBUG_LOG("WORLD: Received (CMSG_GUILD_BANK_UPDATE_TAB)"); + // The inherited reader took a raw eight-byte GUID, then the tab, then two + // NUL-terminated strings. At 18414 the tab leads as a plain byte, the GUID + // is packed, both lengths live in the bit stream, and the strings are NOT + // NUL terminated -- so every field it read was wrong. See + // MopCompactPackets::ReadGuildBankUpdateTab for the derivation. ObjectGuid goGuid; - uint8 TabId; + uint8 TabId = 0; std::string Name; std::string IconIndex; - recv_data >> goGuid; - recv_data >> TabId; - recv_data >> Name; - recv_data >> IconIndex; + if (!MopCompactPackets::ReadGuildBankUpdateTab(recv_data, TabId, Name, IconIndex, goGuid)) + { + DEBUG_LOG("WORLD: CMSG_GUILD_BANK_UPDATE_TAB body rejected"); + return; + } if (Name.empty()) { @@ -1887,6 +1882,32 @@ void WorldSession::HandleGuildBankUpdateTab(WorldPacket& recv_data) return; } + // `guild_bank_tab`.`TabName` and `.TabIcon` are both varchar(100), in the + // database repository at Character/Setup/characterLoadDB.sql:101223. Refuse + // rather than hand the column something it would silently truncate: a + // truncated icon path renders as a missing texture, and whether anything is + // logged depends on SQL mode -- strict mode rejects the UPDATE and + // DatabaseMysql reports it, non-strict truncates silently. + // + // These are not equal units. The table is CHARSET=utf8, so varchar(100) holds + // 100 CHARACTERS -- up to 300 bytes -- while the comparison below is on bytes. + // The guard is therefore deliberately conservative rather than exact, which + // costs little because neither bound is reachable through the stock UI: it + // sanitises a name to 15 characters, and its icon picker sends a bare macro + // filename of about thirty bytes. The Lua binding itself is less restrained -- + // SetGuildBankTabInfo accepts any non-empty icon string and its copy loop + // permits 256 bytes -- so the icon limb IS reachable by a modified client, + // which is the case this guard exists for. Only that limb can fire at all, + // the reader already capping the name at 64; the name limb is kept so the + // guard still holds if that cap is ever widened. Do not "simplify" the + // sanitiser's 16 to a byte count -- 16 characters is not 16 bytes. + if (Name.size() > 100 || IconIndex.size() > 100) + { + DEBUG_LOG("WORLD: CMSG_GUILD_BANK_UPDATE_TAB refused, name %zu / icon %zu bytes exceeds storage", + Name.size(), IconIndex.size()); + return; + } + if (!GetPlayer()->GetGameObjectIfCanInteractWith(goGuid, GAMEOBJECT_TYPE_GUILD_BANK)) { return; @@ -1915,7 +1936,61 @@ void WorldSession::HandleGuildBankUpdateTab(WorldPacket& recv_data) } pGuild->SetGuildBankTabInfo(TabId, Name, IconIndex); - pGuild->DisplayGuildBankTabsInfo(this, TabId); + + // Every online member is told, not just the actor. A tab name is shared + // state, and a rename only its author can see leaves everyone else reading a + // stale label until they reopen the bank. + // + // 0x0BF1 carries this. Two earlier versions of this comment claimed nothing + // confirmed that, reasoning from the absence of any capture at 18414 and from + // the fork-sourced name in Opcodes.h. Both were wrong, because neither asked + // the client. Its INBOUND parser settles it: sub_6A224B reads exactly the + // shape BuildGuildBankTabModified writes, and sub_96ED66 takes the result, + // sanitises the name through the same 16-character limiter the outbound path + // uses, stores both strings in the tab cache at 0x11F4140, and raises event + // 0x1AF. The step that binds the VALUE to that parser -- the guild SMSG + // dispatcher sub_68EC4C and its two tables -- is spelled out on the opcode's + // own line in Opcodes.h, because it is the part a re-derivation cannot guess: + // sub_6A224B holds no opcode literal, and the only push 0xbf1 in the image is + // an FMOD line number. (Not Opcodes_reference.h: its rows are generated and + // carry only the status overlay.) Corpus silence is not evidence of absence + // when the binary itself can be asked. + ByteBuffer tabModified; + if (MopGuildBankPackets::BuildGuildBankTabModified(tabModified, TabId, Name, IconIndex)) + { + WorldPacket modified(SMSG_GUILD_EVENT_BANK_TAB_MODIFIED, tabModified.size()); + modified.append(tabModified.contents(), tabModified.size()); + pGuild->BroadcastPacket(&modified); + } + else + { + // Unreachable behind the guards above. By this point the rename is applied + // in memory and its UPDATE has been QUEUED -- not committed: once the world + // has loaded, AllowAsyncTransactions is on and Database::Execute only hands + // the statement to the delay thread, whose result nobody checks. So what a + // silent skip here would cost is the notification, not the rename: every + // member, the actor included, would hold the old name until they reopened + // the bank, with nothing in the log to explain it. + sLog.outError("CMSG_GUILD_BANK_UPDATE_TAB: guild %u tab %u renamed but the " + "event body was refused; members will hold a stale name until they reopen", + GuildId, uint32(TabId)); + } + + // No bank list follows. The inherited handler sent one, and an earlier + // version of this comment justified keeping it by claiming the event only + // refreshed cached metadata while the list did the repainting. That is + // backwards. Event 0x1AF is GUILDBANK_UPDATE_TABS, and its handler at + // Blizzard_GuildBankUI.lua:125 calls GuildBankFrame_SelectAvailableTab(), + // which calls GuildBankFrame_UpdateTabs() and GuildBankFrame_Update() on + // every path -- so the event repaints the frame by itself, from the tab + // cache sub_96ED66 has just written. BroadcastPacket includes the actor, so + // the renamer is repainted by the same packet as everyone else. + // + // A bank list additionally ships every slot of the tab, which a rename has + // not changed. There is no capture of this exchange at 18414 to say what + // retail sends, so the choice is between a send that is provably sufficient + // and one that is merely inherited; keeping the extra list would be assuming + // evidence rather than having it. } void WorldSession::HandleGuildBankLogQuery(WorldPacket& recv_data)