Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dnsdist: SIGSEGV on OpenBSD/amd64/clang13 #11113

Closed
omoerbeek opened this issue Dec 20, 2021 · 6 comments · Fixed by #11176
Closed

dnsdist: SIGSEGV on OpenBSD/amd64/clang13 #11113

omoerbeek opened this issue Dec 20, 2021 · 6 comments · Fixed by #11176

Comments

@omoerbeek
Copy link
Member

omoerbeek commented Dec 20, 2021

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
Copy link
Member Author

omoerbeek commented Dec 21, 2021

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
Copy link
Member Author

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

@omoerbeek
Copy link
Member Author

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

@rgacogne
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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

omoerbeek added a commit to omoerbeek/pdns that referenced this issue Jan 12, 2022
omoerbeek added a commit to omoerbeek/pdns that referenced this issue Jan 12, 2022
rgacogne pushed a commit to rgacogne/pdns that referenced this issue Apr 20, 2022
…ake the code a tiny bit more pretty.

Fixes PowerDNS#11113.

(cherry picked from commit 36c5a9e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants