From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 17 Jun 2024 10:16:38 +0000 (+0200)
Subject: dnsdist: Fix a race condition with custom Lua web handlers
X-Git-Tag: rec-5.2.0-alpha0~15^2~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e8678b4a9077a39e7645fdabd1b3dc28a0757659;p=thirdparty%2Fpdns.git

dnsdist: Fix a race condition with custom Lua web handlers

Custom web handlers written in Lua modify the global Lua context,
but until now they did not take the lock protecting it so a data
race condition was possible.
Reported by TSAN while running our unit tests.
---

diff --git a/pdns/dnsdistdist/dnsdist-lua-web.cc b/pdns/dnsdistdist/dnsdist-lua-web.cc
index 0498aed4fc..35cf0d0f09 100644
--- a/pdns/dnsdistdist/dnsdist-lua-web.cc
+++ b/pdns/dnsdistdist/dnsdist-lua-web.cc
@@ -25,14 +25,14 @@
 #include "dnsdist-lua.hh"
 #include "dnsdist-web.hh"
 
-void registerWebHandler(const std::string& endpoint, std::function<void(const YaHTTP::Request&, YaHTTP::Response&)> handler);
+void registerWebHandler(const std::string& endpoint, std::function<void(const YaHTTP::Request&, YaHTTP::Response&)> handler, bool isLua);
 
 void setupLuaWeb(LuaContext& luaCtx)
 {
 #ifndef DISABLE_LUA_WEB_HANDLERS
   luaCtx.writeFunction("registerWebHandler", [](const std::string& path, std::function<void(const YaHTTP::Request*, YaHTTP::Response*)> handler) {
     /* LuaWrapper does a copy for objects passed by reference, so we pass a pointer */
-    registerWebHandler(path, [handler](const YaHTTP::Request& req, YaHTTP::Response& resp) { handler(&req, &resp); });
+    registerWebHandler(path, [handler](const YaHTTP::Request& req, YaHTTP::Response& resp) { handler(&req, &resp); }, true);
   });
 
   luaCtx.registerMember<std::string(YaHTTP::Request::*)>("path", [](const YaHTTP::Request& req) -> std::string { return req.url.path; }, [](YaHTTP::Request& req, const std::string& path) { (void) path; });
@@ -78,4 +78,3 @@ void setupLuaWeb(LuaContext& luaCtx)
   });
 #endif /* DISABLE_LUA_WEB_HANDLERS */
 }
-
diff --git a/pdns/dnsdistdist/dnsdist-web.cc b/pdns/dnsdistdist/dnsdist-web.cc
index 7fe717b1d3..4e18a63752 100644
--- a/pdns/dnsdistdist/dnsdist-web.cc
+++ b/pdns/dnsdistdist/dnsdist-web.cc
@@ -1717,14 +1717,20 @@ static void handleRings(const YaHTTP::Request& req, YaHTTP::Response& resp)
 }
 
 using WebHandler = std::function<void(const YaHTTP::Request&, YaHTTP::Response&)>;
-static SharedLockGuarded<std::unordered_map<std::string, WebHandler>> s_webHandlers;
+struct WebHandlerContext
+{
+  WebHandler d_handler;
+  bool d_isLua;
+};
+
+static SharedLockGuarded<std::unordered_map<std::string, WebHandlerContext>> s_webHandlers;
 
-void registerWebHandler(const std::string& endpoint, WebHandler handler);
+void registerWebHandler(const std::string& endpoint, WebHandler handler, bool isLua = false);
 
-void registerWebHandler(const std::string& endpoint, WebHandler handler)
+void registerWebHandler(const std::string& endpoint, WebHandler handler, bool isLua)
 {
   auto handlers = s_webHandlers.write_lock();
-  (*handlers)[endpoint] = std::move(handler);
+  (*handlers).emplace(std::make_pair(endpoint, WebHandlerContext{std::move(handler), isLua}));
 }
 
 void clearWebHandlers()
@@ -1868,17 +1874,23 @@ static void connectionThread(WebClientConnection&& conn)
       resp.status = 405;
     }
     else {
-      WebHandler handler;
+      std::optional<WebHandlerContext> handlerCtx{std::nullopt};
       {
         auto handlers = s_webHandlers.read_lock();
         const auto webHandlersIt = handlers->find(req.url.path);
         if (webHandlersIt != handlers->end()) {
-          handler = webHandlersIt->second;
+          handlerCtx = webHandlersIt->second;
         }
       }
 
-      if (handler) {
-        handler(req, resp);
+      if (handlerCtx) {
+        if (handlerCtx->d_isLua) {
+          auto lua = g_lua.lock();
+          handlerCtx->d_handler(req, resp);
+        }
+        else {
+          handlerCtx->d_handler(req, resp);
+        }
       }
       else {
         resp.status = 404;