dnsdist: SIGSEGV on OpenBSD/amd64/clang13

[omoerbeek]

OpenBSD recently moved to clang13. I'm seeing:

Program received signal SIGSEGV, Segmentation fault.
0x00000b08926f7810 in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string (this=0x100157b8, __str=...)
    at /usr/include/c++/v1/string:1992
1992        : __r_(_VSTD::move(__str.__r_))
(gdb) bt
#0  0x00000b08926f7810 in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string (this=0x100157b8, __str=...)
    at /usr/include/c++/v1/string:1992
#1  ServerPolicy::ServerPolicy (this=0x100157b8) at ./dnsdist-lbpolicies.hh:32
#2  LuaContext::Pusher<ServerPolicy, void>::push<ServerPolicy> (
    state=0x10000378, value=...)
    at ./ext/luawrapper/include/LuaContext.hpp:1647
#3  0x00000b08926f772e in LuaContext::setTable<ServerPolicy, ServerPolicy> (
    state=0x10000378, index=0xb089237abc0 "firstAvailable", data=...)
    at ./ext/luawrapper/include/LuaContext.hpp:1034
#4  0x00000b08926580fa in LuaContext::writeVariable<char const (&) [15], ServerPolicy> (this=0xb08929c9068 <g_lua+8>, data=..., data=...)
    at ./ext/luawrapper/include/LuaContext.hpp:737
#5  setupLuaBindings (luaCtx=..., client=<optimized out>)
    at dnsdist-lua-bindings.cc:71
#6  0x00000b0892832538 in setupLua (luaCtx=..., client=<optimized out>, 
    configCheck=<optimized out>, config=...) at dnsdist-lua.cc:2870
#7  0x00000b0892930177 in main (argc=<optimized out>, argv=<optimized out>)
    at dnsdist.cc:2468

Looks like a move or copy constructor is called on an object that does not like to be moved or copied. Will investigate later (if nobody beats me to it).

• Program: dnsdist
• Issue type: Bug report

Short description

Environment

• Operating system: OpenBSD
• Software version: current
• Software source: git master

[omoerbeek]

Looked at the code, but saw no obvious mistake in ServerPolicy. LuaWrapper code is too smart/esoteric for me to grasp. The above backtrace is from an amd64 system, the issue does not seem to happen on arm64 (also using clang-13).

The crash does not happen on Debian bookwork using clang-13. A fellow OpenBSD dev pointed me at https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=259921 so I'm investigating that.

[omoerbeek]

It does seem to be a code generation issue specific to OpenBSD/amd64. Both arch and debian do not exhibit the problem.

[omoerbeek]

Workaround is to compile dnsdist-lua-bindings.cc with -O1 for now.

[rgacogne]

I don't see anything wrong either, the ServerPolicy object is properly passed with std::forward all the way, the initial constructor does copy the strings and the default-generated move constructor should thus be allowed to move them to the new object. It's a shot in the dark but perhaps explicitly defining the move constructor would help:

ServerPolicy(ServerPolicy&& rhs) = default;

?

[omoerbeek]

There are at least two ways to get rid of the issue. Both 1 and 2 are sufficient on their own to make the problem go away. I suspect the moving of a temp object triggers a bug in optimizations related to copy elision.

1. Defining a default copy ct: ServerPolicy(const ServerPolicy&) = default; Defining (only) a move constructor as suggested does not compile as it implicitly deletes the copy ct. Defining both a default copy and move ct does not work and results in the very same crash.
2. Use the patch below which avoids the compiler generated temp object and uses explicit ones. And also avoids having to type the same string twice...

diff --git a/pdns/dnsdist-lua-bindings.cc b/pdns/dnsdist-lua-bindings.cc
index bc264ab67..c5b66c6d1 100644
--- a/pdns/dnsdist-lua-bindings.cc
+++ b/pdns/dnsdist-lua-bindings.cc
@@ -69,12 +69,18 @@ void setupLuaBindings(LuaContext& luaCtx, bool client)
   luaCtx.registerFunction("toString", &ServerPolicy::toString);
   luaCtx.registerFunction("__tostring", &ServerPolicy::toString);

-  luaCtx.writeVariable("firstAvailable", ServerPolicy{"firstAvailable", firstAvailable, false});
-  luaCtx.writeVariable("roundrobin", ServerPolicy{"roundrobin", roundrobin, false});
-  luaCtx.writeVariable("wrandom", ServerPolicy{"wrandom", wrandom, false});
-  luaCtx.writeVariable("whashed", ServerPolicy{"whashed", whashed, false});
-  luaCtx.writeVariable("chashed", ServerPolicy{"chashed", chashed, false});
-  luaCtx.writeVariable("leastOutstanding", ServerPolicy{"leastOutstanding", leastOutstanding, false});
+  ServerPolicy policies[] = {
+    ServerPolicy{"firstAvailable", firstAvailable, false},
+    ServerPolicy{"roundrobin", roundrobin, false},
+    ServerPolicy{"wrandom", wrandom, false},
+    ServerPolicy{"whashed", whashed, false},
+    ServerPolicy{"chashed", chashed, false},
+    ServerPolicy{"leastOutstanding", leastOutstanding, false}
+  };
+  for (auto& policy : policies) {
+    luaCtx.writeVariable(policy.d_name, policy);
+  }
+
 #endif /* DISABLE_POLICIES_BINDINGS */

   /* ServerPool */

[rgacogne]

The suggested patch looks good to me, I'd merge that :)