[2100] prohibit overriding original data on LabelSequence::serialize.

added a test case for it and updated the doc.
rbtree/domaintree was modified to avoid such operations.

src/lib/datasrc/memory/domaintree.h

@@ -1648,8 +1648,12 @@ DomainTree<T, DT>::insert(util::MemorySegment& mem_sgmt,
     isc::dns::LabelSequence target_labels(target_name);
 
     int order = -1;
+    // For possible LabelSequence serialization we always store labels data
+    // in the separate local buffer.
+    uint8_t labels_buf[dns::LabelSequence::MAX_SERIALIZED_LENGTH];
     while (current != NULL) {
-        const dns::LabelSequence current_labels(current->getLabels());
+        const dns::LabelSequence current_labels(
+            dns::LabelSequence(current->getLabels(), labels_buf));
         const isc::dns::NameComparisonResult compare_result =
             target_labels.compare(current_labels);
         const isc::dns::NameComparisonResult::NameRelation relation =

src/lib/datasrc/rbtree.h

@@ -1622,8 +1622,12 @@ RBTree<T>::insert(util::MemorySegment& mem_sgmt,
     isc::dns::LabelSequence target_labels(target_name);
 
     int order = -1;
+    // For possible LabelSequence serialization we always store labels data
+    // in the separate local buffer.
+    uint8_t labels_buf[dns::LabelSequence::MAX_SERIALIZED_LENGTH];
     while (current != NULL) {
-        const dns::LabelSequence current_labels(current->getLabels());
+        const dns::LabelSequence current_labels(
+            dns::LabelSequence(current->getLabels(), labels_buf));
         const isc::dns::NameComparisonResult compare_result =
             target_labels.compare(current_labels);
         const isc::dns::NameComparisonResult::NameRelation relation =

src/lib/dns/labelsequence.cc

@@ -91,6 +91,19 @@ LabelSequence::getSerializedLength() const {
     return (1 + getLabelCount() + getDataLength());
 }
 
+namespace {
+// Check if buf is not in the range of [bp, ep), which means
+// - end of buffer is before bp, or
+// - beginning of buffer is on or after ep
+bool
+isOutOfRange(const uint8_t* bp, const uint8_t* ep,
+             const uint8_t* buf, size_t buf_len)
+{
+    return (bp >= buf + buf_len || // end of buffer is before bp
+            ep <= buf);            // beginning of buffer is on or after ep
+}
+}
+
 void
 LabelSequence::serialize(void* buf, size_t buf_len) const {
     const size_t expected_size = getSerializedLength();
@@ -101,12 +114,19 @@ LabelSequence::serialize(void* buf, size_t buf_len) const {
     const size_t offsets_len = getLabelCount();
     assert(offsets_len < 256);  // should be in the 8-bit range
 
+    // Overridden check.  Buffer shouldn't overwrap the offset of name data
+    // regions.
     uint8_t* bp = reinterpret_cast<uint8_t*>(buf);
+    const size_t ndata_len = getDataLength();
+    if (!isOutOfRange(offsets_, offsets_ + offsets_len, bp, buf_len) ||
+        !isOutOfRange(data_, data_ + ndata_len, bp, buf_len)) {
+        isc_throw(BadValue, "serialize would break the source sequence");
+    }
+
     *bp++ = offsets_len;
     for (size_t i = 0; i < offsets_len; ++i) {
         *bp++ = offsets_[first_label_ + i] - offsets_[first_label_];
     }
-    const size_t ndata_len = getDataLength();
     std::memcpy(bp, &data_[offsets_[first_label_]], ndata_len);
     bp += ndata_len;
 

src/lib/dns/labelsequence.h

@@ -179,6 +179,34 @@ public:
     /// the value returned by getSerializedLength() (it can be larger than
     /// that).
     ///
+    /// Be careful about where the buffer is located; due to the nature
+    /// of the buffer, it's quite possible that the memory region is being used
+    /// to construct another active \c LabelSequence.  In such a case
+    /// the serialization would silently break that sequence object, and
+    /// it will be very difficult to identify the cause.  This method
+    /// has minimal level checks to avoid such disruption: If the serialization
+    /// would break "this" \c LabelSequence object, it doesn't write anything
+    /// to the given buffer and throw a \c isc::BadValue exception.
+    ///
+    /// In general, it should be safe to call this method on a
+    /// \c LabelSequence object constructed from a \c Name object or
+    /// a copy of such \c LabelSequence.  When you construct \c LabelSequence
+    /// from pre-serialized data, calling this method on it can be unsafe.
+    /// One safe (but a bit less efficient) way in such a case is to make
+    /// the source \c LabelSequence temporary and immediately create a
+    /// local copy using an explicit buffer, and call this method on the
+    /// latter:
+    /// \code
+    ///    // don't do this, it's not safe (and would result in exception):
+    ///    // LabelSequence(buf).serialize(buf, buf_len);
+    ///
+    ///    // The following are the safe way:
+    ///    uint8_t ext_buf[LabelSequence::MAX_SERIALIZED_LENGTH];
+    ///    LabelSequence seq(LabelSequence(buf), ext_buf);
+    ///    ... (strip the labels, etc)
+    ///    seq.serialize(buf, buf_len); // it's safe to override buf here
+    /// \endcode
+    ///
     /// The serialized image would be as follows:
     /// - olen: number of offsets (1 byte)
     /// - binary sequence of offsets (olen bytes, verbatim copy of offsets_
@@ -186,13 +214,14 @@ public:
     /// - binary sequence of name data (length determined by itself, verbatim
     ///   copy of data_ of the corresponding size)
     ///
-    /// Applications must use the resulting image opaque value and must not
+    /// Applications must use the resulting image as opaque value and must not
     /// use it for other purposes than input to the corresponding constructor
     /// to restore it.  Application behavior that assumes the specific
     /// organization of the image is not guaranteed.
     ///
     /// \throw isc::BadValue buf_len is too short (this method never throws
-    /// otherwise)
+    /// otherwise) or the serialization would override internal data of
+    /// of the source LabelSequence.
     ///
     /// \param buf Pointer to the placeholder to dump the serialized image
     /// \param buf_len The size of available region in \c buf

src/lib/dns/tests/labelsequence_unittest.cc

@@ -712,8 +712,9 @@ TEST_F(LabelSequenceTest, LeftShiftOperator) {
 }
 
 TEST_F(LabelSequenceTest, serialize) {
-    // placeholder for serialized data
-    uint8_t labels_buf[LabelSequence::MAX_SERIALIZED_LENGTH];
+    // placeholder for serialized data.  We use a sufficiently large space
+    // for testing the overwrapping cases below.
+    uint8_t labels_buf[LabelSequence::MAX_SERIALIZED_LENGTH * 3];
 
     // vector to store expected and actual data
     vector<LabelSequence> actual_labelseqs;
@@ -777,6 +778,32 @@ TEST_F(LabelSequenceTest, serialize) {
 
         EXPECT_EQ(NameComparisonResult::EQUAL,
                   LabelSequence(labels_buf).compare(*itl).getRelation());
+
+        // Shift the data to the middle of the buffer for overwrap check
+        uint8_t* const bp = labels_buf;
+        std::memcpy(bp + serialized_len, bp, serialized_len);
+        // Memory layout is now as follows:
+        //   <- ser_len ->          <- ser_len ------>
+        // bp             bp+ser_len                  bp+(ser_len*2)
+        //                           olen,odata,ndata
+
+        // end of buffer would be the first byte of offsets: invalid.
+        EXPECT_THROW(LabelSequence(bp + serialized_len).
+                     serialize(bp + 2, serialized_len),
+                     isc::BadValue);
+        // begin of buffer would be the last byte of ndata: invalid.
+        EXPECT_THROW(LabelSequence(bp + serialized_len).
+                     serialize(bp + (2 * serialized_len) - 1, serialized_len),
+                     isc::BadValue);
+        // A boundary safe case: buffer is placed after the sequence data.
+        // should cause no disruption.
+        LabelSequence(bp + serialized_len).
+                     serialize(bp + 2 * serialized_len, serialized_len);
+        // A boundary safe case: buffer is placed before the sequence data
+        // should cause no disruption. (but the original serialized data will
+        // be overridden, so it can't be used any more)
+        LabelSequence(bp + serialized_len).
+                     serialize(bp + 1, serialized_len);
     }
 
     EXPECT_THROW(ls1.serialize(labels_buf, ls1.getSerializedLength() - 1),