BIND 10 trac2095, updated. 84e7463c44719b7f4e97ad176c6861bd1c8b0a81 [2095] introduce a wrapper for encode() to do overrun checks consistently.

BIND 10 source code commits bind10-changes at lists.isc.org
Mon Jul 30 20:01:14 UTC 2012


The branch, trac2095 has been updated
       via  84e7463c44719b7f4e97ad176c6861bd1c8b0a81 (commit)
       via  85608831a3455139bac8e551b95e4137835aede3 (commit)
       via  90f167386a5405e571f58bddfec252a56e2f1e9c (commit)
      from  14c5b23041be0b3ac1446a3fb6239859ca082d64 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
commit 84e7463c44719b7f4e97ad176c6861bd1c8b0a81
Author: JINMEI Tatuya <jinmei at isc.org>
Date:   Mon Jul 30 13:00:26 2012 -0700

    [2095] introduce a wrapper for encode() to do overrun checks consistently.

commit 85608831a3455139bac8e551b95e4137835aede3
Author: JINMEI Tatuya <jinmei at isc.org>
Date:   Mon Jul 30 11:56:20 2012 -0700

    [2095] simplified RdataFieldComposer::updateOtherData().
    
    it didn't have to be a loop because of implicit assumptions on how the fields
    are to be composed.  the simpler version should be more understandable.
    the assumptions are now more explicitly documented.  also, one more test case
    was added.

commit 90f167386a5405e571f58bddfec252a56e2f1e9c
Author: JINMEI Tatuya <jinmei at isc.org>
Date:   Mon Jul 30 09:55:19 2012 -0700

    [2095] tried to improve an exception what() message.
    
    in response to a review comment.

-----------------------------------------------------------------------

Summary of changes:
 src/lib/datasrc/memory/rdata_encoder.cc            |   93 +++++++++++---------
 .../datasrc/memory/tests/rdata_encoder_unittest.cc |   43 ++++++---
 2 files changed, 81 insertions(+), 55 deletions(-)

-----------------------------------------------------------------------
diff --git a/src/lib/datasrc/memory/rdata_encoder.cc b/src/lib/datasrc/memory/rdata_encoder.cc
index f256d44..05ecfb5 100644
--- a/src/lib/datasrc/memory/rdata_encoder.cc
+++ b/src/lib/datasrc/memory/rdata_encoder.cc
@@ -65,6 +65,22 @@ struct RdataFieldSpec {
 };
 
 /// Specification of RDATA in terms of internal encoding.
+///
+/// The fields must be a sequence of:
+/// <0 or more fixed/var-len data fields>,
+/// <0 or more domain name fields>,
+/// <0 or more fixed/var-len data fields>,
+/// <0 or more domain name fields>,
+/// ...and so on.
+/// There must not be more than one consecutive data fields (i.e., without
+/// interleaved by a domain name); it would just be inefficient in terms of
+/// memory footprint and iterating over the fields, and it would break
+/// some assumption within the encoder implementation.  For consecutive
+/// data fields in the DNS protocol, if all fields have fixed lengths, they
+/// should be combined into a single fixed-length field (like the last 20
+/// bytes of SOA RDATA).  If there's a variable length field, they should be
+/// combined into a single variable-length field (such as DNSKEY, which has
+/// 3 fixed-length field followed by one variable-length field).
 struct RdataEncodeSpec {
     const uint16_t field_count; // total number of fields (# of fields member)
     const uint16_t name_count;  // number of domain name fields
@@ -286,24 +302,20 @@ public:
     // Called for each domain name in the RDATA, from the RDATA's toWire()
     // implementation.
     virtual void writeName(const Name& name, bool compress) {
-        // First, after checking the spec consistency, see if we have other
-        // data already stored in the renderer's buffer.
-        if (current_field_ >= encode_spec_->field_count) {
-            isc_throw(BadValue,
-                      "RDATA encoder encounters an unexpected name data: " <<
-                      name);
-        }
+        // First, see if we have other data already stored in the renderer's
+        // buffer, and handle it appropriately.
         updateOtherData();
-        // If there are data fields, current_field_ has been updated.  We
-        // should still be expected to have a domain name in the spec.
+
+        // Then, we still have a field in the spec, and it must be a domain
+        // name field.  Since we know we've passed any prior data field, the
+        // next field must be a domain name as long as it exists; otherwise
+        // it's a bug in the spec (not a bogus input).  So we assert() that
+        // condition.
         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);
@@ -336,11 +348,10 @@ public:
     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) {
-            updateOtherData();
-        }
+        updateOtherData();
         if (current_field_ != encode_spec_->field_count) {
-            isc_throw(BadValue, "RDATA encoder finds missing field");
+            isc_throw(BadValue,
+                      "RDATA encoder didn't find all expected fields");
         }
     }
 
@@ -356,42 +367,38 @@ private:
     // if it contains variable-length field, record its length.
     size_t last_data_pos_;
     void updateOtherData() {
+        // If we've reached the end of the fields or we are expecting a
+        // domain name, there's nothing to do here.
+        if (current_field_ >= encode_spec_->field_count ||
+            encode_spec_->fields[current_field_].type ==
+            RdataFieldSpec::DOMAIN_NAME) {
+            return;
+        }
+
         const size_t cur_pos = getLength();
-        size_t data_len = cur_pos - last_data_pos_;
-        while (current_field_ < encode_spec_->field_count) {
-            const RdataFieldSpec& field = encode_spec_->fields[current_field_];
-            if (field.type == RdataFieldSpec::DOMAIN_NAME) {
-                return;
+        const size_t data_len = cur_pos - last_data_pos_;
+
+        const RdataFieldSpec& field = encode_spec_->fields[current_field_];
+        if (field.type == RdataFieldSpec::FIXEDLEN_DATA) {
+            // The data length of a fixed length field must be the one
+            // specified in the field spec.
+            if (data_len != field.fixeddata_len) {
+                isc_throw(BadValue,
+                          "RDATA encoding: available data too short for the "
+                          "type");
             }
-            ++current_field_;
-            if (field.type == RdataFieldSpec::FIXEDLEN_DATA) {
-                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.  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).
+        } else {
+            // For encoding purposes, a variable-length data field is
+            // 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;
-        }
-
-        // 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");
         }
 
+        ++current_field_;
         last_data_pos_ = cur_pos;
     }
 
diff --git a/src/lib/datasrc/memory/tests/rdata_encoder_unittest.cc b/src/lib/datasrc/memory/tests/rdata_encoder_unittest.cc
index 882262f..2854c37 100644
--- a/src/lib/datasrc/memory/tests/rdata_encoder_unittest.cc
+++ b/src/lib/datasrc/memory/tests/rdata_encoder_unittest.cc
@@ -132,6 +132,8 @@ protected:
                      const vector<ConstRdataPtr>& rdata_list,
                      size_t expected_varlen_fields,
                      const vector<ConstRdataPtr>& rrsig_list);
+    // A wraper for RdataEncoder::encode() with buffer overrun check.
+    void encodeWrapper(size_t data_len);
 
     void addRdataCommon(const vector<ConstRdataPtr>& rrsigs);
     void addRdataMultiCommon(const vector<ConstRdataPtr>& rrsigs);
@@ -148,6 +150,23 @@ protected:
     vector<ConstRdataPtr> rdata_list_;
 };
 
+
+void
+RdataEncoderTest::encodeWrapper(size_t data_len) {
+    // make sure the data buffer is large enough for the canary
+    encoded_data_.resize(data_len + 2);
+    // set the canary data
+    encoded_data_.at(data_len) = 0xde;
+    encoded_data_.at(data_len + 1) = 0xad;
+    // encode, then check the canary is intact
+    encoder_.encode(&encoded_data_[0], data_len);
+    EXPECT_EQ(0xde, encoded_data_.at(data_len));
+    EXPECT_EQ(0xad, encoded_data_.at(data_len + 1));
+    // shrink the data buffer to the originally expected size (some tests
+    // expect that).  the actual encoded data should be intact.
+    encoded_data_.resize(data_len);
+}
+
 void
 RdataEncoderTest::checkEncode(RRClass rrclass, RRType rrtype,
                               const vector<ConstRdataPtr>& rdata_list,
@@ -200,8 +219,7 @@ RdataEncoderTest::checkEncode(RRClass rrclass, RRType rrtype,
     BOOST_FOREACH(const ConstRdataPtr& rdata, rrsig_list) {
         encoder_.addSIGRdata(*rdata);
     }
-    encoded_data_.resize(encoder_.getStorageLength());
-    encoder_.encode(&encoded_data_[0], encoded_data_.size());
+    encodeWrapper(encoder_.getStorageLength());
 
     // If this type of RDATA is expected to contain variable-length fields,
     // we brute force the encoded data, exploiting our knowledge of actual
@@ -330,8 +348,7 @@ TEST_F(RdataEncoderTest, encodeLargeRdata) {
 
     encoder_.start(RRClass::IN(), RRType::DHCID());
     encoder_.addRdata(large_dhcid);
-    encoded_data_.resize(encoder_.getStorageLength());
-    encoder_.encode(&encoded_data_[0], encoded_data_.size());
+    encodeWrapper(encoder_.getStorageLength());
 
     // The encoded data should be identical to the original one.
     ASSERT_LT(sizeof(uint16_t), encoder_.getStorageLength());
@@ -356,9 +373,8 @@ TEST_F(RdataEncoderTest, badAddRdata) {
     // Some operations must follow start().
     EXPECT_THROW(encoder_.addRdata(*a_rdata_), isc::InvalidOperation);
     EXPECT_THROW(encoder_.getStorageLength(), isc::InvalidOperation);
-    encoded_data_.resize(256); // allocate space of some arbitrary size
-    EXPECT_THROW(encoder_.encode(&encoded_data_[0], encoded_data_.size()),
-                 isc::InvalidOperation);
+    // will allocate space of some arbitrary size (256 bytes)
+    EXPECT_THROW(encodeWrapper(256), isc::InvalidOperation);
 
     // Bad buffer for encode
     encoder_.start(RRClass::IN(), RRType::A());
@@ -366,12 +382,11 @@ TEST_F(RdataEncoderTest, badAddRdata) {
     const size_t buf_len = encoder_.getStorageLength();
     // NULL buffer for encode
     EXPECT_THROW(encoder_.encode(NULL, buf_len), isc::BadValue);
-    // buffer length is too short
+    // buffer length is too short (we don't use the wrraper because we don't
+    // like to tweak the length arg to encode()).
     encoded_data_.resize(buf_len - 1);
     EXPECT_THROW(encoder_.encode(&encoded_data_[0], buf_len - 1),
                  isc::BadValue);
-    encoded_data_.resize(buf_len + 1);
-    encoder_.encode(&encoded_data_[1], buf_len);
 
     // Type of RDATA and the specified RR type don't match.  addRdata() should
     // detect this inconsistency.
@@ -401,6 +416,11 @@ TEST_F(RdataEncoderTest, badAddRdata) {
     encoder_.start(RRClass::IN(), RRType::MX());
     EXPECT_THROW(encoder_.addRdata(*txt_rdata), isc::BadValue);
 
+    // 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);
+
     // Likewise.  Inconsistent name compression policy.
     const ConstRdataPtr ns_rdata =
         createRdata(RRType::NS(), RRClass::IN(), "ns.example");
@@ -432,8 +452,7 @@ TEST_F(RdataEncoderTest, addSIGRdataOnly) {
     // (in a partially broken zone) and it's accepted.
     encoder_.start(RRClass::IN(), RRType::A());
     encoder_.addSIGRdata(*rrsig_rdata_);
-    encoded_data_.resize(encoder_.getStorageLength());
-    encoder_.encode(&encoded_data_[0], encoded_data_.size());
+    encodeWrapper(encoder_.getStorageLength());
     ASSERT_LT(sizeof(uint16_t), encoder_.getStorageLength());
 
     // The encoded data should be identical to the given one.



More information about the bind10-changes mailing list