dnsdist: Be careful not to hold onto a Runtime Configuration

diff --git a/pdns/dnsdistdist/dnsdist-configuration.hh b/pdns/dnsdistdist/dnsdist-configuration.hh
index 7f36034d765208d3ef2293bd7039ca7a34b817af..ed1e570bc6875972996e2634da0d80724c2e9864 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-configuration.hh
+++ b/pdns/dnsdistdist/dnsdist-configuration.hh
@@ -214,10 +214,21 @@ struct RuntimeConfiguration
   bool d_applyACLToProxiedClients{false};
 };
 
+/* Be careful not to hold on this for too long, it can be invalidated
+   by the next call to getCurrentRuntimeConfiguration() from the
+   same thread, so better be sure that any function you are not calling
+   while holding to this reference does not call getCurrentRuntimeConfiguration()
+   itself. When in doubt, better call getCurrentRuntimeConfiguration() twice.
+*/
 const RuntimeConfiguration& getCurrentRuntimeConfiguration();
+/* Get the runtime-immutable configuration */
 const Configuration& getImmutableConfiguration();
+/* Update the runtime-immutable part of the configuration. This function can only be called
+   during configuration time (isConfigurationDone() returns false), and will throw otherwise. */
 void updateImmutableConfiguration(const std::function<void(Configuration&)>& mutator);
 void updateRuntimeConfiguration(const std::function<void(RuntimeConfiguration&)>& mutator);
+/* Whether parsing the configuration is done, meaning the runtime-immutable part of the
+   configuration is now sealed */
 bool isConfigurationDone();
 void setConfigurationDone();
 }

diff --git a/pdns/dnsdistdist/dnsdist-lua-actions.cc b/pdns/dnsdistdist/dnsdist-lua-actions.cc
index 760715ede198b08a598c7226e036dc3544b773e6..5a73fc4c0290da50bc1f18795a9fa934afa2d354 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-lua-actions.cc
+++ b/pdns/dnsdistdist/dnsdist-lua-actions.cc
@@ -907,8 +907,7 @@ DNSAction::Action SpoofAction::operator()(DNSQuestion* dnsquestion, std::string*
 
   bool dnssecOK = false;
   bool hadEDNS = false;
-  const auto& runtimeConfiguration = dnsdist::configuration::getCurrentRuntimeConfiguration();
-  if (runtimeConfiguration.d_addEDNSToSelfGeneratedResponses && queryHasEDNS(*dnsquestion)) {
+  if (dnsdist::configuration::getCurrentRuntimeConfiguration().d_addEDNSToSelfGeneratedResponses && queryHasEDNS(*dnsquestion)) {
     hadEDNS = true;
     dnssecOK = ((getEDNSZ(*dnsquestion) & EDNS_HEADER_FLAG_DO) != 0);
   }
@@ -1011,7 +1010,7 @@ DNSAction::Action SpoofAction::operator()(DNSQuestion* dnsquestion, std::string*
   });
 
   if (hadEDNS && !raw) {
-    addEDNS(dnsquestion->getMutableData(), dnsquestion->getMaximumSize(), dnssecOK, runtimeConfiguration.d_payloadSizeSelfGenAnswers, 0);
+    addEDNS(dnsquestion->getMutableData(), dnsquestion->getMaximumSize(), dnssecOK, dnsdist::configuration::getCurrentRuntimeConfiguration().d_payloadSizeSelfGenAnswers, 0);
   }
 
   return Action::HeaderModify;

diff --git a/pdns/dnsdistdist/dnsdist-lua.cc b/pdns/dnsdistdist/dnsdist-lua.cc
index 667db8d9a8eec78843990fa449258fe5f1d35700..dc33e0c7d99af9f76e82e20762a23ac37646e4c5 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-lua.cc
+++ b/pdns/dnsdistdist/dnsdist-lua.cc
@@ -1598,7 +1598,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck)
 #ifndef DISABLE_DYNBLOCKS
   luaCtx.writeFunction("showDynBlocks", []() {
     setLuaNoSideEffect();
-    const auto runtimeConf = dnsdist::configuration::getCurrentRuntimeConfiguration();
+    const auto dynBlockDefaultAction = dnsdist::configuration::getCurrentRuntimeConfiguration().d_dynBlockAction;
     auto slow = g_dynblockNMG.getCopy();
     timespec now{};
     gettime(&now);
@@ -1610,17 +1610,17 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck)
         if (g_defaultBPFFilter && entry.second.bpf) {
           counter += g_defaultBPFFilter->getHits(entry.first.getNetwork());
         }
-        g_outputBuffer += (fmt % entry.first.toString() % (entry.second.until.tv_sec - now.tv_sec) % counter % (entry.second.warning ? "true" : "false") % DNSAction::typeToString(entry.second.action != DNSAction::Action::None ? entry.second.action : runtimeConf.d_dynBlockAction) % (g_defaultBPFFilter && entry.second.bpf ? "*" : "") % entry.second.reason).str();
+        g_outputBuffer += (fmt % entry.first.toString() % (entry.second.until.tv_sec - now.tv_sec) % counter % (entry.second.warning ? "true" : "false") % DNSAction::typeToString(entry.second.action != DNSAction::Action::None ? entry.second.action : dynBlockDefaultAction) % (g_defaultBPFFilter && entry.second.bpf ? "*" : "") % entry.second.reason).str();
       }
     }
     auto slow2 = g_dynblockSMT.getCopy();
-    slow2.visit([&now, &fmt, &runtimeConf](const SuffixMatchTree<DynBlock>& node) {
+    slow2.visit([&now, &fmt, dynBlockDefaultAction](const SuffixMatchTree<DynBlock>& node) {
       if (now < node.d_value.until) {
         string dom("empty");
         if (!node.d_value.domain.empty()) {
           dom = node.d_value.domain.toString();
         }
-        g_outputBuffer += (fmt % dom % (node.d_value.until.tv_sec - now.tv_sec) % node.d_value.blocks % (node.d_value.warning ? "true" : "false") % DNSAction::typeToString(node.d_value.action != DNSAction::Action::None ? node.d_value.action : runtimeConf.d_dynBlockAction) % "" % node.d_value.reason).str();
+        g_outputBuffer += (fmt % dom % (node.d_value.until.tv_sec - now.tv_sec) % node.d_value.blocks % (node.d_value.warning ? "true" : "false") % DNSAction::typeToString(node.d_value.action != DNSAction::Action::None ? node.d_value.action : dynBlockDefaultAction) % "" % node.d_value.reason).str();
       }
     });
   });
@@ -2226,8 +2226,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck)
     }
 
     {
-      const auto runtimeConfiguration = dnsdist::configuration::getCurrentRuntimeConfiguration();
-      if (runtimeConfiguration.d_snmpEnabled) {
+      if (dnsdist::configuration::getCurrentRuntimeConfiguration().d_snmpEnabled) {
         errlog("snmpAgent() cannot be used twice!");
         g_outputBuffer = "snmpAgent() cannot be used twice!\n";
         return;
@@ -2243,8 +2242,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck)
   });
 
   luaCtx.writeFunction("sendCustomTrap", [](const std::string& str) {
-    const auto runtimeConfiguration = dnsdist::configuration::getCurrentRuntimeConfiguration();
-    if (g_snmpAgent != nullptr && runtimeConfiguration.d_snmpTrapsEnabled) {
+    if (g_snmpAgent != nullptr && dnsdist::configuration::getCurrentRuntimeConfiguration().d_snmpTrapsEnabled) {
       g_snmpAgent->sendCustomTrap(str);
     }
   });

diff --git a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
index 089deb52ea58b3981159caa4327ed95d471a4637..842459ffa8fab1b6f9b210aac6ee89da0250552a 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
+++ b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
@@ -362,8 +362,7 @@ void IncomingHTTP2Connection::handleIO()
   gettimeofday(&now, nullptr);
 
   try {
-    const auto& currentConfig = dnsdist::configuration::getCurrentRuntimeConfiguration();
-    if (maxConnectionDurationReached(currentConfig.d_maxTCPConnectionDuration, now)) {
+    if (maxConnectionDurationReached(dnsdist::configuration::getCurrentRuntimeConfiguration().d_maxTCPConnectionDuration, now)) {
       vinfolog("Terminating DoH connection from %s because it reached the maximum TCP connection duration", d_ci.remote.toStringWithPort());
       stopIO();
       d_connectionClosing = true;

diff --git a/pdns/dnsdistdist/dnsdist-tcp.cc b/pdns/dnsdistdist/dnsdist-tcp.cc
index 09be1441d24629f4a7a33ae2e95b188bcf7ca4ee..2d97e49cc19d9804101feae00a9d7d49325c5b48 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-tcp.cc
+++ b/pdns/dnsdistdist/dnsdist-tcp.cc
@@ -485,12 +485,11 @@ void IncomingTCPConnectionState::handleResponse(const struct timeval& now, TCPRe
     return;
   }
 
-  const auto config = dnsdist::configuration::getCurrentRuntimeConfiguration();
   if (!response.isAsync()) {
     try {
       auto& ids = response.d_idstate;
       std::shared_ptr<DownstreamState> backend = response.d_ds ? response.d_ds : (response.d_connection ? response.d_connection->getDS() : nullptr);
-      if (backend == nullptr || !responseContentMatches(response.d_buffer, ids.qname, ids.qtype, ids.qclass, backend, config.d_allowEmptyResponse)) {
+      if (backend == nullptr || !responseContentMatches(response.d_buffer, ids.qname, ids.qtype, ids.qclass, backend, dnsdist::configuration::getCurrentRuntimeConfiguration().d_allowEmptyResponse)) {
         state->terminateClientConnection();
         return;
       }
@@ -1056,8 +1055,7 @@ void IncomingTCPConnectionState::handleIO()
     iostate = IOState::Done;
     IOStateGuard ioGuard(d_ioState);
 
-    const auto& currentConfig = dnsdist::configuration::getCurrentRuntimeConfiguration();
-    if (maxConnectionDurationReached(currentConfig.d_maxTCPConnectionDuration, now)) {
+    if (maxConnectionDurationReached(dnsdist::configuration::getCurrentRuntimeConfiguration().d_maxTCPConnectionDuration, now)) {
       vinfolog("Terminating TCP connection from %s because it reached the maximum TCP connection duration", d_ci.remote.toStringWithPort());
       // will be handled by the ioGuard
       // handleNewIOState(state, IOState::Done, fd, handleIOCallback);

diff --git a/pdns/dnsdistdist/dnsdist.cc b/pdns/dnsdistdist/dnsdist.cc
index 1ef3c00320538033f3e58ab916bfc7beb51273e8..44c2f9de2f0ff23551de6637fab08c369e13bc1e 100644 (file)
--- a/pdns/dnsdistdist/dnsdist.cc
+++ b/pdns/dnsdistdist/dnsdist.cc
@@ -1048,25 +1048,28 @@ static bool applyRulesToQuery(LocalHolders& holders, DNSQuestion& dnsQuestion, c
     g_rings.insertQuery(now, dnsQuestion.ids.origRemote, dnsQuestion.ids.qname, dnsQuestion.ids.qtype, dnsQuestion.getData().size(), *dnsQuestion.getHeader(), dnsQuestion.getProtocol());
   }
 
-  const auto runtimeConfig = dnsdist::configuration::getCurrentRuntimeConfiguration();
-  if (runtimeConfig.d_queryCountConfig.d_enabled) {
-    string qname = dnsQuestion.ids.qname.toLogString();
-    bool countQuery{true};
-    if (runtimeConfig.d_queryCountConfig.d_filter) {
-      auto lock = g_lua.lock();
-      std::tie(countQuery, qname) = runtimeConfig.d_queryCountConfig.d_filter(&dnsQuestion);
-    }
-
-    if (countQuery) {
-      auto records = dnsdist::QueryCount::g_queryCountRecords.write_lock();
-      if (records->count(qname) == 0) {
-        (*records)[qname] = 0;
+  {
+    const auto runtimeConfig = dnsdist::configuration::getCurrentRuntimeConfiguration();
+    if (runtimeConfig.d_queryCountConfig.d_enabled) {
+      string qname = dnsQuestion.ids.qname.toLogString();
+      bool countQuery{true};
+      if (runtimeConfig.d_queryCountConfig.d_filter) {
+        auto lock = g_lua.lock();
+        std::tie(countQuery, qname) = runtimeConfig.d_queryCountConfig.d_filter(&dnsQuestion);
+      }
+
+      if (countQuery) {
+        auto records = dnsdist::QueryCount::g_queryCountRecords.write_lock();
+        if (records->count(qname) == 0) {
+          (*records)[qname] = 0;
+        }
+        (*records)[qname]++;
       }
-      (*records)[qname]++;
     }
   }
 
 #ifndef DISABLE_DYNBLOCKS
+  const auto defaultDynBlockAction = dnsdist::configuration::getCurrentRuntimeConfiguration().d_dynBlockAction;
   auto setRCode = [&dnsQuestion](uint8_t rcode) {
     dnsdist::PacketMangling::editDNSHeaderFromPacket(dnsQuestion.getMutableData(), [rcode](dnsheader& header) {
       header.rcode = rcode;
@@ -1085,7 +1088,7 @@ static bool applyRulesToQuery(LocalHolders& holders, DNSQuestion& dnsQuestion, c
     if (now < got->second.until) {
       DNSAction::Action action = got->second.action;
       if (action == DNSAction::Action::None) {
-        action = runtimeConfig.d_dynBlockAction;
+        action = defaultDynBlockAction;
       }
 
       switch (action) {
@@ -1162,7 +1165,7 @@ static bool applyRulesToQuery(LocalHolders& holders, DNSQuestion& dnsQuestion, c
     if (now < got->until) {
       DNSAction::Action action = got->action;
       if (action == DNSAction::Action::None) {
-        action = runtimeConfig.d_dynBlockAction;
+        action = defaultDynBlockAction;
       }
       switch (action) {
       case DNSAction::Action::NoOp:
@@ -2267,7 +2270,6 @@ static void maintThread()
 
   for (;;) {
     std::this_thread::sleep_for(std::chrono::seconds(interval));
-    const auto& config = dnsdist::configuration::getCurrentRuntimeConfiguration();
 
     {
       auto lua = g_lua.lock();
@@ -2289,7 +2291,7 @@ static void maintThread()
     }
 
     counter++;
-    if (counter >= config.d_cacheCleaningDelay) {
+    if (counter >= dnsdist::configuration::getCurrentRuntimeConfiguration().d_cacheCleaningDelay) {
       /* keep track, for each cache, of whether we should keep
        expired entries */
       std::map<std::shared_ptr<DNSDistPacketCache>, bool> caches;
@@ -2325,7 +2327,7 @@ static void maintThread()
           continue;
         }
         const auto& packetCache = pair.first;
-        size_t upTo = (packetCache->getMaxEntries() * (100 - config.d_cacheCleaningPercentage)) / 100;
+        size_t upTo = (packetCache->getMaxEntries() * (100 - dnsdist::configuration::getCurrentRuntimeConfiguration().d_cacheCleaningPercentage)) / 100;
         packetCache->purgeExpired(upTo, now);
       }
       counter = 0;