[2440] handle duplicate RDATA in RdataSet create appropriately.

src/lib/datasrc/memory/rdataset.cc

@@ -70,42 +70,37 @@ RdataSet::create(util::MemorySegment& mem_sgmt, RdataEncoder& encoder,
         isc_throw(BadValue, "RR class doesn't match between RRset and RRSIG");
     }
 
-    // Check assumptions on the number of RDATAs
-    const size_t old_rdata_count =
-        old_rdataset ? old_rdataset->getRdataCount() : 0;
-    const size_t old_sig_count =
-        old_rdataset ? old_rdataset->getSigRdataCount() : 0;
-    if (rrset && (rrset->getRdataCount() + old_rdata_count) > MAX_RDATA_COUNT)
-    {
-        isc_throw(RdataSetError, "Too many RDATAs for RdataSet: "
-                  << rrset->getRdataCount() << ", must be <= "
-                  << MAX_RDATA_COUNT);
-    }
-    if (sig_rrset && (sig_rrset->getRdataCount() + old_sig_count) >
-        MAX_RRSIG_COUNT) {
-        isc_throw(RdataSetError, "Too many RRSIGs for RdataSet: "
-                  << sig_rrset->getRdataCount() << ", must be <= "
-                  << MAX_RRSIG_COUNT);
-    }
-
     const RRClass rrclass = rrset ? rrset->getClass() : sig_rrset->getClass();
     const RRType rrtype = rrset ? rrset->getType() :
         getCoveredType(sig_rrset->getRdataIterator()->getCurrent());
     const RRTTL rrttl = rrset ? rrset->getTTL() : sig_rrset->getTTL();
     if (old_rdataset) {
         encoder.start(rrclass, rrtype, old_rdataset->getDataBuf(),
-                      old_rdata_count, old_sig_count);
+                      old_rdataset->getRdataCount(),
+                      old_rdataset->getSigRdataCount());
     } else {
         encoder.start(rrclass, rrtype);
     }
 
+    // Store RDATAs to be added and check assumptions on the number of them
+    size_t rdata_count = old_rdataset ? old_rdataset->getRdataCount() : 0;
     if (rrset) {
         for (RdataIteratorPtr it = rrset->getRdataIterator();
              !it->isLast();
              it->next()) {
-            encoder.addRdata(it->getCurrent());
+            if (encoder.addRdata(it->getCurrent())) {
+                ++rdata_count;
+            }
         }
     }
+    if (rdata_count > MAX_RDATA_COUNT) {
+        isc_throw(RdataSetError, "Too many RDATAs for RdataSet: "
+                  << rrset->getRdataCount() << ", must be <= "
+                  << MAX_RDATA_COUNT);
+    }
+
+    // Same for RRSIG
+    size_t rrsig_count = old_rdataset ? old_rdataset->getSigRdataCount() : 0;
     if (sig_rrset) {
         for (RdataIteratorPtr it = sig_rrset->getRdataIterator();
              !it->isLast();
@@ -114,18 +109,22 @@ RdataSet::create(util::MemorySegment& mem_sgmt, RdataEncoder& encoder,
             if (getCoveredType(it->getCurrent()) != rrtype) {
                 isc_throw(BadValue, "Type covered doesn't match");
             }
-            encoder.addSIGRdata(it->getCurrent());
+            if (encoder.addSIGRdata(it->getCurrent())) {
+                ++rrsig_count;
+            }
         }
     }
-    const size_t rrsig_count =
-        old_sig_count + (sig_rrset ? sig_rrset->getRdataCount() : 0);
+    if (rrsig_count > MAX_RRSIG_COUNT) {
+        isc_throw(RdataSetError, "Too many RRSIGs for RdataSet: "
+                  << sig_rrset->getRdataCount() << ", must be <= "
+                  << MAX_RRSIG_COUNT);
+    }
+
     const size_t ext_rrsig_count_len =
         rrsig_count >= MANY_RRSIG_COUNT ? sizeof(uint16_t) : 0;
     const size_t data_len = encoder.getStorageLength();
     void* p = mem_sgmt.allocate(sizeof(RdataSet) + ext_rrsig_count_len +
                                 data_len);
-    const size_t rdata_count =
-        old_rdata_count + (rrset ? rrset->getRdataCount() : 0);
     RdataSet* rdataset = new(p) RdataSet(rrtype, rdata_count, rrsig_count,
                                          rrttl);
     if (rrsig_count >= MANY_RRSIG_COUNT) {

src/lib/datasrc/tests/memory/rdataset_unittest.cc

@@ -238,6 +238,32 @@ TEST_F(RdataSetTest, mergeCreate) {
     }
 }
 
+TEST_F(RdataSetTest, duplicate) {
+    // Create RRset and RRSIG containing duplicate RDATA.
+    ConstRRsetPtr dup_rrset =
+        textToRRset("www.example.com. 1076895760 IN A 192.0.2.1\n"
+                    "www.example.com. 1076895760 IN A 192.0.2.1\n");
+    ConstRRsetPtr dup_rrsig =
+        textToRRset("www.example.com. 1076895760 IN RRSIG " +
+                    def_rrsig_txt_[0] +
+                    "\nwww.example.com. 1076895760 IN RRSIG " +
+                    def_rrsig_txt_[0]);
+
+    // After suppressing duplicates, it should be the same as the default
+    // RdataSet.  Check that.
+    SegmentObjectHolder<RdataSet, RRClass> holder1(
+        mem_sgmt_,
+        RdataSet::create(mem_sgmt_, encoder_, dup_rrset, dup_rrsig), rrclass);
+    checkRdataSet(*holder1.get(), def_rdata_txt_, def_rrsig_txt_);
+
+    // Confirm the same thing for the merge mode.
+    SegmentObjectHolder<RdataSet, RRClass> holder2(
+        mem_sgmt_,
+        RdataSet::create(mem_sgmt_, encoder_, *holder1.get(), a_rrset_,
+                         rrsig_rrset_), rrclass);
+    checkRdataSet(*holder2.get(), def_rdata_txt_, def_rrsig_txt_);
+}
+
 TEST_F(RdataSetTest, getNext) {
     RdataSet* rdataset = RdataSet::create(mem_sgmt_, encoder_, a_rrset_,
                                           ConstRRsetPtr());
@@ -313,8 +339,8 @@ TEST_F(RdataSetTest, find) {
 }
 
 // A helper function to create an RRset containing the given number of
-// unique RDATAs.
-ConstRRsetPtr
+// unique RDATAs.  We return non const pointer so that we can extend it.
+RRsetPtr
 getRRsetWithRdataCount(size_t rdata_count) {
     RRsetPtr rrset(new RRset(Name("example.com"), RRClass::IN(), RRType::TXT(),
                              RRTTL(3600)));
@@ -329,17 +355,22 @@ void
 RdataSetTest::checkCreateManyRRs(CreateFn create_fn, size_t n_old_rdata) {
     // RRset with possible maximum number of RDATAs, taking into account
     // "pre-existing" RDATAs
-    RdataSet* rdataset = create_fn(mem_sgmt_, encoder_,
-                                   getRRsetWithRdataCount(8191 - n_old_rdata),
+    RRsetPtr large_rrset = getRRsetWithRdataCount(8191 - n_old_rdata);
+    RdataSet* rdataset = create_fn(mem_sgmt_, encoder_, large_rrset,
                                    ConstRRsetPtr());
     EXPECT_EQ(8191, rdataset->getRdataCount());
     EXPECT_EQ(0, rdataset->getSigRdataCount());
-    RdataSet::destroy(mem_sgmt_, rdataset, RRClass::IN());
+    RdataSet::destroy(mem_sgmt_, rdataset, rrclass);
+
+    // Duplicate RDATA will be ignored in this check.
+    large_rrset->addRdata(createRdata(RRType::TXT(), rrclass, "0"));
+    rdataset = create_fn(mem_sgmt_, encoder_, large_rrset, ConstRRsetPtr());
+    EXPECT_EQ(8191, rdataset->getRdataCount());
+    RdataSet::destroy(mem_sgmt_, rdataset, rrclass);
 
     // Exceeding that will result in an exception.
-    EXPECT_THROW(create_fn(mem_sgmt_, encoder_,
-                           getRRsetWithRdataCount(8192 - n_old_rdata),
-                           ConstRRsetPtr()),
+    large_rrset->addRdata(createRdata(RRType::TXT(), rrclass, "8192"));
+    EXPECT_THROW(create_fn(mem_sgmt_, encoder_, large_rrset, ConstRRsetPtr()),
                  RdataSetError);
     // To be very sure even try larger number than the threshold
     EXPECT_THROW(create_fn(mem_sgmt_, encoder_,
@@ -383,7 +414,7 @@ TEST_F(RdataSetTest, createWithRRSIG) {
 
 // A helper function to create an RRSIG RRset containing the given number of
 // unique RDATAs.
-ConstRRsetPtr
+RRsetPtr
 getRRSIGWithRdataCount(size_t sig_count) {
     RRsetPtr rrset(new RRset(Name("example.com"), RRClass::IN(),
                              RRType::RRSIG(), RRTTL(3600)));
@@ -425,14 +456,26 @@ RdataSetTest::checkCreateManyRRSIGs(CreateFn create_fn, size_t n_old_sig) {
 
     // Up to 2^16-1 RRSIGs are allowed (although that would be useless
     // in practice)
-    rdataset = create_fn(mem_sgmt_, encoder_, a_rrset_,
-                         getRRSIGWithRdataCount(65535 - n_old_sig));
+    RRsetPtr large_rrsig = getRRSIGWithRdataCount(65535 - n_old_sig);
+    rdataset = create_fn(mem_sgmt_, encoder_, a_rrset_, large_rrsig);
+    EXPECT_EQ(65535, rdataset->getSigRdataCount());
+    RdataSet::destroy(mem_sgmt_, rdataset, RRClass::IN());
+
+    // Duplicate shouldn't be counted
+    large_rrsig->addRdata(
+        createRdata(RRType::RRSIG(), rrclass,
+                    "A 5 2 0 20120814220826 20120715220826 1234 "
+                    "example.com. FAKE"));
+    rdataset = create_fn(mem_sgmt_, encoder_, a_rrset_, large_rrsig);
     EXPECT_EQ(65535, rdataset->getSigRdataCount());
     RdataSet::destroy(mem_sgmt_, rdataset, RRClass::IN());
 
     // Exceeding this limit will result in an exception.
-    EXPECT_THROW(create_fn(mem_sgmt_, encoder_, a_rrset_,
-                           getRRSIGWithRdataCount(65536 - n_old_sig)),
+    large_rrsig->addRdata(
+        createRdata(RRType::RRSIG(), rrclass,
+                    "A 5 2 65536 20120814220826 20120715220826 1234 "
+                    "example.com. FAKE"));
+    EXPECT_THROW(create_fn(mem_sgmt_, encoder_, a_rrset_, large_rrsig),
                  RdataSetError);
     // To be very sure even try larger number than the threshold
     EXPECT_THROW(create_fn(mem_sgmt_, encoder_, a_rrset_,
@@ -511,11 +554,11 @@ RdataSetTest::checkBadCreate(CreateFn create_fn) {
                  isc::BadValue);
 }
 
-TEST_F(RdataSetTest, badCeate) {
+TEST_F(RdataSetTest, badCreate) {
     checkBadCreate(boost::bind(&RdataSet::create, _1, _2, _3, _4));
 }
 
-TEST_F(RdataSetTest, badMergeCeate) {
+TEST_F(RdataSetTest, badMergeCreate) {
     // The 'old RdataSet' for merge.  Its content doesn't matter; the test
     // should trigger exception before examining it.
     SegmentObjectHolder<RdataSet, RRClass> holder(