[2095] handled various error cases

src/lib/datasrc/memory/rdata_encoder.cc

@@ -286,14 +286,31 @@ public:
     virtual void setTruncated() {}
     virtual void setLengthLimit(size_t) {}
     virtual void setCompressMode(CompressMode) {}
-    virtual void writeName(const Name& name, bool /*compress*/) {
-        assert(current_field_ < encode_spec_->field_count); // TBD
-
+    virtual void writeName(const Name& name, bool compress) {
+        if (current_field_ >= encode_spec_->field_count) {
+            isc_throw(BadValue,
+                      "RDATA encoder encounters an unexpected name data: " <<
+                      name);
+        }
         extendData();
+        if (current_field_ >= encode_spec_->field_count) {
+            isc_throw(BadValue,
+                      "RDATA encoder encounters an unexpected name data: " <<
+                      name);
+        }
 
+        // At this point it's ensured we still have a field in the spec,
+        // and it's a domain name field.
         const RdataFieldSpec& field =
             encode_spec_->fields[current_field_++];
-        assert(field.type == RdataFieldSpec::DOMAIN_NAME); // TBD
+        assert(field.type == RdataFieldSpec::DOMAIN_NAME);
+
+        // It would be compressed iff the field has that attribute.
+        if (compress !=
+            ((field.name_attributes & NAMEATTR_COMPRESSIBLE) != 0)) {
+            isc_throw(BadValue, "RDATA encoder error, inconsistent name "
+                      "compression policy: " << name);
+        }
 
         const LabelSequence labels(name);
         labels.serialize(labels_placeholder_, sizeof(labels_placeholder_));
@@ -314,10 +331,14 @@ public:
         current_field_ = 0;
     }
     void endRdata() {
+        // Handle any remaining data (there should be no more name).  Then
+        // we should reach the end of the fields.
         if (current_field_ < encode_spec_->field_count) {
             extendData();
         }
-        assert(current_field_ == encode_spec_->field_count); // TBD
+        if (current_field_ != encode_spec_->field_count) {
+            isc_throw(BadValue, "RDATA encoder finds missing field");
+        }
     }
     vector<pair<size_t, size_t> > data_positions_;
     vector<pair<size_t, Name> > names_;
@@ -340,18 +361,32 @@ private:
             }
             ++current_field_;
             if (field.type == RdataFieldSpec::FIXEDLEN_DATA) {
-                // TBD: validation
+                if (data_len < field.fixeddata_len) {
+                    isc_throw(BadValue,
+                              "RDATA encoding: available data too short "
+                              "for the type");
+                }
                 data_len -= field.fixeddata_len;
                 continue;
             }
-            // We are looking at a variable-length data field.
-            // TBD: 16bit len check
+            // We are looking at a variable-length data field.  For encoding
+            // purposes, it's a single field covering all data, even if it
+            // may consist of multiple fields as DNS RDATA (e.g. TXT).
+            if (data_len > 0xffff) {
+                isc_throw(RdataEncodingError, "RDATA field is too large: "
+                          << data_len << " bytes");
+            }
             data_lengths_.push_back(data_len);
             data_len = 0;
             break;
         }
-        // TBD: data_len must be 0;
-        assert(data_len == 0);
+
+        // We've reached the end of the RDATA or just consumed a variable
+        // length data field.  So we should have eaten all available data
+        // at this moment.
+        if (data_len != 0) {
+            isc_throw(BadValue, "redundant data for encoding in RDATA");
+        }
 
         // We added this much data from last time
         data_positions_.push_back(

src/lib/datasrc/memory/rdata_encoder.h

@@ -15,6 +15,8 @@
 #ifndef DATASRC_MEMORY_RDATA_ENCODER_H
 #define DATASRC_MEMORY_RDATA_ENCODER_H 1
 
+#include <exceptions/exceptions.h>
+
 #include <dns/labelsequence.h>
 #include <dns/rdata.h>
 #include <dns/rrclass.h>
@@ -29,6 +31,17 @@ namespace isc {
 namespace datasrc {
 namespace memory {
 
+/// \brief General error in RDATA encoding.
+///
+/// This is thrown when \c RdataEncoder encounters a rare, unsupported
+/// situation. a method is called for a name or RRset which
+/// is not in or below the zone.
+class RdataEncodingError : public Exception {
+public:
+    RdataEncodingError(const char* file, size_t line, const char* what) :
+        Exception(file, line, what) {}
+};
+
 /// \brief Attributes of domain name fields of encoded RDATA.
 ///
 /// The enum values define special traits of the name that can affect how
@@ -71,7 +84,16 @@ public:
 
     /// \brief TBD
     ///
+    /// This implementation does not support RDATA (or any subfield of it)
+    /// whose size exceeds 65535 bytes (max uint16_t value).  Such RDATA
+    /// may not necessarily considered invalid in terms of protocol
+    /// specification, but in practice it's mostly useless because the
+    /// corresponding RR won't fit in any valid DNS message.
+    ///
     /// \throw InvalidOperation called before start().
+    /// \throw BadValue inconsistent data found.
+    /// \throw RdataEncodingError A very unusual case, such as over 64KB RDATA.
+    /// \throw std::bad_alloc Internal memory allocation failure.
     void addRdata(const dns::rdata::Rdata& rdata);
 
     /// \brief TBD

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

@@ -14,10 +14,13 @@
 
 #include <exceptions/exceptions.h>
 
+#include <util/buffer.h>
+
 #include <dns/name.h>
 #include <dns/labelsequence.h>
 #include <dns/messagerenderer.h>
 #include <dns/rdata.h>
+#include <dns/rdataclass.h>
 #include <dns/rrclass.h>
 #include <dns/rrtype.h>
 
@@ -111,7 +114,9 @@ renderDataField(MessageRenderer* renderer, const uint8_t* data,
 class RdataEncoderTest : public ::testing::Test {
 protected:
     RdataEncoderTest() : a_rdata_(createRdata(RRType::A(), RRClass::IN(),
-                                              "192.0.2.53"))
+                                              "192.0.2.53")),
+                         aaaa_rdata_(createRdata(RRType::AAAA(), RRClass::IN(),
+                                                 "2001:db8::53"))
     {}
 
     // This helper test method constructs encodes the given list of RDATAs
@@ -124,6 +129,7 @@ protected:
                      size_t expected_varlen_fields);
 
     const ConstRdataPtr a_rdata_;     // commonly used RDATA
+    const ConstRdataPtr aaaa_rdata_;  // commonly used RDATA
     RdataEncoder encoder_;
     vector<uint8_t> encoded_data_;
     MessageRenderer expected_renderer_;
@@ -279,9 +285,55 @@ TEST_F(RdataEncoderTest, badAddRdata) {
                  isc::BadValue);
     encoded_data_.resize(buf_len + 1);
     encoder_.encode(&encoded_data_[1], buf_len);
-}
 
-// TODO: add before start
+    // Type of RDATA and the specified RR type don't match.  addRdata() should
+    // detect this inconsistency.
+    encoder_.start(RRClass::IN(), RRType::AAAA());
+    EXPECT_THROW(encoder_.addRdata(*a_rdata_), isc::BadValue);
+
+    // Likewise.
+    encoder_.start(RRClass::IN(), RRType::A());
+    EXPECT_THROW(encoder_.addRdata(*aaaa_rdata_), isc::BadValue);
+
+    // 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);
+
+    // 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);
+
+    // 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);
+
+    // 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);
+
+    // 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);
+
+    // RDATA len exceeds the 16-bit range.  Technically not invalid, but
+    // we don't support that (and it's practically useless anyway).
+    encoded_data_.resize(65536); // use encoded_data_ for placeholder
+    isc::util::InputBuffer buffer(&encoded_data_[0], encoded_data_.size());
+    encoder_.start(RRClass::IN(), RRType::DHCID());
+    EXPECT_THROW(encoder_.addRdata(in::DHCID(buffer, encoded_data_.size())),
+                                   RdataEncodingError);
+}
 
 // Note: in our implementation RRSIG is treated as opaque data (including
 // the signer name).  We use "com" for signer so it won't be a compress