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

@@ -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<eAddFriendResponseCode>(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);

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);

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);

dNet/ClientPackets.cpp

@@ -46,9 +46,7 @@ void ClientPackets::HandleChatMessage(const SystemAddress& sysAddr, Packet* pack
 		return;
 	}
 
-	CINSTREAM;
+	CINSTREAM_SKIP_HEADER;
-	uint64_t header;
-	inStream.Read(header);
 
 	char chatChannel;
 	uint16_t unknown;
@@ -82,9 +80,7 @@ void ClientPackets::HandleClientPositionUpdate(const SystemAddress& sysAddr, Pac
 		return;
 	}
 
-	CINSTREAM;
+	CINSTREAM_SKIP_HEADER;
-	uint64_t header;
-	inStream.Read(header);
 
 	Entity* entity = EntityManager::Instance()->GetEntity(user->GetLastUsedChar()->GetObjectID());
 	if (!entity) return;

dWorldServer/WorldServer.cpp

@@ -554,10 +554,9 @@ void HandlePacketChat(Packet* packet) {
 		if (static_cast<eConnectionType>(packet->data[1]) == eConnectionType::CHAT_INTERNAL) {
 			switch (static_cast<eChatInternalMessageType>(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;
+				CINSTREAM_SKIP_HEADER;
-				LWOOBJID header;
-				inStream.Read(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;
+				CINSTREAM_SKIP_HEADER;
-				LWOOBJID header;
-				inStream.Read(header);
 
 				LWOOBJID teamID = 0;
 				char lootOption = 0;
@@ -1213,10 +1207,8 @@ void HandlePacket(Packet* packet) {
 
 	case eWorldMessageType::ROUTE_PACKET: {
 		//Yeet to chat
-		CINSTREAM;
+		CINSTREAM_SKIP_HEADER;
-		uint64_t header = 0;
 		uint32_t size = 0;
-		inStream.Read(header);
 		inStream.Read(size);
 
 		if (size > 20000) {

tests/dCommonTests/CMakeLists.txt

@@ -1,5 +1,6 @@
 set(DCOMMONTEST_SOURCES
 	"AMFDeserializeTests.cpp"
+	"HeaderSkipTest.cpp"
 	"TestLDFFormat.cpp"
 	"TestNiPoint3.cpp"
 	"TestEncoding.cpp"

tests/dCommonTests/HeaderSkipTest.cpp

@@ -0,0 +1,37 @@
+#include <gtest/gtest.h>
+
+#include "dCommonDependencies.h"
+#include "dCommonVars.h"
+#include "BitStream.h"
+
+#define PacketUniquePtr std::unique_ptr<Packet>
+
+TEST(dCommonTests, HeaderSkipExcessTest) {
+	PacketUniquePtr packet = std::make_unique<Packet>();
+	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<Packet>();
+	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<Packet>();
+	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);
+}