[2440] suppress duplicate Rdata in RdataEncoder::addRdata().

also, as a cleanup, removed an unused class member variable.

src/lib/datasrc/memory/rdata_serialization.cc

@@ -25,13 +25,20 @@
 #include <dns/rrclass.h>
 #include <dns/rrtype.h>
 
+#include <boost/static_assert.hpp>
+#include <boost/function.hpp>
+#include <boost/bind.hpp>
+#include <boost/optional.hpp>
+
 #include <cassert>
 #include <cstring>
+#include <set>
 #include <vector>
-#include <boost/static_assert.hpp>
 
 using namespace isc::dns;
+using namespace isc::dns::rdata;
 using std::vector;
+using std::set;
 
 namespace isc {
 namespace datasrc {
@@ -222,7 +229,6 @@ getRdataEncodeSpec(const RRClass& rrclass, const RRType& rrtype) {
 }
 
 namespace {
-
 // This class is a helper for RdataEncoder to divide the content of RDATA
 // fields for encoding by "abusing" the  message rendering logic.
 // The idea is to identify domain name fields in the writeName() method,
@@ -368,16 +374,33 @@ private:
 
 } // end of unnamed namespace
 
+namespace {
+bool
+RdataLess(const ConstRdataPtr& rdata1, const ConstRdataPtr& rdata2) {
+    return (rdata1->compare(*rdata2) < 0);
+}
+}
+
 struct RdataEncoder::RdataEncoderImpl {
     RdataEncoderImpl() : encode_spec_(NULL), rrsig_buffer_(0),
-                         rdata_count_(0)
+                         rdatas_(boost::bind(RdataLess, _1, _2)),
+                         rrsigs_(boost::bind(RdataLess, _1, _2))
     {}
 
     const RdataEncodeSpec* encode_spec_; // encode spec of current RDATA set
     RdataFieldComposer field_composer_;
     util::OutputBuffer rrsig_buffer_;
-    size_t rdata_count_;
     vector<uint16_t> rrsig_lengths_;
+
+    boost::optional<RRClass> current_class_;
+    boost::optional<RRType> current_type_;
+
+    typedef boost::function<bool(const ConstRdataPtr&, const ConstRdataPtr&)>
+    RdataCmp;
+    // added unique Rdatas
+    set<ConstRdataPtr, RdataCmp> rdatas_;
+    // added unique RRSIG Rdatas
+    set<ConstRdataPtr, RdataCmp> rrsigs_;
 };
 
 RdataEncoder::RdataEncoder() :
@@ -395,27 +418,38 @@ RdataEncoder::start(RRClass rrclass, RRType rrtype) {
     }
 
     impl_->encode_spec_ = &getRdataEncodeSpec(rrclass, rrtype);
+    impl_->current_class_ = rrclass;
+    impl_->current_type_ = rrtype;
     impl_->field_composer_.clearLocal(impl_->encode_spec_);
     impl_->rrsig_buffer_.clear();
-    impl_->rdata_count_ = 0;
     impl_->rrsig_lengths_.clear();
+    impl_->rdatas_.clear();
+    impl_->rrsigs_.clear();
 }
 
 void
-RdataEncoder::addRdata(const rdata::Rdata& rdata) {
+RdataEncoder::addRdata(const Rdata& rdata) {
     if (impl_->encode_spec_ == NULL) {
         isc_throw(InvalidOperation,
                   "RdataEncoder::addRdata performed before start");
     }
 
+    // Simply ignore duplicate RDATA.  Creating RdataPtr also checks the
+    // given Rdata is of the correct RR type.
+    ConstRdataPtr rdatap = createRdata(*impl_->current_type_,
+                                       *impl_->current_class_, rdata);
+    if (impl_->rdatas_.find(rdatap) != impl_->rdatas_.end()) {
+        return;
+    }
+
     impl_->field_composer_.startRdata();
     rdata.toWire(impl_->field_composer_);
     impl_->field_composer_.endRdata();
-    ++impl_->rdata_count_;
+    impl_->rdatas_.insert(rdatap);
 }
 
 void
-RdataEncoder::addSIGRdata(const rdata::Rdata& sig_rdata) {
+RdataEncoder::addSIGRdata(const Rdata& sig_rdata) {
     if (impl_->encode_spec_ == NULL) {
         isc_throw(InvalidOperation,
                   "RdataEncoder::addSIGRdata performed before start");

src/lib/datasrc/memory/rdata_serialization.h

@@ -181,7 +181,7 @@ public:
     /// new session from \c start() should this method throws an exception.
     ///
     /// \throw InvalidOperation called before start().
-    /// \throw BadValue inconsistent data found.
+    /// \throw std::bad_cast The given Rdata is of different RR type.
     /// \throw RdataEncodingError A very unusual case, such as over 64KB RDATA.
     /// \throw std::bad_alloc Internal memory allocation failure.
     ///

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

@@ -34,7 +34,9 @@
 #include <boost/foreach.hpp>
 
 #include <cstring>
+#include <algorithm>
 #include <set>
+#include <stdexcept>
 #include <string>
 #include <vector>
 
@@ -166,7 +168,8 @@ public:
                      vector<ConstRdataPtr>());
 
     void addRdataCommon(const vector<ConstRdataPtr>& rrsigs);
-    void addRdataMultiCommon(const vector<ConstRdataPtr>& rrsigs);
+    void addRdataMultiCommon(const vector<ConstRdataPtr>& rrsigs,
+                             bool duplicate = false);
 };
 
 // Used across more classes and scopes. But it's just uninteresting
@@ -532,6 +535,11 @@ RdataSerializationTest::encodeWrapper(size_t data_len) {
     encoded_data_.resize(data_len);
 }
 
+bool
+rdataMatch(ConstRdataPtr rdata1, ConstRdataPtr rdata2) {
+    return (rdata1->compare(*rdata2) == 0);
+}
+
 template<class DecoderStyle>
 void
 RdataEncodeDecodeTest<DecoderStyle>::
@@ -552,14 +560,26 @@ checkEncode(RRClass rrclass, RRType rrtype,
     actual_renderer_.clear();
     encoded_data_.clear();
 
-    // Build expected wire-format data
+    // Build expected wire-format data, skipping duplicate Rdata.
     expected_renderer_.writeName(dummy_name);
+    vector<ConstRdataPtr> rdata_uniq_list;
     BOOST_FOREACH(const ConstRdataPtr& rdata, rdata_list) {
-        rdata->toWire(expected_renderer_);
+        if (std::find_if(rdata_uniq_list.begin(), rdata_uniq_list.end(),
+                         boost::bind(rdataMatch, rdata, _1)) ==
+            rdata_uniq_list.end()) {
+            rdata_uniq_list.push_back(rdata);
+            rdata->toWire(expected_renderer_);
+        }
     }
     expected_renderer_.writeName(dummyName2());
+    vector<ConstRdataPtr> rrsig_uniq_list;
     BOOST_FOREACH(const ConstRdataPtr& rdata, rrsig_list) {
-        rdata->toWire(expected_renderer_);
+        if (std::find_if(rrsig_uniq_list.begin(), rrsig_uniq_list.end(),
+                         boost::bind(rdataMatch, rdata, _1)) ==
+            rrsig_uniq_list.end()) {
+            rrsig_uniq_list.push_back(rdata);
+            rdata->toWire(expected_renderer_);
+        }
     }
 
     // Then build wire format data using the encoded data.
@@ -577,9 +597,9 @@ checkEncode(RRClass rrclass, RRType rrtype,
     const size_t storage_len = encoder_.getStorageLength();
     encodeWrapper(storage_len);
 
-    DecoderStyle::decode(rrclass, rrtype, rdata_list.size(), rrsig_list.size(),
-                         expected_varlen_fields, encoded_data_, storage_len,
-                         actual_renderer_);
+    DecoderStyle::decode(rrclass, rrtype, rdata_uniq_list.size(),
+                         rrsig_uniq_list.size(), expected_varlen_fields,
+                         encoded_data_, storage_len, actual_renderer_);
 
     // Two sets of wire-format data should be identical.
     matchWireData(expected_renderer_.getData(), expected_renderer_.getLength(),
@@ -619,7 +639,7 @@ TYPED_TEST(RdataEncodeDecodeTest, addRdata) {
 template<class DecoderStyle>
 void
 RdataEncodeDecodeTest<DecoderStyle>::
-addRdataMultiCommon(const vector<ConstRdataPtr>& rrsigs) {
+addRdataMultiCommon(const vector<ConstRdataPtr>& rrsigs, bool duplicate) {
     // Similar to addRdata(), but test with multiple RDATAs.
     // Four different cases are tested: a single fixed-len RDATA (A),
     // fixed-len data + domain name (MX), variable-len data only (TXT),
@@ -629,12 +649,19 @@ addRdataMultiCommon(const vector<ConstRdataPtr>& rrsigs) {
     rdata_list_.clear();
     rdata_list_.push_back(a_rdata_);
     rdata_list_.push_back(a_rdata2);
+    if (duplicate) {      // if duplicate is true, add duplicate Rdata
+        rdata_list_.push_back(a_rdata_);
+    }
     checkEncode(RRClass::IN(), RRType::A(), rdata_list_, 0, rrsigs);
 
     ConstRdataPtr mx_rdata1 = createRdata(RRType::MX(), RRClass::IN(),
                                           "5 mx1.example.com.");
     ConstRdataPtr mx_rdata2 = createRdata(RRType::MX(), RRClass::IN(),
                                           "10 mx2.example.com.");
+    if (duplicate) { // check duplicate detection is case insensitive for names
+        rdata_list_.push_back(createRdata(RRType::MX(), RRClass::IN(),
+                                          "5 MX1.example.COM."));
+    }
     rdata_list_.clear();
     rdata_list_.push_back(mx_rdata1);
     rdata_list_.push_back(mx_rdata2);
@@ -644,6 +671,9 @@ addRdataMultiCommon(const vector<ConstRdataPtr>& rrsigs) {
                                            "foo bar baz");
     ConstRdataPtr txt_rdata2 = createRdata(RRType::TXT(), RRClass::IN(),
                                           "another text data");
+    if (duplicate) {
+        rdata_list_.push_back(txt_rdata1);
+    }
     rdata_list_.clear();
     rdata_list_.push_back(txt_rdata1);
     rdata_list_.push_back(txt_rdata2);
@@ -655,6 +685,9 @@ addRdataMultiCommon(const vector<ConstRdataPtr>& rrsigs) {
     ConstRdataPtr naptr_rdata2 =
         createRdata(RRType::NAPTR(), RRClass::IN(),
                     "200 100 \"s\" \"http\" \"\" _http._tcp.example.com");
+    if (duplicate) {
+        rdata_list_.push_back(naptr_rdata1);
+    }
     rdata_list_.clear();
     rdata_list_.push_back(naptr_rdata1);
     rdata_list_.push_back(naptr_rdata2);
@@ -705,6 +738,8 @@ TYPED_TEST(RdataEncodeDecodeTest, addRdataMulti) {
     vector<ConstRdataPtr> rrsigs;
     this->addRdataMultiCommon(rrsigs); // test without RRSIGs (empty vector)
 
+    this->addRdataMultiCommon(rrsigs, true); // ditto, but with duplicated data
+
     // Tests with two RRSIGs
     rrsigs.push_back(this->rrsig_rdata_);
     rrsigs.push_back(createRdata(RRType::RRSIG(), RRClass::IN(),
@@ -732,50 +767,44 @@ TEST_F(RdataSerializationTest, badAddRdata) {
     EXPECT_THROW(encoder_.encode(&encoded_data_[0], buf_len - 1),
                  isc::BadValue);
 
-    // Type of RDATA and the specified RR type don't match.  addRdata() should
-    // detect this inconsistency.
+    // Some of the following checks confirm that adding an Rdata of the
+    // wrong RR type will be rejected.  Several different cases are checked,
+    // but there shouldn't be any essential difference among these cases in
+    // the tested code; these cases were considered because in an older version
+    // of implementation rejected them for possibly different reasons, and
+    // we simply keep these cases as they are not so many (and may help detect
+    // future possible regression).
     encoder_.start(RRClass::IN(), RRType::AAAA());
-    EXPECT_THROW(encoder_.addRdata(*a_rdata_), isc::BadValue);
+    EXPECT_THROW(encoder_.addRdata(*a_rdata_), std::bad_cast);
 
-    // Likewise.
     encoder_.start(RRClass::IN(), RRType::A());
-    EXPECT_THROW(encoder_.addRdata(*aaaa_rdata_), isc::BadValue);
+    EXPECT_THROW(encoder_.addRdata(*aaaa_rdata_), std::bad_cast);
 
-    // Likewise.  The encoder expects the first name completes the data, and
-    // throws on the second due as an unexpected name field.
     const ConstRdataPtr rp_rdata =
         createRdata(RRType::RP(), RRClass::IN(), "a.example. b.example");
     encoder_.start(RRClass::IN(), RRType::NS());
-    EXPECT_THROW(encoder_.addRdata(*rp_rdata), isc::BadValue);
+    EXPECT_THROW(encoder_.addRdata(*rp_rdata), std::bad_cast);
 
-    // Likewise.  The encoder considers the name data a variable length data
-    // field, and throws on the first name.
     encoder_.start(RRClass::IN(), RRType::DHCID());
-    EXPECT_THROW(encoder_.addRdata(*rp_rdata), isc::BadValue);
+    EXPECT_THROW(encoder_.addRdata(*rp_rdata), std::bad_cast);
 
-    // Likewise.  The text RDATA (2 bytes) will be treated as MX preference,
-    // and the encoder will still expect to see a domain name.
     const ConstRdataPtr txt_rdata = createRdata(RRType::TXT(), RRClass::IN(),
                                                 "a");
     encoder_.start(RRClass::IN(), RRType::MX());
-    EXPECT_THROW(encoder_.addRdata(*txt_rdata), isc::BadValue);
+    EXPECT_THROW(encoder_.addRdata(*txt_rdata), std::bad_cast);
 
-    // Similar to the previous one, but in this case there's no data field
-    // in the spec.
     encoder_.start(RRClass::IN(), RRType::NS());
-    EXPECT_THROW(encoder_.addRdata(*txt_rdata), isc::BadValue);
+    EXPECT_THROW(encoder_.addRdata(*txt_rdata), std::bad_cast);
 
-    // Likewise.  Inconsistent name compression policy.
     const ConstRdataPtr ns_rdata =
         createRdata(RRType::NS(), RRClass::IN(), "ns.example.");
     encoder_.start(RRClass::IN(), RRType::DNAME());
-    EXPECT_THROW(encoder_.addRdata(*ns_rdata), isc::BadValue);
+    EXPECT_THROW(encoder_.addRdata(*ns_rdata), std::bad_cast);
 
-    // Same as the previous one, opposite inconsistency.
     const ConstRdataPtr dname_rdata =
         createRdata(RRType::DNAME(), RRClass::IN(), "dname.example.");
     encoder_.start(RRClass::IN(), RRType::NS());
-    EXPECT_THROW(encoder_.addRdata(*dname_rdata), isc::BadValue);
+    EXPECT_THROW(encoder_.addRdata(*dname_rdata), std::bad_cast);
 
     // RDATA len exceeds the 16-bit range.  Technically not invalid, but
     // we don't support that (and it's practically useless anyway).