]> git.ipfire.org Git - thirdparty/squid.git/blobdiff - src/dns_internal.cc
Source Format Enforcement (#532)
[thirdparty/squid.git] / src / dns_internal.cc
index 2d395892740353e1d4fd238de03c8207b2046cfe..849a987c5d222401aecf187396c0f9e00ec54c1d 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
+ * Copyright (C) 1996-2020 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
@@ -9,7 +9,9 @@
 /* DEBUG: section 78    DNS lookups; interacts with dns/rfc1035.cc */
 
 #include "squid.h"
+#include "base/CodeContext.h"
 #include "base/InstanceId.h"
+#include "base/RunnersRegistry.h"
 #include "comm.h"
 #include "comm/Connection.h"
 #include "comm/ConnOpener.h"
@@ -95,8 +97,6 @@ static const char *Rcodes[] = {
     "Bad OPT Version or TSIG Signature Failure"
 };
 
-typedef struct _ns ns;
-
 typedef struct _sp sp;
 
 class idns_query
@@ -104,27 +104,10 @@ class idns_query
     CBDATA_CLASS(idns_query);
 
 public:
-    idns_query() :
-        sz(0),
-        query_id(0),
-        nsends(0),
-        need_vc(0),
-        permit_mdns(false),
-        pending(0),
-        callback(NULL),
-        callback_data(NULL),
-        attempt(0),
-        rcode(0),
-        queue(NULL),
-        slave(NULL),
-        master(NULL),
-        domain(0),
-        do_searchpath(0),
-        message(NULL),
-        ancount(0),
-        error(NULL)
+    idns_query():
+        codeContext(CodeContext::Current())
     {
-        memset(&hash, 0, sizeof(hash));
+        callback = nullptr;
         memset(&query, 0, sizeof(query));
         *buf = 0;
         *name = 0;
@@ -148,31 +131,34 @@ public:
     char buf[RESOLV_BUFSZ];
     char name[NS_MAXDNAME + 1];
     char orig[NS_MAXDNAME + 1];
-    ssize_t sz;
-    unsigned short query_id; /// random query ID sent to server; changes with every query sent
-    InstanceId<idns_query> xact_id; /// identifies our "transaction", stays constant when query is retried
+    ssize_t sz = 0;
+    unsigned short query_id = 0; ///< random query ID sent to server; changes with every query sent
+    InstanceId<idns_query> xact_id; ///< identifies our "transaction", stays constant when query is retried
 
-    int nsends;
-    int need_vc;
-    bool permit_mdns;
-    int pending;
+    int nsends = 0;
+    int need_vc = 0;
+    bool permit_mdns = false;
+    int pending = 0;
 
     struct timeval start_t;
     struct timeval sent_t;
     struct timeval queue_t;
     dlink_node lru;
+
     IDNSCB *callback;
-    void *callback_data;
-    int attempt;
-    int rcode;
-    idns_query *queue;
-    idns_query *slave;  // single linked list
-    idns_query *master; // single pointer to a shared master
-    unsigned short domain;
-    unsigned short do_searchpath;
-    rfc1035_message *message;
-    int ancount;
-    const char *error;
+    void *callback_data = nullptr;
+    CodeContext::Pointer codeContext; ///< requestor's context
+
+    int attempt = 0;
+    int rcode = 0;
+    idns_query *queue = nullptr;
+    idns_query *slave = nullptr;  // single linked list
+    idns_query *master = nullptr; // single pointer to a shared master
+    unsigned short domain = 0;
+    unsigned short do_searchpath = 0;
+    rfc1035_message *message = nullptr;
+    int ancount = 0;
+    const char *error = nullptr;
 };
 
 InstanceIdDefinitions(idns_query,  "dns");
@@ -184,40 +170,56 @@ class nsvc
     CBDATA_CLASS(nsvc);
 
 public:
-    explicit nsvc(int nsv) : ns(nsv), msglen(0), read_msglen(0), msg(new MemBuf()), queue(new MemBuf()), busy(true) {}
+    explicit nsvc(size_t nsv) : ns(nsv), msg(new MemBuf()), queue(new MemBuf()) {}
     ~nsvc();
 
-    int ns;
+    size_t ns = 0;
     Comm::ConnectionPointer conn;
-    unsigned short msglen;
-    int read_msglen;
-    MemBuf *msg;
-    MemBuf *queue;
-    bool busy;
+    unsigned short msglen = 0;
+    int read_msglen = 0;
+    MemBuf *msg = nullptr;
+    MemBuf *queue = nullptr;
+    bool busy = true;
 };
 
 CBDATA_CLASS_INIT(nsvc);
 
-struct _ns {
+class ns
+{
+public:
     Ip::Address S;
-    int nqueries;
-    int nreplies;
+    int nqueries = 0;
+    int nreplies = 0;
 #if WHEN_EDNS_RESPONSES_ARE_PARSED
-    int last_seen_edns;
+    int last_seen_edns = 0;
 #endif
-    bool mDNSResolver;
-    nsvc *vc;
+    bool mDNSResolver = false;
+    nsvc *vc = nullptr;
 };
 
+namespace Dns
+{
+
+/// manage DNS internal component
+class ConfigRr : public RegisteredRunner
+{
+public:
+    /* RegisteredRunner API */
+    virtual void startReconfigure() override;
+    virtual void endingShutdown() override;
+};
+
+RunnerRegistrationEntry(ConfigRr);
+
+} // namespace Dns
+
 struct _sp {
     char domain[NS_MAXDNAME];
     int queries;
 };
 
-static ns *nameservers = NULL;
+static std::vector<ns> nameservers;
 static sp *searchpath = NULL;
-static int nns = 0;
-static int nns_alloc = 0;
 static int nns_mdns_count = 0;
 static int npc = 0;
 static int npc_alloc = 0;
@@ -258,7 +260,6 @@ static OBJH idnsStats;
 static void idnsAddNameserver(const char *buf);
 static void idnsAddMDNSNameservers();
 static void idnsAddPathComponent(const char *buf);
-static void idnsFreeNameservers(void);
 static void idnsFreeSearchpath(void);
 static bool idnsParseNameservers(void);
 static bool idnsParseResolvConf(void);
@@ -284,6 +285,7 @@ static void idnsRcodeCount(int, int);
 static CLCB idnsVCClosed;
 static unsigned short idnsQueryID(void);
 static void idnsSendSlaveAAAAQuery(idns_query *q);
+static void idnsCallbackOnEarlyError(IDNSCB *callback, void *cbdata, const char *error);
 
 static void
 idnsCheckMDNS(idns_query *q)
@@ -309,14 +311,14 @@ idnsAddMDNSNameservers()
     // mDNS resolver addresses are explicit multicast group IPs
     if (Ip::EnableIpv6) {
         idnsAddNameserver("FF02::FB");
-        nameservers[nns-1].S.port(5353);
-        nameservers[nns-1].mDNSResolver = true;
+        nameservers.back().S.port(5353);
+        nameservers.back().mDNSResolver = true;
         ++nns_mdns_count;
     }
 
     idnsAddNameserver("224.0.0.251");
-    nameservers[nns-1].S.port(5353);
-    nameservers[nns-1].mDNSResolver = true;
+    nameservers.back().S.port(5353);
+    nameservers.back().mDNSResolver = true;
 
     ++nns_mdns_count;
 }
@@ -342,33 +344,14 @@ idnsAddNameserver(const char *buf)
         return;
     }
 
-    if (nns == nns_alloc) {
-        int oldalloc = nns_alloc;
-        ns *oldptr = nameservers;
-
-        if (nns_alloc == 0)
-            nns_alloc = 2;
-        else
-            nns_alloc <<= 1;
-
-        nameservers = (ns *)xcalloc(nns_alloc, sizeof(*nameservers));
-
-        if (oldptr && oldalloc)
-            memcpy(nameservers, oldptr, oldalloc * sizeof(*nameservers));
-
-        if (oldptr)
-            safe_free(oldptr);
-    }
-
-    assert(nns < nns_alloc);
+    nameservers.emplace_back(ns());
     A.port(NS_DEFAULTPORT);
-    nameservers[nns].S = A;
+    nameservers.back().S = A;
 #if WHEN_EDNS_RESPONSES_ARE_PARSED
-    nameservers[nns].last_seen_edns = RFC1035_DEFAULT_PACKET_SZ;
+    nameservers.back().last_seen_edns = RFC1035_DEFAULT_PACKET_SZ;
     // TODO generate a test packet to probe this NS from EDNS size and ability.
 #endif
-    debugs(78, 3, "idnsAddNameserver: Added nameserver #" << nns << " (" << A << ")");
-    ++nns;
+    debugs(78, 3, "Added nameserver #" << nameservers.size()-1 << " (" << A << ")");
 }
 
 static void
@@ -400,13 +383,6 @@ idnsAddPathComponent(const char *buf)
     ++npc;
 }
 
-static void
-idnsFreeNameservers(void)
-{
-    safe_free(nameservers);
-    nns = nns_alloc = 0;
-}
-
 static void
 idnsFreeSearchpath(void)
 {
@@ -418,9 +394,9 @@ static bool
 idnsParseNameservers(void)
 {
     bool result = false;
-    for (wordlist *w = Config.dns_nameservers; w; w = w->next) {
-        debugs(78, DBG_IMPORTANT, "Adding nameserver " << w->key << " from squid.conf");
-        idnsAddNameserver(w->key);
+    for (auto &i : Config.dns.nameservers) {
+        debugs(78, DBG_IMPORTANT, "Adding nameserver " << i << " from squid.conf");
+        idnsAddNameserver(i.c_str());
         result = true;
     }
     return result;
@@ -761,12 +737,12 @@ idnsStats(StoreEntry * sentry)
     storeAppendPrintf(sentry, "IP ADDRESS                                     # QUERIES # REPLIES Type\n");
     storeAppendPrintf(sentry, "---------------------------------------------- --------- --------- --------\n");
 
-    for (i = 0; i < nns; ++i) {
+    for (const auto &server : nameservers) {
         storeAppendPrintf(sentry, "%-45s %9d %9d %s\n",  /* Let's take the maximum: (15 IPv4/45 IPv6) */
-                          nameservers[i].S.toStr(buf,MAX_IPSTRLEN),
-                          nameservers[i].nqueries,
-                          nameservers[i].nreplies,
-                          nameservers[i].mDNSResolver?"multicast":"recurse");
+                          server.S.toStr(buf,MAX_IPSTRLEN),
+                          server.nqueries,
+                          server.nreplies,
+                          server.mDNSResolver?"multicast":"recurse");
     }
 
     storeAppendPrintf(sentry, "\nRcode Matrix:\n");
@@ -876,7 +852,7 @@ idnsInitVCConnected(const Comm::ConnectionPointer &conn, Comm::Flag status, int,
 
     if (status != Comm::OK || !conn) {
         char buf[MAX_IPSTRLEN] = "";
-        if (vc->ns < nns)
+        if (vc->ns < nameservers.size())
             nameservers[vc->ns].S.toStr(buf,MAX_IPSTRLEN);
         debugs(78, DBG_IMPORTANT, HERE << "Failed to connect to nameserver " << buf << " using TCP.");
         return;
@@ -903,15 +879,15 @@ nsvc::~nsvc()
 {
     delete queue;
     delete msg;
-    if (ns < nns) // XXX: Dns::Shutdown may have freed nameservers[]
+    if (ns < nameservers.size()) // XXX: idnsShutdownAndFreeState may have freed nameservers[]
         nameservers[ns].vc = NULL;
 }
 
 static void
-idnsInitVC(int nsv)
+idnsInitVC(size_t nsv)
 {
+    assert(nsv < nameservers.size());
     nsvc *vc = new nsvc(nsv);
-    assert(nsv < nns);
     assert(vc->conn == NULL); // MUST be NULL from the construction process!
     nameservers[nsv].vc = vc;
 
@@ -933,9 +909,9 @@ idnsInitVC(int nsv)
 }
 
 static void
-idnsSendQueryVC(idns_query * q, int nsn)
+idnsSendQueryVC(idns_query * q, size_t nsn)
 {
-    assert(nsn < nns);
+    assert(nsn < nameservers.size());
     if (nameservers[nsn].vc == NULL)
         idnsInitVC(nsn);
 
@@ -962,12 +938,19 @@ idnsSendQueryVC(idns_query * q, int nsn)
 static void
 idnsSendQuery(idns_query * q)
 {
+    // XXX: DNS sockets get closed during reconfigure produces a race between
+    // any already active connections (or ones received between closing DNS
+    // sockets and server listening sockets) and the reconfigure completing
+    // (Runner syncConfig() being run). Transactions which loose this race will
+    // produce DNS timeouts (or whatever the caller set) as their queries never
+    // get queued to be re-tried after the DNS socekts are re-opened.
+
     if (DnsSocketA < 0 && DnsSocketB < 0) {
         debugs(78, DBG_IMPORTANT, "WARNING: idnsSendQuery: Can't send query, no DNS socket!");
         return;
     }
 
-    if (nns <= 0) {
+    if (nameservers.empty()) {
         debugs(78, DBG_IMPORTANT, "WARNING: idnsSendQuery: Can't send query, no DNS nameservers known!");
         return;
     }
@@ -977,14 +960,15 @@ idnsSendQuery(idns_query * q)
     assert(q->lru.prev == NULL);
 
     int x = -1, y = -1;
-    int nsn;
+    size_t nsn;
+    const auto nsCount = nameservers.size();
 
     do {
         // only use mDNS resolvers for mDNS compatible queries
         if (!q->permit_mdns)
-            nsn = nns_mdns_count + q->nsends % (nns-nns_mdns_count);
+            nsn = nns_mdns_count + q->nsends % (nsCount - nns_mdns_count);
         else
-            nsn = q->nsends % nns;
+            nsn = q->nsends % nsCount;
 
         if (q->need_vc) {
             idnsSendQueryVC(q, nsn);
@@ -1006,7 +990,7 @@ idnsSendQuery(idns_query * q)
         if (x < 0 && nameservers[nsn].S.isIPv4())
             debugs(50, DBG_IMPORTANT, MYNAME << "FD " << DnsSocketA << ": sendto: " << xstrerr(xerrno));
 
-    } while ( (x<0 && y<0) && q->nsends % nns != 0);
+    } while ( (x<0 && y<0) && q->nsends % nsCount != 0);
 
     if (y > 0) {
         fd_bytes(DnsSocketB, y, FD_WRITE);
@@ -1025,9 +1009,7 @@ idnsSendQuery(idns_query * q)
 static int
 idnsFromKnownNameserver(Ip::Address const &from)
 {
-    int i;
-
-    for (i = 0; i < nns; ++i) {
+    for (int i = 0; static_cast<size_t>(i) < nameservers.size(); ++i) {
         if (nameservers[i].S != from)
             continue;
 
@@ -1077,95 +1059,97 @@ idnsQueryID()
     return id;
 }
 
-static void
-idnsCallback(idns_query *q, const char *error)
+/// \returns whether master or associated queries are still waiting for replies
+static bool
+idnsStillPending(const idns_query *master)
 {
-    IDNSCB *callback;
-    void *cbdata;
+    assert(!master->master); // we were given the master transaction
+    for (const idns_query *qi = master; qi; qi = qi->slave) {
+        if (qi->pending)
+            return true;
+    }
+    return false;
+}
 
-    if (error)
-        q->error = error;
+static std::ostream &
+operator <<(std::ostream &os, const idns_query &answered)
+{
+    if (answered.error)
+        os << "error \"" << answered.error << "\"";
+    else
+        os << answered.ancount << " records";
+    return os;
+}
+
+static void
+idnsCallbackOnEarlyError(IDNSCB *callback, void *cbdata, const char *error)
+{
+    // A cbdataReferenceValid() check asserts on unlocked cbdata: Early errors,
+    // by definition, happen before we store/cbdataReference() cbdata.
+    debugs(78, 6, "\"" << error << "\" for " << cbdata);
+    callback(cbdata, nullptr, 0, "Internal error", true); // hide error details
+}
 
-    if (q->master)
-        q = q->master;
+/// safely sends one set of DNS records (or an error) to the caller
+static bool
+idnsCallbackOneWithAnswer(IDNSCB *callback, void *cbdata, const idns_query &answered, const bool lastAnswer)
+{
+    if (!cbdataReferenceValid(cbdata))
+        return false;
+    const rfc1035_rr *records = answered.message ? answered.message->answer : nullptr;
+    debugs(78, 6, (lastAnswer ? "last " : "") << answered << " for " << cbdata);
+    callback(cbdata, records, answered.ancount, answered.error, lastAnswer);
+    return true;
+}
 
-    // If any of our subqueries are still pending then wait for them to complete before continuing
-    for (idns_query *q2 = q; q2; q2 = q2->slave) {
-        if (q2->pending) {
-            return;
-        }
+static void
+idnsCallbackNewCallerWithOldAnswers(IDNSCB *callback, void *cbdata, const idns_query * const master)
+{
+    const bool lastAnswer = false;
+    // iterate all queries to act on answered ones
+    for (auto query = master; query; query = query->slave) {
+        if (query->pending)
+            continue; // no answer yet
+        // no CallBack(CodeContext...) -- we always run in requestor's context
+        if (!idnsCallbackOneWithAnswer(callback, cbdata, *query, lastAnswer))
+            break; // the caller disappeared
     }
+}
 
-    /* Merge results */
-    rfc1035_message *message = q->message;
-    q->message = NULL;
-    int n = q->ancount;
-    error = q->error;
-
-    while ( idns_query *q2 = q->slave ) {
-        debugs(78, 6, HERE << "Merging DNS results " << q->name << " A has " << n << " RR, AAAA has " << q2->ancount << " RR");
-        q->slave = q2->slave;
-        q2->slave = NULL;
-        if ( !q2->error ) {
-            if (n > 0) {
-                // two sets of RR need merging
-                rfc1035_rr *result = (rfc1035_rr*) xmalloc( sizeof(rfc1035_rr)*(n + q2->ancount) );
-                if (Config.dns.v4_first) {
-                    memcpy(result, message->answer, (sizeof(rfc1035_rr)*n) );
-                    memcpy(result+n, q2->message->answer, (sizeof(rfc1035_rr)*q2->ancount) );
-                } else {
-                    memcpy(result, q2->message->answer, (sizeof(rfc1035_rr)*q2->ancount) );
-                    memcpy(result+q2->ancount, message->answer, (sizeof(rfc1035_rr)*n) );
-                }
-                n += q2->ancount;
-                // HACK WARNING, the answer rr:s have been copied in-place to
-                // result, do not free them here
-                safe_free(message->answer);
-                safe_free(q2->message->answer);
-                message->answer = result;
-                message->ancount += q2->message->ancount;
-            } else {
-                // first response empty or failed, just use the second
-                rfc1035MessageDestroy(&message);
-                message = q2->message;
-                q2->message = NULL;
-                n = q2->ancount;
-                error = NULL;
-            }
-        }
-        delete q2;
+static void
+idnsCallbackAllCallersWithNewAnswer(const idns_query * const answered, const bool lastAnswer)
+{
+    debugs(78, 8, (lastAnswer ? "last " : "") << *answered);
+    const auto master = answered->master ? answered->master : answered;
+    // iterate all queued lookup callers
+    for (auto looker = master; looker; looker = looker->queue) {
+        CallBack(looker->codeContext, [&] {
+            (void)idnsCallbackOneWithAnswer(looker->callback, looker->callback_data,
+            *answered, lastAnswer);
+        });
     }
+}
 
-    debugs(78, 6, HERE << "Sending " << n << " (" << (error ? error : "OK") << ") DNS results to caller.");
-
-    callback = q->callback;
-    q->callback = NULL;
-    const rfc1035_rr *answers = message ? message->answer : NULL;
-
-    if (cbdataReferenceValidDone(q->callback_data, &cbdata))
-        callback(cbdata, answers, n, error);
-
-    while (q->queue) {
-        idns_query *q2 = q->queue;
-        q->queue = q2->queue;
-        q2->queue = NULL;
+static void
+idnsCallback(idns_query *q, const char *error)
+{
+    if (error)
+        q->error = error;
 
-        callback = q2->callback;
-        q2->callback = NULL;
+    auto master = q->master ? q->master : q;
 
-        if (cbdataReferenceValidDone(q2->callback_data, &cbdata))
-            callback(cbdata, answers, n, error);
+    const bool lastAnswer = !idnsStillPending(master);
+    idnsCallbackAllCallersWithNewAnswer(q, lastAnswer);
 
-        delete q2;
-    }
+    if (!lastAnswer)
+        return; // wait for more answers
 
-    if (q->hash.key) {
-        hash_remove_link(idns_lookup_hash, &q->hash);
-        q->hash.key = NULL;
+    if (master->hash.key) {
+        hash_remove_link(idns_lookup_hash, &master->hash);
+        master->hash.key = nullptr;
     }
 
-    rfc1035MessageDestroy(&message);
-    delete q;
+    delete master;
 }
 
 static void
@@ -1208,8 +1192,8 @@ idnsGrokReply(const char *buf, size_t sz, int /*from_ns*/)
             // the altered NS was limiting the whole group.
             max_shared_edns = q->edns_seen;
             // may be limited by one of the others still
-            for (int i = 0; i < nns; ++i)
-                max_shared_edns = min(max_shared_edns, nameservers[i].last_seen_edns);
+            for (const auto &server : nameservers)
+                max_shared_edns = min(max_shared_edns, server.last_seen_edns);
         } else {
             nameservers[from_ns].last_seen_edns = q->edns_seen;
             // maybe reduce the global limit downwards to accomodate this NS
@@ -1411,10 +1395,11 @@ idnsCheckQueue(void *)
     idns_query *q;
     event_queued = 0;
 
-    if (0 == nns)
+    if (nameservers.empty())
         /* name servers went away; reconfiguring or shutting down */
         return;
 
+    const auto nsCount = nameservers.size();
     for (n = lru_list.tail; n; n = p) {
 
         p = n->prev;
@@ -1425,7 +1410,7 @@ idnsCheckQueue(void *)
             break;
 
         /* Query timer still running? */
-        if ((time_msec_t)tvSubMsec(q->sent_t, current_time) < (Config.Timeout.idns_retransmit * 1 << ((q->nsends - 1) / nns))) {
+        if ((time_msec_t)tvSubMsec(q->sent_t, current_time) < (Config.Timeout.idns_retransmit * 1 << ((q->nsends - 1) / nsCount))) {
             dlinkDelete(&q->lru, &lru_list);
             q->queue_t = current_time;
             dlinkAdd(q, &q->lru, &lru_list);
@@ -1480,7 +1465,7 @@ idnsReadVC(const Comm::ConnectionPointer &conn, char *buf, size_t len, Comm::Fla
         return;
     }
 
-    assert(vc->ns < nns);
+    assert(vc->ns < nameservers.size());
     debugs(78, 3, HERE << conn << ": received " << vc->msg->contentSize() << " bytes via TCP from " << nameservers[vc->ns].S << ".");
 
     idnsGrokReply(vc->msg->buf, vc->msg->contentSize(), vc->ns);
@@ -1553,7 +1538,7 @@ Dns::Init(void)
     static int init = 0;
 
     if (DnsSocketA < 0 && DnsSocketB < 0) {
-        Ip::Address addrV6; // since we don't want to alter Config.Addrs.udp_* and dont have one of our own.
+        Ip::Address addrV6; // since we do not want to alter Config.Addrs.udp_* and do not have one of our own.
 
         if (!Config.Addrs.udp_outgoing.isNoAddr())
             addrV6 = Config.Addrs.udp_outgoing;
@@ -1599,7 +1584,7 @@ Dns::Init(void)
         }
     }
 
-    assert(0 == nns);
+    assert(nameservers.empty());
     idnsAddMDNSNameservers();
     bool nsFound = idnsParseNameservers();
 
@@ -1641,12 +1626,14 @@ Dns::Init(void)
     Mgr::RegisterAction("idns", "Internal DNS Statistics", idnsStats, 0, 1);
 }
 
-void
-Dns::Shutdown(void)
+static void
+idnsShutdownAndFreeState(const char *reason)
 {
     if (DnsSocketA < 0 && DnsSocketB < 0)
         return;
 
+    debugs(78, 2, reason << ": Closing DNS sockets");
+
     if (DnsSocketA >= 0 ) {
         comm_close(DnsSocketA);
         DnsSocketA = -1;
@@ -1657,18 +1644,30 @@ Dns::Shutdown(void)
         DnsSocketB = -1;
     }
 
-    for (int i = 0; i < nns; ++i) {
-        if (nsvc *vc = nameservers[i].vc) {
+    for (const auto &server : nameservers) {
+        if (const auto vc = server.vc) {
             if (Comm::IsConnOpen(vc->conn))
                 vc->conn->close();
         }
     }
 
     // XXX: vcs are not closed/freed yet and may try to access nameservers[]
-    idnsFreeNameservers();
+    nameservers.clear();
     idnsFreeSearchpath();
 }
 
+void
+Dns::ConfigRr::endingShutdown()
+{
+    idnsShutdownAndFreeState("Shutdown");
+}
+
+void
+Dns::ConfigRr::startReconfigure()
+{
+    idnsShutdownAndFreeState("Reconfigure");
+}
+
 static int
 idnsCachedLookup(const char *key, IDNSCB * callback, void *data)
 {
@@ -1677,6 +1676,9 @@ idnsCachedLookup(const char *key, IDNSCB * callback, void *data)
     if (!old)
         return 0;
 
+    // XXX: We are collapsing this DNS query (B) onto another one (A), but there
+    // is no code to later send B if the A answer has unshareable 0 TTL records.
+
     idns_query *q = new idns_query;
     // no query_id on this instance.
 
@@ -1686,6 +1688,13 @@ idnsCachedLookup(const char *key, IDNSCB * callback, void *data)
     q->queue = old->queue;
     old->queue = q;
 
+    // This check must follow cbdataReference() above because our callback code
+    // needs a locked cbdata to call cbdataReferenceValid().
+    if (idnsStillPending(old))
+        idnsCallbackNewCallerWithOldAnswers(callback, data, old);
+    // else: idns_lookup_hash is not a cache so no pending lookups means we are
+    // in a reentrant lookup and will be called back when dequeued.
+
     return 1;
 }
 
@@ -1735,7 +1744,7 @@ idnsALookup(const char *name, IDNSCB * callback, void *data)
     // Prevent buffer overflow on q->name
     if (nameLength > NS_MAXDNAME) {
         debugs(23, DBG_IMPORTANT, "SECURITY ALERT: DNS name too long to perform lookup: '" << name << "'. see access.log for details.");
-        callback(data, NULL, 0, "Internal error");
+        idnsCallbackOnEarlyError(callback, data, "huge name");
         return;
     }
 
@@ -1746,7 +1755,7 @@ idnsALookup(const char *name, IDNSCB * callback, void *data)
     q->query_id = idnsQueryID();
 
     int nd = 0;
-    for (unsigned int i = 0; i < nameLength; ++i)
+    for (size_t i = 0; i < nameLength; ++i)
         if (name[i] == '.')
             ++nd;
 
@@ -1771,7 +1780,7 @@ idnsALookup(const char *name, IDNSCB * callback, void *data)
 
     if (q->sz < 0) {
         /* problem with query data -- query not sent */
-        callback(data, NULL, 0, "Internal error");
+        idnsCallbackOnEarlyError(callback, data, "rfc3596BuildAQuery error");
         delete q;
         return;
     }
@@ -1809,7 +1818,7 @@ idnsPTRLookup(const Ip::Address &addr, IDNSCB * callback, void *data)
 
     if (q->sz < 0) {
         /* problem with query data -- query not sent */
-        callback(data, NULL, 0, "Internal error");
+        idnsCallbackOnEarlyError(callback, data, "rfc3596BuildPTRQuery error");
         delete q;
         return;
     }
@@ -1833,7 +1842,7 @@ idnsPTRLookup(const Ip::Address &addr, IDNSCB * callback, void *data)
 variable_list *
 snmp_netDnsFn(variable_list * Var, snint * ErrP)
 {
-    int i, n = 0;
+    int n = 0;
     variable_list *Answer = NULL;
     MemBuf tmp;
     debugs(49, 5, "snmp_netDnsFn: Processing request: " << snmpDebugOid(Var->name, Var->name_length, tmp));
@@ -1843,8 +1852,8 @@ snmp_netDnsFn(variable_list * Var, snint * ErrP)
 
     case DNS_REQ:
 
-        for (i = 0; i < nns; ++i)
-            n += nameservers[i].nqueries;
+        for (const auto &server : nameservers)
+            n += server.nqueries;
 
         Answer = snmp_var_new_integer(Var->name, Var->name_length,
                                       n,
@@ -1853,8 +1862,8 @@ snmp_netDnsFn(variable_list * Var, snint * ErrP)
         break;
 
     case DNS_REP:
-        for (i = 0; i < nns; ++i)
-            n += nameservers[i].nreplies;
+        for (const auto &server : nameservers)
+            n += server.nreplies;
 
         Answer = snmp_var_new_integer(Var->name, Var->name_length,
                                       n,
@@ -1864,7 +1873,7 @@ snmp_netDnsFn(variable_list * Var, snint * ErrP)
 
     case DNS_SERVERS:
         Answer = snmp_var_new_integer(Var->name, Var->name_length,
-                                      nns,
+                                      nameservers.size(),
                                       SMI_COUNTER32);
 
         break;