From: Remi Gacogne Date: Thu, 30 May 2024 12:45:36 +0000 (+0200) Subject: dnsdist: Be careful not to hold onto a Runtime Configuration X-Git-Tag: rec-5.2.0-alpha1~172^2~29 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=93d8e02672905f59c722fc79fc88090883ceaf47;p=thirdparty%2Fpdns.git 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 7f36034d76..ed1e570bc6 100644 --- 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& mutator); void updateRuntimeConfiguration(const std::function& 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 760715ede1..5a73fc4c02 100644 --- 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 667db8d9a8..dc33e0c7d9 100644 --- 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& node) { + slow2.visit([&now, &fmt, dynBlockDefaultAction](const SuffixMatchTree& 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 089deb52ea..842459ffa8 100644 --- 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 09be1441d2..2d97e49cc1 100644 --- 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 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 1ef3c00320..44c2f9de2f 100644 --- 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, 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;