Safely handle block deletion (#13315)

Co-authored-by: Jude Melton-Houghton <jwmhjwmh@gmail.com>

src/map.cpp

@@ -1309,6 +1309,8 @@ ServerMap::~ServerMap()
 	*/
 	delete dbase;
 	delete dbase_ro;
+
+	deleteDetachedBlocks();
 }
 
 MapgenParams *ServerMap::getMapgenParams()
@@ -1888,12 +1890,31 @@ bool ServerMap::deleteBlock(v3s16 blockpos)
 		MapSector *sector = getSectorNoGenerate(p2d);
 		if (!sector)
 			return false;
-		sector->deleteBlock(block);
+		// It may not be safe to delete the block from memory at the moment
+		// (pointers to it could still be in use)
+		sector->detachBlock(block);
+		m_detached_blocks.push_back(block);
 	}
 
 	return true;
 }
 
+void ServerMap::deleteDetachedBlocks()
+{
+	for (MapBlock *block : m_detached_blocks) {
+		assert(block->isOrphan());
+		delete block;
+	}
+	m_detached_blocks.clear();
+}
+
+void ServerMap::step()
+{
+	// Delete from memory blocks removed by deleteBlocks() only when pointers
+	// to them are (probably) no longer in use
+	deleteDetachedBlocks();
+}
+
 void ServerMap::PrintInfo(std::ostream &out)
 {
 	out<<"ServerMap: ";

src/map.h

@@ -412,8 +412,15 @@ public:
 	// Database version
 	void loadBlock(std::string *blob, v3s16 p3d, MapSector *sector, bool save_after_load=false);
 
+	// Blocks are removed from the map but not deleted from memory until
+	// deleteDetachedBlocks() is called, since pointers to them may still exist
+	// when deleteBlock() is called.
 	bool deleteBlock(v3s16 blockpos) override;
 
+	void deleteDetachedBlocks();
+
+	void step();
+
 	void updateVManip(v3s16 pos);
 
 	// For debug printing
@@ -457,6 +464,9 @@ private:
 
 	std::set<v3s16> m_chunks_in_progress;
 
+	// used by deleteBlock() and deleteDetachedBlocks()
+	MapBlockVect m_detached_blocks;
+
 	// Queued transforming water nodes
 	UniqueQueue<v3s16> m_transforming_liquid;
 	f32 m_transforming_liquid_loop_count_multiplier = 1.0f;

src/mapblock.h

@@ -86,6 +86,19 @@ public:
 		return m_parent;
 	}
 
+	// Any server-modding code can "delete" arbitrary blocks (i.e. with
+	// core.delete_area), which makes them orphan. Avoid using orphan blocks for
+	// anything.
+	bool isOrphan() const
+	{
+		return !m_parent;
+	}
+
+	void makeOrphan()
+	{
+		m_parent = nullptr;
+	}
+
 	void reallocate()
 	{
 		for (u32 i = 0; i < nodecount; i++)

src/mapsector.cpp

@@ -109,6 +109,12 @@ void MapSector::insertBlock(MapBlock *block)
 }
 
 void MapSector::deleteBlock(MapBlock *block)
+{
+	detachBlock(block);
+	delete block;
+}
+
+void MapSector::detachBlock(MapBlock *block)
 {
 	s16 block_y = block->getPos().Y;
 
@@ -118,8 +124,8 @@ void MapSector::deleteBlock(MapBlock *block)
 	// Remove from container
 	m_blocks.erase(block_y);
 
-	// Delete
-	delete block;
+	// Mark as removed
+	block->makeOrphan();
 }
 
 void MapSector::getBlocks(MapBlockVect &dest)

src/mapsector.h

@@ -58,6 +58,9 @@ public:
 
 	void deleteBlock(MapBlock *block);
 
+	// Remove a block from the map and the sector without deleting it
+	void detachBlock(MapBlock *block);
+
 	void getBlocks(MapBlockVect &dest);
 
 	bool empty() const { return m_blocks.empty(); }

src/server.cpp

@@ -666,6 +666,11 @@ void Server::AsyncRunStep(bool initial_step)
 			-1);
 	}
 
+	/*
+		Note: Orphan MapBlock ptrs become dangling after this call.
+	*/
+	m_env->getServerMap().step();
+
 	/*
 		Listen to the admin chat, if available
 	*/

src/serverenvironment.cpp

@@ -282,6 +282,8 @@ void LBMManager::applyLBMs(ServerEnvironment *env, MapBlock *block,
 						continue;
 					for (auto lbmdef : *lbm_list) {
 						lbmdef->trigger(env, pos + pos_of_block, n, dtime_s);
+						if (block->isOrphan())
+							return;
 						n = block->getNodeNoCheck(pos);
 						if (n.getContent() != c)
 							break; // The node was changed and the LBMs no longer apply
@@ -966,6 +968,9 @@ public:
 				aabm.abm->trigger(m_env, p, n,
 					active_object_count, active_object_count_wider);
 
+				if (block->isOrphan())
+					return;
+
 				// Count surrounding objects again if the abms added any
 				if(m_env->m_added_objects > 0) {
 					active_object_count = countObjects(block, map, active_object_count_wider);
@@ -1016,13 +1021,17 @@ void ServerEnvironment::activateBlock(MapBlock *block, u32 additional_dtime)
 
 	// Activate stored objects
 	activateObjects(block, dtime_s);
+	if (block->isOrphan())
+		return;
 
 	/* Handle LoadingBlockModifiers */
 	m_lbm_mgr.applyLBMs(this, block, stamp, (float)dtime_s);
+	if (block->isOrphan())
+		return;
 
 	// Run node timers
 	block->step((float)dtime_s, [&](v3s16 p, MapNode n, f32 d) -> bool {
-		return m_script->node_on_timer(p, n, d);
+		return !block->isOrphan() && m_script->node_on_timer(p, n, d);
 	});
 }
 
@@ -1996,6 +2005,8 @@ void ServerEnvironment::activateObjects(MapBlock *block, u32 dtime_s)
 			<< " type=" << (int)s_obj.type << std::endl;
 		// This will also add the object to the active static list
 		addActiveObjectRaw(obj, false, dtime_s);
+		if (block->isOrphan())
+			return;
 	}
 
 	// Clear stored list