[trac1062] refactor and fixes

- check for bad data in the db
- work with data in 'bad' order (sigs before data for example)

src/lib/datasrc/database.cc

@@ -22,6 +22,8 @@
 
 #include <datasrc/data_source.h>
 
+#include <boost/foreach.hpp>
+
 using isc::dns::Name;
 
 namespace isc {
@@ -66,6 +68,88 @@ DatabaseClient::Finder::Finder(boost::shared_ptr<DatabaseConnection>
     zone_id_(zone_id)
 { }
 
+namespace {
+    // Adds the given Rdata to the given RRset
+    // If the rrset does not exist, one is created
+    // adds the given rdata to the set
+    void addOrCreate(isc::dns::RRsetPtr& rrset,
+                     const isc::dns::Name& name,
+                     const isc::dns::RRClass& cls,
+                     const isc::dns::RRType& type,
+                     const isc::dns::RRTTL& ttl,
+                     const std::string& rdata_str)
+    {
+        if (!rrset) {
+            rrset.reset(new isc::dns::RRset(name, cls, type, ttl));
+        } else {
+            if (ttl < rrset->getTTL()) {
+                rrset->setTTL(ttl);
+            }
+            // make sure the type is correct
+            if (type != rrset->getType()) {
+                isc_throw(DataSourceError,
+                          "attempt to add multiple types to RRset in find()");
+            }
+        }
+        if (rdata_str != "") {
+            try {
+                rrset->addRdata(isc::dns::rdata::createRdata(type, cls, rdata_str));
+            } catch (const isc::dns::rdata::InvalidRdataText& ivrt) {
+                // at this point, rrset may have been initialised for no reason,
+                // and won't be used. But the caller would drop the shared_ptr
+                // on such an error anyway, so we don't care.
+                isc_throw(DataSourceError,
+                          "bad rdata in database for " << name.toText() << " "
+                          << type.toText() << " " << ivrt.what());
+            }
+        }
+    }
+
+    // This class keeps a short-lived store of RRSIG records encountered
+    // during a call to find(). If the backend happens to return signatures
+    // before the actual data, we might not know which signatures we will need
+    // So if they may be relevant, we store the in this class.
+    //
+    // (If this class seems useful in other places, we might want to move
+    // it to util. That would also provide an opportunity to add unit tests)
+    class RRsigStore {
+    public:
+        // add the given signature Rdata to the store
+        // The signature MUST be of the RRSIG type (the caller
+        // must make sure of this)
+        void addSig(isc::dns::rdata::RdataPtr sig_rdata) {
+            const isc::dns::RRType& type_covered =
+                static_cast<isc::dns::rdata::generic::RRSIG*>(
+                    sig_rdata.get())->typeCovered();
+            if (!haveSigsFor(type_covered)) {
+                sigs[type_covered] = std::vector<isc::dns::rdata::RdataPtr>();
+            }
+            sigs.find(type_covered)->second.push_back(sig_rdata);
+        }
+
+        // Returns true if this store contains signatures covering the
+        // given type
+        bool haveSigsFor(isc::dns::RRType type) {
+            return (sigs.count(type) > 0);
+        }
+
+        // If the store contains signatures for the type of the given
+        // rrset, they are appended to it.
+        void appendSignatures(isc::dns::RRsetPtr& rrset) {
+            if (haveSigsFor(rrset->getType())) {
+                BOOST_FOREACH(isc::dns::rdata::RdataPtr sig,
+                              sigs.find(rrset->getType())->second) {
+                    rrset->addRRsig(sig);
+                }
+            }
+        }
+
+    private:
+        std::map<isc::dns::RRType, std::vector<isc::dns::rdata::RdataPtr> > sigs;
+    };
+}
+
+
 ZoneFinder::FindResult
 DatabaseClient::Finder::find(const isc::dns::Name& name,
                              const isc::dns::RRType& type,
@@ -77,6 +161,7 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
     bool records_found = false;
     isc::dns::RRsetPtr result_rrset;
     ZoneFinder::Result result_status = SUCCESS;
+    RRsigStore sig_store;
 
     connection_->searchForRecords(zone_id_, name.toText());
 
@@ -91,75 +176,53 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
                       "Datasource backend did not return 4 columns in getNextRecord()");
         }
 
-        const isc::dns::RRType cur_type(columns[0]);
-        const isc::dns::RRTTL cur_ttl(columns[1]);
-        //cur_sigtype(columns[2]);
-
-        if (cur_type == type) {
-            if (!result_rrset) {
-                result_rrset = isc::dns::RRsetPtr(new isc::dns::RRset(name,
-                                                                      getClass(),
-                                                                      cur_type,
-                                                                      cur_ttl));
-                result_status = SUCCESS;
-            } else {
-                // We have existing data from earlier calls, do some checks
-                // and updates if necessary
-                if (cur_ttl < result_rrset->getTTL()) {
-                    result_rrset->setTTL(cur_ttl);
-                }
-            }
-
-            result_rrset->addRdata(isc::dns::rdata::createRdata(cur_type,
-                                                                getClass(),
-                                                                columns[3]));
-        } else if (cur_type == isc::dns::RRType::CNAME()) {
-            // There should be no other data, so cur_rrset should be empty,
-            // except for signatures
-            if (result_rrset) {
-                if (result_rrset->getRdataCount() > 0) {
-                    isc_throw(DataSourceError, "CNAME found but it is not the only record for " + name.toText());
-                }
-            } else {
-                result_rrset = isc::dns::RRsetPtr(new isc::dns::RRset(name,
-                                                                    getClass(),
-                                                                    cur_type,
-                                                                    cur_ttl));
-            }
-            result_rrset->addRdata(isc::dns::rdata::createRdata(cur_type,
-                                                                getClass(),
-                                                                columns[3]));
-            result_status = CNAME;
-        } else if (cur_type == isc::dns::RRType::RRSIG()) {
-            isc::dns::rdata::RdataPtr cur_rrsig(
-                isc::dns::rdata::createRdata(cur_type, getClass(), columns[3]));
-            const isc::dns::RRType& type_covered =
-                static_cast<isc::dns::rdata::generic::RRSIG*>(
-                    cur_rrsig.get())->typeCovered();
-            // Ignore the RRSIG data we got if it does not cover the type
-            // that was requested or CNAME
-            // see if we have RRset data yet, and whether it has an RRsig yet
-            if (type_covered == type || type_covered == isc::dns::RRType::CNAME()) {
-                if (!result_rrset) {
-                // no data at all yet, assume the RRset data is coming, and
-                // that the type covered will match
-                    result_rrset = isc::dns::RRsetPtr(new isc::dns::RRset(name,
-                                                                          getClass(),
-                                                                          type_covered,
-                                                                          cur_ttl));
+        try {
+            const isc::dns::RRType cur_type(columns[0]);
+            const isc::dns::RRTTL cur_ttl(columns[1]);
+            //cur_sigtype(columns[2]);
+
+            if (cur_type == type) {
+                addOrCreate(result_rrset, name, getClass(), cur_type, cur_ttl, columns[3]);
+                    //isc::dns::rdata::createRdata(cur_type, getClass(), columns[3]));
+            } else if (cur_type == isc::dns::RRType::CNAME()) {
+                // There should be no other data, so cur_rrset should be empty,
+                // except for signatures, of course
+                if (result_rrset) {
+                    if (result_rrset->getRdataCount() > 0) {
+                        isc_throw(DataSourceError, "CNAME found but it is not the only record for " + name.toText());
+                    }
                 }
-                result_rrset->addRRsig(cur_rrsig);
+                addOrCreate(result_rrset, name, getClass(), cur_type, cur_ttl, columns[3]);
+                    //isc::dns::rdata::createRdata(cur_type, getClass(), columns[3]));
+                result_status = CNAME;
+            } else if (cur_type == isc::dns::RRType::RRSIG()) {
+                // If we get signatures before we get the actual data, we can't know
+                // which ones to keep and which to drop...
+                // So we keep a separate store of any signature that may be relevant
+                // and add them to the final RRset when we are done.
+                isc::dns::rdata::RdataPtr cur_rrsig(
+                    isc::dns::rdata::createRdata(cur_type, getClass(), columns[3]));
+                sig_store.addSig(cur_rrsig);
             }
+        } catch (const isc::dns::InvalidRRType& irt) {
+            isc_throw(DataSourceError,
+                      "Invalid RRType in database for " << name << ": " << columns[0]);
+        } catch (const isc::dns::InvalidRRTTL& irttl) {
+            isc_throw(DataSourceError,
+                      "Invalid TTL in database for " << name << ": " << columns[1]);
         }
     }
 
-    if (result_rrset) {
-        return (FindResult(result_status, result_rrset));
-    } else if (records_found) {
-        return (FindResult(NXRRSET, isc::dns::ConstRRsetPtr()));
+    if (!result_rrset) {
+        if (records_found) {
+            result_status = NXRRSET;
+        } else {
+            result_status = NXDOMAIN;
+        }
     } else {
-        return (FindResult(NXDOMAIN, isc::dns::ConstRRsetPtr()));
+        sig_store.appendSignatures(result_rrset);
     }
+    return (FindResult(result_status, result_rrset));
 }
 
 Name

src/lib/datasrc/tests/database_unittest.cc

@@ -127,6 +127,7 @@ private:
         // some DNSSEC-'signed' data
         addRecord("A", "3600", "", "192.0.2.1");
         addRecord("RRSIG", "3600", "", "A 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
+        addRecord("RRSIG", "3600", "", "A 5 3 3600 20000101000000 20000201000000 12346 example.org. FAKEFAKEFAKE");
         addRecord("AAAA", "3600", "", "2001:db8::1");
         addRecord("AAAA", "3600", "", "2001:db8::2");
         addRecord("RRSIG", "3600", "", "AAAA 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
@@ -134,11 +135,18 @@ private:
         addRecord("CNAME", "3600", "", "www.example.org.");
         addRecord("RRSIG", "3600", "", "CNAME 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
         addCurName("signedcname1.example.org.");
+        // special case might fail; sig is for cname, which isn't there (should be ignored)
+        // (ignoring of 'normal' other type is done above by www.)
+        addRecord("A", "3600", "", "192.0.2.1");
+        addRecord("RRSIG", "3600", "", "A 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
+        addRecord("RRSIG", "3600", "", "CNAME 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
+        addCurName("acnamesig1.example.org.");
 
         // let's pretend we have a database that is not careful
         // about the order in which it returns data
         addRecord("RRSIG", "3600", "", "A 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
         addRecord("AAAA", "3600", "", "2001:db8::2");
+        addRecord("RRSIG", "3600", "", "A 5 3 3600 20000101000000 20000201000000 12346 example.org. FAKEFAKEFAKE");
         addRecord("A", "3600", "", "192.0.2.1");
         addRecord("RRSIG", "3600", "", "AAAA 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
         addRecord("AAAA", "3600", "", "2001:db8::1");
@@ -147,12 +155,28 @@ private:
         addRecord("CNAME", "3600", "", "www.example.org.");
         addCurName("signedcname2.example.org.");
 
+        addRecord("RRSIG", "3600", "", "CNAME 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
+        addRecord("A", "3600", "", "192.0.2.1");
+        addRecord("RRSIG", "3600", "", "A 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
+        addCurName("acnamesig2.example.org.");
+
+        addRecord("RRSIG", "3600", "", "CNAME 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
+        addRecord("RRSIG", "3600", "", "A 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
+        addRecord("A", "3600", "", "192.0.2.1");
+        addCurName("acnamesig3.example.org.");
+
         // also add some intentionally bad data
         cur_name.push_back(std::vector<std::string>());
         addCurName("emptyvector.example.org.");
         addRecord("A", "3600", "", "192.0.2.1");
         addRecord("CNAME", "3600", "", "www.example.org.");
         addCurName("badcname.example.org.");
+        addRecord("A", "3600", "", "bad");
+        addCurName("badrdata.example.org.");
+        addRecord("BAD_TYPE", "3600", "", "192.0.2.1");
+        addCurName("badtype.example.org.");
+        addRecord("A", "badttl", "", "192.0.2.1");
+        addCurName("badttl.example.org.");
     }
 };
 
@@ -263,7 +287,7 @@ TEST_F(DatabaseClientTest, find) {
                ZoneFinder::NXDOMAIN, 0, 0);
     doFindTest(finder, isc::dns::Name("signed1.example.org."),
                isc::dns::RRType::A(), isc::dns::RRType::A(),
-               ZoneFinder::SUCCESS, 1, 1);
+               ZoneFinder::SUCCESS, 1, 2);
     doFindTest(finder, isc::dns::Name("signed1.example.org."),
                isc::dns::RRType::AAAA(), isc::dns::RRType::AAAA(),
                ZoneFinder::SUCCESS, 2, 1);
@@ -276,7 +300,7 @@ TEST_F(DatabaseClientTest, find) {
 
     doFindTest(finder, isc::dns::Name("signed2.example.org."),
                isc::dns::RRType::A(), isc::dns::RRType::A(),
-               ZoneFinder::SUCCESS, 1, 1);
+               ZoneFinder::SUCCESS, 1, 2);
     doFindTest(finder, isc::dns::Name("signed2.example.org."),
                isc::dns::RRType::AAAA(), isc::dns::RRType::AAAA(),
                ZoneFinder::SUCCESS, 2, 1);
@@ -287,6 +311,16 @@ TEST_F(DatabaseClientTest, find) {
                isc::dns::RRType::A(), isc::dns::RRType::CNAME(),
                ZoneFinder::CNAME, 1, 1);
 
+    doFindTest(finder, isc::dns::Name("acnamesig1.example.org."),
+               isc::dns::RRType::A(), isc::dns::RRType::A(),
+               ZoneFinder::SUCCESS, 1, 1);
+    doFindTest(finder, isc::dns::Name("acnamesig2.example.org."),
+               isc::dns::RRType::A(), isc::dns::RRType::A(),
+               ZoneFinder::SUCCESS, 1, 1);
+    doFindTest(finder, isc::dns::Name("acnamesig3.example.org."),
+               isc::dns::RRType::A(), isc::dns::RRType::A(),
+               ZoneFinder::SUCCESS, 1, 1);
+
     EXPECT_THROW(finder->find(isc::dns::Name("emptyvector.example.org."),
                                               isc::dns::RRType::A(),
                                               NULL, ZoneFinder::FIND_DEFAULT),
@@ -295,6 +329,18 @@ TEST_F(DatabaseClientTest, find) {
                                               isc::dns::RRType::A(),
                                               NULL, ZoneFinder::FIND_DEFAULT),
                  DataSourceError);
+    EXPECT_THROW(finder->find(isc::dns::Name("badrdata.example.org."),
+                                              isc::dns::RRType::A(),
+                                              NULL, ZoneFinder::FIND_DEFAULT),
+                 DataSourceError);
+    EXPECT_THROW(finder->find(isc::dns::Name("badtype.example.org."),
+                                              isc::dns::RRType::A(),
+                                              NULL, ZoneFinder::FIND_DEFAULT),
+                 DataSourceError);
+    EXPECT_THROW(finder->find(isc::dns::Name("badttl.example.org."),
+                                              isc::dns::RRType::A(),
+                                              NULL, ZoneFinder::FIND_DEFAULT),
+                 DataSourceError);
 
 }