[2498] Fix compareCharStrings for empty charstrings

src/lib/dns/rdata/generic/detail/char_string.cc

@@ -118,6 +118,15 @@ charStringToString(const CharString& char_string) {
 
 int compareCharStrings(const detail::CharString& self,
                        const detail::CharString& other) {
+    if (self.size() == 0 && other.size() == 0) {
+        return (0);
+    }
+    if (self.size() == 0) {
+        return (-1);
+    }
+    if (other.size() == 0) {
+        return (1);
+    }
     const size_t self_len = self[0];
     const size_t other_len = other[0];
     const size_t cmp_len = std::min(self_len, other_len);
@@ -138,7 +147,7 @@ int compareCharStrings(const detail::CharString& self,
 size_t
 bufferToCharString(isc::util::InputBuffer& buffer, size_t rdata_len,
                    CharString& target) {
-    if (rdata_len < 1 || buffer.getLength() < 1) {
+    if (rdata_len < 1 || buffer.getLength() - buffer.getPosition() < 1) {
         isc_throw(isc::dns::DNSMessageFORMERR,
                   "insufficient data to read character-string length");
     }
@@ -148,9 +157,10 @@ bufferToCharString(isc::util::InputBuffer& buffer, size_t rdata_len,
                   "character string length is too large: " <<
                   static_cast<int>(len));
     }
-    if (buffer.getLength() < len) {
+    if (buffer.getLength() - buffer.getPosition() < len) {
         isc_throw(isc::dns::DNSMessageFORMERR,
-                  "not enough data in buffer to read character-string");
+                  "not enough data in buffer to read character-string of len"
+                  << static_cast<int>(len));
     }
 
     target.resize(len + 1);

src/lib/dns/rdata/generic/detail/char_string.h

@@ -92,7 +92,7 @@ int compareCharStrings(const CharString& self, const CharString& other);
 /// (used for integrity checks in the wire data)
 /// \param target The \c CharString where the result will be stored. Any
 /// existing data in the target will be overwritten.
-/// \throw DNSMessageFormERR If the available data is not enough to read
+/// \throw DNSMessageFORMERR If the available data is not enough to read
 /// the character-string, or if the character-string length is out of bounds
 /// \return The number of bytes read
 size_t bufferToCharString(isc::util::InputBuffer& buffer, size_t rdata_len,

src/lib/dns/tests/rdata_char_string_unittest.cc

@@ -14,8 +14,10 @@
 
 #include <util/unittests/wiredata.h>
 
+#include <dns/exceptions.h>
 #include <dns/rdata.h>
 #include <dns/rdata/generic/detail/char_string.h>
+#include <util/buffer.h>
 
 #include <gtest/gtest.h>
 
@@ -25,8 +27,10 @@
 using namespace isc::dns;
 using namespace isc::dns::rdata;
 using isc::dns::rdata::generic::detail::CharString;
+using isc::dns::rdata::generic::detail::bufferToCharString;
 using isc::dns::rdata::generic::detail::stringToCharString;
 using isc::dns::rdata::generic::detail::charStringToString;
+using isc::dns::rdata::generic::detail::compareCharStrings;
 using isc::util::unittests::matchWireData;
 
 namespace {
@@ -171,4 +175,78 @@ TEST_F(CharStringTest, charStringToString) {
     }
 }
 
+TEST_F(CharStringTest, bufferToCharString) {
+    const size_t chstr_size = sizeof(test_charstr);
+    isc::util::InputBuffer buf(test_charstr, chstr_size);
+    size_t read = bufferToCharString(buf, chstr_size, chstr);
+
+    EXPECT_EQ(chstr_size, read);
+    EXPECT_EQ("Test String", charStringToString(chstr));
+}
+
+TEST_F(CharStringTest, bufferToCharString_bad) {
+    const size_t chstr_size = sizeof(test_charstr);
+    isc::util::InputBuffer buf(test_charstr, chstr_size);
+    // Set valid data in both so we can make sure the charstr is not
+    // modified
+    bufferToCharString(buf, chstr_size, chstr);
+    ASSERT_EQ("Test String", charStringToString(chstr));
+
+    // Should be at end of buffer now, so it should fail
+    EXPECT_THROW(bufferToCharString(buf, chstr_size - 1, chstr),
+                 DNSMessageFORMERR);
+    EXPECT_EQ("Test String", charStringToString(chstr));
+
+    // reset and try to read with too low rdata_len
+    buf.setPosition(0);
+    EXPECT_THROW(bufferToCharString(buf, chstr_size - 1, chstr),
+                 DNSMessageFORMERR);
+    EXPECT_EQ("Test String", charStringToString(chstr));
+
+    // set internal charstring len too high
+    const uint8_t test_charstr_err[] = {
+        sizeof("Test String") + 1,
+        'T', 'e', 's', 't', ' ', 'S', 't', 'r', 'i', 'n', 'g'
+    };
+    buf = isc::util::InputBuffer(test_charstr_err, sizeof(test_charstr_err));
+    EXPECT_THROW(bufferToCharString(buf, chstr_size, chstr),
+                 DNSMessageFORMERR);
+    EXPECT_EQ("Test String", charStringToString(chstr));
+
+}
+
+
+
+TEST_F(CharStringTest, compareCharString) {
+    CharString charstr;
+    CharString charstr2;
+    CharString charstr_small1;
+    CharString charstr_small2;
+    CharString charstr_large1;
+    CharString charstr_large2;
+    CharString charstr_empty;
+
+    stringToCharString(createStringRegion("test string"), charstr);
+    stringToCharString(createStringRegion("test string"), charstr2);
+    stringToCharString(createStringRegion("test strin"), charstr_small1);
+    stringToCharString(createStringRegion("test strina"), charstr_small2);
+    stringToCharString(createStringRegion("test stringa"), charstr_large1);
+    stringToCharString(createStringRegion("test strinz"), charstr_large2);
+
+    EXPECT_EQ(0, compareCharStrings(charstr, charstr2));
+    EXPECT_EQ(0, compareCharStrings(charstr2, charstr));
+    EXPECT_EQ(1, compareCharStrings(charstr, charstr_small1));
+    EXPECT_EQ(1, compareCharStrings(charstr, charstr_small2));
+    EXPECT_EQ(-1, compareCharStrings(charstr, charstr_large1));
+    EXPECT_EQ(-1, compareCharStrings(charstr, charstr_large2));
+    EXPECT_EQ(-1, compareCharStrings(charstr_small1, charstr));
+    EXPECT_EQ(-1, compareCharStrings(charstr_small2, charstr));
+    EXPECT_EQ(1, compareCharStrings(charstr_large1, charstr));
+    EXPECT_EQ(1, compareCharStrings(charstr_large2, charstr));
+
+    EXPECT_EQ(-1, compareCharStrings(charstr_empty, charstr));
+    EXPECT_EQ(1, compareCharStrings(charstr, charstr_empty));
+    EXPECT_EQ(0, compareCharStrings(charstr_empty, charstr_empty));
+}
+
 } // unnamed namespace