From 213c3c37b0ac03d9ee6ffeb76e69e4f8dc5617e9 Mon Sep 17 00:00:00 2001 From: David Markowitz <39972741+EmosewaMC@users.noreply.github.com> Date: Thu, 15 Dec 2022 22:10:58 -0800 Subject: [PATCH 1/2] Fix crash in BasicAttackBehavior (#862) * 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 * Remove case 2 * Update SkillComponent.cpp --- dGame/dBehaviors/BasicAttackBehavior.cpp | 85 ++++++++++++++++-------- dGame/dBehaviors/BasicAttackBehavior.h | 14 ++-- dGame/dComponents/SkillComponent.cpp | 29 +------- 3 files changed, 65 insertions(+), 63 deletions(-) diff --git a/dGame/dBehaviors/BasicAttackBehavior.cpp b/dGame/dBehaviors/BasicAttackBehavior.cpp index fe773f36..f166f00c 100644 --- a/dGame/dBehaviors/BasicAttackBehavior.cpp +++ b/dGame/dBehaviors/BasicAttackBehavior.cpp @@ -14,43 +14,65 @@ void BasicAttackBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bi auto* destroyableComponent = entity->GetComponent(); if (destroyableComponent != nullptr) { PlayFx(u"onhit", entity->GetObjectID()); - destroyableComponent->Damage(this->m_maxDamage, context->originator, context->skillID); + destroyableComponent->Damage(this->m_MaxDamage, context->originator, context->skillID); } - this->m_onSuccess->Handle(context, bitStream, branch); + this->m_OnSuccess->Handle(context, bitStream, branch); return; } bitStream->AlignReadToByteBoundary(); - uint16_t allocatedBits; - bitStream->Read(allocatedBits); - + uint16_t allocatedBits{}; + if (!bitStream->Read(allocatedBits) || allocatedBits == 0) { + Game::logger->LogDebug("BasicAttackBehavior", "No allocated bits"); + return; + } + Game::logger->LogDebug("BasicAttackBehavior", "Number of allocated bits %i", allocatedBits); const auto baseAddress = bitStream->GetReadOffset(); - if (bitStream->ReadBit()) { // Blocked + bool isBlocked{}; + bool isImmune{}; + bool isSuccess{}; + + if (!bitStream->Read(isBlocked)) { + Game::logger->LogDebug("BasicAttackBehavior", "Unable to read isBlocked"); return; } - if (bitStream->ReadBit()) { // Immune + if (isBlocked) return; + + if (!bitStream->Read(isImmune)) { + Game::logger->LogDebug("BasicAttackBehavior", "Unable to read isImmune"); return; } - if (bitStream->ReadBit()) { // Success - uint32_t unknown; - bitStream->Read(unknown); + if (isImmune) return; - uint32_t damageDealt; - bitStream->Read(damageDealt); + if (bitStream->Read(isSuccess) && isSuccess) { // Success + uint32_t unknown{}; + if (!bitStream->Read(unknown)) { + Game::logger->LogDebug("BasicAttackBehavior", "Unable to read unknown"); + return; + } + + uint32_t damageDealt{}; + if (!bitStream->Read(damageDealt)) { + Game::logger->LogDebug("BasicAttackBehavior", "Unable to read damageDealt"); + return; + } // A value that's too large may be a cheating attempt, so we set it to MIN too - if (damageDealt > this->m_maxDamage || damageDealt < this->m_minDamage) { - damageDealt = this->m_minDamage; + if (damageDealt > this->m_MaxDamage || damageDealt < this->m_MinDamage) { + damageDealt = this->m_MinDamage; } auto* entity = EntityManager::Instance()->GetEntity(branch.target); - bool died; - bitStream->Read(died); + bool died{}; + if (!bitStream->Read(died)) { + Game::logger->LogDebug("BasicAttackBehavior", "Unable to read died"); + return; + } if (entity != nullptr) { auto* destroyableComponent = entity->GetComponent(); @@ -61,15 +83,18 @@ void BasicAttackBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bi } } - uint8_t successState; - bitStream->Read(successState); + uint8_t successState{}; + if (!bitStream->Read(successState)) { + Game::logger->LogDebug("BasicAttackBehavior", "Unable to read success state"); + return; + } switch (successState) { case 1: - this->m_onSuccess->Handle(context, bitStream, branch); + this->m_OnSuccess->Handle(context, bitStream, branch); break; default: - Game::logger->Log("BasicAttackBehavior", "Unknown success state (%i)!", successState); + Game::logger->LogDebug("BasicAttackBehavior", "Unknown success state (%i)!", successState); break; } @@ -79,7 +104,7 @@ void BasicAttackBehavior::Handle(BehaviorContext* context, RakNet::BitStream* bi void BasicAttackBehavior::Calculate(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) { auto* self = EntityManager::Instance()->GetEntity(context->originator); if (self == nullptr) { - Game::logger->Log("BasicAttackBehavior", "Invalid self entity (%llu)!", context->originator); + Game::logger->LogDebug("BasicAttackBehavior", "Invalid self entity (%llu)!", context->originator); return; } @@ -99,7 +124,7 @@ void BasicAttackBehavior::Calculate(BehaviorContext* context, RakNet::BitStream* uint32_t unknown3 = 0; bitStream->Write(unknown3); - auto damage = this->m_minDamage; + auto damage = this->m_MinDamage; auto* entity = EntityManager::Instance()->GetEntity(branch.target); if (entity == nullptr) { @@ -124,10 +149,10 @@ void BasicAttackBehavior::Calculate(BehaviorContext* context, RakNet::BitStream* switch (successState) { case 1: - this->m_onSuccess->Calculate(context, bitStream, branch); + this->m_OnSuccess->Calculate(context, bitStream, branch); break; default: - Game::logger->Log("BasicAttackBehavior", "Unknown success state (%i)!", successState); + Game::logger->LogDebug("BasicAttackBehavior", "Unknown success state (%i)!", successState); break; } @@ -140,11 +165,13 @@ void BasicAttackBehavior::Calculate(BehaviorContext* context, RakNet::BitStream* } void BasicAttackBehavior::Load() { - this->m_minDamage = GetInt("min damage"); - if (this->m_minDamage == 0) this->m_minDamage = 1; + this->m_MinDamage = GetInt("min damage"); + if (this->m_MinDamage == 0) this->m_MinDamage = 1; - this->m_maxDamage = GetInt("max damage"); - if (this->m_maxDamage == 0) this->m_maxDamage = 1; + this->m_MaxDamage = GetInt("max damage"); + if (this->m_MaxDamage == 0) this->m_MaxDamage = 1; - this->m_onSuccess = GetAction("on_success"); + this->m_OnSuccess = GetAction("on_success"); + + this->m_OnFailArmor = GetAction("on_fail_armor"); } diff --git a/dGame/dBehaviors/BasicAttackBehavior.h b/dGame/dBehaviors/BasicAttackBehavior.h index 8e23d443..9c08141c 100644 --- a/dGame/dBehaviors/BasicAttackBehavior.h +++ b/dGame/dBehaviors/BasicAttackBehavior.h @@ -4,12 +4,6 @@ class BasicAttackBehavior final : public Behavior { public: - uint32_t m_minDamage; - - uint32_t m_maxDamage; - - Behavior* m_onSuccess; - explicit BasicAttackBehavior(const uint32_t behaviorId) : Behavior(behaviorId) { } @@ -18,4 +12,12 @@ public: void Calculate(BehaviorContext* context, RakNet::BitStream* bitStream, BehaviorBranchContext branch) override; void Load() override; +private: + uint32_t m_MinDamage; + + uint32_t m_MaxDamage; + + Behavior* m_OnSuccess; + + Behavior* m_OnFailArmor; }; diff --git a/dGame/dComponents/SkillComponent.cpp b/dGame/dComponents/SkillComponent.cpp index f7e8e7d9..c749dbaf 100644 --- a/dGame/dComponents/SkillComponent.cpp +++ b/dGame/dComponents/SkillComponent.cpp @@ -319,34 +319,7 @@ void SkillComponent::CalculateUpdate(const float deltaTime) { const auto distance = Vector3::DistanceSquared(targetPosition, closestPoint); if (distance > 3 * 3) { - /* - if (entry.TrackTarget && distance <= entry.TrackRadius) - { - const auto rotation = NiQuaternion::LookAtUnlocked(position, targetPosition); - - const auto speed = entry.Velocity.Length(); - - const auto homingTarget = rotation.GetForwardVector() * speed; - - Vector3 homing; - - // Move towards - - const auto difference = homingTarget - entry.Velocity; - const auto mag = difference.Length(); - if (mag <= speed || mag == 0) - { - homing = homingTarget; - } - else - { - entry.Velocity + homingTarget / mag * speed; - } - - entry.Velocity = homing; - } - */ - + // TODO There is supposed to be an implementation for homing projectiles here continue; } From 1ed3af63b9f2be42ecd1c7966c65a6ee604f90a7 Mon Sep 17 00:00:00 2001 From: Daniel Seiler Date: Fri, 16 Dec 2022 07:32:36 +0100 Subject: [PATCH 2/2] Log some recvfrom errors on linux (#885) --- thirdparty/raknet/Source/SocketLayer.cpp | 51 +++++++++++++++++++++++- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/thirdparty/raknet/Source/SocketLayer.cpp b/thirdparty/raknet/Source/SocketLayer.cpp index b6af8751..6cb4f7c6 100644 --- a/thirdparty/raknet/Source/SocketLayer.cpp +++ b/thirdparty/raknet/Source/SocketLayer.cpp @@ -417,8 +417,14 @@ int SocketLayer::RecvFrom( const SOCKET s, RakPeer *rakPeer, int *errorCode, uns assert( 0 ); #endif - *errorCode = -1; - return -1; + //*errorCode = -1; +#ifdef __linux__ + char str[INET_ADDRSTRLEN]; + inet_ntop(AF_INET, &(sa.sin_addr), str, INET_ADDRSTRLEN); + printf("[RakNet] SocketLayer::RecvFrom: empty datagram from %s", str); +#endif + //return -1; + return 0; } if ( len > 0 ) @@ -479,6 +485,47 @@ int SocketLayer::RecvFrom( const SOCKET s, RakPeer *rakPeer, int *errorCode, uns } #endif } +#elif defined(__linux__) + if (len < -1) + { + printf("[RakNet] SocketLayer::RecvFrom: Unexpected return value."); + return -1; + } + + int local_errno = errno; + if (local_errno == EAGAIN || local_errno == EWOULDBLOCK) + { + return 0; // no data + } + + if (local_errno == EINTR) + { + printf("[RakNet] SocketLayer::RecvFrom: The receive was interrupted by delivery of a signal before any data were available."); + return 0; // log, but ignore + } + + char str[INET_ADDRSTRLEN]; + inet_ntop(AF_INET, &(sa.sin_addr), str, INET_ADDRSTRLEN); + printf("[RakNet] SocketLayer::RecvFrom: Error receiving data from %s", str); + + switch (local_errno) { + case EINVAL: + printf("[RakNet] SocketLayer::RecvFrom: Invalid argument passed."); break; + case ENOMEM: + case ENOBUFS: + printf("[RakNet] SocketLayer::RecvFrom: Could not allocate memory for recvmsg()."); break; + case EFAULT: + printf("[RakNet] SocketLayer::RecvFrom: The receive buffer pointer(s) point outside the process's address space."); break; + case EBADF: + printf("[RakNet] SocketLayer::RecvFrom: The argument sockfd is an invalid descriptor."); break; + case ENOTSOCK: + printf("[RakNet] SocketLayer::RecvFrom: The argument sockfd does not refer to a socket."); break; + case EPIPE: + printf("[RakNet] SocketLayer::RecvFrom: The connection was unexpectedly closed or shut down by the other end. "); break; + default: + printf("[RakNet] SocketLayer::RecvFrom: Unknown Error %d", local_errno); break; + } + return -1; #endif }