From a2ca2733702f2be1e8a0277c1f9ba64edc0c1b65 Mon Sep 17 00:00:00 2001 From: David Markowitz <39972741+EmosewaMC@users.noreply.github.com> Date: Fri, 16 Dec 2022 13:23:02 -0800 Subject: [PATCH] Cleanup behavior bitstream reads (#888) * Add failArmor server side Address out of bounds reading in behavior Address the basicAttackBehavior reading out of bounds memory and reading bits that didnt exist, which occasionally caused crashes and also caused the behavior to do undefined behavior due to the bad reads. Tested that attacking a wall anywhere with a projectile now does not crash the game. Tested with logs that the behavior correctly returned when there were no allocated bits or returned when other states were met. Add back logs and add fail handle Remove comment block Revert "Add back logs and add fail handle" This reverts commit db19be0906fc8bf35bf89037e2bfba39f5ef9c0c. Split out checks * Cleanup Behavior streams --- dGame/dBehaviors/AirMovementBehavior.cpp | 27 +++++++++++++------ dGame/dBehaviors/AirMovementBehavior.h | 2 +- dGame/dBehaviors/AreaOfEffectBehavior.cpp | 16 ++++++++--- dGame/dBehaviors/AttackDelayBehavior.cpp | 7 +++-- dGame/dBehaviors/ChainBehavior.cpp | 15 +++++++---- dGame/dBehaviors/ChargeUpBehavior.cpp | 8 ++++-- dGame/dBehaviors/ForceMovementBehavior.cpp | 23 +++++++++++----- dGame/dBehaviors/InterruptBehavior.cpp | 15 ++++++++--- dGame/dBehaviors/KnockbackBehavior.cpp | 9 +++++-- dGame/dBehaviors/MovementSwitchBehavior.cpp | 9 ++++--- dGame/dBehaviors/ProjectileAttackBehavior.cpp | 21 ++++++++++----- dGame/dBehaviors/StunBehavior.cpp | 7 +++-- dGame/dBehaviors/SwitchBehavior.cpp | 5 +++- dGame/dBehaviors/SwitchMultipleBehavior.cpp | 12 +++++---- dGame/dBehaviors/TacArcBehavior.cpp | 18 +++++++++---- 15 files changed, 138 insertions(+), 56 deletions(-) diff --git a/dGame/dBehaviors/AirMovementBehavior.cpp b/dGame/dBehaviors/AirMovementBehavior.cpp index 469ac6e..932297b 100644 --- a/dGame/dBehaviors/AirMovementBehavior.cpp +++ b/dGame/dBehaviors/AirMovementBehavior.cpp @@ -2,11 +2,16 @@ #include "BehaviorBranchContext.h" #include "BehaviorContext.h" #include "EntityManager.h" +#include "Game.h" +#include "dLogger.h" void AirMovementBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) { - uint32_t handle; + uint32_t handle{}; - bitStream->Read(handle); + if (!bitStream->Read(handle)) { + Game::logger->Log("AirMovementBehavior", "Unable to read handle from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + } context->RegisterSyncBehavior(handle, this, branch); } @@ -17,14 +22,20 @@ void AirMovementBehavior::Calculate(BehaviorContext* context, RakNet::BitStream* bitStream->Write(handle); } -void AirMovementBehavior::Sync(BehaviorContext* context, RakNet::BitStream* bit_stream, BehaviorBranchContext branch) { - uint32_t behaviorId; +void AirMovementBehavior::Sync(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) { + uint32_t behaviorId{}; - bit_stream->Read(behaviorId); + if (!bitStream->Read(behaviorId)) { + Game::logger->Log("AirMovementBehavior", "Unable to read behaviorId from bitStream, aborting Sync! %i", bitStream->GetNumberOfUnreadBits()); + return; + } - LWOOBJID target; + LWOOBJID target{}; - bit_stream->Read(target); + if (!bitStream->Read(target)) { + Game::logger->Log("AirMovementBehavior", "Unable to read target from bitStream, aborting Sync! %i", bitStream->GetNumberOfUnreadBits()); + return; + } auto* behavior = CreateBehavior(behaviorId); @@ -32,7 +43,7 @@ void AirMovementBehavior::Sync(BehaviorContext* context, RakNet::BitStream* bit_ branch.target = target; } - behavior->Handle(context, bit_stream, branch); + behavior->Handle(context, bitStream, branch); } void AirMovementBehavior::Load() { diff --git a/dGame/dBehaviors/AirMovementBehavior.h b/dGame/dBehaviors/AirMovementBehavior.h index 323edf3..89dc1e0 100644 --- a/dGame/dBehaviors/AirMovementBehavior.h +++ b/dGame/dBehaviors/AirMovementBehavior.h @@ -16,7 +16,7 @@ public: void Calculate(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) override; - void Sync(BehaviorContext* context, RakNet::BitStream* bit_stream, BehaviorBranchContext branch) override; + void Sync(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) override; void Load() override; }; diff --git a/dGame/dBehaviors/AreaOfEffectBehavior.cpp b/dGame/dBehaviors/AreaOfEffectBehavior.cpp index 898d1f9..ca3bdf9 100644 --- a/dGame/dBehaviors/AreaOfEffectBehavior.cpp +++ b/dGame/dBehaviors/AreaOfEffectBehavior.cpp @@ -9,11 +9,16 @@ #include "BehaviorContext.h" #include "RebuildComponent.h" #include "DestroyableComponent.h" +#include "Game.h" +#include "dLogger.h" void AreaOfEffectBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) { - uint32_t targetCount; + uint32_t targetCount{}; - bitStream->Read(targetCount); + if (!bitStream->Read(targetCount)) { + Game::logger->Log("AreaOfEffectBehavior", "Unable to read targetCount from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + } if (targetCount > this->m_maxTargets) { return; @@ -24,9 +29,12 @@ void AreaOfEffectBehavior::Handle(BehaviorContext* context, RakNet::BitStream* b targets.reserve(targetCount); for (auto i = 0u; i < targetCount; ++i) { - LWOOBJID target; + LWOOBJID target{}; - bitStream->Read(target); + if (!bitStream->Read(target)) { + Game::logger->Log("AreaOfEffectBehavior", "failed to read in target %i from bitStream, aborting target Handle!", i); + return; + }; targets.push_back(target); } diff --git a/dGame/dBehaviors/AttackDelayBehavior.cpp b/dGame/dBehaviors/AttackDelayBehavior.cpp index 450741e..201921a 100644 --- a/dGame/dBehaviors/AttackDelayBehavior.cpp +++ b/dGame/dBehaviors/AttackDelayBehavior.cpp @@ -5,9 +5,12 @@ #include "dLogger.h" void AttackDelayBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStream, const BehaviorBranchContext branch) { - uint32_t handle; + uint32_t handle{}; - bitStream->Read(handle); + if (!bitStream->Read(handle)) { + Game::logger->Log("AttackDelayBehavior", "Unable to read handle from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + }; for (auto i = 0u; i < this->m_numIntervals; ++i) { context->RegisterSyncBehavior(handle, this, branch); diff --git a/dGame/dBehaviors/ChainBehavior.cpp b/dGame/dBehaviors/ChainBehavior.cpp index 3ef8c15..ec0f896 100644 --- a/dGame/dBehaviors/ChainBehavior.cpp +++ b/dGame/dBehaviors/ChainBehavior.cpp @@ -4,14 +4,19 @@ #include "dLogger.h" void ChainBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStream, const BehaviorBranchContext branch) { - uint32_t chain_index; + uint32_t chainIndex{}; - bitStream->Read(chain_index); + if (!bitStream->Read(chainIndex)) { + Game::logger->Log("ChainBehavior", "Unable to read chainIndex from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + } - chain_index--; + chainIndex--; - if (chain_index < this->m_behaviors.size()) { - this->m_behaviors.at(chain_index)->Handle(context, bitStream, branch); + if (chainIndex < this->m_behaviors.size()) { + this->m_behaviors.at(chainIndex)->Handle(context, bitStream, branch); + } else { + Game::logger->Log("ChainBehavior", "chainIndex out of bounds, aborting handle of chain %i bits unread %i", chainIndex, bitStream->GetNumberOfUnreadBits()); } } diff --git a/dGame/dBehaviors/ChargeUpBehavior.cpp b/dGame/dBehaviors/ChargeUpBehavior.cpp index fa9fa34..1b9ba43 100644 --- a/dGame/dBehaviors/ChargeUpBehavior.cpp +++ b/dGame/dBehaviors/ChargeUpBehavior.cpp @@ -1,12 +1,16 @@ #include "ChargeUpBehavior.h" #include "BehaviorBranchContext.h" #include "BehaviorContext.h" +#include "Game.h" #include "dLogger.h" void ChargeUpBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStream, const BehaviorBranchContext branch) { - uint32_t handle; + uint32_t handle{}; - bitStream->Read(handle); + if (!bitStream->Read(handle)) { + Game::logger->Log("ChargeUpBehavior", "Unable to read handle from bitStream, aborting Handle! variable_type"); + return; + }; context->RegisterSyncBehavior(handle, this, branch); } diff --git a/dGame/dBehaviors/ForceMovementBehavior.cpp b/dGame/dBehaviors/ForceMovementBehavior.cpp index e1ac613..e095447 100644 --- a/dGame/dBehaviors/ForceMovementBehavior.cpp +++ b/dGame/dBehaviors/ForceMovementBehavior.cpp @@ -3,23 +3,34 @@ #include "BehaviorContext.h" #include "ControllablePhysicsComponent.h" #include "EntityManager.h" +#include "Game.h" +#include "dLogger.h" void ForceMovementBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStream, const BehaviorBranchContext branch) { if (this->m_hitAction->m_templateId == BehaviorTemplates::BEHAVIOR_EMPTY && this->m_hitEnemyAction->m_templateId == BehaviorTemplates::BEHAVIOR_EMPTY && this->m_hitFactionAction->m_templateId == BehaviorTemplates::BEHAVIOR_EMPTY) { return; } - uint32_t handle; - bitStream->Read(handle); + uint32_t handle{}; + if (!bitStream->Read(handle)) { + Game::logger->Log("ForceMovementBehavior", "Unable to read handle from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + } context->RegisterSyncBehavior(handle, this, branch); } void ForceMovementBehavior::Sync(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) { - uint32_t next; - bitStream->Read(next); + uint32_t next{}; + if (!bitStream->Read(next)) { + Game::logger->Log("ForceMovementBehavior", "Unable to read target from bitStream, aborting Sync! %i", bitStream->GetNumberOfUnreadBits()); + return; + } - LWOOBJID target; - bitStream->Read(target); + LWOOBJID target{}; + if (!bitStream->Read(target)) { + Game::logger->Log("ForceMovementBehavior", "Unable to read target from bitStream, aborting Sync! %i", bitStream->GetNumberOfUnreadBits()); + return; + } branch.target = target; auto* behavior = CreateBehavior(next); diff --git a/dGame/dBehaviors/InterruptBehavior.cpp b/dGame/dBehaviors/InterruptBehavior.cpp index b42eadb..9035c09 100644 --- a/dGame/dBehaviors/InterruptBehavior.cpp +++ b/dGame/dBehaviors/InterruptBehavior.cpp @@ -11,7 +11,10 @@ void InterruptBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitS if (branch.target != context->originator) { bool unknown = false; - bitStream->Read(unknown); + if (!bitStream->Read(unknown)) { + Game::logger->Log("InterruptBehavior", "Unable to read unknown1 from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + }; if (unknown) return; } @@ -19,7 +22,10 @@ void InterruptBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitS if (!this->m_interruptBlock) { bool unknown = false; - bitStream->Read(unknown); + if (!bitStream->Read(unknown)) { + Game::logger->Log("InterruptBehavior", "Unable to read unknown2 from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + }; if (unknown) return; } @@ -28,7 +34,10 @@ void InterruptBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitS { bool unknown = false; - bitStream->Read(unknown); + if (!bitStream->Read(unknown)) { + Game::logger->Log("InterruptBehavior", "Unable to read unknown3 from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + }; } if (branch.target == context->originator) return; diff --git a/dGame/dBehaviors/KnockbackBehavior.cpp b/dGame/dBehaviors/KnockbackBehavior.cpp index 83c8201..1b878ed 100644 --- a/dGame/dBehaviors/KnockbackBehavior.cpp +++ b/dGame/dBehaviors/KnockbackBehavior.cpp @@ -6,11 +6,16 @@ #include "EntityManager.h" #include "GameMessages.h" #include "DestroyableComponent.h" +#include "Game.h" +#include "dLogger.h" void KnockbackBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) { - bool unknown; + bool unknown{}; - bitStream->Read(unknown); + if (!bitStream->Read(unknown)) { + Game::logger->Log("KnockbackBehavior", "Unable to read unknown from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + }; } void KnockbackBehavior::Calculate(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) { diff --git a/dGame/dBehaviors/MovementSwitchBehavior.cpp b/dGame/dBehaviors/MovementSwitchBehavior.cpp index 17ed5c4..c893e6e 100644 --- a/dGame/dBehaviors/MovementSwitchBehavior.cpp +++ b/dGame/dBehaviors/MovementSwitchBehavior.cpp @@ -13,9 +13,11 @@ void MovementSwitchBehavior::Handle(BehaviorContext* context, RakNet::BitStream* return; } - uint32_t movementType; - - bitStream->Read(movementType); + uint32_t movementType{}; + if (!bitStream->Read(movementType)) { + Game::logger->Log("MovementSwitchBehavior", "Unable to read movementType from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + }; switch (movementType) { case 1: @@ -55,4 +57,3 @@ void MovementSwitchBehavior::Load() { this->m_jumpAction = GetAction("jump_action"); } - diff --git a/dGame/dBehaviors/ProjectileAttackBehavior.cpp b/dGame/dBehaviors/ProjectileAttackBehavior.cpp index 350234e..c14f032 100644 --- a/dGame/dBehaviors/ProjectileAttackBehavior.cpp +++ b/dGame/dBehaviors/ProjectileAttackBehavior.cpp @@ -8,9 +8,12 @@ #include "../dWorldServer/ObjectIDManager.h" void ProjectileAttackBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) { - LWOOBJID target; + LWOOBJID target{}; - bitStream->Read(target); + if (!bitStream->Read(target)) { + Game::logger->Log("ProjectileAttackBehavior", "Unable to read target from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + }; auto* entity = EntityManager::Instance()->GetEntity(context->originator); @@ -30,15 +33,21 @@ void ProjectileAttackBehavior::Handle(BehaviorContext* context, RakNet::BitStrea if (m_useMouseposit) { NiPoint3 targetPosition = NiPoint3::ZERO; - bitStream->Read(targetPosition); + if (!bitStream->Read(targetPosition)) { + Game::logger->Log("ProjectileAttackBehavior", "Unable to read targetPosition from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + }; } auto* targetEntity = EntityManager::Instance()->GetEntity(target); for (auto i = 0u; i < this->m_projectileCount; ++i) { - LWOOBJID projectileId; + LWOOBJID projectileId{}; - bitStream->Read(projectileId); + if (!bitStream->Read(projectileId)) { + Game::logger->Log("ProjectileAttackBehavior", "Unable to read projectileId from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + }; branch.target = target; branch.isProjectile = true; @@ -98,7 +107,7 @@ void ProjectileAttackBehavior::Calculate(BehaviorContext* context, RakNet::BitSt for (auto i = 0u; i < this->m_projectileCount; ++i) { auto id = static_cast(ObjectIDManager::Instance()->GenerateObjectID()); - id = GeneralUtils::SetBit(id, OBJECT_BIT_CLIENT); + id = GeneralUtils::SetBit(id, OBJECT_BIT_SPAWNED); bitStream->Write(id); diff --git a/dGame/dBehaviors/StunBehavior.cpp b/dGame/dBehaviors/StunBehavior.cpp index 59a8144..065b722 100644 --- a/dGame/dBehaviors/StunBehavior.cpp +++ b/dGame/dBehaviors/StunBehavior.cpp @@ -14,8 +14,11 @@ void StunBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStream return; } - bool blocked; - bitStream->Read(blocked); + bool blocked{}; + if (!bitStream->Read(blocked)) { + Game::logger->Log("StunBehavior", "Unable to read blocked from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + }; auto* target = EntityManager::Instance()->GetEntity(branch.target); diff --git a/dGame/dBehaviors/SwitchBehavior.cpp b/dGame/dBehaviors/SwitchBehavior.cpp index 78271b9..f6eb3ff 100644 --- a/dGame/dBehaviors/SwitchBehavior.cpp +++ b/dGame/dBehaviors/SwitchBehavior.cpp @@ -10,7 +10,10 @@ void SwitchBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStre auto state = true; if (this->m_imagination > 0 || !this->m_isEnemyFaction) { - bitStream->Read(state); + if (!bitStream->Read(state)) { + Game::logger->Log("SwitchBehavior", "Unable to read state from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + return; + }; } auto* entity = EntityManager::Instance()->GetEntity(context->originator); diff --git a/dGame/dBehaviors/SwitchMultipleBehavior.cpp b/dGame/dBehaviors/SwitchMultipleBehavior.cpp index 946e5e0..e92393f 100644 --- a/dGame/dBehaviors/SwitchMultipleBehavior.cpp +++ b/dGame/dBehaviors/SwitchMultipleBehavior.cpp @@ -9,10 +9,12 @@ #include "EntityManager.h" -void SwitchMultipleBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bit_stream, BehaviorBranchContext branch) { - float value; +void SwitchMultipleBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) { + float value{}; - bit_stream->Read(value); + if (!bitStream->Read(value)) { + Game::logger->Log("SwitchMultipleBehavior", "Unable to read value from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + }; uint32_t trigger = 0; @@ -30,10 +32,10 @@ void SwitchMultipleBehavior::Handle(BehaviorContext* context, RakNet::BitStream* auto* behavior = this->m_behaviors.at(trigger).second; - behavior->Handle(context, bit_stream, branch); + behavior->Handle(context, bitStream, branch); } -void SwitchMultipleBehavior::Calculate(BehaviorContext* context, RakNet::BitStream* bit_stream, BehaviorBranchContext branch) { +void SwitchMultipleBehavior::Calculate(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) { // TODO } diff --git a/dGame/dBehaviors/TacArcBehavior.cpp b/dGame/dBehaviors/TacArcBehavior.cpp index 8789f9b..38efdea 100644 --- a/dGame/dBehaviors/TacArcBehavior.cpp +++ b/dGame/dBehaviors/TacArcBehavior.cpp @@ -20,12 +20,16 @@ void TacArcBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStre bool hit = false; - bitStream->Read(hit); + if (!bitStream->Read(hit)) { + Game::logger->Log("TacArcBehavior", "Unable to read hit from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + }; if (this->m_checkEnv) { bool blocked = false; - bitStream->Read(blocked); + if (!bitStream->Read(blocked)) { + Game::logger->Log("TacArcBehavior", "Unable to read blocked from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + }; if (blocked) { this->m_blockedAction->Handle(context, bitStream, branch); @@ -37,7 +41,9 @@ void TacArcBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStre if (hit) { uint32_t count = 0; - bitStream->Read(count); + if (!bitStream->Read(count)) { + Game::logger->Log("TacArcBehavior", "Unable to read count from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + }; if (count > m_maxTargets && m_maxTargets > 0) { count = m_maxTargets; @@ -46,9 +52,11 @@ void TacArcBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bitStre std::vector targets; for (auto i = 0u; i < count; ++i) { - LWOOBJID id; + LWOOBJID id{}; - bitStream->Read(id); + if (!bitStream->Read(id)) { + Game::logger->Log("TacArcBehavior", "Unable to read id from bitStream, aborting Handle! %i", bitStream->GetNumberOfUnreadBits()); + }; targets.push_back(id); }