From 57a73a843598daadfd1063f9ca3a47a828592898 Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 17 Jun 2024 12:42:02 +0200
Subject: [PATCH] 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.
---
 pdns/dnsdist-web.cc                 | 28 ++++++++++++++++++++--------
 pdns/dnsdistdist/dnsdist-lua-web.cc |  5 ++---
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/pdns/dnsdist-web.cc b/pdns/dnsdist-web.cc
index 265206a725..84ea0798dd 100644
--- a/pdns/dnsdist-web.cc
+++ b/pdns/dnsdist-web.cc
@@ -1721,14 +1721,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{false};
+};
+
+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)[endpoint] = WebHandlerContext{std::move(handler), isLua};
 }
 
 void clearWebHandlers()
@@ -1864,17 +1870,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;
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 */
 }
-
-- 
2.47.2