From: Jelte Jansen Date: Wed, 25 Jul 2012 16:05:02 +0000 (+0200) Subject: [2148] extend() improvements and more tests X-Git-Tag: trac2351_base~157^2~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3a5fcdca4afbd234efd5c7bad28b3ac1dc306a43;p=thirdparty%2Fkea.git [2148] extend() improvements and more tests --- diff --git a/src/lib/dns/labelsequence.cc b/src/lib/dns/labelsequence.cc index 55c4de432d..25ecee77be 100644 --- a/src/lib/dns/labelsequence.cc +++ b/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 diff --git a/src/lib/dns/labelsequence.h b/src/lib/dns/labelsequence.h index 3802557612..cb9fc9c7a3 100644 --- a/src/lib/dns/labelsequence.h +++ b/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 diff --git a/src/lib/dns/tests/labelsequence_unittest.cc b/src/lib/dns/tests/labelsequence_unittest.cc index f619a1c59a..79484131a1 100644 --- a/src/lib/dns/tests/labelsequence_unittest.cc +++ b/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); +*/ } }