From cd74c02e2f53d57ba19a9a63398ea7d3bc7cb942 Mon Sep 17 00:00:00 2001 From: dmiller Date: Tue, 18 Aug 2026 16:06:54 +0000 Subject: [PATCH] Let late replies count for DNS, but don't clobber results Enforce invariants and use asserts to be sure requests aren't left unfinished or double-freed. --- libnetutil/massdns.cc | 144 ++++++++++++++++++++++++++---------------- 1 file changed, 88 insertions(+), 56 deletions(-) diff --git a/libnetutil/massdns.cc b/libnetutil/massdns.cc index 7cb08269c..9c4559aa6 100644 --- a/libnetutil/massdns.cc +++ b/libnetutil/massdns.cc @@ -119,6 +119,7 @@ #include #include #include +#include #include "massdns.h" #include "netutil.h" #include @@ -228,9 +229,9 @@ struct dns_server { struct request { enum status_t { - READY, - WRITE_PENDING, - DONE + READY, // May be sent again + NORESULT, // Do not re-send. Okay to process matching answers. + DONE // Do not re-send. }; DNS::ResolverImpl *impl; DNS::Request *targ; @@ -402,6 +403,9 @@ public: ResolverImpl::etchosts_init(); init_host_cache(); } + ~ResolverImpl(void) { + reset(); + } /* Forward lookup table from /etc/hosts */ typedef std::pair NameRecord; static std::map etchosts; @@ -413,8 +417,17 @@ public: stat.reset(); stat.servers = servs.size(); init_host_cache(); - new_reqs.clear(); - deferred_reqs.clear(); + while (!new_reqs.empty()) { + request *req = new_reqs.front(); + new_reqs.pop_front(); + mark_as_done(req, false); + } + while (!deferred_reqs.empty()) { + request *req = new_reqs.front(); + new_reqs.pop_front(); + mark_as_done(req, false); + } + clear_done(); total_reqs = 0; } void add_dns_server(const std::string &hostname); @@ -450,6 +463,7 @@ private: std::list servs; std::list new_reqs; std::list deferred_reqs; + std::set done; int total_reqs; nsock_pool dnspool; nsock_proxychain proxy_chain; @@ -474,6 +488,8 @@ private: void process_request(int action, request *tpreq, dns_server *server); bool process_result(const std::string &name, const Record *rr, request *tpreq); bool system_resolve(DNS::Request &reqt); + void mark_as_done(request *req, bool has_answer); + void clear_done(void); void output_summary(const Stats &stat); void handle_read(nsock_pool nsp, nsock_event evt, dns_server *srv); @@ -758,24 +774,26 @@ bool DNS::ResolverImpl::resolve_nsock() std::list::iterator reqI; for(reqI = deferred_reqs.begin(); reqI != deferred_reqs.end(); reqI++) { - - status_cb(&stat); - output_summary(stat); - request *tpreq = *reqI; - if (system_resolve(*tpreq->targ)) { - stat.ok++; - } - else { - stat.nx++; + if (tpreq->status != request::DONE) { + status_cb(&stat); + output_summary(stat); + + if (system_resolve(*tpreq->targ)) { + stat.ok++; + } + else { + stat.nx++; + } } - delete tpreq; + mark_as_done(tpreq, true); } output_summary(stat); } deferred_reqs.clear(); + clear_done(); return true; } @@ -786,12 +804,11 @@ bool DNS::ResolverImpl::resolve_system() while (total_reqs > 0) { request *tpreq = new_reqs.front(); new_reqs.pop_front(); - total_reqs--; // System resolver can handle DNS::ANY as AF_UNSPEC, so no need for // alt_req's AAAA request. if (tpreq->alt_req) { - delete tpreq; + mark_as_done(tpreq, true); stat.actual--; continue; } @@ -805,9 +822,10 @@ bool DNS::ResolverImpl::resolve_system() else { stat.nx++; } - delete tpreq; + mark_as_done(tpreq, true); } assert(new_reqs.empty()); + clear_done(); return true; } @@ -840,8 +858,18 @@ void DNS::ResolverImpl::close_dns_servers() { if (serverI->status != dns_server::DISCONNECTED) { nsock_iod_delete(serverI->nsd, NSOCK_PENDING_SILENT); serverI->status = dns_server::DISCONNECTED; - serverI->to_process.clear(); - serverI->in_process.clear(); + while (!serverI->to_process.empty()) { + request *req = serverI->to_process.front(); + serverI->to_process.pop_front(); + assert(req->status != request::READY); + mark_as_done(req, false); + } + while (!serverI->in_process.empty()) { + request *req = serverI->in_process.front(); + serverI->in_process.pop_front(); + assert(req->status != request::READY); + mark_as_done(req, false); + } serverI->records.clear(); } } @@ -855,18 +883,20 @@ bool DNS::ResolverImpl::server_send(dns_server &serv) { } request *tpreq = NULL; - if (!new_reqs.empty()) { - tpreq = new_reqs.front(); - assert(tpreq != NULL); - assert(tpreq->targ != NULL); - tpreq->first_server = &serv; - new_reqs.pop_front(); - } else if (!serv.to_process.empty()) { - tpreq = serv.to_process.front(); - serv.to_process.pop_front(); - } else { - return false; - } + do { + if (!new_reqs.empty()) { + tpreq = new_reqs.front(); + assert(tpreq != NULL); + assert(tpreq->targ != NULL); + tpreq->first_server = &serv; + new_reqs.pop_front(); + } else if (!serv.to_process.empty()) { + tpreq = serv.to_process.front(); + serv.to_process.pop_front(); + } else { + return false; + } + } while (tpreq->status != request::READY); assert(tpreq != NULL); assert(tpreq->targ != NULL); @@ -922,14 +952,6 @@ void DNS::ResolverImpl::handle_write(nsock_pool nsp, nsock_event evt, request *r server->in_process.remove(req); server->to_process.push_front(req); } - - if (req->status == request::DONE) { - delete req; - } - else { - assert(req->status == request::WRITE_PENDING); - req->status = request::READY; - } } static DNS::RECORD_TYPE wire_type(DNS::RECORD_TYPE t) { @@ -969,7 +991,6 @@ void DNS::ResolverImpl::put_dns_packet_on_wire(request *req, dns_server *srv) { srv->in_process.push_front(req); srv->records[req->id] = req; - req->status = request::WRITE_PENDING; nsock_write(dnspool, srv->nsd, &DNS::ResolverImpl::write_evt_handler, WRITE_TIMEOUT, req, reinterpret_cast(packet), plen); } @@ -1050,14 +1071,7 @@ int DNS::ResolverImpl::deal_with_timedout_reads(bool adjust_timing) { output_summary(stat); stat.dropped++; - total_reqs--; - servI->records.erase(tpreq->id); - if (tpreq->status != request::WRITE_PENDING) { - delete tpreq; - } - else { - tpreq->status = request::DONE; - } + mark_as_done(tpreq, false); tpreq = NULL; // **** OR We start at the back of this server's queue @@ -1104,8 +1118,8 @@ void DNS::ResolverImpl::process_request(int action, request *tpreq, dns_server * server->records.erase(tpreq->id); server->in_process.remove(tpreq); server->to_process.remove(tpreq); + mark_as_done(tpreq, action == ACTION_FINISHED); server->reqs_on_wire--; - total_reqs--; if (action == ACTION_SYSTEM_RESOLVE) { // System resolver can handle DNS::ANY as AF_UNSPEC, so no need for // alt_req's AAAA request. @@ -1116,12 +1130,6 @@ void DNS::ResolverImpl::process_request(int action, request *tpreq, dns_server * } stat.actual--; } - if (tpreq->status != request::WRITE_PENDING) { - delete tpreq; - } - else { - tpreq->status = request::DONE; - } tpreq = NULL; break; @@ -1232,6 +1240,10 @@ void DNS::ResolverImpl::handle_read(nsock_pool nsp, nsock_event evt, dns_server } request *tpreq = infoI->second; assert(p.id == tpreq->id); + // If a different response completed the request, don't continue + if (tpreq->status == request::DONE) { + return; + } DNS::Request *reqt = tpreq->targ; assert(reqt != NULL); // Response must contain a matching query. p.queries.empty() was already checked. @@ -1659,6 +1671,26 @@ bool DNS::ResolverImpl::system_resolve(DNS::Request &reqt) return true; } +void DNS::ResolverImpl::mark_as_done(request *req, bool has_answer) +{ + if (req->status == request::READY) { + total_reqs--; + } + req->status = (has_answer ? request::DONE : request::NORESULT); + done.insert(req); +} + +void DNS::ResolverImpl::clear_done(void) +{ + for (std::set::iterator it = done.begin(); + it != done.end(); ++it) { + request *req = *it; + assert(req->status != request::READY); + delete req; + } + done.clear(); +} + static const char hex[] = "0123456789abcdef"; bool DNS::Factory::ipToPtr(const sockaddr_storage &ip, std::string &ptr) {