From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Wed, 13 Mar 2024 11:55:39 +0000 (+0100)
Subject: Avoid race setting serverID
X-Git-Tag: rec-5.1.0-alpha1~82^2~9
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e124362c3422a982c8c90a6a6625fcec60760493;p=thirdparty%2Fpdns.git

Avoid race setting serverID
---

diff --git a/pdns/recursordist/rec-main.cc b/pdns/recursordist/rec-main.cc
index dd959baadf..1013c48207 100644
--- a/pdns/recursordist/rec-main.cc
+++ b/pdns/recursordist/rec-main.cc
@@ -3202,7 +3202,8 @@ int main(int argc, char** argv)
     handleRuntimeDefaults(startupLog);
 
     if (auto ttl = ::arg().asNum("system-resolver-ttl"); ttl != 0) {
-      pdns::RecResolve::setInstanceParameters(ttl, []() { reloadZoneConfiguration(g_yamlSettings); });
+      // Cannot use SyncRes::s_serverID, it is nt set yet
+      pdns::RecResolve::setInstanceParameters(arg()["server-id"], ttl, []() { reloadZoneConfiguration(g_yamlSettings); });
     }
 
     g_recCache = std::make_unique<MemRecursorCache>(::arg().asNum("record-cache-shards"));
diff --git a/pdns/recursordist/rec-system-resolve.cc b/pdns/recursordist/rec-system-resolve.cc
index 9d32813381..a77f689fac 100644
--- a/pdns/recursordist/rec-system-resolve.cc
+++ b/pdns/recursordist/rec-system-resolve.cc
@@ -32,7 +32,6 @@
 #include "logging.hh"
 #include "noinitvector.hh"
 #include "threadname.hh"
-#include "syncres.hh"
 
 namespace
 {
@@ -91,11 +90,13 @@ std::string serverID()
 }
 } // anonymous namespace
 
-std::function<void()> pdns::RecResolve::s_callback;
+std::string pdns::RecResolve::s_serverID;
 time_t pdns::RecResolve::s_ttl{0};
+std::function<void()> pdns::RecResolve::s_callback;
 
-void pdns::RecResolve::setInstanceParameters(time_t ttl, const std::function<void()>& callback)
+void pdns::RecResolve::setInstanceParameters(std::string serverID, time_t ttl, const std::function<void()>& callback)
 {
+  pdns::RecResolve::s_serverID = std::move(serverID);
   pdns::RecResolve::s_ttl = ttl;
   pdns::RecResolve::s_callback = callback;
 }
@@ -250,9 +251,9 @@ void pdns::RecResolve::Refresher::refreshLoop()
       if (lastSelfCheck < time(nullptr) - 3600) {
         lastSelfCheck = time(nullptr);
         auto resolvedServerID = serverID();
-        if (resolvedServerID == SyncRes::s_serverID) {
+        if (resolvedServerID == s_serverID) {
           auto log = g_slog->withName("system-resolver");
-          log->info(Logr::Error, "id.server/CH/TXT resolves to my own server identidy", "id.server", Logging::Loggable(resolvedServerID));
+          log->info(Logr::Error, "id.server/CH/TXT resolves to my own server identity", "id.server", Logging::Loggable(resolvedServerID));
         }
       }
       changes = d_resolver.refresh(time(nullptr));
diff --git a/pdns/recursordist/rec-system-resolve.hh b/pdns/recursordist/rec-system-resolve.hh
index f20af5a56f..4268fa925b 100644
--- a/pdns/recursordist/rec-system-resolve.hh
+++ b/pdns/recursordist/rec-system-resolve.hh
@@ -38,7 +38,7 @@ class RecResolve
 {
 public:
   // Should be called before any getInstance() call is done
-  static void setInstanceParameters(time_t ttl, const std::function<void()>& callback);
+  static void setInstanceParameters(std::string serverID, time_t ttl, const std::function<void()>& callback);
   static RecResolve& getInstance();
 
   RecResolve(time_t ttl = 60, const std::function<void()>& callback = nullptr);
@@ -95,6 +95,7 @@ private:
 
   Refresher d_refresher;
 
+  static std::string s_serverID;
   static std::function<void()> s_callback;
   static time_t s_ttl;
 };
diff --git a/pdns/recursordist/settings/rust/Cargo.toml b/pdns/recursordist/settings/rust/Cargo.toml
index 80ab080e98..b635af385c 100644
--- a/pdns/recursordist/settings/rust/Cargo.toml
+++ b/pdns/recursordist/settings/rust/Cargo.toml
@@ -13,7 +13,7 @@ serde = { version = "1.0", features = ["derive"] }
 serde_yaml = "0.9"
 ipnet = "2.8"
 once_cell = "1.18.0"
-hostname-validator = "1.1.1"
+hostname-validator = "1.1.1" # This is temporary. PR 13819 has the infra to call C++ from, so we can arrange for DNSName::ishostname() to be called instead of importing another crate aftyer that one is merged.
 
 [build-dependencies]
 cxx-build = "1.0"
diff --git a/pdns/recursordist/test-rec-system-resolve.cc b/pdns/recursordist/test-rec-system-resolve.cc
index 82707d6dc8..7c559b6706 100644
--- a/pdns/recursordist/test-rec-system-resolve.cc
+++ b/pdns/recursordist/test-rec-system-resolve.cc
@@ -11,7 +11,7 @@ BOOST_AUTO_TEST_SUITE(rec_system_resolve)
 BOOST_AUTO_TEST_CASE(test_basic_resolve)
 {
 
-  pdns::RecResolve::setInstanceParameters(60, nullptr);
+  pdns::RecResolve::setInstanceParameters("foo", 60, nullptr);
   auto& sysResolve = pdns::RecResolve::getInstance();
 
   auto address = sysResolve.lookupAndRegister("localhost", time(nullptr));