From: Otto Moerbeek Date: Fri, 4 Aug 2023 11:07:59 +0000 (+0200) Subject: rec: Do not assume the records are in a particular order when determining if an answer is X-Git-Tag: rec-4.9.2~10^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f3e89185f7b64ebf7f1371d7f28d0cd62c95fda7;p=thirdparty%2Fpdns.git rec: Do not assume the records are in a particular order when determining if an answer is NODATA. (cherry picked from commit fa5f61e94e1bd354d42923a844c59b3be232c29f) --- diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc index 95ea4fd7d6..7d24f72a23 100644 --- a/pdns/recursordist/pdns_recursor.cc +++ b/pdns/recursordist/pdns_recursor.cc @@ -776,30 +776,12 @@ int getFakePTRRecords(const DNSName& qname, vector& ret) return rcode; } -static bool answerIsNOData(uint16_t requestedType, int rcode, const std::vector& records) -{ - if (rcode != RCode::NoError) { - return false; - } - for (const auto& rec : records) { - if (rec.d_place != DNSResourceRecord::ANSWER) { - /* no records in the answer section */ - return true; - } - if (rec.d_type == requestedType) { - /* we have a record, of the right type, in the right section */ - return false; - } - } - return true; -} - // RFC 6147 section 5.1 all rcodes except NXDomain should be candidate for dns64 // for NoError, check if it is NoData static bool dns64Candidate(uint16_t requestedType, int rcode, const std::vector& records) { if (rcode == RCode::NoError) { - return answerIsNOData(requestedType, rcode, records); + return SyncRes::answerIsNOData(requestedType, rcode, records); } return rcode != RCode::NXDomain; } @@ -1298,7 +1280,7 @@ void startDoResolve(void* p) // NOLINT(readability-function-cognitive-complexity bool luaHookHandled = false; if (dc->d_luaContext) { PolicyResult policyResult = PolicyResult::NoAction; - if (answerIsNOData(dc->d_mdp.d_qtype, res, ret)) { + if (SyncRes::answerIsNOData(dc->d_mdp.d_qtype, res, ret)) { if (dc->d_luaContext->nodata(dq, res, sr.d_eventTrace)) { luaHookHandled = true; shouldNotValidate = true; diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index f5efdfcb26..6f83d00747 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -5953,3 +5953,26 @@ int SyncRes::getRootNS(struct timeval now, asyncresolve_t asyncCallback, unsigne } return res; } + +bool SyncRes::answerIsNOData(uint16_t requestedType, int rcode, const std::vector& records) +{ + if (rcode != RCode::NoError) { + return false; + } + + // NOLINTNEXTLINE(readability-use-anyofallof) + for (const auto& rec : records) { + if (rec.d_place == DNSResourceRecord::ANSWER && rec.d_type == requestedType) { + /* we have a record, of the right type, in the right section */ + return false; + } + } + return true; +#if 0 + // This code should be equivalent to the code above, clang-tidy prefers any_of() + // I have doubts if that is easier to read + return !std::any_of(records.begin(), records.end(), [=](const DNSRecord& rec) { + return rec.d_place == DNSResourceRecord::ANSWER && rec.d_type == requestedType; + }); +#endif +} diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh index 2b3a3082d1..c12edd8074 100644 --- a/pdns/recursordist/syncres.hh +++ b/pdns/recursordist/syncres.hh @@ -498,6 +498,8 @@ public: return false; } + static bool answerIsNOData(uint16_t requestedType, int rcode, const std::vector& records); + static thread_local ThreadLocalStorage t_sstorage; static pdns::stat_t s_ecsqueries; diff --git a/pdns/recursordist/test-syncres_cc10.cc b/pdns/recursordist/test-syncres_cc10.cc index 4353d843fc..474c8cf8f4 100644 --- a/pdns/recursordist/test-syncres_cc10.cc +++ b/pdns/recursordist/test-syncres_cc10.cc @@ -1958,4 +1958,39 @@ BOOST_AUTO_TEST_CASE(test_locked_nonauth_update_to_auth) BOOST_CHECK_GT(secondTTL, 0); } +BOOST_AUTO_TEST_CASE(test_nodata_ok) +{ + vector vec; + vec.emplace_back("nz.compass.com", nullptr, QType::CNAME, QClass::IN, 60, 0, DNSResourceRecord::ANSWER); + vec.emplace_back("nz.compass.com", nullptr, QType::RRSIG, QClass::IN, 60, 0, DNSResourceRecord::ANSWER); + vec.emplace_back("kslicmitv6qe1behk70g8q7e572vabp0.kompass.com", nullptr, QType::NSEC3, QClass::IN, 60, 0, DNSResourceRecord::AUTHORITY); + vec.emplace_back("kslicmitv6qe1behk70g8q7e572vabp0.kompass.com", nullptr, QType::RRSIG, QClass::IN, 60, 0, DNSResourceRecord::AUTHORITY); + + BOOST_CHECK(SyncRes::answerIsNOData(QType::A, RCode::NoError, vec)); +} + +BOOST_AUTO_TEST_CASE(test_nodata_not) +{ + vector vec; + vec.emplace_back("kc-pro.westeurope.cloudapp.azure.com", nullptr, QType::A, QClass::IN, 60, 0, DNSResourceRecord::ANSWER); + vec.emplace_back("nz.compass.com", nullptr, QType::CNAME, QClass::IN, 60, 0, DNSResourceRecord::ANSWER); + vec.emplace_back("nz.compass.com", nullptr, QType::RRSIG, QClass::IN, 60, 0, DNSResourceRecord::ANSWER); + vec.emplace_back("kslicmitv6qe1behk70g8q7e572vabp0.kompass.com", nullptr, QType::NSEC3, QClass::IN, 60, 0, DNSResourceRecord::AUTHORITY); + vec.emplace_back("kslicmitv6qe1behk70g8q7e572vabp0.kompass.com", nullptr, QType::RRSIG, QClass::IN, 60, 0, DNSResourceRecord::AUTHORITY); + + BOOST_CHECK(!SyncRes::answerIsNOData(QType::A, RCode::NoError, vec)); +} + +BOOST_AUTO_TEST_CASE(test_nodata_out_of_order) +{ + vector vec; + vec.emplace_back("nz.compass.com", nullptr, QType::CNAME, QClass::IN, 60, 0, DNSResourceRecord::ANSWER); + vec.emplace_back("nz.compass.com", nullptr, QType::RRSIG, QClass::IN, 60, 0, DNSResourceRecord::ANSWER); + vec.emplace_back("kslicmitv6qe1behk70g8q7e572vabp0.kompass.com", nullptr, QType::NSEC3, QClass::IN, 60, 0, DNSResourceRecord::AUTHORITY); + vec.emplace_back("kslicmitv6qe1behk70g8q7e572vabp0.kompass.com", nullptr, QType::RRSIG, QClass::IN, 60, 0, DNSResourceRecord::AUTHORITY); + vec.emplace_back("kc-pro.westeurope.cloudapp.azure.com", nullptr, QType::A, QClass::IN, 60, 0, DNSResourceRecord::ANSWER); + + BOOST_CHECK(!SyncRes::answerIsNOData(QType::A, RCode::NoError, vec)); +} + BOOST_AUTO_TEST_SUITE_END()