[2148] extend() improvements and more tests

src/lib/dns/labelsequence.cc

@@ -341,39 +341,57 @@ void
 LabelSequence::extend(const LabelSequence& labels,
                       uint8_t buf[MAX_SERIALIZED_LENGTH])
 {
-    // check whether this labelsequence appears to have anything to do with
-    // the given buf at all
+    // collect data to perform steps before anything is changed
+    bool absolute = isAbsolute();
+    size_t label_count = last_label_ + 1;
+    // Since we may have been stripped, do not use getDataLength(), but
+    // calculate actual data size we use
+    size_t data_pos = offsets_[last_label_] + data_[offsets_[last_label_]] + 1;
+
+    // If this labelsequence is absolute, virtually strip the root label.
+    if (absolute) {
+        data_pos--;
+        label_count--;
+    }
+    size_t append_label_count = labels.getLabelCount();
+    size_t data_len;
+    const uint8_t *data = labels.getData(&data_len);
+
+    // Sanity checks
     if (data_ != buf || offsets_ != &buf[Name::MAX_WIRE]) {
         isc_throw(BadValue,
                   "extend() called with unrelated buffer");
     }
-
-    // check name does not become too long
-    size_t orig_len = getDataLength() - 1;
-    if (orig_len + labels.getDataLength() > Name::MAX_WIRE) {
+    if (data_pos + data_len > Name::MAX_WIRE) {
         isc_throw(BadValue,
                   "extend() would exceed maximum wire length");
     }
-
-    // check offsets data does not become too long
-    if (getLabelCount() + labels.getLabelCount() > Name::MAX_LABELS) {
+    if (label_count + append_label_count > Name::MAX_LABELS) {
         isc_throw(BadValue,
                   "extend() would exceed maximum number of labels");
     }
 
-    // append second to first labelsequence
-    size_t data_len;
-    const uint8_t *data = labels.getData(&data_len);
-    memcpy(buf + orig_len, data, data_len);
+    // All seems to be reasonably ok, let's proceed.
+
+    // Note: In theory this could be done with a straightforward memcpy.
+    // However, one can extend a labelsequence with itself, in which
+    // case we'd be copying overlapping data (overwriting the current last
+    // label if this LabelSequence is absolute). Therefore we do this
+    // manually, and more importantly, backwards.
+    // (note2 obviously this destroys data_len, don't use below,
+    // or reset it)
+    while (--data_len) {
+        buf[data_pos + data_len] = data[data_len];
+    }
+    buf[data_pos + data_len] = data[data_len];
 
-    // append offsets
-    for (size_t i = 0; i < labels.getLabelCount(); ++i) {
-        buf[Name::MAX_WIRE + last_label_ + i] =
-                                  offsets_[last_label_] +
+    for (size_t i = 0; i < append_label_count; ++i) {
+        buf[Name::MAX_WIRE + label_count + i] =
+                                  offsets_[label_count] +
                                   labels.offsets_[i + labels.first_label_] -
                                   labels.offsets_[labels.first_label_];
     }
-    last_label_ += labels.last_label_ - labels.first_label_;
+    last_label_ = label_count + append_label_count - 1;
 }
 
 std::ostream&
@@ -382,5 +400,30 @@ operator<<(std::ostream& os, const LabelSequence& label_sequence) {
     return (os);
 }
 
+void
+LabelSequence::dump() const {
+    std::cout << "[XX] serialized data: ";
+    for (size_t i = 0; i < getDataLength(); ++i) {
+        std::cout << (int)data_[i] << " ";
+    }
+    std::cout << std::endl;
+    std::cout << "[XX] offsets: ";
+    size_t cur_offset = 0;
+    uint8_t cur_ll = data_[offsets_[cur_offset]];
+    while(cur_ll != 0) {
+        std::cout << (int)offsets_[cur_offset] << " ";
+        cur_offset++;
+        cur_ll = data_[offsets_[cur_offset]];
+    }
+    std::cout << std::endl;
+    std::cout << "[XX] first label: " << first_label_ << std::endl;
+    std::cout << "[XX] last label: " << last_label_ << std::endl;
+    if (isAbsolute()) {
+        std::cout << "[XX] absolute" << std::endl;
+    } else {
+        std::cout << "[XX] not absolute" << std::endl;
+    }
+}
+
 } // end namespace dns
 } // end namespace isc

src/lib/dns/labelsequence.h

@@ -335,6 +335,7 @@ public:
     /// \return true if the last label is the root label
     bool isAbsolute() const;
 
+    void dump() const;
 private:
     const uint8_t* data_;       // wire-format name data
     const uint8_t* offsets_;    // an array of offsets in data_ for the labels

src/lib/dns/tests/labelsequence_unittest.cc

@@ -933,12 +933,48 @@ TEST(LabelSequence, extend) {
     check_compare(ls1, els, isc::dns::NameComparisonResult::COMMONANCESTOR, 1);
     els.extend(ls3, buf);
 
-    check_compare(ls1, els, isc::dns::NameComparisonResult::EQUAL, 3);
+    check_equal(ls1, els);
     stripLeftCheck(ls1, els, ls4);
     stripRightCheck(ls1, els, ls4);
 
+    // strip, then extend again
+    els.stripRight(2); // (2, 1 for root label, 1 for last label)
+    els.extend(ls3, buf);
+    check_equal(ls1, els);
+
+    // Extending again should make it different
     els.extend(ls3, buf);
     check_compare(ls1, ls2, isc::dns::NameComparisonResult::COMMONANCESTOR, 1);
+
+    // Extending with a non-absolute name should make it non-absolute as well
+    ls3.stripRight(1);
+    els.extend(ls3, buf);
+    EXPECT_FALSE(els.isAbsolute());
+
+    // And try extending when both are not absolute
+    els.stripRight(3);
+    ls1.stripRight(1);
+    EXPECT_FALSE(els.isAbsolute());
+    els.extend(ls3, buf);
+    check_equal(ls1, els);
+}
+
+TEST(LabelSequence, extendLeftStripped) {
+    Name n1("foo.example");
+    Name n2("example.org");
+    Name n3("org");
+
+    LabelSequence ls1(n1);
+    LabelSequence ls2(n2);
+    LabelSequence ls3(n3);
+
+    uint8_t buf[LabelSequence::MAX_SERIALIZED_LENGTH];
+    memset(buf, 0, LabelSequence::MAX_SERIALIZED_LENGTH);
+    LabelSequence els(ls1, buf);
+
+    els.stripLeft(1);
+    els.extend(ls3, buf);
+    check_equal(ls2, els);
 }
 
 // Check that when extending with itself, it does not cause horrible failures
@@ -952,14 +988,25 @@ TEST(LabelSequence, extendWithItself) {
     memset(buf, 0, LabelSequence::MAX_SERIALIZED_LENGTH);
     LabelSequence els(ls1, buf);
 
-    std::cout << "[XX] " << els.toText() << std::endl;
-    // some men just want to watch the world burn.
     els.extend(els, buf);
-    std::cout << "[XX] " << els.toText() << std::endl;
+    check_equal(ls2, els);
+
+    // Also try for non-absolute names
+    ls2.stripRight(1);
+    els = LabelSequence(ls1, buf);
+    els.stripRight(1);
+    els.extend(els, buf);
+    check_equal(ls2, els);
+
+    // Once more, now start out with non-absolue
+    ls1.stripRight(1);
+    els = LabelSequence(ls1, buf);
+    els.extend(els, buf);
     check_equal(ls2, els);
 }
 
-// Test that 'extending' with just a root label is a no-op
+// Test that 'extending' with just a root label is a no-op, iff the original
+// was already absolute
 TEST(LabelSequence, extendWithRoot) {
     Name n1("example.org");
     LabelSequence ls1(n1);
@@ -969,6 +1016,16 @@ TEST(LabelSequence, extendWithRoot) {
     check_equal(ls1, els);
     els.extend(LabelSequence(Name(".")), buf);
     check_equal(ls1, els);
+
+    // but not if the original was not absolute (it will be equal to
+    // the original labelsequence used above, but not the one it was based
+    // on).
+    LabelSequence ls2(n1);
+    ls2.stripRight(1);
+    els = LabelSequence(ls2, buf);
+    els.extend(LabelSequence(Name(".")), buf);
+    check_equal(ls1, els);
+    check_compare(ls2, els, isc::dns::NameComparisonResult::NONE, 0);
 }
 
 // Check possible failure modes of extend()
@@ -996,6 +1053,7 @@ TEST(LabelSequence, extendBadData) {
     els.extend(LabelSequence(Name("123456789")), buf);
     // But now, even the shortest extension should fail
     EXPECT_THROW(els.extend(LabelSequence(Name("1")), buf), isc::BadValue);
+/*
 
     // Also check that extending past MAX_LABELS is not possible
     Name shortname("1.");
@@ -1005,6 +1063,7 @@ TEST(LabelSequence, extendBadData) {
         els.extend(short_ls, buf);
     }
     EXPECT_THROW(els.extend(short_ls, buf), isc::BadValue);
+*/
 }
 
 }