From: Remi Gacogne Date: Tue, 31 Mar 2026 14:12:09 +0000 (+0200) Subject: dnsdist: Do not keep the parsed EDNS options around X-Git-Tag: auth-5.1.0-beta1~72^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=295fff2bbd49491105366176d135de34afc41ca8;p=thirdparty%2Fpdns.git dnsdist: Do not keep the parsed EDNS options around The idea to keep the EDNS options around to avoid parsing them a second time was a nice one, but invalidation is error-prone and this is rarely useful in practice. Signed-off-by: Remi Gacogne --- diff --git a/pdns/dnsdistdist/dnsdist-actions-factory.cc b/pdns/dnsdistdist/dnsdist-actions-factory.cc index 1eb69b11f1..d6b40f8c0c 100644 --- a/pdns/dnsdistdist/dnsdist-actions-factory.cc +++ b/pdns/dnsdistdist/dnsdist-actions-factory.cc @@ -1780,21 +1780,16 @@ public: } // We need to check if EDNS Options exist - if (dnsquestion->ednsOptions == nullptr && !parseEDNSOptions(*dnsquestion)) { + auto ednsOptions = parseEDNSOptions(*dnsquestion); + if (!ednsOptions) { // Maybe parsed, but no EDNS found return Action::None; } - if (dnsquestion->ednsOptions == nullptr) { - // Parsing failed, log a warning and return - VERBOSESLOG(infolog("parsing EDNS options failed while looking for OpenTelemetry Trace ID"), - dnsquestion->getLogger()->info(Logr::Info, "Parsing EDNS options failed while looking for OpenTelemetry Trace ID")); - return Action::None; - } if (d_useIncomingTraceparent) { pdns::trace::TraceID traceID; pdns::trace::SpanID spanID; - if (pdns::trace::extractOTraceIDs(*(dnsquestion->ednsOptions), EDNSOptionCode::EDNSOptionCodeEnum(d_traceparentOptionCode), traceID, spanID)) { + if (pdns::trace::extractOTraceIDs(*(ednsOptions), EDNSOptionCode::EDNSOptionCodeEnum(d_traceparentOptionCode), traceID, spanID)) { tracer->setTraceID(traceID); if (spanID != pdns::trace::s_emptySpanID) { tracer->setRootSpanID(spanID); @@ -1821,8 +1816,6 @@ public: size_t existingOptLen = optLen; removeEDNSOptionFromOPT(reinterpret_cast(&dnsquestion->getMutableData().at(optStart)), &optLen, d_traceparentOptionCode); dnsquestion->getMutableData().resize(dnsquestion->getData().size() - (existingOptLen - optLen)); - // Ensure the EDNS Option View is not out of date - dnsquestion->ednsOptions.reset(); } return Action::None; diff --git a/pdns/dnsdistdist/dnsdist-ecs.cc b/pdns/dnsdistdist/dnsdist-ecs.cc index 17f121e752..e94e6503ab 100644 --- a/pdns/dnsdistdist/dnsdist-ecs.cc +++ b/pdns/dnsdistdist/dnsdist-ecs.cc @@ -466,23 +466,20 @@ static bool replaceEDNSClientSubnetOption(PacketBuffer& packet, size_t maximumSi /* This function looks for an OPT RR, return true if a valid one was found (even if there was no options) and false otherwise. */ -bool parseEDNSOptions(const DNSQuestion& dnsQuestion) +std::optional parseEDNSOptions(const DNSQuestion& dnsQuestion) { + EDNSOptionViewMap ednsOptions{}; const auto dnsHeader = dnsQuestion.getHeader(); - if (dnsQuestion.ednsOptions != nullptr) { - return true; - } - - // dnsQuestion.ednsOptions is mutable - dnsQuestion.ednsOptions = std::make_unique(); - if (ntohs(dnsHeader->arcount) == 0) { /* nothing in additional so no EDNS */ - return false; + return std::nullopt; } if (ntohs(dnsHeader->ancount) != 0 || ntohs(dnsHeader->nscount) != 0 || ntohs(dnsHeader->arcount) > 1) { - return slowParseEDNSOptions(dnsQuestion.getData(), *dnsQuestion.ednsOptions); + if (slowParseEDNSOptions(dnsQuestion.getData(), ednsOptions)) { + return ednsOptions; + } + return std::nullopt; } size_t remaining = 0; @@ -491,11 +488,14 @@ bool parseEDNSOptions(const DNSQuestion& dnsQuestion) if (res == 0) { // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) - res = getEDNSOptions(reinterpret_cast(&dnsQuestion.getData().at(optRDPosition)), remaining, *dnsQuestion.ednsOptions); - return (res == 0); + res = getEDNSOptions(reinterpret_cast(&dnsQuestion.getData().at(optRDPosition)), remaining, ednsOptions); + if (res != 0) { + return std::nullopt; + } + return ednsOptions; } - return false; + return std::nullopt; } static bool addECSToExistingOPT(PacketBuffer& packet, size_t maximumSize, const string& newECSOption, size_t optRDLenPosition, bool& ecsAdded) diff --git a/pdns/dnsdistdist/dnsdist-ecs.hh b/pdns/dnsdistdist/dnsdist-ecs.hh index 300195a110..3692ffec9f 100644 --- a/pdns/dnsdistdist/dnsdist-ecs.hh +++ b/pdns/dnsdistdist/dnsdist-ecs.hh @@ -23,6 +23,7 @@ #include +#include "ednsoptions.hh" #include "iputils.hh" #include "noinitvector.hh" @@ -46,7 +47,7 @@ bool setNegativeAndAdditionalSOA(DNSQuestion& dnsQuestion, bool nxd, const DNSNa bool handleEDNSClientSubnet(DNSQuestion& dnsQuestion, bool& ednsAdded, bool& ecsAdded); bool handleEDNSClientSubnet(PacketBuffer& packet, size_t maximumSize, size_t qnameWireLength, bool& ednsAdded, bool& ecsAdded, bool overrideExisting, const string& newECSOption); -bool parseEDNSOptions(const DNSQuestion& dnsQuestion); +std::optional parseEDNSOptions(const DNSQuestion& dnsQuestion); bool queryHasEDNS(const DNSQuestion& dnsQuestion); bool getEDNS0Record(const PacketBuffer& packet, EDNS0Record& edns0); diff --git a/pdns/dnsdistdist/dnsdist-lua-bindings-dnsquestion.cc b/pdns/dnsdistdist/dnsdist-lua-bindings-dnsquestion.cc index 1b2162940c..f6e36155e2 100644 --- a/pdns/dnsdistdist/dnsdist-lua-bindings-dnsquestion.cc +++ b/pdns/dnsdistdist/dnsdist-lua-bindings-dnsquestion.cc @@ -164,15 +164,13 @@ void setupLuaBindingsDNSQuestion([[maybe_unused]] LuaContext& luaCtx) return true; }); }); - luaCtx.registerFunction (DNSQuestion::*)() const>("getEDNSOptions", [](const DNSQuestion& dnsQuestion) { - if (dnsQuestion.ednsOptions == nullptr) { - parseEDNSOptions(dnsQuestion); - if (dnsQuestion.ednsOptions == nullptr) { - throw std::runtime_error("parseEDNSOptions should have populated the EDNS options"); - } + luaCtx.registerFunction (DNSQuestion::*)() const>("getEDNSOptions", [](const DNSQuestion& dnsQuestion) -> std::map { + auto ednsOptions = parseEDNSOptions(dnsQuestion); + if (!ednsOptions) { + return {}; } - return *dnsQuestion.ednsOptions; + return *ednsOptions; }); luaCtx.registerFunction("getTrailingData", [](const DNSQuestion& dnsQuestion) { return dnsQuestion.getTrailingData(); @@ -516,15 +514,13 @@ void setupLuaBindingsDNSQuestion([[maybe_unused]] LuaContext& luaCtx) }); }); - luaCtx.registerFunction (DNSResponse::*)() const>("getEDNSOptions", [](const DNSResponse& dnsQuestion) { - if (dnsQuestion.ednsOptions == nullptr) { - parseEDNSOptions(dnsQuestion); - if (dnsQuestion.ednsOptions == nullptr) { - throw std::runtime_error("parseEDNSOptions should have populated the EDNS options"); - } + luaCtx.registerFunction (DNSResponse::*)() const>("getEDNSOptions", [](const DNSResponse& dnsQuestion) -> std::map { + auto ednsOptions = parseEDNSOptions(dnsQuestion); + if (!ednsOptions) { + return {}; } - return *dnsQuestion.ednsOptions; + return *ednsOptions; }); luaCtx.registerFunction("getTrailingData", [](const DNSResponse& dnsQuestion) { return dnsQuestion.getTrailingData(); diff --git a/pdns/dnsdistdist/dnsdist-lua-ffi.cc b/pdns/dnsdistdist/dnsdist-lua-ffi.cc index c764b63677..4c953d7e2e 100644 --- a/pdns/dnsdistdist/dnsdist-lua-ffi.cc +++ b/pdns/dnsdistdist/dnsdist-lua-ffi.cc @@ -399,16 +399,13 @@ static void fill_edns_option(const EDNSOptionViewValue& value, dnsdist_ffi_ednso // returns the length of the resulting 'out' array. 'out' is not set if the length is 0 size_t dnsdist_ffi_dnsquestion_get_edns_options(dnsdist_ffi_dnsquestion_t* dq, const dnsdist_ffi_ednsoption_t** out) { - if (dq->dq->ednsOptions == nullptr) { - parseEDNSOptions(*(dq->dq)); - - if (dq->dq->ednsOptions == nullptr) { - return 0; - } + auto ednsOptions = parseEDNSOptions(*(dq->dq)); + if (!ednsOptions) { + return 0; } size_t totalCount = 0; - for (const auto& option : *dq->dq->ednsOptions) { + for (const auto& option : *ednsOptions) { totalCount += option.second.values.size(); } @@ -418,7 +415,7 @@ size_t dnsdist_ffi_dnsquestion_get_edns_options(dnsdist_ffi_dnsquestion_t* dq, c dq->ednsOptionsVect->clear(); dq->ednsOptionsVect->resize(totalCount); size_t pos = 0; - for (const auto& option : *dq->dq->ednsOptions) { + for (const auto& option : *ednsOptions) { for (const auto& entry : option.second.values) { fill_edns_option(entry, dq->ednsOptionsVect->at(pos)); dq->ednsOptionsVect->at(pos).optionCode = option.first; diff --git a/pdns/dnsdistdist/dnsdist.hh b/pdns/dnsdistdist/dnsdist.hh index 1810227b0d..de74e31e7f 100644 --- a/pdns/dnsdistdist/dnsdist.hh +++ b/pdns/dnsdistdist/dnsdist.hh @@ -43,7 +43,6 @@ #include "dnsdist-doh-common.hh" #include "doq.hh" #include "doh3.hh" -#include "ednsoptions.hh" #include "iputils.hh" #include "misc.hh" #include "mplexer.hh" @@ -78,7 +77,6 @@ struct DNSQuestion } PacketBuffer& getMutableData() { - ednsOptions.reset(); return data; } @@ -201,7 +199,6 @@ public: InternalQueryState& ids; std::unique_ptr ecs{nullptr}; std::string sni; /* Server Name Indication, if any (DoT or DoH) */ - mutable std::unique_ptr ednsOptions; /* this needs to be mutable because it is parsed just in time, when DNSQuestion is read-only */ std::shared_ptr d_incomingTCPState{nullptr}; std::unique_ptr> proxyProtocolValues{nullptr}; uint16_t ecsPrefixLength; diff --git a/pdns/dnsdistdist/test-dnsdist_cc.cc b/pdns/dnsdistdist/test-dnsdist_cc.cc index 91440d50bc..9e3cdb6e40 100644 --- a/pdns/dnsdistdist/test-dnsdist_cc.cc +++ b/pdns/dnsdistdist/test-dnsdist_cc.cc @@ -164,11 +164,11 @@ static void validateECS(const PacketBuffer& packet, const ComboAddress& expected ids.qname = DNSName(reinterpret_cast(packet.data()), packet.size(), sizeof(dnsheader), false, &ids.qtype, &ids.qclass); // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) DNSQuestion dnsQuestion(ids, const_cast(packet)); - BOOST_CHECK(parseEDNSOptions(dnsQuestion)); - BOOST_REQUIRE(dnsQuestion.ednsOptions != nullptr); - BOOST_CHECK_EQUAL(dnsQuestion.ednsOptions->size(), 1U); - const auto& ecsOption = dnsQuestion.ednsOptions->find(EDNSOptionCode::ECS); - BOOST_REQUIRE(ecsOption != dnsQuestion.ednsOptions->cend()); + auto ednsOptions = parseEDNSOptions(dnsQuestion); + BOOST_REQUIRE(ednsOptions); + BOOST_CHECK_EQUAL(ednsOptions->size(), 1U); + const auto& ecsOption = ednsOptions->find(EDNSOptionCode::ECS); + BOOST_REQUIRE(ecsOption != ednsOptions->cend()); string expectedOption; generateECSOption(expected, expectedOption, expected.sin4.sin_family == AF_INET ? ECSSourcePrefixV4 : ECSSourcePrefixV6); @@ -2466,11 +2466,11 @@ BOOST_AUTO_TEST_CASE(test_setEDNSOption) BOOST_CHECK_EQUAL(edns0.extRCode, 0U); BOOST_CHECK_EQUAL(ntohs(edns0.extFlags), EDNS_HEADER_FLAG_DO); - BOOST_REQUIRE(parseEDNSOptions(dnsQuestion)); - BOOST_REQUIRE(dnsQuestion.ednsOptions != nullptr); - BOOST_CHECK_EQUAL(dnsQuestion.ednsOptions->size(), 1U); - const auto& ecsOption = dnsQuestion.ednsOptions->find(EDNSOptionCode::COOKIE); - BOOST_REQUIRE(ecsOption != dnsQuestion.ednsOptions->cend()); + auto ednsOptions = parseEDNSOptions(dnsQuestion); + BOOST_REQUIRE(ednsOptions); + BOOST_CHECK_EQUAL(ednsOptions->size(), 1U); + const auto& ecsOption = ednsOptions->find(EDNSOptionCode::COOKIE); + BOOST_REQUIRE(ecsOption != ednsOptions->cend()); BOOST_REQUIRE_EQUAL(ecsOption->second.values.size(), 1U); BOOST_CHECK_EQUAL(cookiesOptionStr, std::string(ecsOption->second.values.at(0).content, ecsOption->second.values.at(0).size));