#3133 (fix for #3075) breaks "concurrent" use of default.dig_up

[j-r]

The approach to eliminate the recursive behavior of default.dig_up chosen in #3133 ignores all calls to default.dig_up during the iteration, even if they are not targeting the same column of nodes.

This is unexpected and undocumented behavior.

To replace the recursion by an iterative solution it would be better to have default.dig_up add positions to a work queue.

But looking at it, the recursion itself isn't really the problem, nobody needs digging mile high papyrus stacks (that's why a height limitation was introduced in #3133, too). The actual problem is that the potential stack overflow can be used as a denial of service attack. So IMO a better solution is to treat triggering a large stack depth as a protection violation. This has the added benefit of generically mitigating this kind of attack, because protection checks are common.

local old_is_protected = minetest.is_protected
function minetest.is_protected(pos, name)
    if debug.getinfo(100) then
        return true
    end
    return old_is_protected(pos, name)
end

Might be a good idea to have this in minetest itself, perhaps as a more efficient C++ solution.

To mitigate the recursion depth a bit, the call to minetest.node_dig could be turned it into a tail call with

--- a/mods/default/functions.lua
+++ b/mods/default/functions.lua
@@ -298,7 +298,7 @@ function default.dig_up(pos, node, digger)
        local np = {x = pos.x, y = pos.y + 1, z = pos.z}
        local nn = minetest.get_node(np)
        if nn.name == node.name then
-               minetest.node_dig(np, nn, digger)
+               return minetest.node_dig(np, nn, digger)
        end
 end

[sfan5]

Does it actually happen in practice that dig_up is called on another column during the callback of an existing dig_node run?

To replace the recursion by an iterative solution it would be better to have default.dig_up add positions to a work queue.

That sounds like a workable solution.

So IMO a better solution is to treat triggering a large stack depth as a protection violation.

Absolutely not. is_protected must define deterministically or it becomes a liability.

[j-r]

Does it actually happen in practice that dig_up is called on another column during the callback of an existing dig_node run?

Well, dig_up is uncommon, but register_on_dignode callbacks are run in the context of the dig_up starting from the 2nd dug node. These are commonly used to manipulate other nodes.

So IMO a better solution is to treat triggering a large stack depth as a protection violation.

Absolutely not. is_protected must define deterministically or it becomes a liability.

It's deterministically preventing unprivileged players from crashing the server:-) Anyway, I don't see another established way to handle such security incidents. Doing it this way leaves a clear trail in the action log and can be used to auto ban players by existing tools.

EDIT: to prevent the stack overflow in this case it would be enough to just add the stack depth check into dig_up which is probably the least intrusive change.