Check circular area hierarchy on startup

This also prevents 'areas:isAreaOwner' from getting stuck in an infinite loop.

README.md

@@ -101,10 +101,10 @@ Commands
   * `/area_pos {set,set1,set2,get}` -- Sets the area positions by punching
 	nodes or shows the current area positions.
 
-  * `/area_pos1 [X,Y,Z|X Y Z]` -- Sets area position one to your position or
+  * `/area_pos1 [X,Y,Z|X Y Z|X, Y, Z]` -- Sets area position one to your position or
 	the one supplied.
 
-  * `/area_pos2 [X,Y,Z|X Y Z]` -- Sets area position two to your position or
+  * `/area_pos2 [X,Y,Z|X Y Z|X, Y, Z]` -- Sets area position two to your position or
 	the one supplied.
 
   * `/areas_cleanup` -- Removes all ownerless areas.

internal.lua

@@ -60,6 +60,8 @@ function areas:load()
 	end
 	file:close()
 	self:populateStore()
+
+	areas:_checkHierarchy()
 end
 
 --- Checks an AreaStore ID.
@@ -207,6 +209,7 @@ function areas:isSubarea(pos1, pos2, id)
 end
 
 -- Returns a table (list) of children of an area given its identifier
+-- This is not recursive, meaning that only children and not grand-children are returned.
 function areas:getChildren(id)
 	local children = {}
 	for cid, area in pairs(self.areas) do
@@ -342,10 +345,13 @@ function areas:isAreaOwner(id, name)
 	if cur and minetest.check_player_privs(name, self.adminPrivs) then
 		return true
 	end
-	while cur do
+	local seen = {}
+	while cur and not seen[cur] do
 		if cur.owner == name then
 			return true
 		elseif cur.parent then
+			-- Prevent lock-ups
+			seen[cur] = true
 			cur = self.areas[cur.parent]
 		else
 			return false
@@ -353,3 +359,56 @@ function areas:isAreaOwner(id, name)
 	end
 	return false
 end
+
+local function get_parent_chain_if_recursive(area, completed)
+	-- Get uppermost parent
+	local affected = {}
+	while area do
+		if affected[area] then
+			-- List of affected areas
+			return affected
+		end
+		if completed[area] then
+			-- Already checked by another function call --> all OK
+			return nil
+		end
+		affected[area] = true
+		completed[area] = true
+
+		area = areas.areas[area.parent]
+	end
+	return nil -- all OK
+end
+
+--- Internal function to ensure there are no circular parent/children occurrences
+function areas:_checkHierarchy()
+	local needs_save = false
+	local completed = {}
+	for _, area_1 in pairs(self.areas) do
+		local chain = get_parent_chain_if_recursive(area_1, completed)
+		if chain then
+			-- How can it be fixed if there is a longer chain?
+			local list = {}
+			for area, _ in pairs(chain) do
+				list[#list + 1] = area.parent
+			end
+
+			local instruction
+			if #list == 1 then
+				-- Trivial case, can be resolved in-place
+				instruction = "The issue was corrected automatically."
+				area_1.parent = nil
+				needs_save = true
+			else
+				instruction = "Please resolve this conflict manually. Expect issues."
+			end
+
+			core.log("error", "[areas] LOGIC ERROR! Detected a circular area hierarchy in the "
+				.. "following area ID(s): " .. table.concat(list, ", ") .. ". " .. instruction)
+		end
+	end
+	if needs_save then
+		-- Prevent repetitive spam upon startup
+		self:save()
+	end
+end

pos.lua

@@ -69,68 +69,53 @@ minetest.register_chatcommand("select_area", {
 	end,
 })
 
+local function area_pos_handler(name, param, nr)
+	local pos
+	local player = minetest.get_player_by_name(name)
+	if player then
+		pos = vector.round(player:get_pos())
+	end
+
+	-- Input parsing
+	local error_msg
+	local found, _, x_str, y_str, z_str = param:find(
+		"^(~?-?%d*)[, ] *(~?-?%d*)[, ] *(~?-?%d*)$")
+	if found then
+		pos, error_msg = parse_relative_pos(x_str, y_str, z_str, pos)
+	elseif param ~= "" then
+		return false, S("Invalid usage, see /help @1.", "area_pos" .. nr)
+	end
+	if not pos then
+		return false, error_msg or S("Unable to get position.")
+	end
+
+	-- Assign the position
+	pos = posLimit(vector.round(pos))
+	if nr == 1 then
+		areas:setPos1(name, pos)
+	else
+		areas:setPos2(name, pos)
+	end
+	return true, S("Area position @1 set to @2", tostring(nr),
+			minetest.pos_to_string(pos))
+end
+
 minetest.register_chatcommand("area_pos1", {
-	params = "[X Y Z|X,Y,Z]",
+	params = "[X Y Z|X,Y,Z|X, Y, Z]",
 	description = S("Set area protection region position @1 to your"
 		.." location or the one specified", "1"),
 	privs = {},
 	func = function(name, param)
-		local pos
+		return area_pos_handler(name, param, 1)
-		local player = minetest.get_player_by_name(name)
-		if player then
-			pos = vector.round(player:get_pos())
-		end
-		local found, _, x_str, y_str, z_str = param:find(
-			"^(~?-?%d*)[, ](~?-?%d*)[, ](~?-?%d*)$")
-		if found then
-			local get_pos, reason = parse_relative_pos(x_str, y_str, z_str, pos)
-			if get_pos then
-				pos = get_pos
-			elseif not get_pos and reason then
-				return false, reason
-			end
-		elseif param ~= "" then
-			return false, S("Invalid usage, see /help @1.", "area_pos1")
-		end
-		if not pos then
-			return false, S("Unable to get position.")
-		end
-		pos = posLimit(vector.round(pos))
-		areas:setPos1(name, pos)
-		return true, S("Area position @1 set to @2", "1",
-				minetest.pos_to_string(pos))
 	end,
 })
 
 minetest.register_chatcommand("area_pos2", {
-	params = "[X Y Z|X,Y,Z]",
+	params = "[X Y Z|X,Y,Z|X, Y, Z]",
 	description = S("Set area protection region position @1 to your"
 		.." location or the one specified", "2"),
 	func = function(name, param)
-		local pos
+		return area_pos_handler(name, param, 2)
-		local player = minetest.get_player_by_name(name)
-		if player then
-			pos = vector.round(player:get_pos())
-		end
-		local found, _, x_str, y_str, z_str = param:find(
-			"^(~?-?%d*)[, ](~?-?%d*)[, ](~?-?%d*)$")
-		if found then
-			local get_pos, reason = parse_relative_pos(x_str, y_str, z_str, pos)
-			if get_pos then
-				pos = get_pos
-			elseif not get_pos and reason then
-				return false, reason
-			end
-		elseif param ~= "" then
-			return false, S("Invalid usage, see /help @1.", "area_pos2")
-		end
-		if not pos then
-			return false, S("Unable to get position.")
-		end
-		pos = posLimit(vector.round(pos))
-		areas:setPos2(name, pos)
-		return true, S("Area position @1 set to @2", "2",
-			minetest.pos_to_string(pos))
 	end,
 })