Hardening the DNSClient code

- add a timeout to avoid deadlock without a dns server
- test against existing terrasync.flightgear.org RR's

diff --git a/simgear/io/DNSClient.cxx b/simgear/io/DNSClient.cxx
index 813f2c138f44ee127b81556f1c8c727ae87a9d35..2c58edd52e0716d30b68d95a243c2df8fd656c4d 100644 (file)
--- a/simgear/io/DNSClient.cxx
+++ b/simgear/io/DNSClient.cxx
@@ -31,23 +31,25 @@ namespace DNS {
 
 class Client::ClientPrivate {
 public:
-       ClientPrivate() {
-               if (dns_init(NULL, 0) < 0)
-                       SG_LOG(SG_IO, SG_ALERT, "Can't init udns library" );
+    ClientPrivate() {
+        if (dns_init(NULL, 0) < 0)
+            SG_LOG(SG_IO, SG_ALERT, "Can't init udns library" );
 
-               if( dns_open(NULL) < 0 )
-                       SG_LOG(SG_IO, SG_ALERT, "Can't open udns context" );
-       }
+        if( dns_open(NULL) < 0 )
+            SG_LOG(SG_IO, SG_ALERT, "Can't open udns context" );
+    }
 
-       ~ClientPrivate() {
-               dns_close(NULL);
-       }
+    ~ClientPrivate() {
+        dns_close(NULL);
+    }
 };
 
 Request::Request( const std::string & dn ) :
-               _dn(dn),
-               _type(DNS_T_ANY),
-               _complete(false)
+        _dn(dn),
+        _type(DNS_T_ANY),
+        _complete(false),
+        _timeout_secs(5),
+        _start(0)
 {
 }
 
@@ -55,40 +57,61 @@ Request::~Request()
 {
 }
 
+bool Request::isTimeout() const
+{
+    return (time(NULL) - _start) > _timeout_secs;
+}
+
 NAPTRRequest::NAPTRRequest( const std::string & dn ) :
-               Request(dn)
+        Request(dn)
+{
+    _type = DNS_T_NAPTR;
+}
+
+static bool sortNAPTR( const NAPTRRequest::NAPTR_ptr a, const NAPTRRequest::NAPTR_ptr b )
 {
-       _type = DNS_T_NAPTR;
+    if( a->order > b->order ) return false;
+    if( a->order < b->order ) return true;
+    return a->preference < b->preference;
 }
 
 static void dnscbNAPTR(struct dns_ctx *ctx, struct dns_rr_naptr *result, void *data)
 {
-       NAPTRRequest * r = static_cast<NAPTRRequest*>(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++) {
-                       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;
-               }
-               free(result);
-       }
-       r->setComplete();
+    NAPTRRequest * r = static_cast<NAPTRRequest*>(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 )
+                return;
+
+            //TODO: case ignore and result flags may have more than one flag
+            if( !r->qflags.empty() && r->qflags != result->dnsnaptr_naptr[i].flags )
+                return;
+
+            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 );
+        free(result);
+    }
+    r->setComplete();
 }
 
 void NAPTRRequest::submit()
 {
-       if (!dns_submit_naptr(NULL, getDn().c_str(), 0, dnscbNAPTR, this )) {
-               SG_LOG(SG_IO, SG_ALERT, "Can't submit dns request for " << getDn());
-               return;
-       }
+    if (!dns_submit_naptr(NULL, getDn().c_str(), 0, dnscbNAPTR, this )) {
+        SG_LOG(SG_IO, SG_ALERT, "Can't submit dns request for " << getDn());
+        return;
+    }
+    _start = time(NULL);
 }
 
 
@@ -97,21 +120,21 @@ Client::~Client()
 }
 
 Client::Client() :
-               d(new ClientPrivate)
+    d(new ClientPrivate)
 {
 }
 
 void Client::makeRequest(const Request_ptr& r)
 {
-       r->submit();
+    r->submit();
 }
 
 
 void Client::update(int waitTimeout)
 {
-       time_t now = time(NULL);
-       if( dns_timeouts( NULL, waitTimeout, now ) < 0 )
-               return;
+    time_t now = time(NULL);
+    if( dns_timeouts( NULL, waitTimeout, now ) < 0 )
+        return;
     dns_ioevent(NULL, now);
 }
 

diff --git a/simgear/io/DNSClient.hxx b/simgear/io/DNSClient.hxx
index 5a7604dde4510947d9548f6ecc76adb3334326ec..b23e6dc5a48a7411c76db88f27bbcf267eda5f50 100644 (file)
--- a/simgear/io/DNSClient.hxx
+++ b/simgear/io/DNSClient.hxx
@@ -39,40 +39,47 @@ namespace DNS
 class Request : public SGReferenced
 {
 public:
-       Request( const std::string & dn );
-       virtual ~Request();
-       std::string getDn() const { return _dn; }
-       int getType() const { return _type; }
-       bool complete() const { return _complete; }
-       void setComplete( bool b = true ) { _complete = b; }
-
-       virtual void submit() = 0;
+    Request( const std::string & dn );
+    virtual ~Request();
+    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; }
+
+    virtual void submit() = 0;
+
+    std::string cname;
+    std::string qname;
+    unsigned ttl;
 protected:
-       std::string _dn;
-       int _type;
-       bool _complete;
+    std::string _dn;
+    int _type;
+    bool _complete;
+    time_t _timeout_secs;
+    time_t _start;
 };
 
 class NAPTRRequest : public Request
 {
 public:
-       NAPTRRequest( const std::string & dn );
-       virtual void submit();
-
-       struct NAPTR : SGReferenced {
-               int order;
-               int preference;
-               std::string flags;
-               std::string service;
-               std::string regexp;
-               std::string replacement;
-       };
-       typedef SGSharedPtr<NAPTR> NAPTR_ptr;
-       typedef std::vector<NAPTR_ptr> NAPTR_list;
-       std::string cname;
-       std::string qname;
-       unsigned ttl;
-       NAPTR_list entries;
+    NAPTRRequest( const std::string & dn );
+    virtual void submit();
+
+    struct NAPTR : SGReferenced {
+        int order;
+        int preference;
+        std::string flags;
+        std::string service;
+        std::string regexp;
+        std::string replacement;
+    };
+    typedef SGSharedPtr<NAPTR> NAPTR_ptr;
+    typedef std::vector<NAPTR_ptr> NAPTR_list;
+    NAPTR_list entries;
+
+    std::string qflags;
+    std::string qservice;
 };
 
 typedef SGSharedPtr<Request> Request_ptr;

diff --git a/simgear/io/test_DNS.cxx b/simgear/io/test_DNS.cxx
index df2a10e8afc41325a13f91af564b5a4d64205b11..0e9d17ff5b2954cab6977dfd5f3fbdd4e91a3478 100644 (file)
--- a/simgear/io/test_DNS.cxx
+++ b/simgear/io/test_DNS.cxx
@@ -14,6 +14,7 @@
 #include "test_DNS.hxx"
 
 #include <simgear/debug/logstream.hxx>
+#include <simgear/misc/strutils.hxx>
 
 using std::cout;
 using std::cerr;
@@ -34,33 +35,80 @@ int main(int argc, char* argv[])
     sglog().setLogLevels( SG_ALL, SG_DEBUG );
 
     DNS::Client cl;
+#define EXISTING_RECORD "terrasync.flightgear.org"
 
     // test existing NAPTR
-    // fgtest.t3r.de.          600     IN      NAPTR   999 99 "U" "test" "!^.*$!http://dnstest.flightgear.org/!" .
+    // fgtest.t3r.de.                600        IN        NAPTR        999 99 "U" "test" "!^.*$!http://dnstest.flightgear.org/!" .
     {
-       DNS::NAPTRRequest * naptrRequest = new DNS::NAPTRRequest("fgtest.t3r.de");
-       DNS::Request_ptr r(naptrRequest);
-       cl.makeRequest(r);
-       while( !r->complete() )
-               cl.update(0);
-
-       COMPARE(naptrRequest->entries.size(), 1 );
-       COMPARE(naptrRequest->entries[0]->order, 999 );
-       COMPARE(naptrRequest->entries[0]->preference, 99 );
-       COMPARE(naptrRequest->entries[0]->service, "test" );
-       COMPARE(naptrRequest->entries[0]->regexp, "!^.*$!http://dnstest.flightgear.org/!" );
-       COMPARE(naptrRequest->entries[0]->replacement, "" );
+        DNS::NAPTRRequest * naptrRequest = new DNS::NAPTRRequest(EXISTING_RECORD);
+        DNS::Request_ptr r(naptrRequest);
+        cl.makeRequest(r);
+        while( !r->isComplete() && !r->isTimeout()) {
+            usleep(200000);
+            cl.update(0);
+        }
+
+        if( r->isTimeout() ) {
+            cerr << "timeout testing existing record " EXISTING_RECORD << endl;
+            return EXIT_FAILURE;
+        }
+        if(naptrRequest->entries.empty()) {
+            cerr << "no results for " EXISTING_RECORD << endl;
+            return EXIT_FAILURE;
+        }
+
+        // test for ascending preference/order
+        int order = -1, preference = -1;
+        for( DNS::NAPTRRequest::NAPTR_list::const_iterator it = naptrRequest->entries.begin(); it != naptrRequest->entries.end(); ++it ) {
+            // currently only support "U" which implies empty replacement
+            COMPARE((*it)->flags, "U" );
+            COMPARE(naptrRequest->entries[0]->replacement, "" );
+
+            // currently only support ws20
+            COMPARE((*it)->service, "ws20" );
+
+            if( (*it)->order < order ) {
+                cerr << "NAPTR entries not ascending for field 'order'" << endl;
+                return EXIT_FAILURE;
+            } else if( (*it)->order > order ) {
+                order = (*it)->order;
+                preference = (*it)->preference;
+            } else {
+                if( (*it)->preference < preference ) {
+                    cerr << "NAPTR entries not ascending for field 'preference', order=" << order << endl;
+                    return EXIT_FAILURE;
+                }
+                preference = (*it)->preference;
+            }
+
+            if( false == simgear::strutils::starts_with( (*it)->regexp, "!^.*$!" ) ) {
+                cerr << "NAPTR entry with unsupported regexp: " << (*it)->regexp << endl;
+                return EXIT_FAILURE;
+            }
+
+            if( false == simgear::strutils::ends_with( (*it)->regexp, "!" ) ) {
+                cerr << "NAPTR entry with unsupported regexp: " << (*it)->regexp << endl;
+                return EXIT_FAILURE;
+            }
+
+        }
     }
 
     // test non-existing NAPTR
     {
-       DNS::NAPTRRequest * naptrRequest = new DNS::NAPTRRequest("jurkxkqdiufqzpfvzqok.prozhqrlcaavbxifkkhf");
-       DNS::Request_ptr r(naptrRequest);
-       cl.makeRequest(r);
-       while( !r->complete() )
-               cl.update(0);
-
-       COMPARE(naptrRequest->entries.size(), 0 );
+        DNS::NAPTRRequest * naptrRequest = new DNS::NAPTRRequest("jurkxkqdiufqzpfvzqok.prozhqrlcaavbxifkkhf");
+        DNS::Request_ptr r(naptrRequest);
+        cl.makeRequest(r);
+        while( !r->isComplete() && !r->isTimeout()) {
+            usleep(200000);
+            cl.update(0);
+        }
+
+        if( r->isTimeout() ) {
+            cerr << "timeout testing non-existing record." << endl;
+            return EXIT_FAILURE;
+        }
+        COMPARE(naptrRequest->entries.size(), 0 );
     }
 
     cout << "all tests passed ok" << endl;