From 7aad6e4bc2b399b26ca732a25ac9204f4bdbf6d6 Mon Sep 17 00:00:00 2001 From: David Markowitz <39972741+EmosewaMC@users.noreply.github.com> Date: Mon, 8 May 2023 04:31:10 -0700 Subject: [PATCH] Make header skips more obvious (#1074) * Make header skips more obvious seeing inStream.Read(LWOOBJID) is much less clear than a macro which very clearly skips the header. * Formatting pass --- dChatServer/ChatPacketHandler.cpp | 36 +++++++++----------------- dChatServer/PlayerContainer.cpp | 12 +++------ dCommon/dEnums/dCommonVars.h | 2 ++ dNet/ClientPackets.cpp | 8 ++---- dWorldServer/WorldServer.cpp | 18 ++++--------- tests/dCommonTests/CMakeLists.txt | 1 + tests/dCommonTests/HeaderSkipTest.cpp | 37 +++++++++++++++++++++++++++ 7 files changed, 63 insertions(+), 51 deletions(-) create mode 100644 tests/dCommonTests/HeaderSkipTest.cpp diff --git a/dChatServer/ChatPacketHandler.cpp b/dChatServer/ChatPacketHandler.cpp index 9b5ca761..878cc71c 100644 --- a/dChatServer/ChatPacketHandler.cpp +++ b/dChatServer/ChatPacketHandler.cpp @@ -22,10 +22,9 @@ extern PlayerContainer playerContainer; void ChatPacketHandler::HandleFriendlistRequest(Packet* packet) { //Get from the packet which player we want to do something with: - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID = 0; inStream.Read(playerID); - inStream.Read(playerID); auto player = playerContainer.GetPlayerData(playerID); if (!player) return; @@ -99,10 +98,9 @@ void ChatPacketHandler::HandleFriendRequest(Packet* packet) { auto maxNumberOfBestFriendsAsString = Game::config->GetValue("max_number_of_best_friends"); // If this config option doesn't exist, default to 5 which is what live used. auto maxNumberOfBestFriends = maxNumberOfBestFriendsAsString != "" ? std::stoi(maxNumberOfBestFriendsAsString) : 5U; - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID requestorPlayerID; inStream.Read(requestorPlayerID); - inStream.Read(requestorPlayerID); uint32_t spacing{}; inStream.Read(spacing); std::string playerName = ""; @@ -247,10 +245,9 @@ void ChatPacketHandler::HandleFriendRequest(Packet* packet) { } void ChatPacketHandler::HandleFriendResponse(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID; inStream.Read(playerID); - inStream.Read(playerID); eAddFriendResponseCode clientResponseCode = static_cast(packet->data[0x14]); std::string friendName = PacketUtils::ReadString(0x15, packet, true); @@ -323,10 +320,9 @@ void ChatPacketHandler::HandleFriendResponse(Packet* packet) { } void ChatPacketHandler::HandleRemoveFriend(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID; inStream.Read(playerID); - inStream.Read(playerID); std::string friendName = PacketUtils::ReadString(0x14, packet, true); //we'll have to query the db here to find the user, since you can delete them while they're offline. @@ -381,10 +377,9 @@ void ChatPacketHandler::HandleRemoveFriend(Packet* packet) { } void ChatPacketHandler::HandleChatMessage(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID = LWOOBJID_EMPTY; inStream.Read(playerID); - inStream.Read(playerID); auto* sender = playerContainer.GetPlayerData(playerID); @@ -501,10 +496,9 @@ void ChatPacketHandler::HandlePrivateChatMessage(Packet* packet) { } void ChatPacketHandler::HandleTeamInvite(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID; inStream.Read(playerID); - inStream.Read(playerID); std::string invitedPlayer = PacketUtils::ReadString(0x14, packet, true); auto* player = playerContainer.GetPlayerData(playerID); @@ -542,10 +536,9 @@ void ChatPacketHandler::HandleTeamInvite(Packet* packet) { } void ChatPacketHandler::HandleTeamInviteResponse(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID = LWOOBJID_EMPTY; inStream.Read(playerID); - inStream.Read(playerID); uint32_t size = 0; inStream.Read(size); char declined = 0; @@ -576,10 +569,9 @@ void ChatPacketHandler::HandleTeamInviteResponse(Packet* packet) { } void ChatPacketHandler::HandleTeamLeave(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID = LWOOBJID_EMPTY; inStream.Read(playerID); - inStream.Read(playerID); uint32_t size = 0; inStream.Read(size); @@ -593,10 +585,9 @@ void ChatPacketHandler::HandleTeamLeave(Packet* packet) { } void ChatPacketHandler::HandleTeamKick(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID = LWOOBJID_EMPTY; inStream.Read(playerID); - inStream.Read(playerID); std::string kickedPlayer = PacketUtils::ReadString(0x14, packet, true); @@ -624,10 +615,9 @@ void ChatPacketHandler::HandleTeamKick(Packet* packet) { } void ChatPacketHandler::HandleTeamPromote(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID = LWOOBJID_EMPTY; inStream.Read(playerID); - inStream.Read(playerID); std::string promotedPlayer = PacketUtils::ReadString(0x14, packet, true); @@ -647,10 +637,9 @@ void ChatPacketHandler::HandleTeamPromote(Packet* packet) { } void ChatPacketHandler::HandleTeamLootOption(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID = LWOOBJID_EMPTY; inStream.Read(playerID); - inStream.Read(playerID); uint32_t size = 0; inStream.Read(size); @@ -671,10 +660,9 @@ void ChatPacketHandler::HandleTeamLootOption(Packet* packet) { } void ChatPacketHandler::HandleTeamStatusRequest(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID = LWOOBJID_EMPTY; inStream.Read(playerID); - inStream.Read(playerID); auto* team = playerContainer.GetTeam(playerID); auto* data = playerContainer.GetPlayerData(playerID); diff --git a/dChatServer/PlayerContainer.cpp b/dChatServer/PlayerContainer.cpp index dcc9ebb9..689ffd77 100644 --- a/dChatServer/PlayerContainer.cpp +++ b/dChatServer/PlayerContainer.cpp @@ -19,9 +19,8 @@ PlayerContainer::~PlayerContainer() { } void PlayerContainer::InsertPlayer(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; PlayerData* data = new PlayerData(); - inStream.SetReadOffset(inStream.GetReadOffset() + 64); inStream.Read(data->playerID); uint32_t len; @@ -52,9 +51,8 @@ void PlayerContainer::InsertPlayer(Packet* packet) { } void PlayerContainer::RemovePlayer(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID; - inStream.Read(playerID); //skip header inStream.Read(playerID); //Before they get kicked, we need to also send a message to their friends saying that they disconnected. @@ -97,9 +95,8 @@ void PlayerContainer::RemovePlayer(Packet* packet) { } void PlayerContainer::MuteUpdate(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID; - inStream.Read(playerID); //skip header inStream.Read(playerID); time_t expire = 0; inStream.Read(expire); @@ -118,9 +115,8 @@ void PlayerContainer::MuteUpdate(Packet* packet) { } void PlayerContainer::CreateTeamServer(Packet* packet) { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID; - inStream.Read(playerID); //skip header inStream.Read(playerID); size_t membersSize = 0; inStream.Read(membersSize); diff --git a/dCommon/dEnums/dCommonVars.h b/dCommon/dEnums/dCommonVars.h index e11866f1..f67145da 100644 --- a/dCommon/dEnums/dCommonVars.h +++ b/dCommon/dEnums/dCommonVars.h @@ -28,8 +28,10 @@ constexpr uint32_t lowFrameDelta = FRAMES_TO_MS(lowFramerate); //========== MACROS =========== +#define HEADER_SIZE 8 #define CBITSTREAM RakNet::BitStream bitStream; #define CINSTREAM RakNet::BitStream inStream(packet->data, packet->length, false); +#define CINSTREAM_SKIP_HEADER CINSTREAM if (inStream.GetNumberOfUnreadBits() >= BYTES_TO_BITS(HEADER_SIZE)) inStream.IgnoreBytes(HEADER_SIZE); else inStream.IgnoreBits(inStream.GetNumberOfUnreadBits()); #define CMSGHEADER PacketUtils::WriteHeader(bitStream, eConnectionType::CLIENT, eClientMessageType::GAME_MSG); #define SEND_PACKET Game::server->Send(&bitStream, sysAddr, false); #define SEND_PACKET_BROADCAST Game::server->Send(&bitStream, UNASSIGNED_SYSTEM_ADDRESS, true); diff --git a/dNet/ClientPackets.cpp b/dNet/ClientPackets.cpp index 4ccbb609..1c22bdcd 100644 --- a/dNet/ClientPackets.cpp +++ b/dNet/ClientPackets.cpp @@ -46,9 +46,7 @@ void ClientPackets::HandleChatMessage(const SystemAddress& sysAddr, Packet* pack return; } - CINSTREAM; - uint64_t header; - inStream.Read(header); + CINSTREAM_SKIP_HEADER; char chatChannel; uint16_t unknown; @@ -82,9 +80,7 @@ void ClientPackets::HandleClientPositionUpdate(const SystemAddress& sysAddr, Pac return; } - CINSTREAM; - uint64_t header; - inStream.Read(header); + CINSTREAM_SKIP_HEADER; Entity* entity = EntityManager::Instance()->GetEntity(user->GetLastUsedChar()->GetObjectID()); if (!entity) return; diff --git a/dWorldServer/WorldServer.cpp b/dWorldServer/WorldServer.cpp index 26cbe7ba..5dabce1c 100644 --- a/dWorldServer/WorldServer.cpp +++ b/dWorldServer/WorldServer.cpp @@ -554,10 +554,9 @@ void HandlePacketChat(Packet* packet) { if (static_cast(packet->data[1]) == eConnectionType::CHAT_INTERNAL) { switch (static_cast(packet->data[3])) { case eChatInternalMessageType::ROUTE_TO_PLAYER: { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerID; inStream.Read(playerID); - inStream.Read(playerID); auto player = EntityManager::Instance()->GetEntity(playerID); if (!player) return; @@ -576,9 +575,7 @@ void HandlePacketChat(Packet* packet) { } case eChatInternalMessageType::ANNOUNCEMENT: { - CINSTREAM; - LWOOBJID header; - inStream.Read(header); + CINSTREAM_SKIP_HEADER; std::string title; std::string msg; @@ -615,11 +612,10 @@ void HandlePacketChat(Packet* packet) { } case eChatInternalMessageType::MUTE_UPDATE: { - CINSTREAM; + CINSTREAM_SKIP_HEADER; LWOOBJID playerId; time_t expire = 0; inStream.Read(playerId); - inStream.Read(playerId); inStream.Read(expire); auto* entity = EntityManager::Instance()->GetEntity(playerId); @@ -634,9 +630,7 @@ void HandlePacketChat(Packet* packet) { } case eChatInternalMessageType::TEAM_UPDATE: { - CINSTREAM; - LWOOBJID header; - inStream.Read(header); + CINSTREAM_SKIP_HEADER; LWOOBJID teamID = 0; char lootOption = 0; @@ -1213,10 +1207,8 @@ void HandlePacket(Packet* packet) { case eWorldMessageType::ROUTE_PACKET: { //Yeet to chat - CINSTREAM; - uint64_t header = 0; + CINSTREAM_SKIP_HEADER; uint32_t size = 0; - inStream.Read(header); inStream.Read(size); if (size > 20000) { diff --git a/tests/dCommonTests/CMakeLists.txt b/tests/dCommonTests/CMakeLists.txt index dd282cb5..444afbdd 100644 --- a/tests/dCommonTests/CMakeLists.txt +++ b/tests/dCommonTests/CMakeLists.txt @@ -1,5 +1,6 @@ set(DCOMMONTEST_SOURCES "AMFDeserializeTests.cpp" + "HeaderSkipTest.cpp" "TestLDFFormat.cpp" "TestNiPoint3.cpp" "TestEncoding.cpp" diff --git a/tests/dCommonTests/HeaderSkipTest.cpp b/tests/dCommonTests/HeaderSkipTest.cpp new file mode 100644 index 00000000..a8f78078 --- /dev/null +++ b/tests/dCommonTests/HeaderSkipTest.cpp @@ -0,0 +1,37 @@ +#include + +#include "dCommonDependencies.h" +#include "dCommonVars.h" +#include "BitStream.h" + +#define PacketUniquePtr std::unique_ptr + +TEST(dCommonTests, HeaderSkipExcessTest) { + PacketUniquePtr packet = std::make_unique(); + unsigned char headerAndData[] = { 0x53, 0x02, 0x00, 0x07, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00 }; // positive + packet->data = headerAndData; + packet->length = sizeof(headerAndData); + CINSTREAM_SKIP_HEADER; + ASSERT_EQ(inStream.GetNumberOfUnreadBits(), 64); + ASSERT_EQ(inStream.GetNumberOfBitsAllocated(), 128); +} + +TEST(dCommonTests, HeaderSkipExactDataTest) { + PacketUniquePtr packet = std::make_unique(); + unsigned char header[] = { 0x53, 0x02, 0x00, 0x07, 0x00, 0x00, 0x00, 0x00 }; // positive + packet->data = header; + packet->length = sizeof(header); + CINSTREAM_SKIP_HEADER; + ASSERT_EQ(inStream.GetNumberOfUnreadBits(), 0); + ASSERT_EQ(inStream.GetNumberOfBitsAllocated(), 64); +} + +TEST(dCommonTests, HeaderSkipNotEnoughDataTest) { + PacketUniquePtr packet = std::make_unique(); + unsigned char notEnoughData[] = { 0x53, 0x02, 0x00, 0x07, 0x00, 0x00 }; // negative + packet->data = notEnoughData; + packet->length = sizeof(notEnoughData); + CINSTREAM_SKIP_HEADER; + ASSERT_EQ(inStream.GetNumberOfUnreadBits(), 0); + ASSERT_EQ(inStream.GetNumberOfBitsAllocated(), 48); +}