SEGV (on Linux) with latest Minetest Master

[lhofhansl]

Minetest version

Latest master (a8cf10b0b523bf4c3234be29b0dd51e719cb00c1)

Active renderer

N/A

Irrlicht device

N/A

Operating system and version

Linux, Fedora 38

CPU model

Intel

GPU model

N/A

OpenGL version

N/A

Summary

Here's the backtrace:

Thread 44 "Emerge-10" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff4923a6c0 (LWP 29773)]
0x0000000000000000 in ?? ()
(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x0000000000a9867f in ServerEnvironment::addActiveObjectRaw (this=this@entry=0x2b46c50, object_u=std::unique_ptr<ServerActiveObject> = {...},
set_changed=set_changed@entry=true, dtime_s=dtime_s@entry=0) at /home/lars/dev/minetest/minetest/src/serverenvironment.cpp:1889
#2  0x0000000000a98ce9 in ServerEnvironment::addActiveObject (this=this@entry=0x2b46c50, object=std::unique_ptr<ServerActiveObject> = {...})
at /usr/include/c++/13/bits/unique_ptr.h:197
#3  0x0000000000831bb1 in ModApiEnv::l_add_entity (L=0x7fffd7fe0380, L@entry=<error reading variable: value has been optimized out>)
at /usr/include/c++/13/bits/unique_ptr.h:197
#4  0x00000000007fcfca in script_exception_wrapper (L=0x7fffd7fe0380, f=<optimized out>)
at /home/lars/dev/minetest/minetest/src/script/common/c_internal.cpp:41
#5  0x00007ffff7943af0 in lj_BC_FUNCCW () from /lib64/libluajit-5.1.so.2
#6  0x00007ffff7956be0 in lua_pcall () from /lib64/libluajit-5.1.so.2
#7  0x000000000081637a in ScriptApiNode::node_on_construct (this=0x138b7c8, p=..., node=...)
at /home/lars/dev/minetest/minetest/src/script/cpp_api/s_node.cpp:171
#8  0x0000000000a97d8d in ServerEnvironment::setNode (this=this@entry=0x2b46c50, p=..., n=...)
at /home/lars/dev/minetest/minetest/src/serverenvironment.cpp:1119
#9  0x000000000082e71a in ModApiEnv::l_set_node (L=0x7fffd7fe0380, L@entry=<error reading variable: value has been optimized out>)
at /home/lars/dev/minetest/minetest/src/script/lua_api/l_env.cpp:256
#10 0x00000000007fcfca in script_exception_wrapper (L=0x7fffd7fe0380, f=<optimized out>)
at /home/lars/dev/minetest/minetest/src/script/common/c_internal.cpp:41
#11 0x00007ffff7943af0 in lj_BC_FUNCCW () from /lib64/libluajit-5.1.so.2
#12 0x00007ffff7956be0 in lua_pcall () from /lib64/libluajit-5.1.so.2
#13 0x000000000081637a in ScriptApiNode::node_on_construct (this=0x138b7c8, p=..., node=...)
at /home/lars/dev/minetest/minetest/src/script/cpp_api/s_node.cpp:171
#14 0x0000000000a97d8d in ServerEnvironment::setNode (this=this@entry=0x2b46c50, p=..., n=...)

This is due to some commit AFTER b12be04

Steps to reproduce

Somewhat reliably with Mineclonia. Just starting a new world, after a few seconds MT crashes.

[lhofhansl]

If I have time this evening I'll narrow the commits a bit more.

Update:
Still after e7dd973
And after 2ea8d9c

So prime suspect: 0383c44 (#13880)

[lhofhansl]

Yep. Master without 0383c44 (#13880) does not crash.

[savilli]

The thread name Emerge-10 looks sus. I think calling minetest.add_entity from an emerge thread causes thread safety issues.

[savilli]

Ah no, the reason is more simple. In ModifySafeMap we replace the value with an empty unique_ptr here, thinking it's equivalent to removal the key from the map. But emplace here doesn't think so and fails because there is such key in the map already. Therefore ServerActiveObject becomes destroyed, and we access it here.

No crashes with this patch:

diff --git a/src/util/container.h b/src/util/container.h
index 985a9a447..a8dd74c4d 100644
--- a/src/util/container.h
+++ b/src/util/container.h
@@ -377,9 +377,9 @@ class ModifySafeMap
                        return;
                }
                if (m_iterating)
-                       m_new.emplace(key, value);
+                       m_new[key] = value;
                else
-                       m_values.emplace(key, value);
+                       m_values[key] = value;
        }
 
        void put(const K &key, V &&value) {
@@ -388,9 +388,9 @@ class ModifySafeMap
                        return;
                }
                if (m_iterating)
-                       m_new.emplace(key, std::move(value));
+                       m_new[key] = std::move(value);
                else
-                       m_values.emplace(key, std::move(value));
+                       m_values[key] = std::move(value);
        }

[lhofhansl]

@savilli I can confirm that your change works.

[sfan5]

Good catch on the bug, but I discovered a few more edge cases I need to fix.

The thread name Emerge-10 looks sus. I think calling minetest.add_entity from an emerge thread causes thread safety issues.

That's okay actually. on_geneated runs on the emerge thread but while the env lock is locked.