From 5ecc46623a3ac29a01f2b692acaa4304e0708bc9 Mon Sep 17 00:00:00 2001 From: Kelsi Date: Sat, 14 Mar 2026 10:09:04 -0700 Subject: [PATCH] fix(combatlog): consume full spell go target lists when capped --- src/game/packet_parsers_classic.cpp | 47 +++++++++++++++-------------- src/game/packet_parsers_tbc.cpp | 47 +++++++++++++++-------------- src/game/world_packets.cpp | 46 ++++++++++++++-------------- 3 files changed, 74 insertions(+), 66 deletions(-) diff --git a/src/game/packet_parsers_classic.cpp b/src/game/packet_parsers_classic.cpp index 046839d0..4528ef72 100644 --- a/src/game/packet_parsers_classic.cpp +++ b/src/game/packet_parsers_classic.cpp @@ -410,33 +410,34 @@ bool ClassicPacketParsers::parseSpellGo(network::Packet& packet, SpellGoData& da // Hit targets if (rem() < 1) return true; - data.hitCount = packet.readUInt8(); - // Cap hit count to prevent OOM from huge target lists - if (data.hitCount > 128) { - LOG_WARNING("[Classic] Spell go: hitCount capped (requested=", (int)data.hitCount, ")"); - data.hitCount = 128; + const uint8_t rawHitCount = packet.readUInt8(); + if (rawHitCount > 128) { + LOG_WARNING("[Classic] Spell go: hitCount capped (requested=", (int)rawHitCount, ")"); } - data.hitTargets.reserve(data.hitCount); - for (uint8_t i = 0; i < data.hitCount && rem() >= 1; ++i) { - data.hitTargets.push_back(UpdateObjectParser::readPackedGuid(packet)); + const uint8_t storedHitLimit = std::min(rawHitCount, 128); + data.hitTargets.reserve(storedHitLimit); + for (uint16_t i = 0; i < rawHitCount && rem() >= 1; ++i) { + const uint64_t targetGuid = UpdateObjectParser::readPackedGuid(packet); + if (i < storedHitLimit) { + data.hitTargets.push_back(targetGuid); + } } + data.hitCount = static_cast(data.hitTargets.size()); // Check if we read all expected hits - if (data.hitTargets.size() < data.hitCount) { + if (data.hitTargets.size() < rawHitCount) { LOG_WARNING("[Classic] Spell go: truncated hit targets at index ", (int)data.hitTargets.size(), - "/", (int)data.hitCount); - data.hitCount = data.hitTargets.size(); + "/", (int)rawHitCount); } // Miss targets if (rem() < 1) return true; - data.missCount = packet.readUInt8(); - // Cap miss count to prevent OOM - if (data.missCount > 128) { - LOG_WARNING("[Classic] Spell go: missCount capped (requested=", (int)data.missCount, ")"); - data.missCount = 128; + const uint8_t rawMissCount = packet.readUInt8(); + if (rawMissCount > 128) { + LOG_WARNING("[Classic] Spell go: missCount capped (requested=", (int)rawMissCount, ")"); } - data.missTargets.reserve(data.missCount); - for (uint8_t i = 0; i < data.missCount && rem() >= 2; ++i) { + const uint8_t storedMissLimit = std::min(rawMissCount, 128); + data.missTargets.reserve(storedMissLimit); + for (uint16_t i = 0; i < rawMissCount && rem() >= 2; ++i) { SpellGoMissEntry m; m.targetGuid = UpdateObjectParser::readPackedGuid(packet); if (rem() < 1) break; @@ -446,13 +447,15 @@ bool ClassicPacketParsers::parseSpellGo(network::Packet& packet, SpellGoData& da (void)packet.readUInt32(); (void)packet.readUInt8(); } - data.missTargets.push_back(m); + if (i < storedMissLimit) { + data.missTargets.push_back(m); + } } + data.missCount = static_cast(data.missTargets.size()); // Check if we read all expected misses - if (data.missTargets.size() < data.missCount) { + if (data.missTargets.size() < rawMissCount) { LOG_WARNING("[Classic] Spell go: truncated miss targets at index ", (int)data.missTargets.size(), - "/", (int)data.missCount); - data.missCount = data.missTargets.size(); + "/", (int)rawMissCount); } LOG_DEBUG("[Classic] Spell go: spell=", data.spellId, " hits=", (int)data.hitCount, diff --git a/src/game/packet_parsers_tbc.cpp b/src/game/packet_parsers_tbc.cpp index d218926a..ba2393c3 100644 --- a/src/game/packet_parsers_tbc.cpp +++ b/src/game/packet_parsers_tbc.cpp @@ -1277,32 +1277,33 @@ bool TbcPacketParsers::parseSpellGo(network::Packet& packet, SpellGoData& data) return true; } - data.hitCount = packet.readUInt8(); - // Cap hit count to prevent OOM from huge target lists - if (data.hitCount > 128) { - LOG_WARNING("[TBC] Spell go: hitCount capped (requested=", (int)data.hitCount, ")"); - data.hitCount = 128; + const uint8_t rawHitCount = packet.readUInt8(); + if (rawHitCount > 128) { + LOG_WARNING("[TBC] Spell go: hitCount capped (requested=", (int)rawHitCount, ")"); } - data.hitTargets.reserve(data.hitCount); - for (uint8_t i = 0; i < data.hitCount && packet.getReadPos() + 8 <= packet.getSize(); ++i) { - data.hitTargets.push_back(packet.readUInt64()); // full GUID in TBC + const uint8_t storedHitLimit = std::min(rawHitCount, 128); + data.hitTargets.reserve(storedHitLimit); + for (uint16_t i = 0; i < rawHitCount && packet.getReadPos() + 8 <= packet.getSize(); ++i) { + const uint64_t targetGuid = packet.readUInt64(); // full GUID in TBC + if (i < storedHitLimit) { + data.hitTargets.push_back(targetGuid); + } } + data.hitCount = static_cast(data.hitTargets.size()); // Check if we read all expected hits - if (data.hitTargets.size() < data.hitCount) { + if (data.hitTargets.size() < rawHitCount) { LOG_WARNING("[TBC] Spell go: truncated hit targets at index ", (int)data.hitTargets.size(), - "/", (int)data.hitCount); - data.hitCount = data.hitTargets.size(); + "/", (int)rawHitCount); } if (packet.getReadPos() < packet.getSize()) { - data.missCount = packet.readUInt8(); - // Cap miss count to prevent OOM - if (data.missCount > 128) { - LOG_WARNING("[TBC] Spell go: missCount capped (requested=", (int)data.missCount, ")"); - data.missCount = 128; + const uint8_t rawMissCount = packet.readUInt8(); + if (rawMissCount > 128) { + LOG_WARNING("[TBC] Spell go: missCount capped (requested=", (int)rawMissCount, ")"); } - data.missTargets.reserve(data.missCount); - for (uint8_t i = 0; i < data.missCount && packet.getReadPos() + 9 <= packet.getSize(); ++i) { + const uint8_t storedMissLimit = std::min(rawMissCount, 128); + data.missTargets.reserve(storedMissLimit); + for (uint16_t i = 0; i < rawMissCount && packet.getReadPos() + 9 <= packet.getSize(); ++i) { SpellGoMissEntry m; m.targetGuid = packet.readUInt64(); // full GUID in TBC m.missType = packet.readUInt8(); @@ -1313,13 +1314,15 @@ bool TbcPacketParsers::parseSpellGo(network::Packet& packet, SpellGoData& data) (void)packet.readUInt32(); (void)packet.readUInt8(); } - data.missTargets.push_back(m); + if (i < storedMissLimit) { + data.missTargets.push_back(m); + } } + data.missCount = static_cast(data.missTargets.size()); // Check if we read all expected misses - if (data.missTargets.size() < data.missCount) { + if (data.missTargets.size() < rawMissCount) { LOG_WARNING("[TBC] Spell go: truncated miss targets at index ", (int)data.missTargets.size(), - "/", (int)data.missCount); - data.missCount = data.missTargets.size(); + "/", (int)rawMissCount); } } diff --git a/src/game/world_packets.cpp b/src/game/world_packets.cpp index 659003d7..686dba75 100644 --- a/src/game/world_packets.cpp +++ b/src/game/world_packets.cpp @@ -3714,43 +3714,43 @@ bool SpellGoParser::parse(network::Packet& packet, SpellGoData& data) { // Timestamp in 3.3.5a packet.readUInt32(); - data.hitCount = packet.readUInt8(); - // Cap hit count to prevent DoS via massive arrays - if (data.hitCount > 128) { - LOG_WARNING("Spell go: hitCount capped (requested=", (int)data.hitCount, ")"); - data.hitCount = 128; + const uint8_t rawHitCount = packet.readUInt8(); + if (rawHitCount > 128) { + LOG_WARNING("Spell go: hitCount capped (requested=", (int)rawHitCount, ")"); } + const uint8_t storedHitLimit = std::min(rawHitCount, 128); - data.hitTargets.reserve(data.hitCount); - for (uint8_t i = 0; i < data.hitCount; ++i) { + data.hitTargets.reserve(storedHitLimit); + for (uint16_t i = 0; i < rawHitCount; ++i) { // WotLK hit targets are packed GUIDs, like the caster and miss targets. if (packet.getSize() - packet.getReadPos() < 1) { - LOG_WARNING("Spell go: truncated hit targets at index ", (int)i, "/", (int)data.hitCount); - data.hitCount = i; + LOG_WARNING("Spell go: truncated hit targets at index ", i, "/", (int)rawHitCount); break; } - data.hitTargets.push_back(UpdateObjectParser::readPackedGuid(packet)); + const uint64_t targetGuid = UpdateObjectParser::readPackedGuid(packet); + if (i < storedHitLimit) { + data.hitTargets.push_back(targetGuid); + } } + data.hitCount = static_cast(data.hitTargets.size()); // Validate missCount field exists if (packet.getSize() - packet.getReadPos() < 1) { return true; // Valid, just no misses } - data.missCount = packet.readUInt8(); - // Cap miss count to prevent DoS - if (data.missCount > 128) { - LOG_WARNING("Spell go: missCount capped (requested=", (int)data.missCount, ")"); - data.missCount = 128; + const uint8_t rawMissCount = packet.readUInt8(); + if (rawMissCount > 128) { + LOG_WARNING("Spell go: missCount capped (requested=", (int)rawMissCount, ")"); } + const uint8_t storedMissLimit = std::min(rawMissCount, 128); - data.missTargets.reserve(data.missCount); - for (uint8_t i = 0; i < data.missCount; ++i) { + data.missTargets.reserve(storedMissLimit); + for (uint16_t i = 0; i < rawMissCount; ++i) { // Each miss entry: packed GUID(1-8 bytes) + missType(1 byte). // REFLECT additionally appends uint32 reflectSpellId + uint8 reflectResult. if (packet.getSize() - packet.getReadPos() < 2) { - LOG_WARNING("Spell go: truncated miss targets at index ", (int)i, "/", (int)data.missCount); - data.missCount = i; + LOG_WARNING("Spell go: truncated miss targets at index ", i, "/", (int)rawMissCount); break; } SpellGoMissEntry m; @@ -3758,15 +3758,17 @@ bool SpellGoParser::parse(network::Packet& packet, SpellGoData& data) { m.missType = (packet.getSize() - packet.getReadPos() >= 1) ? packet.readUInt8() : 0; if (m.missType == 11) { if (packet.getSize() - packet.getReadPos() < 5) { - LOG_WARNING("Spell go: truncated reflect payload at miss index ", (int)i, "/", (int)data.missCount); - data.missCount = i; + LOG_WARNING("Spell go: truncated reflect payload at miss index ", i, "/", (int)rawMissCount); break; } (void)packet.readUInt32(); (void)packet.readUInt8(); } - data.missTargets.push_back(m); + if (i < storedMissLimit) { + data.missTargets.push_back(m); + } } + data.missCount = static_cast(data.missTargets.size()); LOG_DEBUG("Spell go: spell=", data.spellId, " hits=", (int)data.hitCount, " misses=", (int)data.missCount);