rec: rework of #14822: fix a difference between record-cache hit and miss in some...

Fixes: CNAME with target non-existent record in auth zone causes segfault

diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc
index d5d9f7c275fa5c777f798fbf7585cc178ac23009..6d29075199c56b7d8d24a79b9d5476266e718a96 100644 (file)
--- a/pdns/recursordist/syncres.cc
+++ b/pdns/recursordist/syncres.cc
@@ -1718,22 +1718,34 @@ int SyncRes::doResolve(const DNSName& qname, const QType qtype, vector<DNSRecord
   LOG(prefix << qname << ": doResolve" << endl);
 
   // Look in cache only
+  vector<DNSRecord> retq;
   bool old = setCacheOnly(true);
   bool fromCache = false;
   // For cache peeking, we tell doResolveNoQNameMinimization not to consider the (non-recursive) forward case.
   // Otherwise all queries in a forward domain will be forwarded, while we want to consult the cache.
-  const auto retSizeBeforeCall = ret.size();
-  int res = doResolveNoQNameMinimization(qname, qtype, ret, depth, beenthere, context, &fromCache, nullptr);
+  int res{};
+  try {
+    // The cache lookup below can have three types of result:
+    // Case 1: successful. In that case the records will be added to the end result below and we're done.
+    // Case 2: unsuccessful. In that case the records in retq wil be discarded. E.g. there
+    // might be records as the lookup found a CNAME chain, but the target is missing from the cache.
+    // Case 3: an exception is thrown, in that case we're still interested in the (partial) results in retq.
+    // This can e.g. happen on a too-long CNAME chain.
+    res = doResolveNoQNameMinimization(qname, qtype, retq, depth, beenthere, context, &fromCache, nullptr);
+  }
+  catch (...) {
+    ret.insert(ret.end(), std::make_move_iterator(retq.begin()), std::make_move_iterator(retq.end()));
+    throw;
+  }
   setCacheOnly(old);
   if (fromCache) {
     LOG(prefix << qname << ": Step0 Found in cache" << endl);
     if (d_appliedPolicy.d_type != DNSFilterEngine::PolicyType::None && (d_appliedPolicy.d_kind == DNSFilterEngine::PolicyKind::NXDOMAIN || d_appliedPolicy.d_kind == DNSFilterEngine::PolicyKind::NODATA)) {
       ret.clear();
     }
+    ret.insert(ret.end(), std::make_move_iterator(retq.begin()), std::make_move_iterator(retq.end()));
     return res;
   }
-  // Erase unwanted cache lookup preliminary results in the returned records
-  ret.resize(retSizeBeforeCall);
   LOG(prefix << qname << ": Step0 Not cached" << endl);
 
   const unsigned int qnamelen = qname.countLabels();
@@ -1830,7 +1842,7 @@ int SyncRes::doResolve(const DNSName& qname, const QType qtype, vector<DNSRecord
       LOG(prefix << qname << ": Step4 Resolve A for child " << child << endl);
       bool oldFollowCNAME = d_followCNAME;
       d_followCNAME = false;
-      vector<DNSRecord> retq;
+      retq.clear();
       StopAtDelegation stopAtDelegation = Stop;
       res = doResolveNoQNameMinimization(child, QType::A, retq, depth, beenthere, context, nullptr, &stopAtDelegation);
       d_followCNAME = oldFollowCNAME;

diff --git a/pdns/recursordist/test-syncres_cc3.cc b/pdns/recursordist/test-syncres_cc3.cc
index 2599b0fd258c3c4623035488ddec39e597a073eb..621802900bc10627cecd4847e6af8aec69de7235 100644 (file)
--- a/pdns/recursordist/test-syncres_cc3.cc
+++ b/pdns/recursordist/test-syncres_cc3.cc
@@ -1478,6 +1478,139 @@ BOOST_AUTO_TEST_CASE(test_auth_zone_oob_cname)
   BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Indeterminate);
 }
 
+BOOST_AUTO_TEST_CASE(test_auth_cname_to_oob_target)
+{
+  std::unique_ptr<SyncRes> resolver;
+  initSR(resolver, false, false);
+
+  resolver->setQNameMinimization();
+  primeHints();
+
+  size_t queriesCount = 0;
+  const DNSName target("cname.example.com.");
+  const DNSName existingname("existing.test.xx.");
+  const DNSName target1Cname("cname1.example.com.");
+  const DNSName authZone("test.xx");
+
+  SyncRes::AuthDomain authDomain;
+
+  DNSRecord record;
+  record.d_place = DNSResourceRecord::ANSWER;
+  record.d_name = existingname;
+  record.d_type = QType::A;
+  record.d_ttl = 1800;
+  record.setContent(std::make_shared<ARecordContent>("127.0.0.1"));
+  authDomain.d_records.insert(record);
+
+  (*SyncRes::t_sstorage.domainmap)[authZone] = authDomain;
+
+  resolver->setAsyncCallback([&](const ComboAddress& address, const DNSName& domain, int /* type */, bool /* doTCP */, bool /* sendRDQuery */, int /* EDNS0Level */, struct timeval* /* now */, boost::optional<Netmask>& /* srcmask */, const ResolveContext& /* context */, LWResult* res, bool* /* chained */) {
+    queriesCount++;
+    if (isRootServer(address) || domain == DNSName("com") || domain == DNSName("example.com")) {
+      setLWResult(res, 0, false, false, true);
+      addRecordToLW(res, domain, QType::NS, "a.gtld-servers.net.", DNSResourceRecord::AUTHORITY, 172800);
+      addRecordToLW(res, "a.gtld-servers.net.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600);
+      return LWResult::Result::Success;
+    }
+    if (domain == target) {
+      setLWResult(res, 0, true, false, false);
+      addRecordToLW(res, domain, QType::CNAME, target1Cname.toString());
+      return LWResult::Result::Success;
+    }
+    if (domain == target1Cname) {
+      setLWResult(res, 0, true, false, false);
+      addRecordToLW(res, domain, QType::CNAME, existingname.toString());
+      return LWResult::Result::Success;
+    }
+
+    return LWResult::Result::Timeout;
+  });
+
+  vector<DNSRecord> ret;
+  int res = resolver->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, 0);
+  BOOST_REQUIRE_EQUAL(ret.size(), 3U);
+  BOOST_CHECK(ret[0].d_type == QType::CNAME);
+  BOOST_CHECK(ret[1].d_type == QType::CNAME);
+  BOOST_CHECK(ret[2].d_type == QType::A);
+  BOOST_CHECK_EQUAL(resolver->getValidationState(), vState::Indeterminate);
+
+  /* a second time, from the cache */
+  ret.clear();
+  res = resolver->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, 0);
+  BOOST_REQUIRE_EQUAL(ret.size(), 3U);
+  BOOST_CHECK(ret[0].d_type == QType::CNAME);
+  BOOST_CHECK(ret[1].d_type == QType::CNAME);
+  BOOST_CHECK(ret[2].d_type == QType::A);
+  BOOST_CHECK_EQUAL(resolver->getValidationState(), vState::Indeterminate);
+}
+
+BOOST_AUTO_TEST_CASE(test_auth_cname_to_non_existent_oob_target)
+{
+  std::unique_ptr<SyncRes> resolver;
+  initSR(resolver, false, false);
+
+  resolver->setQNameMinimization();
+  primeHints();
+
+  const DNSName target("cname.example.com.");
+  const DNSName existingname("existing.test.xx.");
+  const DNSName target1Cname("cname1.example.com.");
+  const DNSName target2Cname("cname-target.test.xx.");
+  const DNSName authZone("test.xx");
+
+  SyncRes::AuthDomain authDomain;
+
+  DNSRecord record;
+  record.d_place = DNSResourceRecord::ANSWER;
+  record.d_name = existingname;
+  record.d_type = QType::A;
+  record.d_ttl = 1800;
+  record.setContent(std::make_shared<ARecordContent>("127.0.0.1"));
+  authDomain.d_records.insert(record);
+
+  (*SyncRes::t_sstorage.domainmap)[authZone] = authDomain;
+
+  resolver->setAsyncCallback([&](const ComboAddress& address, const DNSName& domain, int /* type */, bool /* doTCP */, bool /* sendRDQuery */, int /* EDNS0Level */, struct timeval* /* now */, boost::optional<Netmask>& /* srcmask */, const ResolveContext& /* context */, LWResult* res, bool* /* chained */) {
+    if (isRootServer(address) || domain == DNSName("com") || domain == DNSName("example.com")) {
+      setLWResult(res, 0, false, false, true);
+      addRecordToLW(res, domain, QType::NS, "a.gtld-servers.net.", DNSResourceRecord::AUTHORITY, 172800);
+      addRecordToLW(res, "a.gtld-servers.net.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600);
+      return LWResult::Result::Success;
+    }
+    if (domain == target) {
+      setLWResult(res, 0, true, false, false);
+      addRecordToLW(res, domain, QType::CNAME, target1Cname.toString());
+      return LWResult::Result::Success;
+    }
+    if (domain == target1Cname) {
+      setLWResult(res, 0, true, false, false);
+      addRecordToLW(res, domain, QType::CNAME, target2Cname.toString());
+      return LWResult::Result::Success;
+    }
+
+    return LWResult::Result::Timeout;
+  });
+
+  vector<DNSRecord> ret;
+  int res = resolver->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, 3);
+  BOOST_REQUIRE_EQUAL(ret.size(), 2U);
+  BOOST_CHECK(ret[0].d_type == QType::CNAME);
+  BOOST_CHECK(ret[1].d_type == QType::CNAME);
+  BOOST_CHECK_EQUAL(resolver->getValidationState(), vState::Indeterminate);
+
+  /* a second time, from the cache */
+  ret.clear();
+  res = resolver->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, 3);
+  BOOST_REQUIRE_EQUAL(ret.size(), 2U);
+  BOOST_CHECK(ret[0].d_type == QType::CNAME);
+  BOOST_CHECK(ret[1].d_type == QType::CNAME);
+  BOOST_CHECK_EQUAL(resolver->getValidationState(), vState::Indeterminate);
+}
+
 BOOST_AUTO_TEST_CASE(test_auth_zone)
 {
   std::unique_ptr<SyncRes> sr;