From 16aeb84cd35996a6b41f10cbc48a677eeccc911c Mon Sep 17 00:00:00 2001 From: Tiger Wang Date: Sat, 2 Jan 2021 13:50:34 +0000 Subject: Fix potential destruction crashes (#5095) * Fix potential destruction crashes * Fix destructors accessing destroyted objects * Fix cPlayer not destroying windows (Destroyed never called) * Tentatively fixes #4608, fixes #3236, fixes #3262 - Remove cEntity::Destroyed() and replace with cEntity::OnRemoveFromWorld() * Add missing call to OnRemoveFromWorld --- src/Entities/Entity.cpp | 71 ++++++++------------ src/Entities/Entity.h | 9 +-- src/Entities/EntityEffect.h | 2 +- src/Entities/Minecart.cpp | 157 ++++++++++++++++++++++++-------------------- src/Entities/Minecart.h | 28 +++++--- src/Entities/Pawn.cpp | 29 +++----- src/Entities/Pawn.h | 7 +- src/Entities/Player.cpp | 3 +- src/Entities/Player.h | 4 +- 9 files changed, 148 insertions(+), 162 deletions(-) (limited to 'src/Entities') diff --git a/src/Entities/Entity.cpp b/src/Entities/Entity.cpp index 579541dd3..438117650 100644 --- a/src/Entities/Entity.cpp +++ b/src/Entities/Entity.cpp @@ -80,32 +80,6 @@ cEntity::cEntity(eEntityType a_EntityType, Vector3d a_Pos, double a_Width, doubl -cEntity::~cEntity() -{ - /* - // DEBUG: - FLOGD("Deleting entity {0} at pos {1:.2f} ~ [{2}, {3}]; ptr {4}", - m_UniqueID, - m_Pos, - GetChunkX(), GetChunkZ(), - static_cast(this) - ); - */ - - if (m_AttachedTo != nullptr) - { - Detach(); - } - if (m_Attachee != nullptr) - { - m_Attachee->Detach(); - } -} - - - - - const char * cEntity::GetClass(void) const { return "cEntity"; @@ -174,7 +148,22 @@ void cEntity::OnAddToWorld(cWorld & a_World) void cEntity::OnRemoveFromWorld(cWorld & a_World) { - RemoveAllLeashedMobs(); + // Remove all mobs from the leashed list of mobs: + while (!m_LeashedMobs.empty()) + { + m_LeashedMobs.front()->Unleash(false, true); + } + + if (m_AttachedTo != nullptr) + { + Detach(); + } + + if (m_Attachee != nullptr) + { + m_Attachee->Detach(); + } + a_World.BroadcastDestroyEntity(*this); } @@ -244,7 +233,6 @@ void cEntity::Destroy() // Also, not storing the returned pointer means automatic destruction VERIFY(a_World.RemoveEntity(*this)); }); - Destroyed(); } @@ -813,10 +801,17 @@ void cEntity::KilledBy(TakeDamageInfo & a_TDI) cRoot::Get()->GetPluginManager()->CallHookKilled(*this, a_TDI, emptystring); } - // Drop loot: - cItems Drops; - GetDrops(Drops, a_TDI.Attacker); - m_World->SpawnItemPickups(Drops, GetPosX(), GetPosY(), GetPosZ()); + // Drop loot, unless the attacker was a creative mode player: + if ( + (a_TDI.Attacker == nullptr) || + !a_TDI.Attacker->IsPlayer() || + !static_cast(a_TDI.Attacker)->IsGameModeCreative() + ) + { + cItems Drops; + GetDrops(Drops, a_TDI.Attacker); + m_World->SpawnItemPickups(Drops, GetPosX(), GetPosY(), GetPosZ()); + } m_World->BroadcastEntityStatus(*this, esGenericDead); } @@ -2328,18 +2323,6 @@ void cEntity::RemoveLeashedMob(cMonster * a_Monster) -void cEntity::RemoveAllLeashedMobs() -{ - while (!m_LeashedMobs.empty()) - { - m_LeashedMobs.front()->Unleash(false, true); - } -} - - - - - void cEntity::BroadcastLeashedMobs() { // If has any mob leashed broadcast every leashed entity to this diff --git a/src/Entities/Entity.h b/src/Entities/Entity.h index 20712e7ea..85cf35661 100644 --- a/src/Entities/Entity.h +++ b/src/Entities/Entity.h @@ -169,7 +169,7 @@ public: cEntity(eEntityType a_EntityType, Vector3d a_Pos, double a_Width, double a_Height); - virtual ~cEntity(); + virtual ~cEntity() = default; /** Spawns the entity in the world; returns true if spawned, false if not (plugin disallowed). Adds the entity to the world. */ @@ -296,7 +296,7 @@ public: // tolua_end /** Destroys the entity, schedules it for memory freeing and broadcasts the DestroyEntity packet */ - virtual void Destroy(); + void Destroy(); // tolua_begin /** Makes this pawn take damage from an attack by a_Attacker. Damage values are calculated automatically and DoTakeDamage() called */ @@ -594,9 +594,6 @@ public: /** Removes a mob from the leashed list of mobs. */ void RemoveLeashedMob(cMonster * a_Monster); - /** Removes all mobs from the leashed list of mobs. */ - void RemoveAllLeashedMobs(); - /** Returs whether the entity has any mob leashed to it. */ bool HasAnyMobLeashed() const { return m_LeashedMobs.size() > 0; } @@ -723,8 +720,6 @@ protected: Should handle degenerate cases such as moving to the same world. */ virtual void DoMoveToWorld(const sWorldChangeInfo & a_WorldChangeInfo); - virtual void Destroyed(void) {} // Called after the entity has been destroyed - /** Applies friction to an entity @param a_Speed The speed vector to apply changes to @param a_SlowdownMultiplier The factor to reduce the speed by */ diff --git a/src/Entities/EntityEffect.h b/src/Entities/EntityEffect.h index eebf0e3c8..b4e01d8d9 100644 --- a/src/Entities/EntityEffect.h +++ b/src/Entities/EntityEffect.h @@ -73,7 +73,7 @@ public: @param a_OtherEffect The other effect to copy */ cEntityEffect & operator =(cEntityEffect a_OtherEffect); - virtual ~cEntityEffect(void) {} + virtual ~cEntityEffect(void) = default; /** Creates a pointer to the proper entity effect from the effect type @warning This function creates raw pointers that must be manually managed. diff --git a/src/Entities/Minecart.cpp b/src/Entities/Minecart.cpp index 59d369c5b..fbe4f756e 100644 --- a/src/Entities/Minecart.cpp +++ b/src/Entities/Minecart.cpp @@ -1203,7 +1203,6 @@ bool cMinecart::DoTakeDamage(TakeDamageInfo & TDI) { if ((TDI.Attacker != nullptr) && TDI.Attacker->IsPlayer() && static_cast(TDI.Attacker)->IsGameModeCreative()) { - Destroy(); TDI.FinalDamage = GetMaxHealth(); // Instant hit for creative SetInvulnerableTicks(0); return Super::DoTakeDamage(TDI); // No drops for creative @@ -1217,43 +1216,32 @@ bool cMinecart::DoTakeDamage(TakeDamageInfo & TDI) m_World->BroadcastEntityMetadata(*this); - if (GetHealth() <= 0) - { - Destroy(); + return true; +} - cItems Drops; - switch (m_Payload) - { - case mpNone: - { - Drops.push_back(cItem(E_ITEM_MINECART, 1, 0)); - break; - } - case mpChest: - { - Drops.push_back(cItem(E_ITEM_CHEST_MINECART, 1, 0)); - break; - } - case mpFurnace: - { - Drops.push_back(cItem(E_ITEM_FURNACE_MINECART, 1, 0)); - break; - } - case mpTNT: - { - Drops.push_back(cItem(E_ITEM_MINECART_WITH_TNT, 1, 0)); - break; - } - case mpHopper: - { - Drops.push_back(cItem(E_ITEM_MINECART_WITH_HOPPER, 1, 0)); - break; - } - } - m_World->SpawnItemPickups(Drops, GetPosX(), GetPosY(), GetPosZ()); + + + +void cMinecart::KilledBy(TakeDamageInfo & a_TDI) +{ + Super::KilledBy(a_TDI); + + Destroy(); +} + + + + + +void cMinecart::OnRemoveFromWorld(cWorld & a_World) +{ + if (m_bIsOnDetectorRail) + { + m_World->SetBlock(m_DetectorRailPosition.x, m_DetectorRailPosition.y, m_DetectorRailPosition.z, E_BLOCK_DETECTOR_RAIL, m_World->GetBlockMeta(m_DetectorRailPosition) & 0x07); } - return true; + + Super::OnRemoveFromWorld(a_World); } @@ -1314,18 +1302,6 @@ void cMinecart::DoSetSpeed(double a_SpeedX, double a_SpeedY, double a_SpeedZ) -void cMinecart::Destroyed() -{ - if (m_bIsOnDetectorRail) - { - m_World->SetBlock(m_DetectorRailPosition.x, m_DetectorRailPosition.y, m_DetectorRailPosition.z, E_BLOCK_DETECTOR_RAIL, m_World->GetBlockMeta(m_DetectorRailPosition) & 0x07); - } -} - - - - - //////////////////////////////////////////////////////////////////////////////// // cRideableMinecart: @@ -1340,6 +1316,15 @@ cRideableMinecart::cRideableMinecart(Vector3d a_Pos, const cItem & a_Content, in +void cRideableMinecart::GetDrops(cItems & a_Drops, cEntity * a_Killer) +{ + a_Drops.emplace_back(E_ITEM_MINECART); +} + + + + + void cRideableMinecart::OnRightClicked(cPlayer & a_Player) { Super::OnRightClicked(a_Player); @@ -1386,6 +1371,31 @@ cMinecartWithChest::cMinecartWithChest(Vector3d a_Pos): +void cMinecartWithChest::GetDrops(cItems & a_Drops, cEntity * a_Killer) +{ + m_Contents.CopyToItems(a_Drops); + a_Drops.emplace_back(E_ITEM_CHEST_MINECART); +} + + + + + +void cMinecartWithChest::OnRemoveFromWorld(cWorld & a_World) +{ + const auto Window = GetWindow(); + if (Window != nullptr) + { + Window->OwnerDestroyed(); + } + + Super::OnRemoveFromWorld(a_World); +} + + + + + void cMinecartWithChest::OnRightClicked(cPlayer & a_Player) { // If the window is not created, open it anew: @@ -1419,32 +1429,6 @@ void cMinecartWithChest::OpenNewWindow() -void cMinecartWithChest::Destroyed() -{ - if (GetWindow() != nullptr) - { - GetWindow()->OwnerDestroyed(); - } - cItems Pickups; - m_Contents.CopyToItems(Pickups); - - - // Schedule the pickups creation for the next world tick - // This avoids a deadlock when terminating the world - // Note that the scheduled task may be run when this object is no longer valid, we need to store everything in the task's captured variables - auto posX = GetPosX(); - auto posY = GetPosY() + 1; - auto posZ = GetPosZ(); - GetWorld()->ScheduleTask(1, [Pickups, posX, posY, posZ](cWorld & World) - { - World.SpawnItemPickups(Pickups, posX, posY, posZ, 4); - }); -} - - - - - //////////////////////////////////////////////////////////////////////////////// // cMinecartWithFurnace: @@ -1459,6 +1443,15 @@ cMinecartWithFurnace::cMinecartWithFurnace(Vector3d a_Pos): +void cMinecartWithFurnace::GetDrops(cItems & a_Drops, cEntity * a_Killer) +{ + a_Drops.emplace_back(E_ITEM_FURNACE_MINECART); +} + + + + + void cMinecartWithFurnace::OnRightClicked(cPlayer & a_Player) { if (a_Player.GetEquippedItem().m_ItemType == E_ITEM_COAL) @@ -1526,6 +1519,15 @@ cMinecartWithTNT::cMinecartWithTNT(Vector3d a_Pos): +void cMinecartWithTNT::GetDrops(cItems & a_Drops, cEntity * a_Killer) +{ + a_Drops.emplace_back(E_ITEM_MINECART_WITH_TNT); +} + + + + + //////////////////////////////////////////////////////////////////////////////// // cMinecartWithHopper: @@ -1536,3 +1538,12 @@ cMinecartWithHopper::cMinecartWithHopper(Vector3d a_Pos): // TODO: Make it suck up blocks and travel further than any other cart and physics and put and take blocks // AND AVARYTHING!! + + + + + +void cMinecartWithHopper::GetDrops(cItems & a_Drops, cEntity * a_Killer) +{ + a_Drops.emplace_back(E_ITEM_MINECART_WITH_HOPPER); +} diff --git a/src/Entities/Minecart.h b/src/Entities/Minecart.h index ad3b3d40d..73011b7b5 100644 --- a/src/Entities/Minecart.h +++ b/src/Entities/Minecart.h @@ -40,12 +40,12 @@ public: virtual void SpawnOn(cClientHandle & a_ClientHandle) override; virtual void HandlePhysics(std::chrono::milliseconds a_Dt, cChunk & a_Chunk) override; virtual bool DoTakeDamage(TakeDamageInfo & TDI) override; - virtual void Destroyed() override; + virtual void KilledBy(TakeDamageInfo & a_TDI) override; + virtual void OnRemoveFromWorld(cWorld & a_World) override; int LastDamage(void) const { return m_LastDamage; } ePayload GetPayload(void) const { return m_Payload; } - protected: ePayload m_Payload; @@ -99,7 +99,7 @@ protected: -class cRideableMinecart : +class cRideableMinecart final : public cMinecart { using Super = cMinecart; @@ -114,6 +114,7 @@ public: int GetBlockHeight(void) const {return m_Height;} // cEntity overrides: + virtual void GetDrops(cItems & a_Drops, cEntity * a_Killer = nullptr) override; virtual void OnRightClicked(cPlayer & a_Player) override; protected: @@ -127,7 +128,7 @@ protected: -class cMinecartWithChest : +class cMinecartWithChest final : public cMinecart, public cItemGrid::cListener, public cEntityWindowOwner @@ -154,7 +155,6 @@ protected: cItemGrid m_Contents; void OpenNewWindow(void); - virtual void Destroyed() override; // cItemGrid::cListener overrides: virtual void OnSlotChanged(cItemGrid * a_Grid, int a_SlotNum) override @@ -173,6 +173,8 @@ protected: } // cEntity overrides: + virtual void GetDrops(cItems & a_Drops, cEntity * a_Killer = nullptr) override; + virtual void OnRemoveFromWorld(cWorld & a_World) override; virtual void OnRightClicked(cPlayer & a_Player) override; } ; @@ -180,7 +182,7 @@ protected: -class cMinecartWithFurnace : +class cMinecartWithFurnace final : public cMinecart { using Super = cMinecart; @@ -192,6 +194,7 @@ public: cMinecartWithFurnace(Vector3d a_Pos); // cEntity overrides: + virtual void GetDrops(cItems & a_Drops, cEntity * a_Killer = nullptr) override; virtual void OnRightClicked(cPlayer & a_Player) override; virtual void Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk) override; @@ -213,22 +216,27 @@ private: -class cMinecartWithTNT : +class cMinecartWithTNT final : public cMinecart { using Super = cMinecart; public: + CLASS_PROTODEF(cMinecartWithTNT) cMinecartWithTNT(Vector3d a_Pos); + +private: + + virtual void GetDrops(cItems & a_Drops, cEntity * a_Killer = nullptr) override; } ; -class cMinecartWithHopper : +class cMinecartWithHopper final : public cMinecart { using Super = cMinecart; @@ -238,4 +246,8 @@ public: CLASS_PROTODEF(cMinecartWithHopper) cMinecartWithHopper(Vector3d a_Pos); + +private: + + virtual void GetDrops(cItems & a_Drops, cEntity * a_Killer = nullptr) override; } ; diff --git a/src/Entities/Pawn.cpp b/src/Entities/Pawn.cpp index 9857fa29c..35efe05ce 100644 --- a/src/Entities/Pawn.cpp +++ b/src/Entities/Pawn.cpp @@ -28,25 +28,6 @@ cPawn::cPawn(eEntityType a_EntityType, double a_Width, double a_Height) : -cPawn::~cPawn() -{ - ASSERT(m_TargetingMe.size() == 0); -} - - - - - -void cPawn::Destroyed() -{ - StopEveryoneFromTargetingMe(); - Super::Destroyed(); -} - - - - - void cPawn::Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk) { std::vector EffectsToTick; @@ -453,6 +434,16 @@ void cPawn::HandleFalling(void) +void cPawn::OnRemoveFromWorld(cWorld & a_World) +{ + StopEveryoneFromTargetingMe(); + Super::OnRemoveFromWorld(a_World); +} + + + + + void cPawn::StopEveryoneFromTargetingMe() { std::vector::iterator i = m_TargetingMe.begin(); diff --git a/src/Entities/Pawn.h b/src/Entities/Pawn.h index f4b1cbbbf..1a9285b77 100644 --- a/src/Entities/Pawn.h +++ b/src/Entities/Pawn.h @@ -23,8 +23,6 @@ public: CLASS_PROTODEF(cPawn) cPawn(eEntityType a_EntityType, double a_Width, double a_Height); - virtual ~cPawn() override; - virtual void Destroyed() override; virtual void Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk) override; virtual void KilledBy(TakeDamageInfo & a_TDI) override; @@ -33,6 +31,7 @@ public: virtual bool IsInvisible() const override; virtual void HandleAir(void) override; virtual void HandleFalling(void); + virtual void OnRemoveFromWorld(cWorld & a_World) override; /** Tells all pawns which are targeting us to stop targeting us. */ void StopEveryoneFromTargetingMe(); @@ -85,7 +84,3 @@ private: /** A list of all monsters that are targeting this pawn. */ std::vector m_TargetingMe; } ; // tolua_export - - - - diff --git a/src/Entities/Player.cpp b/src/Entities/Player.cpp index fb0cdf271..391a5ce71 100644 --- a/src/Entities/Player.cpp +++ b/src/Entities/Player.cpp @@ -280,10 +280,9 @@ cPlayer::~cPlayer(void) -void cPlayer::Destroyed() +void cPlayer::OnRemoveFromWorld(cWorld & a_World) { CloseWindow(false); - Super::Destroyed(); } diff --git a/src/Entities/Player.h b/src/Entities/Player.h index bdcacd608..1e7a17e4f 100644 --- a/src/Entities/Player.h +++ b/src/Entities/Player.h @@ -54,6 +54,8 @@ public: virtual ~cPlayer() override; + virtual void OnRemoveFromWorld(cWorld & a_World) override; + virtual void SpawnOn(cClientHandle & a_Client) override; virtual void Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk) override; @@ -784,8 +786,6 @@ protected: /** Sets the speed and sends it to the client, so that they are forced to move so. */ virtual void DoSetSpeed(double a_SpeedX, double a_SpeedY, double a_SpeedZ) override; - virtual void Destroyed(void) override; - /** Filters out damage for creative mode / friendly fire */ virtual bool DoTakeDamage(TakeDamageInfo & TDI) override; -- cgit v1.2.3