From 526c25aa578b5acb1d06cf8b1985f9a1c9218d8d Mon Sep 17 00:00:00 2001 From: Nils Dagsson Moskopp Date: Sat, 13 Nov 2021 23:29:08 +0100 Subject: [PATCH] Add crash fix and tests for minetest.find_nodes_in_area() For some specific out of bounds values, the volume calculation in minetest.find_nodes_in_area() is off by about four million nodes. Unfortunately that behaviour made it trivial to crash Mineclonia, as Minetest immediately crashes upon encountering large numbers. This commit introduces a wrapper around minetest.find_nodes_in_area() which should avoid a crash. Additionally, three self tests are executed when a server starts; they crash Mineclonia in case the workaround fails. --- mods/MISC/mcl_engine_workarounds/init.lua | 98 +++++++++++++++++++ mods/MISC/mcl_selftests/init.lua | 113 ++++++++++++++++++++++ 2 files changed, 211 insertions(+) create mode 100644 mods/MISC/mcl_engine_workarounds/init.lua create mode 100644 mods/MISC/mcl_selftests/init.lua diff --git a/mods/MISC/mcl_engine_workarounds/init.lua b/mods/MISC/mcl_engine_workarounds/init.lua new file mode 100644 index 00000000..5cd0cece --- /dev/null +++ b/mods/MISC/mcl_engine_workarounds/init.lua @@ -0,0 +1,98 @@ +local clamp = function(value, min, max) + assert( min < max ) + if value < min then + return min + end + if value > max then + return max + end + return value +end + +assert( clamp(000, -100, 100) == 000 ) +assert( clamp(999, -100, 100) == 100 ) +assert( clamp(999, -999, 999) == 999 ) +assert( clamp(998, 999, 1999) == 999 ) +assert( clamp(999, 999, 1999) == 999 ) + +local clamp_s16 = function(value) + -- seems minetest hangs on -32768 and 32767 + return clamp(value, -32767, 32766) +end + +assert( clamp_s16(000000) == 000000 ) +assert( clamp_s16(000001) == 000001 ) +assert( clamp_s16(000010) == 000010 ) +assert( clamp_s16(000100) == 000100 ) +assert( clamp_s16(001000) == 001000 ) +assert( clamp_s16(010000) == 010000 ) +assert( clamp_s16(100000) == 032766 ) + +assert( clamp_s16(-00000) == -00000 ) +assert( clamp_s16(-00009) == -00009 ) +assert( clamp_s16(-00099) == -00099 ) +assert( clamp_s16(-00999) == -00999 ) +assert( clamp_s16(-09999) == -09999 ) +assert( clamp_s16(-99999) == -32767 ) + +local minetest_find_nodes_in_area = minetest.find_nodes_in_area +minetest.find_nodes_in_area = function(minp, maxp, ...) + if + minp.x >= 32767 or minp.x <= -32768 or + minp.y >= 32767 or minp.y <= -32768 or + minp.z >= 32767 or minp.z <= -32768 or + maxp.x >= 32767 or maxp.x <= -32768 or + maxp.y >= 32767 or maxp.y <= -32768 or + maxp.z >= 32767 or maxp.z <= -32768 + then + minetest.log( + "warning", + "find_nodes_in_area() called with coords outside interval (-32768, 32767), clamping arguments: " .. + "minp { x=" .. minp.x .. ", y=" .. minp.y .. " z=" .. minp.z .. " } " .. + "maxp { x=" .. maxp.x .. ", y=" .. maxp.y .. " z=" .. maxp.z .. " } " + ) + return minetest_find_nodes_in_area( + { x=clamp_s16(minp.x), y=clamp_s16(minp.y), z=clamp_s16(minp.z) }, + { x=clamp_s16(maxp.x), y=clamp_s16(maxp.y), z=clamp_s16(maxp.z) }, + ... + ) + else + return minetest_find_nodes_in_area( + minp, + maxp, + ... + ) + end +end + +local test_minetest_find_nodes_in_area_implementation_equivalence = function() + -- If any assertion in this test function fails, the wrapper + -- for minetest.find_nodes_in_area() does not behave like the + -- original function. If you are reading the code because your + -- server crashed, please inform the Mineclonia developers. + for x = -31000, 31000, 15500 do + for y = -31000, 31000, 15500 do + for z = -31000, 31000, 15500 do + for d = 1, 9, 3 do + local minp = { x=x, y=y, z=z } + local maxp = { x=x+d, y=y+d, z=z+d } + local npos_1, nnum_1 = minetest_find_nodes_in_area( + minp, + maxp, + { "air", "ignore" } + ) + local npos_2, nnum_2 = minetest.find_nodes_in_area( + minp, + maxp, + { "air", "ignore" } + ) + assert(#npos_1 == #npos_2) + assert(nnum_1["air"] == nnum_2["air"]) + assert(nnum_1["ignore"] == nnum_2["ignore"]) + end + end + end + end +end + +minetest.after( 0, test_minetest_find_nodes_in_area_implementation_equivalence ) diff --git a/mods/MISC/mcl_selftests/init.lua b/mods/MISC/mcl_selftests/init.lua new file mode 100644 index 00000000..9e0adae0 --- /dev/null +++ b/mods/MISC/mcl_selftests/init.lua @@ -0,0 +1,113 @@ +local test_minetest_find_nodes_in_area_can_count = function(dtime) + -- This function tests that minetest.find_nodes_in_area() can + -- count nodes correctly. If it fails, the engine can not be + -- trusted to actually count how many nodes of a given type + -- are in a given volume. Yes, *it* is bad if that happens. + -- + -- If you are looking at this function because it executes at + -- startup and crashes the game, by far the most stupid thing + -- you could do is disabling it. Only an absolute moron would + -- disable tests that ensure basic functionality still works. + -- + -- Experience has taught me that such warnings are mostly not + -- taken seriously by both Minetest mod & core developers. As + -- there are very few situations in which someone would read + -- the code of the function without a crash, you are probably + -- asking yourself how bad it can be. Surely you will want an + -- example of what will break if this test breaks. The answer + -- to this simple question is equally simple and consists of a + -- heartfelt “What the fuck did you say, you stupid fuckwad?”. + -- + -- Alrighty then, let's get it on … + + local pos = { x=30999, y=30999, z=30999 } + -- You think there is nothing there? Well, here is the thing: + -- With standard settings you can only see map until x=30927, + -- although the renderer can actually render up to x=31007 if + -- you configure it to. Any statements given by minetest core + -- devs that contradict the above assertion are probably lies. + -- + -- In any way, this area should be so far out that no one has + -- built here … yet. Now that you know it is possible, I know + -- you want to. How though? I suggest to figure the technique + -- out yourself, then go on and build invisible lag machines. + + local radius = 3 + local minp = vector.subtract(pos, radius) + local maxp = vector.add(pos, radius) + local nodename = "air" + local c_nodename = minetest.get_content_id(nodename) + + -- Why not use minetest.set_node() here? Well, some mods do + -- trigger on every placement of a node in the entire map … + -- and we do not want to crash those mods in this test case. + -- (Voxelmanip does not trigger callbacks – so all is well.) + -- + -- And now for a funny story: I initially copied the following + -- code from the Minetest developer wiki. Can you spot a typo? + -- + local vm = minetest.get_voxel_manip() + local emin, emax = vm:read_from_map( + minp, + maxp + ) + local area = VoxelArea:new({ + MinEdge=emin, + MaxEdge=emax + }) + local data = vm:get_data() + for z = minp.z, maxp.z do + for y = minp.y, maxp.y do + local vi = area:index(minp.x, y, z) + for x = minp.x, maxp.y do + data[vi] = c_nodename + vi = vi + 1 + end + end + end + vm:set_data(data) + vm:write_to_map() + local npos, nnum = minetest.find_nodes_in_area( + minp, + maxp, + { nodename } + ) + local nodes_expected = math.pow( 1 + (2 * radius), 3 ) + local nodes_counted = nnum[nodename] + local nodes_difference = nodes_expected - nodes_counted + -- Yes, the following line is supposed to crash the game in + -- the case that Minetest forgot how to count the number of + -- nodes in a three dimensional volume it just filled. This + -- function contains an excellent explanation on what not to + -- do further up. I strongly suggest to pester Minetest core + -- devs about this issue and not the author of the function, + -- should this test ever fail. + assert ( 0 == nodes_difference ) +end + +minetest.after( 0, test_minetest_find_nodes_in_area_can_count ) + +local test_minetest_find_nodes_in_area_crash = function(dtime) + -- And now for our feature presentation, where we call the + -- function “minetest.find_nodes_in_area()” with a position + -- out of bounds! Will it crash? Who knows‽ If it does, the + -- workaround is not working though, so it should be patched. + + local pos = { x=32767, y=32767, z=32767 } + -- Note that not all out of bounds values actually crash the + -- function minetest.find_nodes_in_area()“. In fact, the vast + -- majority of out of bounds values do not crash the function. + + local radius = 3 + local minp = vector.subtract(pos, radius) + local maxp = vector.add(pos, radius) + local nodename = "air" + local npos, nnum = minetest.find_nodes_in_area( + minp, + maxp, + { nodename } + ) + -- That's all, folks! +end + +minetest.after( 0, test_minetest_find_nodes_in_area_crash )