1
0

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
This commit is contained in:
Tiger Wang
2021-01-02 13:50:34 +00:00
committed by GitHub
parent 4656f7486d
commit 16aeb84cd3
33 changed files with 409 additions and 464 deletions

View File

@@ -271,6 +271,20 @@ void cBeaconEntity::CopyFrom(const cBlockEntity & a_Src)
void cBeaconEntity::OnRemoveFromWorld()
{
const auto Window = GetWindow();
if (Window != nullptr)
{
// Tell window its owner is destroyed:
Window->OwnerDestroyed();
}
}
void cBeaconEntity::SendTo(cClientHandle & a_Client)
{
a_Client.SendUpdateBlockEntity(*this);
@@ -316,6 +330,3 @@ bool cBeaconEntity::UsedBy(cPlayer * a_Player)
}
return true;
}

View File

@@ -29,6 +29,7 @@ public: // tolua_export
// cBlockEntity overrides:
virtual void CopyFrom(const cBlockEntity & a_Src) override;
virtual void OnRemoveFromWorld() override;
virtual void SendTo(cClientHandle & a_Client) override;
virtual bool Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk) override;
virtual bool UsedBy(cPlayer * a_Player) override;

View File

@@ -28,56 +28,52 @@
void cBlockEntity::SetPos(Vector3i a_NewPos)
cBlockEntity::cBlockEntity(const BLOCKTYPE a_BlockType, const NIBBLETYPE a_BlockMeta, const Vector3i a_Pos, cWorld * const a_World) :
m_Pos(a_Pos),
m_RelX(a_Pos.x - cChunkDef::Width * FAST_FLOOR_DIV(a_Pos.x, cChunkDef::Width)),
m_RelZ(a_Pos.z - cChunkDef::Width * FAST_FLOOR_DIV(a_Pos.z, cChunkDef::Width)),
m_BlockType(a_BlockType),
m_BlockMeta(a_BlockMeta),
m_World(a_World)
{
ASSERT(m_World == nullptr); // Cannot move block entities that represent world blocks (only use this for cBlockArea's BEs)
m_Pos = a_NewPos;
}
bool cBlockEntity::IsBlockEntityBlockType(BLOCKTYPE a_BlockType)
OwnedBlockEntity cBlockEntity::Clone(const Vector3i a_Pos)
{
switch (a_BlockType)
{
case E_BLOCK_BEACON:
case E_BLOCK_BED:
case E_BLOCK_BREWING_STAND:
case E_BLOCK_CHEST:
case E_BLOCK_COMMAND_BLOCK:
case E_BLOCK_DISPENSER:
case E_BLOCK_DROPPER:
case E_BLOCK_ENCHANTMENT_TABLE:
case E_BLOCK_ENDER_CHEST:
case E_BLOCK_END_PORTAL:
case E_BLOCK_FLOWER_POT:
case E_BLOCK_FURNACE:
case E_BLOCK_HEAD:
case E_BLOCK_HOPPER:
case E_BLOCK_JUKEBOX:
case E_BLOCK_LIT_FURNACE:
case E_BLOCK_MOB_SPAWNER:
case E_BLOCK_NOTE_BLOCK:
case E_BLOCK_SIGN_POST:
case E_BLOCK_TRAPPED_CHEST:
case E_BLOCK_WALLSIGN:
{
return true;
}
default:
{
return false;
}
}
auto res = CreateByBlockType(m_BlockType, m_BlockMeta, a_Pos, nullptr);
res->CopyFrom(*this);
return res;
}
OwnedBlockEntity cBlockEntity::CreateByBlockType(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World)
cItems cBlockEntity::ConvertToPickups() const
{
return {};
}
void cBlockEntity::CopyFrom(const cBlockEntity & a_Src)
{
// Nothing to copy, but check that we're copying the right entity:
ASSERT(m_BlockType == a_Src.m_BlockType);
ASSERT(m_BlockMeta == a_Src.m_BlockMeta);
}
OwnedBlockEntity cBlockEntity::CreateByBlockType(const BLOCKTYPE a_BlockType, const NIBBLETYPE a_BlockMeta, const Vector3i a_Pos, cWorld * const a_World)
{
switch (a_BlockType)
{
@@ -117,29 +113,82 @@ OwnedBlockEntity cBlockEntity::CreateByBlockType(BLOCKTYPE a_BlockType, NIBBLETY
OwnedBlockEntity cBlockEntity::Clone(Vector3i a_Pos)
void cBlockEntity::Destroy()
{
auto res = CreateByBlockType(m_BlockType, m_BlockMeta, a_Pos, nullptr);
res->CopyFrom(*this);
return res;
}
cItems cBlockEntity::ConvertToPickups() const
bool cBlockEntity::IsBlockEntityBlockType(const BLOCKTYPE a_BlockType)
{
return {};
switch (a_BlockType)
{
case E_BLOCK_BEACON:
case E_BLOCK_BED:
case E_BLOCK_BREWING_STAND:
case E_BLOCK_CHEST:
case E_BLOCK_COMMAND_BLOCK:
case E_BLOCK_DISPENSER:
case E_BLOCK_DROPPER:
case E_BLOCK_ENCHANTMENT_TABLE:
case E_BLOCK_ENDER_CHEST:
case E_BLOCK_END_PORTAL:
case E_BLOCK_FLOWER_POT:
case E_BLOCK_FURNACE:
case E_BLOCK_HEAD:
case E_BLOCK_HOPPER:
case E_BLOCK_JUKEBOX:
case E_BLOCK_LIT_FURNACE:
case E_BLOCK_MOB_SPAWNER:
case E_BLOCK_NOTE_BLOCK:
case E_BLOCK_SIGN_POST:
case E_BLOCK_TRAPPED_CHEST:
case E_BLOCK_WALLSIGN:
{
return true;
}
default:
{
return false;
}
}
}
void cBlockEntity::CopyFrom(const cBlockEntity & a_Src)
void cBlockEntity::OnRemoveFromWorld()
{
// Nothing to copy, but check that we're copying the right entity:
ASSERT(m_BlockType == a_Src.m_BlockType);
ASSERT(m_BlockMeta == a_Src.m_BlockMeta);
}
void cBlockEntity::SetPos(const Vector3i a_NewPos)
{
ASSERT(m_World == nullptr); // Cannot move block entities that represent world blocks (only use this for cBlockArea's BEs)
m_Pos = a_NewPos;
}
void cBlockEntity::SetWorld(cWorld * const a_World)
{
m_World = a_World;
}
bool cBlockEntity::Tick(const std::chrono::milliseconds a_Dt, cChunk & a_Chunk)
{
UNUSED(a_Dt);
return false;
}

View File

@@ -24,41 +24,14 @@ using cBlockEntities = std::unordered_map<size_t, OwnedBlockEntity>;
class cBlockEntity
{
protected:
cBlockEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World) :
m_Pos(a_Pos),
m_RelX(a_Pos.x - cChunkDef::Width * FAST_FLOOR_DIV(a_Pos.x, cChunkDef::Width)),
m_RelZ(a_Pos.z - cChunkDef::Width * FAST_FLOOR_DIV(a_Pos.z, cChunkDef::Width)),
m_BlockType(a_BlockType),
m_BlockMeta(a_BlockMeta),
m_World(a_World)
{
}
cBlockEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World);
public:
// tolua_end
virtual ~cBlockEntity() {} // force a virtual destructor in all descendants
virtual void Destroy() {}
void SetWorld(cWorld * a_World)
{
m_World = a_World;
}
/** Updates the internally stored position.
Note that this should not ever be used for world-contained block entities, it is meant only for when BEs in a cBlockArea are manipulated.
Asserts that the block entity is not assigned to a world. */
void SetPos(Vector3i a_NewPos);
/** Returns true if the specified blocktype is supposed to have an associated block entity. */
static bool IsBlockEntityBlockType(BLOCKTYPE a_BlockType);
/** Creates a new block entity for the specified block type at the specified absolute pos.
If a_World is valid, then the entity is created bound to that world
Returns nullptr for unknown block types. */
static OwnedBlockEntity CreateByBlockType(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World = nullptr);
virtual ~cBlockEntity() = default; // force a virtual destructor in all descendants
/** Makes an exact copy of this block entity, except for its m_World (set to nullptr), and at a new position.
Uses CopyFrom() to copy the properties. */
@@ -73,6 +46,37 @@ public:
Super::CopyFrom(a_Src) to copy the common ones. */
virtual void CopyFrom(const cBlockEntity & a_Src);
/** Creates a new block entity for the specified block type at the specified absolute pos.
If a_World is valid, then the entity is created bound to that world
Returns nullptr for unknown block types. */
static OwnedBlockEntity CreateByBlockType(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World = nullptr);
virtual void Destroy();
/** Returns true if the specified blocktype is supposed to have an associated block entity. */
static bool IsBlockEntityBlockType(BLOCKTYPE a_BlockType);
/** Called when the block entity is removed from a world. */
virtual void OnRemoveFromWorld();
/** Sends the packet defining the block entity to the client specified.
To send to all eligible clients, use cWorld::BroadcastBlockEntity() */
virtual void SendTo(cClientHandle & a_Client) = 0;
/** Updates the internally stored position.
Note that this should not ever be used for world-contained block entities, it is meant only for when BEs in a cBlockArea are manipulated.
Asserts that the block entity is not assigned to a world. */
void SetPos(Vector3i a_NewPos);
void SetWorld(cWorld * a_World);
/** Ticks the entity; returns true if the chunk should be marked as dirty as a result of this ticking. By default does nothing. */
virtual bool Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk);
/** Called when a player uses this entity; should open the UI window.
returns true if the use was successful, return false to use the block as a "normal" block */
virtual bool UsedBy(cPlayer * a_Player) = 0;
// tolua_begin
// Position, in absolute block coordinates:
@@ -95,21 +99,6 @@ public:
// tolua_end
/** Called when a player uses this entity; should open the UI window.
returns true if the use was successful, return false to use the block as a "normal" block */
virtual bool UsedBy( cPlayer * a_Player) = 0;
/** Sends the packet defining the block entity to the client specified.
To send to all eligible clients, use cWorld::BroadcastBlockEntity() */
virtual void SendTo(cClientHandle & a_Client) = 0;
/** Ticks the entity; returns true if the chunk should be marked as dirty as a result of this ticking. By default does nothing. */
virtual bool Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk)
{
UNUSED(a_Dt);
return false;
}
protected:

View File

@@ -13,7 +13,6 @@
cBrewingstandEntity::cBrewingstandEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World):
Super(a_BlockType, a_BlockMeta, a_Pos, ContentsWidth, ContentsHeight, a_World),
m_IsDestroyed(false),
m_IsBrewing(false),
m_TimeBrewed(0),
m_RemainingFuel(0)
@@ -25,30 +24,6 @@ cBrewingstandEntity::cBrewingstandEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_Blo
cBrewingstandEntity::~cBrewingstandEntity()
{
// Tell window its owner is destroyed
cWindow * Window = GetWindow();
if (Window != nullptr)
{
Window->OwnerDestroyed();
}
}
void cBrewingstandEntity::Destroy()
{
m_IsDestroyed = true;
Super::Destroy();
}
void cBrewingstandEntity::CopyFrom(const cBlockEntity & a_Src)
{
Super::CopyFrom(a_Src);
@@ -70,6 +45,20 @@ void cBrewingstandEntity::CopyFrom(const cBlockEntity & a_Src)
void cBrewingstandEntity::OnRemoveFromWorld()
{
const auto Window = GetWindow();
if (Window != nullptr)
{
// Tell window its owner is destroyed:
Window->OwnerDestroyed();
}
}
void cBrewingstandEntity::SendTo(cClientHandle & a_Client)
{
// Nothing needs to be sent
@@ -206,11 +195,6 @@ void cBrewingstandEntity::OnSlotChanged(cItemGrid * a_ItemGrid, int a_SlotNum)
{
Super::OnSlotChanged(a_ItemGrid, a_SlotNum);
if (m_IsDestroyed)
{
return;
}
ASSERT(a_ItemGrid == &m_Contents);
// Check for fuel

View File

@@ -8,6 +8,7 @@
class cClientHandle;
@@ -43,11 +44,9 @@ public:
/** Constructor used for normal operation */
cBrewingstandEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World);
virtual ~cBrewingstandEntity() override;
// cBlockEntity overrides:
virtual void Destroy() override;
// cBlockEntity overrides:
virtual void CopyFrom(const cBlockEntity & a_Src) override;
virtual void OnRemoveFromWorld() override;
virtual void SendTo(cClientHandle & a_Client) override;
virtual bool Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk) override;
virtual bool UsedBy(cPlayer * a_Player) override;
@@ -110,12 +109,8 @@ public:
/** Gets the recipes. Will be called if the brewing stand gets loaded from the world. */
void LoadRecipes(void);
protected:
/** Set to true when the brewing stand entity has been destroyed to prevent the block being set again */
bool m_IsDestroyed;
/** Set to true if the brewing stand is brewing an item */
bool m_IsBrewing;

View File

@@ -32,22 +32,6 @@ cChestEntity::cChestEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector
cChestEntity::~cChestEntity()
{
if (m_Neighbour != nullptr)
{
// Neighbour may share a window with us, force the window shut
m_Neighbour->DestroyWindow();
m_Neighbour->m_Neighbour = nullptr;
}
DestroyWindow();
}
void cChestEntity::CopyFrom(const cBlockEntity & a_Src)
{
Super::CopyFrom(a_Src);
@@ -63,6 +47,22 @@ void cChestEntity::CopyFrom(const cBlockEntity & a_Src)
void cChestEntity::OnRemoveFromWorld()
{
if (m_Neighbour != nullptr)
{
// Neighbour may share a window with us, force the window shut:
m_Neighbour->DestroyWindow();
m_Neighbour->m_Neighbour = nullptr;
}
DestroyWindow();
}
void cChestEntity::SendTo(cClientHandle & a_Client)
{
// Send a dummy "number of players with chest open" packet to make the chest visible:
@@ -199,11 +199,10 @@ void cChestEntity::OpenNewWindow(void)
void cChestEntity::DestroyWindow()
{
cWindow * Window = GetWindow();
const auto Window = GetWindow();
if (Window != nullptr)
{
Window->OwnerDestroyed();
CloseWindow();
}
}

View File

@@ -37,10 +37,9 @@ public:
/** Constructor used for normal operation */
cChestEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World);
virtual ~cChestEntity() override;
// cBlockEntity overrides:
virtual void CopyFrom(const cBlockEntity & a_Src) override;
virtual void OnRemoveFromWorld() override;
virtual void SendTo(cClientHandle & a_Client) override;
virtual bool UsedBy(cPlayer * a_Player) override;

View File

@@ -26,20 +26,6 @@ cDropSpenserEntity::cDropSpenserEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_Block
cDropSpenserEntity::~cDropSpenserEntity()
{
// Tell window its owner is destroyed
cWindow * Window = GetWindow();
if (Window != nullptr)
{
Window->OwnerDestroyed();
}
}
void cDropSpenserEntity::AddDropSpenserDir(Vector3i & a_RelCoord, NIBBLETYPE a_Direction)
{
switch (a_Direction & E_META_DROPSPENSER_FACING_MASK)
@@ -132,6 +118,20 @@ void cDropSpenserEntity::CopyFrom(const cBlockEntity & a_Src)
void cDropSpenserEntity::OnRemoveFromWorld()
{
const auto Window = GetWindow();
if (Window != nullptr)
{
// Tell window its owner is destroyed:
Window->OwnerDestroyed();
}
}
bool cDropSpenserEntity::Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk)
{
UNUSED(a_Dt);

View File

@@ -43,10 +43,10 @@ public:
// tolua_end
cDropSpenserEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World);
virtual ~cDropSpenserEntity() override;
// cBlockEntity overrides:
virtual void CopyFrom(const cBlockEntity & a_Src) override;
virtual void OnRemoveFromWorld() override;
virtual bool Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk) override;
virtual void SendTo(cClientHandle & a_Client) override;
virtual bool UsedBy(cPlayer * a_Player) override;

View File

@@ -25,23 +25,23 @@ cEnderChestEntity::cEnderChestEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMe
cEnderChestEntity::~cEnderChestEntity()
void cEnderChestEntity::SendTo(cClientHandle & a_Client)
{
cWindow * Window = GetWindow();
if (Window != nullptr)
{
Window->OwnerDestroyed();
}
// Send a dummy "number of players with chest open" packet to make the chest visible:
a_Client.SendBlockAction(m_Pos.x, m_Pos.y, m_Pos.z, 1, 0, m_BlockType);
}
void cEnderChestEntity::SendTo(cClientHandle & a_Client)
void cEnderChestEntity::OnRemoveFromWorld()
{
// Send a dummy "number of players with chest open" packet to make the chest visible:
a_Client.SendBlockAction(m_Pos.x, m_Pos.y, m_Pos.z, 1, 0, m_BlockType);
const auto Window = GetWindow();
if (Window != nullptr)
{
Window->OwnerDestroyed();
}
}

View File

@@ -20,9 +20,9 @@ class cEnderChestEntity :
public: // tolua_export
cEnderChestEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World);
virtual ~cEnderChestEntity() override;
// cBlockEntity overrides:
virtual void OnRemoveFromWorld() override;
virtual bool UsedBy(cPlayer * a_Player) override;
virtual void SendTo(cClientHandle & a_Client) override;

View File

@@ -23,18 +23,9 @@ cFlowerPotEntity::cFlowerPotEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta
void cFlowerPotEntity::Destroy(void)
cItems cFlowerPotEntity::ConvertToPickups() const
{
// Drop the contents as pickups:
if (!m_Item.IsEmpty())
{
ASSERT(m_World != nullptr);
cItems Pickups;
Pickups.Add(m_Item);
m_World->SpawnItemPickups(Pickups, Vector3d(0.5, 0.5, 0.5) + m_Pos);
m_Item.Empty();
}
return cItem(m_Item);
}

View File

@@ -43,7 +43,7 @@ public: // tolua_export
// tolua_end
// cBlockEntity overrides:
virtual void Destroy(void) override;
virtual cItems ConvertToPickups() const override;
virtual void CopyFrom(const cBlockEntity & a_Src) override;
virtual bool UsedBy(cPlayer * a_Player) override;
virtual void SendTo(cClientHandle & a_Client) override;

View File

@@ -25,7 +25,6 @@ enum
cFurnaceEntity::cFurnaceEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World):
Super(a_BlockType, a_BlockMeta, a_Pos, ContentsWidth, ContentsHeight, a_World),
m_CurrentRecipe(nullptr),
m_IsDestroyed(false),
m_IsCooking(a_BlockType == E_BLOCK_LIT_FURNACE),
m_NeedCookTime(0),
m_TimeCooked(0),
@@ -41,30 +40,6 @@ cFurnaceEntity::cFurnaceEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Ve
cFurnaceEntity::~cFurnaceEntity()
{
// Tell window its owner is destroyed
cWindow * Window = GetWindow();
if (Window != nullptr)
{
Window->OwnerDestroyed();
}
}
void cFurnaceEntity::Destroy()
{
m_IsDestroyed = true;
Super::Destroy();
}
void cFurnaceEntity::CopyFrom(const cBlockEntity & a_Src)
{
Super::CopyFrom(a_Src);
@@ -73,7 +48,6 @@ void cFurnaceEntity::CopyFrom(const cBlockEntity & a_Src)
m_CurrentRecipe = src.m_CurrentRecipe;
m_FuelBurnTime = src.m_FuelBurnTime;
m_IsCooking = src.m_IsCooking;
m_IsDestroyed = src.m_IsDestroyed;
m_IsLoading = src.m_IsLoading;
m_LastInput = src.m_LastInput;
m_NeedCookTime = src.m_NeedCookTime;
@@ -85,6 +59,20 @@ void cFurnaceEntity::CopyFrom(const cBlockEntity & a_Src)
void cFurnaceEntity::OnRemoveFromWorld()
{
const auto Window = GetWindow();
if (Window != nullptr)
{
// Tell window its owner is destroyed:
Window->OwnerDestroyed();
}
}
void cFurnaceEntity::SendTo(cClientHandle & a_Client)
{
// Nothing needs to be sent
@@ -259,11 +247,6 @@ void cFurnaceEntity::OnSlotChanged(cItemGrid * a_ItemGrid, int a_SlotNum)
{
Super::OnSlotChanged(a_ItemGrid, a_SlotNum);
if (m_IsDestroyed)
{
return;
}
if (m_IsLoading)
{
return;

View File

@@ -41,11 +41,9 @@ public:
/** Constructor used for normal operation */
cFurnaceEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World);
virtual ~cFurnaceEntity() override;
// cBlockEntity overrides:
virtual void Destroy() override;
virtual void CopyFrom(const cBlockEntity & a_Src) override;
virtual void OnRemoveFromWorld() override;
virtual void SendTo(cClientHandle & a_Client) override;
virtual bool Tick(std::chrono::milliseconds a_Dt, cChunk & a_Chunk) override;
virtual bool UsedBy(cPlayer * a_Player) override;
@@ -118,9 +116,6 @@ protected:
/** The item that is being smelted */
cItem m_LastInput;
/** Set to true when the furnace entity has been destroyed to prevent the block being set again */
bool m_IsDestroyed;
/** Set to true if the furnace is cooking an item */
bool m_IsCooking;