From e77ff9a0215d960122ef3cf45390499994dcab0f Mon Sep 17 00:00:00 2001 From: MadMax Date: Sat, 22 Aug 2026 23:22:13 +0100 Subject: [PATCH 1/2] Guild bank: name a tab, and tell the whole guild it changed CMSG_GUILD_BANK_UPDATE_TAB was dormant, so a tab could be bought and then never named. The inherited handler read a raw GUID, then the tab, then two NUL-terminated strings; at 18414 the tab leads as a plain byte, the GUID is packed, both string lengths live in the bit stream, and neither string is terminated. The reader comes from the client's writer sub_68B694, and which of its two strings is the name comes from the Lua binding SetGuildBankTabInfo(tab, name, iconFileName), the writer being unable to distinguish them. The reply is SMSG_GUILD_EVENT_BANK_TAB_MODIFIED (0x0BF1), broadcast to the guild, and no bank list follows it. An earlier version of this change left that opcode dormant and answered the actor with a list, claiming nothing confirmed the value carried the message. That was wrong, and wrong because it never asked the client. The value routes through the guild SMSG dispatcher sub_68EC4C to parser sub_6A224B, whose body is a 9-bit icon length, a 7-bit name length, the name, a uint32 tab id and the icon; sub_96ED66 then stores both strings in the tab cache and raises event 0x1AF, which is GUILDBANK_UPDATE_TABS. Its Lua handler repaints the frame on every path, so the list the old code sent was redundant as well as unevidenced. The binding from value to parser is recorded in Opcodes.h, because it is the step a re-derivation cannot guess: sub_6A224B holds no opcode literal, and the only push 0xbf1 in the image is an FMOD line number. Co-Authored-By: Claude Opus 5 (1M context) --- src/game/Object/GuildBank.cpp | 13 + src/game/Object/MopGuildBankPackets.h | 68 ++++- src/game/Object/Unit.h | 157 +++++++++++ src/game/Server/Opcodes.cpp | 21 ++ src/game/Server/Opcodes.h | 2 +- src/game/Server/Opcodes_reference.h | 10 +- src/game/Server/WorldSession.cpp | 3 + .../Server/tests/mop_compact_packets_test.cpp | 262 ++++++++++++++++++ src/game/WorldHandlers/GuildHandler.cpp | 98 ++++++- 9 files changed, 621 insertions(+), 13 deletions(-) diff --git a/src/game/Object/GuildBank.cpp b/src/game/Object/GuildBank.cpp index c36b17b8b..4b05cd77c 100644 --- a/src/game/Object/GuildBank.cpp +++ b/src/game/Object/GuildBank.cpp @@ -269,6 +269,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; 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..61fb6dd56 100644 --- a/src/game/Object/Unit.h +++ b/src/game/Object/Unit.h @@ -1180,6 +1180,163 @@ 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; + } + /// 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..8d79fe378 100644 --- a/src/game/Server/Opcodes.cpp +++ b/src/game/Server/Opcodes.cpp @@ -1418,6 +1418,27 @@ 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"); + 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..8a4405155 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=668, DOC=420, DORMANT=429 + * SMSG: ACTIVE=366, DOC=270, DORMANT=288 + * CMSG: ACTIVE=302, DOC=150, DORMANT=141 */ // 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 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..22ff2dfaf 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,264 @@ 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'))); +} + static void test_combo_points_packet() { WorldPacket packet; @@ -2334,6 +2593,9 @@ 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_combo_points_packet(); test_instance_reset_result_bodies(); diff --git a/src/game/WorldHandlers/GuildHandler.cpp b/src/game/WorldHandlers/GuildHandler.cpp index 8359a324b..102cd3d35 100644 --- a/src/game/WorldHandlers/GuildHandler.cpp +++ b/src/game/WorldHandlers/GuildHandler.cpp @@ -1867,15 +1867,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 +1893,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 +1947,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) From 8febf7b8d6d65f36048d608441032df229ad4fbe Mon Sep 17 00:00:00 2001 From: MadMax Date: Sun, 23 Aug 2026 18:13:32 +0100 Subject: [PATCH 2/2] Guild bank: move items, with the destination on the right side Registers CMSG_GUILD_BANK_SWAP_ITEMS (0x136A), previously defined but never registered, and replaces its pre-MoP reader. Four player actions build this one opcode and at 18414 they are four different bodies; BankToBank and AutoStore are bits in the mask rather than plain bytes, so the shape now comes out of the reader. bankTab/bankSlot is the destination of a bank-to-bank move, not its source, which is the inverse of the inherited field names. Co-Authored-By: Claude Opus 5 (1M context) --- src/game/Object/Guild.h | 9 + src/game/Object/GuildBank.cpp | 160 ++++++++++++++++-- src/game/Object/Unit.h | 158 +++++++++++++++++ src/game/Server/Opcodes.cpp | 18 ++ src/game/Server/Opcodes_reference.h | 6 +- .../Server/tests/mop_compact_packets_test.cpp | 143 +++++++++++++++- src/game/WorldHandlers/GuildHandler.cpp | 151 ++++++++--------- 7 files changed, 542 insertions(+), 103 deletions(-) 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 4b05cd77c..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) @@ -1283,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) { @@ -1329,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 { @@ -1346,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 { @@ -1394,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); @@ -1461,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) { @@ -1484,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 { @@ -1560,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); @@ -1632,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); } @@ -1659,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); } @@ -1727,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/Unit.h b/src/game/Object/Unit.h index 61fb6dd56..a47385e3e 100644 --- a/src/game/Object/Unit.h +++ b/src/game/Object/Unit.h @@ -1337,6 +1337,164 @@ namespace MopCompactPackets 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 8d79fe378..fb6326358 100644 --- a/src/game/Server/Opcodes.cpp +++ b/src/game/Server/Opcodes.cpp @@ -1439,6 +1439,24 @@ void InitializeOpcodes() // 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_reference.h b/src/game/Server/Opcodes_reference.h index 8a4405155..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=668, DOC=420, DORMANT=429 + * STATUS TOTALS (excludes 3 shared MSG aliases): ACTIVE=669, DOC=420, DORMANT=428 * SMSG: ACTIVE=366, DOC=270, DORMANT=288 - * CMSG: ACTIVE=302, DOC=150, DORMANT=141 + * CMSG: ACTIVE=303, DOC=150, DORMANT=140 */ // CAVEATS -- read before trusting any single row: @@ -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/tests/mop_compact_packets_test.cpp b/src/game/Server/tests/mop_compact_packets_test.cpp index 22ff2dfaf..c46ba2e58 100644 --- a/src/game/Server/tests/mop_compact_packets_test.cpp +++ b/src/game/Server/tests/mop_compact_packets_test.cpp @@ -1187,6 +1187,7 @@ static void test_guild_bank_update_tab_round_trip() 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 @@ -1292,7 +1293,6 @@ static void test_guild_bank_update_tab_length_boundaries() } } - static void test_guild_bank_tab_modified_body() { ByteBuffer body; @@ -1335,6 +1335,146 @@ static void test_guild_bank_tab_modified_body() 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; @@ -2596,6 +2736,7 @@ int main(int /*argc*/, char** /*argv*/) 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 102cd3d35..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); } }