[8389] Implement check really read received packet size and warning it not all data read. Author: VladimirMangos

* This let more easy catch packet structure chnages at client switch.
    * Fixed structure CMSG_GUILD_BANK_SWAP_ITEMS
    * Fixed structure CMSG_SPLIT_ITEM, CMSG_SELL_ITEM
    * Added read data amount fixes for some other packets.

    Thanks to TOM_RUS in help check correct packets structure.

    Note: not all packets possible fixed. Please report for not fixed cases at errors:
    "opcode %s (0x%.4X) have unprocessed tail data (read stop at %u from %u)"

--HG--
branch : trunk

src/game/CalendarHandler.cpp

@@ -97,6 +97,7 @@ void WorldSession::HandleCalendarAddEvent(WorldPacket &recv_data)
 {
     sLog.outDebug("WORLD: CMSG_CALENDAR_ADD_EVENT");
     recv_data.hexlike();
+    recv_data.rpos(recv_data.wpos());                       // set to end to avoid warnings spam
 
     //std::string unk1, unk2;
     //recv_data >> (std::string)unk1;
@@ -132,6 +133,7 @@ void WorldSession::HandleCalendarUpdateEvent(WorldPacket &recv_data)
 {
     sLog.outDebug("WORLD: CMSG_CALENDAR_UPDATE_EVENT");
     recv_data.hexlike();
+    recv_data.rpos(recv_data.wpos());                       // set to end to avoid warnings spam
 
     //recv_data >> uint64
     //recv_data >> uint64
@@ -150,6 +152,7 @@ void WorldSession::HandleCalendarRemoveEvent(WorldPacket &recv_data)
 {
     sLog.outDebug("WORLD: CMSG_CALENDAR_REMOVE_EVENT");
     recv_data.hexlike();
+    recv_data.rpos(recv_data.wpos());                       // set to end to avoid warnings spam
 
     //recv_data >> uint64
     //recv_data >> uint64
@@ -161,6 +164,7 @@ void WorldSession::HandleCalendarCopyEvent(WorldPacket &recv_data)
 {
     sLog.outDebug("WORLD: CMSG_CALENDAR_COPY_EVENT");
     recv_data.hexlike();
+    recv_data.rpos(recv_data.wpos());                       // set to end to avoid warnings spam
 
     //recv_data >> uint64
     //recv_data >> uint64
@@ -172,6 +176,7 @@ void WorldSession::HandleCalendarEventInvite(WorldPacket &recv_data)
 {
     sLog.outDebug("WORLD: CMSG_CALENDAR_EVENT_INVITE");
     recv_data.hexlike();
+    recv_data.rpos(recv_data.wpos());                       // set to end to avoid warnings spam
 
     //recv_data >> uint64
     //recv_data >> uint64
@@ -185,6 +190,7 @@ void WorldSession::HandleCalendarEventRsvp(WorldPacket &recv_data)
 {
     sLog.outDebug("WORLD: CMSG_CALENDAR_EVENT_RSVP");
     recv_data.hexlike();
+    recv_data.rpos(recv_data.wpos());                       // set to end to avoid warnings spam
 
     //recv_data >> uint64
     //recv_data >> uint64
@@ -196,6 +202,7 @@ void WorldSession::HandleCalendarEventRemoveInvite(WorldPacket &recv_data)
 {
     sLog.outDebug("WORLD: CMSG_CALENDAR_EVENT_REMOVE_INVITE");
     recv_data.hexlike();
+    recv_data.rpos(recv_data.wpos());                       // set to end to avoid warnings spam
 
     //recv_data.readPackGUID(guid)
     //recv_data >> uint64
@@ -207,6 +214,7 @@ void WorldSession::HandleCalendarEventStatus(WorldPacket &recv_data)
 {
     sLog.outDebug("WORLD: CMSG_CALENDAR_EVENT_STATUS");
     recv_data.hexlike();
+    recv_data.rpos(recv_data.wpos());                       // set to end to avoid warnings spam
 
     //recv_data.readPackGUID(guid)
     //recv_data >> uint64
@@ -219,6 +227,7 @@ void WorldSession::HandleCalendarEventModeratorStatus(WorldPacket &recv_data)
 {
     sLog.outDebug("WORLD: CMSG_CALENDAR_EVENT_MODERATOR_STATUS");
     recv_data.hexlike();
+    recv_data.rpos(recv_data.wpos());                       // set to end to avoid warnings spam
 
     //recv_data.readPackGUID(guid)
     //recv_data >> uint64
@@ -231,6 +240,7 @@ void WorldSession::HandleCalendarComplain(WorldPacket &recv_data)
 {
     sLog.outDebug("WORLD: CMSG_CALENDAR_COMPLAIN");
     recv_data.hexlike();
+    recv_data.rpos(recv_data.wpos());                       // set to end to avoid warnings spam
 
     //recv_data >> uint64
     //recv_data >> uint64

src/game/GuildHandler.cpp

@@ -614,12 +614,14 @@ void WorldSession::HandleGuildRankOpcode(WorldPacket& recvPacket)
     guild = objmgr.GetGuildById(GetPlayer()->GetGuildId());
     if(!guild)
     {
+        recvPacket.rpos(recvPacket.wpos());                 // set to end to avoid warnings spam
         SendGuildCommandResult(GUILD_CREATE_S, "", GUILD_PLAYER_NOT_IN_GUILD);
         return;
     }
 
     else if(GetPlayer()->GetGUID() != guild->GetLeader())
     {
+        recvPacket.rpos(recvPacket.wpos());                 // set to end to avoid warnings spam
         SendGuildCommandResult(GUILD_INVITE_S, "", GUILD_PERMISSIONS);
         return;
     }
@@ -1026,10 +1028,14 @@ void WorldSession::HandleGuildBankSwapItems( WorldPacket & recv_data )
     uint64 GoGuid;
     uint8 BankToBank;
 
-    uint8 BankTab, BankTabSlot, AutoStore, AutoStoreCount, PlayerSlot, PlayerBag, SplitedAmount = 0;
+    uint8 BankTab, BankTabSlot, AutoStore;
-    uint8 BankTabDst, BankTabSlotDst, unk2, ToChar = 1;
+    uint8 PlayerSlot = NULL_SLOT;
+    uint8 PlayerBag = NULL_BAG;
+    uint8 BankTabDst, BankTabSlotDst, unk2;
+    uint8 ToChar = 1;
     uint32 ItemEntry, unk1;
-    bool BankToChar = false;
+    uint32 AutoStoreCount = 0;
+    uint32 SplitedAmount = 0;
 
     recv_data >> GoGuid >> BankToBank;
     if (BankToBank)
@@ -1043,10 +1049,11 @@ void WorldSession::HandleGuildBankSwapItems( WorldPacket & recv_data )
         recv_data >> unk2;                                  // always 0
         recv_data >> SplitedAmount;
 
-        if (BankTabSlotDst >= GUILD_BANK_MAX_SLOTS)
+        if (BankTabSlotDst >= GUILD_BANK_MAX_SLOTS || BankTabDst == BankTab && BankTabSlotDst == BankTabSlot)
-            return;
+        {
-        if (BankTabDst == BankTab && BankTabSlotDst == BankTabSlot)
+            recv_data.rpos(recv_data.wpos());               // prevent additional spam at rejected packet
             return;
+        }
     }
     else
     {
@@ -1057,17 +1064,22 @@ void WorldSession::HandleGuildBankSwapItems( WorldPacket & recv_data )
         if (AutoStore)
         {
             recv_data >> AutoStoreCount;
+            recv_data.read_skip<uint8>();                   // ToChar (?), always and expected to be 1 (autostore only triggered in guild->ToChar)
+            recv_data.read_skip<uint32>();                  // unknown, always 0
         }
-        recv_data >> PlayerBag;
+        else
-        recv_data >> PlayerSlot;
-        if (!AutoStore)
         {
+            recv_data >> PlayerBag;
+            recv_data >> PlayerSlot;
             recv_data >> ToChar;
             recv_data >> SplitedAmount;
         }
 
         if (BankTabSlot >= GUILD_BANK_MAX_SLOTS && BankTabSlot != 0xFF)
+        {
+            recv_data.rpos(recv_data.wpos());               // prevent additional spam at rejected packet
             return;
+        }
     }
 
     if (!GetPlayer()->GetGameObjectIfCanInteractWith(GoGuid, GAMEOBJECT_TYPE_GUILD_BANK))
@@ -1202,14 +1214,6 @@ void WorldSession::HandleGuildBankSwapItems( WorldPacket & recv_data )
 
     // Player <-> Bank
 
-    // char->bank autostore click return BankTabSlot = 255 = NULL_SLOT
-    // do similar for bank->char
-    if(AutoStore && ToChar)
-    {
-        PlayerBag = NULL_BAG;
-        PlayerSlot = NULL_SLOT;
-    }
-
     // allow work with inventory only
     if(!Player::IsInventoryPos(PlayerBag,PlayerSlot) && !(PlayerBag == NULL_BAG && PlayerSlot == NULL_SLOT) )
     {

src/game/ItemHandler.cpp

@@ -32,7 +32,8 @@
 void WorldSession::HandleSplitItemOpcode( WorldPacket & recv_data )
 {
     //sLog.outDebug("WORLD: CMSG_SPLIT_ITEM");
-    uint8 srcbag, srcslot, dstbag, dstslot, count;
+    uint8 srcbag, srcslot, dstbag, dstslot;
+    uint32 count;
 
     recv_data >> srcbag >> srcslot >> dstbag >> dstslot >> count;
     //sLog.outDebug("STORAGE: receive srcbag = %u, srcslot = %u, dstbag = %u, dstslot = %u, count = %u", srcbag, srcslot, dstbag, dstslot, count);
@@ -490,12 +491,9 @@ void WorldSession::HandleSellItemOpcode( WorldPacket & recv_data )
 {
     sLog.outDebug(  "WORLD: Received CMSG_SELL_ITEM" );
     uint64 vendorguid, itemguid;
-    uint8 _count;
+    uint32 count;
 
-    recv_data >> vendorguid >> itemguid >> _count;
+    recv_data >> vendorguid >> itemguid >> count;
-
-    // prevent possible overflow, as Trinity uses uint32 for item count
-    uint32 count = _count;
 
     if(!itemguid)
         return;
@@ -971,6 +969,8 @@ void WorldSession::HandleItemNameQueryOpcode(WorldPacket & recv_data)
 {
     uint32 itemid;
     recv_data >> itemid;
+    recv_data.read_skip<uint64>();                          // guid
+
     sLog.outDebug("WORLD: CMSG_ITEM_NAME_QUERY %u", itemid);
     ItemPrototype const *pProto = objmgr.GetItemPrototype( itemid );
     if( pProto )

src/game/MiscHandler.cpp

@@ -931,6 +931,7 @@ void WorldSession::HandleUpdateAccountData(WorldPacket &recv_data)
 
     if(decompressedSize > 0xFFFF)
     {
+        recv_data.rpos(recv_data.wpos());                   // unnneded warning spam in this case
         sLog.outError("UAD: Account data packet too big, size %u", decompressedSize);
         return;
     }
@@ -941,10 +942,13 @@ void WorldSession::HandleUpdateAccountData(WorldPacket &recv_data)
     uLongf realSize = decompressedSize;
     if(uncompress(const_cast<uint8*>(dest.contents()), &realSize, const_cast<uint8*>(recv_data.contents() + recv_data.rpos()), recv_data.size() - recv_data.rpos()) != Z_OK)
     {
+        recv_data.rpos(recv_data.wpos());                   // unnneded warning spam in this case
         sLog.outError("UAD: Failed to decompress account data");
         return;
     }
 
+    recv_data.rpos(recv_data.wpos());                       // uncompress read (recv_data.size() - recv_data.rpos())
+
     std::string adata;
     dest >> adata;
 
@@ -1050,7 +1054,8 @@ void WorldSession::HandleMoveTimeSkippedOpcode( WorldPacket & recv_data )
     /*  WorldSession::Update( getMSTime() );*/
     DEBUG_LOG( "WORLD: Time Lag/Synchronization Resent/Update" );
 
-    recv_data.read_skip2<uint64,uint32>();
+    recv_data.read_skip<uint64>();
+    recv_data.read_skip<uint32>();
     /*
         uint64 guid;
         uint32 time_skipped;

src/game/VoiceChatHandler.cpp

@@ -28,7 +28,8 @@ void WorldSession::HandleVoiceSessionEnableOpcode( WorldPacket & recv_data )
 {
     sLog.outDebug("WORLD: CMSG_VOICE_SESSION_ENABLE");
     // uint8 isVoiceEnabled, uint8 isMicrophoneEnabled
-    recv_data.read_skip2<uint8,uint8>();
+    recv_data.read_skip<uint8>();
+    recv_data.read_skip<uint8>();
     recv_data.hexlike();
 }
 

src/game/WorldSession.cpp

@@ -147,7 +147,7 @@ void WorldSession::QueuePacket(WorldPacket* new_packet)
 }
 
 /// Logging helper for unexpected opcodes
-void WorldSession::logUnexpectedOpcode(WorldPacket* packet, const char *reason)
+void WorldSession::LogUnexpectedOpcode(WorldPacket* packet, const char *reason)
 {
     sLog.outError( "SESSION: received unexpected opcode %s (0x%.4X) %s",
         LookupOpcodeName(packet->GetOpcode()),
@@ -155,6 +155,15 @@ void WorldSession::logUnexpectedOpcode(WorldPacket* packet, const char *reason)
         reason);
 }
 
+/// Logging helper for unexpected opcodes
+void WorldSession::LogUnprocessedTail(WorldPacket *packet)
+{
+    sLog.outError( "SESSION: opcode %s (0x%.4X) have unprocessed tail data (read stop at %u from %u)",
+        LookupOpcodeName(packet->GetOpcode()),
+        packet->GetOpcode(),
+        packet->rpos(),packet->wpos());
+}
+
 /// Update the WorldSession (triggered by World update)
 bool WorldSession::Update(uint32 /*diff*/)
 {
@@ -188,30 +197,40 @@ bool WorldSession::Update(uint32 /*diff*/)
                         {
                             // skip STATUS_LOGGEDIN opcode unexpected errors if player logout sometime ago - this can be network lag delayed packets
                             if(!m_playerRecentlyLogout)
-                                logUnexpectedOpcode(packet, "the player has not logged in yet");
+                                LogUnexpectedOpcode(packet, "the player has not logged in yet");
                         }
                         else if(_player->IsInWorld())
+                        {
                             (this->*opHandle.handler)(*packet);
+                            if (sLog.IsOutDebug() && packet->rpos() < packet->wpos())
+                                LogUnprocessedTail(packet);
+                        }
                         // lag can cause STATUS_LOGGEDIN opcodes to arrive after the player started a transfer
                         break;
                     case STATUS_TRANSFER:
                         if(!_player)
-                            logUnexpectedOpcode(packet, "the player has not logged in yet");
+                            LogUnexpectedOpcode(packet, "the player has not logged in yet");
                         else if(_player->IsInWorld())
-                            logUnexpectedOpcode(packet, "the player is still in world");
+                            LogUnexpectedOpcode(packet, "the player is still in world");
                         else
+                        {
                             (this->*opHandle.handler)(*packet);
+                            if (sLog.IsOutDebug() && packet->rpos() < packet->wpos())
+                                LogUnprocessedTail(packet);
+                        }
                         break;
                     case STATUS_AUTHED:
                         // prevent cheating with skip queue wait
                         if(m_inQueue)
                         {
-                            logUnexpectedOpcode(packet, "the player not pass queue yet");
+                            LogUnexpectedOpcode(packet, "the player not pass queue yet");
                             break;
                         }
 
                         m_playerRecentlyLogout = false;
                         (this->*opHandle.handler)(*packet);
+                        if (sLog.IsOutDebug() && packet->rpos() < packet->wpos())
+                            LogUnprocessedTail(packet);
                         break;
                     case STATUS_NEVER:
                         break;
@@ -221,7 +240,7 @@ bool WorldSession::Update(uint32 /*diff*/)
                         break;
                 }
             }
-            catch(ByteBufferException &exception)
+            catch(ByteBufferException &)
             {
                 sLog.outError("WorldSession::Update ByteBufferException occured while parsing a packet (opcode: %u) from client %s, accountid=%i. Skipped packet.",
                         packet->GetOpcode(), GetRemoteAddress().c_str(), GetAccountId());

src/game/WorldSession.h

@@ -722,7 +722,9 @@ class TRINITY_DLL_SPEC WorldSession
         void moveItems(Item* myItems[], Item* hisItems[]);
 
         // logging helper
-        void logUnexpectedOpcode(WorldPacket *packet, const char * reason);
+        void LogUnexpectedOpcode(WorldPacket *packet, const char * reason);
+        void LogUnprocessedTail(WorldPacket *packet);
+
         Player *_player;
         WorldSocket *m_Socket;
         std::string m_Address;

src/game/WorldSocket.cpp

@@ -655,7 +655,7 @@ int WorldSocket::ProcessIncoming (WorldPacket* new_pct)
             }
         }
     }
-    catch(ByteBufferException &exception)
+    catch(ByteBufferException &)
     {
         sLog.outError("WorldSocket::ProcessIncoming ByteBufferException occured while parsing an instant handled packet (opcode: %u) from client %s, accountid=%i. Disconnected client.",
                 opcode, GetRemoteAddress().c_str(), m_Session?m_Session->GetAccountId():-1);

src/shared/ByteBuffer.h

@@ -265,9 +265,6 @@ class ByteBuffer
         template<typename T>
         void read_skip() { read_skip(sizeof(T)); }
 
-        template<typename T1, typename T2>
-        void read_skip2() { read_skip(sizeof(T1)+sizeof(T2)); }
-
         void read_skip(size_t skip)
         {
             if(_rpos + skip > size())