BIND 10 trac2100, updated. 48ff20698455bb03fadef08daa7c1176534d7e41 [2100] prohibit overriding original data on LabelSequence::serialize.
BIND 10 source code commits
bind10-changes at lists.isc.org
Thu Aug 23 20:12:00 UTC 2012
The branch, trac2100 has been updated
via 48ff20698455bb03fadef08daa7c1176534d7e41 (commit)
via 0c16bd99c081bbdf9a16081e4fc9783edbea0cb9 (commit)
from ef7d33797149380197fbdf259464d8455d6c1400 (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 48ff20698455bb03fadef08daa7c1176534d7e41
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Thu Aug 23 13:10:01 2012 -0700
[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.
commit 0c16bd99c081bbdf9a16081e4fc9783edbea0cb9
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Thu Aug 23 09:27:46 2012 -0700
[2100] use boost::noncopyable to make the class noncopyable by convention
-----------------------------------------------------------------------
Summary of changes:
src/lib/datasrc/memory/domaintree.h | 6 ++++-
src/lib/datasrc/memory/zone_table.h | 14 ++----------
src/lib/datasrc/rbtree.h | 6 ++++-
src/lib/dns/labelsequence.cc | 22 +++++++++++++++++-
src/lib/dns/labelsequence.h | 33 +++++++++++++++++++++++++--
src/lib/dns/tests/labelsequence_unittest.cc | 31 +++++++++++++++++++++++--
6 files changed, 93 insertions(+), 19 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/datasrc/memory/domaintree.h b/src/lib/datasrc/memory/domaintree.h
index 33efc88..2c4bd59 100644
--- a/src/lib/datasrc/memory/domaintree.h
+++ b/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 =
diff --git a/src/lib/datasrc/memory/zone_table.h b/src/lib/datasrc/memory/zone_table.h
index a486b6a..b5da957 100644
--- a/src/lib/datasrc/memory/zone_table.h
+++ b/src/lib/datasrc/memory/zone_table.h
@@ -20,6 +20,7 @@
#include <datasrc/result.h>
#include <datasrc/memory/domaintree.h>
+#include <boost/noncopyable.hpp>
#include <boost/interprocess/offset_ptr.hpp>
namespace isc {
@@ -57,7 +58,7 @@ class ZoneData;
///
/// This class is intended to be used as a backend for the \c MemoryDataSrc
/// class, and is not intended to be used for other general purposes.
-class ZoneTable {
+class ZoneTable : boost::noncopyable {
private:
// The deleter for the zone data stored in the table.
struct ZoneDataDeleter {
@@ -90,17 +91,7 @@ public:
const ZoneData* const zone_data;
};
- ///
- /// \name Constructors and Destructor.
- ///
- /// \b Note:
- /// The copy constructor and the assignment operator are intentionally
- /// defined as private, making this class non copyable.
- //@{
private:
- ZoneTable(const ZoneTable& source);
- ZoneTable& operator=(const ZoneTable& source);
-
/// Constructor.
///
/// An object of this class is always expected to be created by the
@@ -111,7 +102,6 @@ private:
/// It never throws an exception otherwise.
ZoneTable(ZoneTableTree* zones) : zones_(zones)
{}
- //@}
public:
/// \brief Allocate and construct \c ZoneTable
diff --git a/src/lib/datasrc/rbtree.h b/src/lib/datasrc/rbtree.h
index 2ec5347..eb971e8 100644
--- a/src/lib/datasrc/rbtree.h
+++ b/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 =
diff --git a/src/lib/dns/labelsequence.cc b/src/lib/dns/labelsequence.cc
index ba85b91..ed23f26 100644
--- a/src/lib/dns/labelsequence.cc
+++ b/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;
diff --git a/src/lib/dns/labelsequence.h b/src/lib/dns/labelsequence.h
index 186bda6..caadd90 100644
--- a/src/lib/dns/labelsequence.h
+++ b/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
diff --git a/src/lib/dns/tests/labelsequence_unittest.cc b/src/lib/dns/tests/labelsequence_unittest.cc
index e8e2846..2ce1ef7 100644
--- a/src/lib/dns/tests/labelsequence_unittest.cc
+++ b/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),
More information about the bind10-changes
mailing list