From 7f0a83388d6420eb7d31426c883e5e1c67cbf034 Mon Sep 17 00:00:00 2001 From: James Turner Date: Wed, 4 Nov 2020 22:31:43 +0000 Subject: [PATCH] DNSClient: own requests, and cancel them on timeout Fixes crashes where a request times-out, but then is completed by UDN sometime afterwards, with a free-d object. Have the DNS::Client own requests, and be able to retrieve the udns_query to cancel them, in the timeout case. Fixes a couple of Sentry reports. --- simgear/io/DNSClient.cxx | 132 +++++++++++++++++++++++++-------------- simgear/io/DNSClient.hxx | 18 ++++-- 2 files changed, 99 insertions(+), 51 deletions(-) diff --git a/simgear/io/DNSClient.cxx b/simgear/io/DNSClient.cxx index 32a0c401..de0a22a2 100644 --- a/simgear/io/DNSClient.cxx +++ b/simgear/io/DNSClient.cxx @@ -61,6 +61,10 @@ public: struct dns_ctx * ctx; static size_t instanceCounter; + + using RequestVec = std::vector; + + RequestVec _activeRequests; }; size_t Client::ClientPrivate::instanceCounter = 0; @@ -78,6 +82,11 @@ Request::~Request() { } +void Request::cancel() +{ + _cancelled = true; +} + bool Request::isTimeout() const { return (time(NULL) - _start) > _timeout_secs; @@ -114,18 +123,20 @@ static void dnscbSRV(struct dns_ctx *ctx, struct dns_rr_srv *result, void *data) { SRVRequest * r = static_cast(data); if (result) { - r->cname = result->dnssrv_cname; - r->qname = result->dnssrv_qname; - r->ttl = result->dnssrv_ttl; - for (int i = 0; i < result->dnssrv_nrr; i++) { - SRVRequest::SRV_ptr srv(new SRVRequest::SRV); - r->entries.push_back(srv); - srv->priority = result->dnssrv_srv[i].priority; - srv->weight = result->dnssrv_srv[i].weight; - srv->port = result->dnssrv_srv[i].port; - srv->target = result->dnssrv_srv[i].name; + if (!r->isCancelled()) { + r->cname = result->dnssrv_cname; + r->qname = result->dnssrv_qname; + r->ttl = result->dnssrv_ttl; + for (int i = 0; i < result->dnssrv_nrr; i++) { + SRVRequest::SRV_ptr srv(new SRVRequest::SRV); + r->entries.push_back(srv); + srv->priority = result->dnssrv_srv[i].priority; + srv->weight = result->dnssrv_srv[i].weight; + srv->port = result->dnssrv_srv[i].port; + srv->target = result->dnssrv_srv[i].name; + } + std::sort(r->entries.begin(), r->entries.end(), sortSRV); } - std::sort( r->entries.begin(), r->entries.end(), sortSRV ); free(result); } r->setComplete(); @@ -134,11 +145,16 @@ static void dnscbSRV(struct dns_ctx *ctx, struct dns_rr_srv *result, void *data) void SRVRequest::submit( Client * client ) { // if service is defined, pass service and protocol - if (!dns_submit_srv(client->d->ctx, getDn().c_str(), _service.empty() ? NULL : _service.c_str(), _service.empty() ? NULL : _protocol.c_str(), 0, dnscbSRV, this )) { + auto q = dns_submit_srv(client->d->ctx, getDn().c_str(), _service.empty() ? NULL : _service.c_str(), + _service.empty() ? NULL : _protocol.c_str(), + 0, dnscbSRV, this); + + if (!q) { SG_LOG(SG_IO, SG_ALERT, "Can't submit dns request for " << getDn()); return; } _start = time(NULL); + _query = q; } TXTRequest::TXTRequest( const std::string & dn ) : @@ -151,22 +167,24 @@ static void dnscbTXT(struct dns_ctx *ctx, struct dns_rr_txt *result, void *data) { TXTRequest * r = static_cast(data); if (result) { - r->cname = result->dnstxt_cname; - r->qname = result->dnstxt_qname; - r->ttl = result->dnstxt_ttl; - for (int i = 0; i < result->dnstxt_nrr; i++) { - //TODO: interprete the .len field of dnstxt_txt? - auto rawTxt = reinterpret_cast(result->dnstxt_txt[i].txt); - if (!rawTxt) { - continue; - } + if (!r->isCancelled()) { + r->cname = result->dnstxt_cname; + r->qname = result->dnstxt_qname; + r->ttl = result->dnstxt_ttl; + for (int i = 0; i < result->dnstxt_nrr; i++) { + //TODO: interprete the .len field of dnstxt_txt? + auto rawTxt = reinterpret_cast(result->dnstxt_txt[i].txt); + if (!rawTxt) { + continue; + } - const string txt{rawTxt}; - r->entries.push_back(txt); - string_list tokens = simgear::strutils::split( txt, "=", 1 ); - if( tokens.size() == 2 ) { - r->attributes[tokens[0]] = tokens[1]; - } + const string txt{rawTxt}; + r->entries.push_back(txt); + string_list tokens = simgear::strutils::split(txt, "=", 1); + if (tokens.size() == 2) { + r->attributes[tokens[0]] = tokens[1]; + } + } } free(result); } @@ -176,11 +194,13 @@ static void dnscbTXT(struct dns_ctx *ctx, struct dns_rr_txt *result, void *data) void TXTRequest::submit( Client * client ) { // protocol and service an already encoded in DN so pass in NULL for both - if (!dns_submit_txt(client->d->ctx, getDn().c_str(), DNS_C_IN, 0, dnscbTXT, this )) { + auto q = dns_submit_txt(client->d->ctx, getDn().c_str(), DNS_C_IN, 0, dnscbTXT, this); + if (!q) { SG_LOG(SG_IO, SG_ALERT, "Can't submit dns request for " << getDn()); return; } _start = time(NULL); + _query = q; } @@ -195,27 +215,29 @@ static void dnscbNAPTR(struct dns_ctx *ctx, struct dns_rr_naptr *result, void *d { NAPTRRequest * r = static_cast(data); if (result) { - r->cname = result->dnsnaptr_cname; - r->qname = result->dnsnaptr_qname; - r->ttl = result->dnsnaptr_ttl; - for (int i = 0; i < result->dnsnaptr_nrr; i++) { - if( !r->qservice.empty() && r->qservice != result->dnsnaptr_naptr[i].service ) - continue; + if (!r->isCancelled()) { + r->cname = result->dnsnaptr_cname; + r->qname = result->dnsnaptr_qname; + r->ttl = result->dnsnaptr_ttl; + for (int i = 0; i < result->dnsnaptr_nrr; i++) { + if (!r->qservice.empty() && r->qservice != result->dnsnaptr_naptr[i].service) + continue; - //TODO: case ignore and result flags may have more than one flag - if( !r->qflags.empty() && r->qflags != result->dnsnaptr_naptr[i].flags ) - continue; + //TODO: case ignore and result flags may have more than one flag + if (!r->qflags.empty() && r->qflags != result->dnsnaptr_naptr[i].flags) + continue; - NAPTRRequest::NAPTR_ptr naptr(new NAPTRRequest::NAPTR); - r->entries.push_back(naptr); - naptr->order = result->dnsnaptr_naptr[i].order; - naptr->preference = result->dnsnaptr_naptr[i].preference; - naptr->flags = result->dnsnaptr_naptr[i].flags; - naptr->service = result->dnsnaptr_naptr[i].service; - naptr->regexp = result->dnsnaptr_naptr[i].regexp; - naptr->replacement = result->dnsnaptr_naptr[i].replacement; + NAPTRRequest::NAPTR_ptr naptr(new NAPTRRequest::NAPTR); + r->entries.push_back(naptr); + naptr->order = result->dnsnaptr_naptr[i].order; + naptr->preference = result->dnsnaptr_naptr[i].preference; + naptr->flags = result->dnsnaptr_naptr[i].flags; + naptr->service = result->dnsnaptr_naptr[i].service; + naptr->regexp = result->dnsnaptr_naptr[i].regexp; + naptr->replacement = result->dnsnaptr_naptr[i].replacement; + } + std::sort(r->entries.begin(), r->entries.end(), sortNAPTR); } - std::sort( r->entries.begin(), r->entries.end(), sortNAPTR ); free(result); } r->setComplete(); @@ -223,11 +245,13 @@ static void dnscbNAPTR(struct dns_ctx *ctx, struct dns_rr_naptr *result, void *d void NAPTRRequest::submit( Client * client ) { - if (!dns_submit_naptr(client->d->ctx, getDn().c_str(), 0, dnscbNAPTR, this )) { + auto q = dns_submit_naptr(client->d->ctx, getDn().c_str(), 0, dnscbNAPTR, this); + if (!q) { SG_LOG(SG_IO, SG_ALERT, "Can't submit dns request for " << getDn()); return; } _start = time(NULL); + _query = q; } @@ -242,6 +266,7 @@ Client::Client() : void Client::makeRequest(const Request_ptr& r) { + d->_activeRequests.push_back(r); r->submit(this); } @@ -252,6 +277,19 @@ void Client::update(int waitTimeout) return; dns_ioevent(d->ctx, now); + + // drop our owning ref to completed requests, + // and cancel any which timed out + auto it = std::remove_if(d->_activeRequests.begin(), d->_activeRequests.end(), + [this](const Request_ptr& r) { + if (r->isTimeout()) { + dns_cancel(d->ctx, reinterpret_cast(r->_query)); + return true; + } + + return r->isComplete(); + }); + d->_activeRequests.erase(it, d->_activeRequests.end()); } } // of namespace DNS diff --git a/simgear/io/DNSClient.hxx b/simgear/io/DNSClient.hxx index 155ee33b..a71b6c6b 100644 --- a/simgear/io/DNSClient.hxx +++ b/simgear/io/DNSClient.hxx @@ -40,28 +40,38 @@ namespace DNS { class Client; + +using UDNSQueryPtr = void*; + class Request : public SGReferenced { public: Request( const std::string & dn ); virtual ~Request(); - std::string getDn() const { return _dn; } + const std::string& getDn() const { return _dn; } int getType() const { return _type; } bool isComplete() const { return _complete; } bool isTimeout() const; void setComplete( bool b = true ) { _complete = b; } + bool isCancelled() const { return _cancelled; } virtual void submit( Client * client) = 0; + void cancel(); + std::string cname; std::string qname; unsigned ttl; protected: + friend class Client; + + UDNSQueryPtr _query = nullptr; std::string _dn; int _type; bool _complete; time_t _timeout_secs; time_t _start; + bool _cancelled = false; }; typedef SGSharedPtr Request_ptr; @@ -69,7 +79,7 @@ class NAPTRRequest : public Request { public: NAPTRRequest( const std::string & dn ); - virtual void submit( Client * client ); + void submit(Client* client) override; struct NAPTR : SGReferenced { int order; @@ -92,7 +102,7 @@ class SRVRequest : public Request public: SRVRequest( const std::string & dn ); SRVRequest( const std::string & dn, const string & service, const string & protocol ); - virtual void submit( Client * client ); + void submit(Client* client) override; struct SRV : SGReferenced { int priority; @@ -112,7 +122,7 @@ class TXTRequest : public Request { public: TXTRequest( const std::string & dn ); - virtual void submit( Client * client ); + void submit(Client* client) override; typedef std::vector TXT_list; typedef std::map TXT_Attribute_map;