addressing review comments for ticket #96:

- added more tests for TXT and AAAA
- updated what() message for exceptions so that they can be more helpful.
- (this is a kind of feature extesion but) supported multiple character-strings
  in the from wire constructor


git-svn-id: svn://bind10.isc.org/svn/bind10/trunk@1517 e5f2f494-b856-4b98-b285-d166d9295462

src/lib/dns/rdata/generic/txt_16.cc

@@ -32,17 +32,36 @@ using namespace std;
 // BEGIN_ISC_NAMESPACE
 // BEGIN_RDATA_NAMESPACE
 
-TXT::TXT(InputBuffer& buffer, size_t rdata_len UNUSED_PARAM) {
-    // TBD: this is a simple, incomplete implementation that only supports
-    // a single character-string.
-    const uint8_t len = buffer.readUint8();
-    vector<uint8_t> data(len + 1);
-    data[0] = len;
-    buffer.readData(&data[0] + 1, len);
-    string_list_.push_back(data);
+TXT::TXT(InputBuffer& buffer, size_t rdata_len) {
+    if (rdata_len > MAX_RDLENGTH) {
+        isc_throw(InvalidRdataLength, "RDLENGTH too large: " << rdata_len);
+    }
+
+    if (rdata_len == 0) {       // note that this couldn't happen in the loop.
+        isc_throw(DNSMessageFORMERR,
+                  "Error in parsing TXT RDATA: 0-length character string");
+    }
+
+    do {
+        const uint8_t len = buffer.readUint8();
+        if (rdata_len < len + 1) {
+            isc_throw(DNSMessageFORMERR,
+                      "Error in parsing TXT RDATA: character string length "
+                      "is too large: " << static_cast<int>(len));
+        }
+        vector<uint8_t> data(len + 1);
+        data[0] = len;
+        buffer.readData(&data[0] + 1, len);
+        string_list_.push_back(data);
+
+        rdata_len -= (len + 1);
+    } while (rdata_len > 0);
 }
 
 TXT::TXT(const std::string& txtstr) {
+    // TBD: this is a simple, incomplete implementation that only supports
+    // a single character-string.
+
     size_t length = txtstr.size();
     size_t pos_begin = 0;
 
@@ -50,8 +69,16 @@ TXT::TXT(const std::string& txtstr) {
         pos_begin = 1;
         length -= 2;
     }
+
     if (length > MAX_CHARSTRING_LEN) {
-        isc_throw(CharStringTooLong, "");
+        isc_throw(CharStringTooLong, "TXT RDATA construction from text: "
+                  "string length is too long: " << length);
+    }
+
+    // TBD: right now, we don't support escaped characters
+    if (txtstr.find('\\') != string::npos) {
+        isc_throw(InvalidRdataText, "TXT RDATA from text: "
+                  "escaped character is currently not supported: " << txtstr);
     }
 
     vector<uint8_t> data;

src/lib/dns/rdata/in_1/a_1.cc

@@ -24,8 +24,10 @@
 #include <arpa/inet.h> // XXX: for inet_pton/ntop(), not exist in C++ standards
 #include <sys/socket.h> // for AF_INET/AF_INET6
 
-#include "buffer.h"
 #include <exceptions/exceptions.h>
+
+#include "buffer.h"
+#include "exceptions.h"
 #include "messagerenderer.h"
 #include "rdata.h"
 #include "rdataclass.h"
@@ -35,57 +37,60 @@ using namespace std;
 // BEGIN_ISC_NAMESPACE
 // BEGIN_RDATA_NAMESPACE
 
-A::A(const string& addrstr)
-{
+A::A(const string& addrstr) {
     // RFC1035 states textual representation of IN/A RDATA is
     // "four decimal numbers separated by dots without any embedded spaces".
     // This is exactly what inet_pton() accepts for AF_INET.  In particular,
     // it rejects an abbreviated form such as "10.1" meaning "10.0.0.1".
     if (inet_pton(AF_INET, addrstr.c_str(), &addr_) != 1) {
         isc_throw(InvalidRdataText,
-                  "failed to parse IPv4 address for IN/A RDATA");
+                  "IN/A RDATA construction from text failed: Address cannot be "
+                  "converted: " << addrstr);
     }
 }
 
-A::A(InputBuffer& buffer, size_t rdata_len)
-{
+A::A(InputBuffer& buffer, size_t rdata_len) {
     if (rdata_len != sizeof(addr_)) {
-        isc_throw(InvalidRdataLength, "Length mismatch for IN/A RDATA");
+        isc_throw(DNSMessageFORMERR,
+                  "IN/A RDATA construction from wire failed: Invalid length: "
+                  << rdata_len);
+    }
+    if (buffer.getLength() - buffer.getPosition() < sizeof(addr_)) {
+        isc_throw(DNSMessageFORMERR,
+                  "IN/A RDATA construction from wire failed: "
+                  "insufficient buffer length: "
+                  << buffer.getLength() - buffer.getPosition());
     }
     buffer.readData(&addr_, sizeof(addr_));
 }
 
-A::A(const A& other) :
-    Rdata(), addr_(other.addr_)
+A::A(const A& other) : Rdata(), addr_(other.addr_)
 {}
 
 void
-A::toWire(OutputBuffer& buffer) const
-{
+A::toWire(OutputBuffer& buffer) const {
     buffer.writeData(&addr_, sizeof(addr_));
 }
 
 void
-A::toWire(MessageRenderer& renderer) const
-{
+A::toWire(MessageRenderer& renderer) const {
     renderer.writeData(&addr_, sizeof(addr_));
 }
 
 string
-A::toText() const
-{
+A::toText() const {
     char addr_string[sizeof("255.255.255.255")];
 
     if (inet_ntop(AF_INET, &addr_, addr_string, sizeof(addr_string)) == NULL) {
-        isc_throw(Unexpected, "inet_ntop failed for an IPv4 address");
+        isc_throw(Unexpected,
+                  "Failed to convert IN/A RDATA to textual IPv4 address");
     }
 
-    return (string(addr_string));
+    return (addr_string);
 }
 
 int
-A::compare(const Rdata& other) const
-{
+A::compare(const Rdata& other) const {
     const A& other_a = dynamic_cast<const A&>(other);
     return (memcmp(&addr_, &other_a.addr_, sizeof(addr_)));
 }

src/lib/dns/rdata/in_1/aaaa_28.cc

@@ -24,8 +24,10 @@
 #include <arpa/inet.h> // XXX: for inet_pton/ntop(), not exist in C++ standards
 #include <sys/socket.h> // for AF_INET/AF_INET6
 
-#include "buffer.h"
 #include <exceptions/exceptions.h>
+
+#include "buffer.h"
+#include "exceptions.h"
 #include "messagerenderer.h"
 #include "rdata.h"
 #include "rdataclass.h"
@@ -35,18 +37,25 @@ using namespace std;
 // BEGIN_ISC_NAMESPACE
 // BEGIN_RDATA_NAMESPACE
 
-AAAA::AAAA(const string& addrstr)
-{
+AAAA::AAAA(const string& addrstr) {
     if (inet_pton(AF_INET6, addrstr.c_str(), &addr_) != 1) {
         isc_throw(InvalidRdataText,
-                  "failed to parse IPv6 address for IN/AAAA RDATA");
+                  "IN/AAAA RDATA construction from text failed: "
+                  "Address cannot be converted: " << addrstr);
     }
 }
 
-AAAA::AAAA(InputBuffer& buffer, size_t rdata_len)
-{
+AAAA::AAAA(InputBuffer& buffer, size_t rdata_len) {
     if (rdata_len != sizeof(addr_)) {
-        isc_throw(InvalidRdataLength, "Length mismatch for IN/AAAA RDATA");
+        isc_throw(DNSMessageFORMERR,
+                  "IN/AAAA RDATA construction from wire failed: "
+                  "Invalid length: " << rdata_len);
+    }
+    if (buffer.getLength() - buffer.getPosition() < sizeof(addr_)) {
+        isc_throw(DNSMessageFORMERR,
+                  "IN/AAAA RDATA construction from wire failed: "
+                  "insufficient buffer length: "
+                  << buffer.getLength() - buffer.getPosition());
     }
     buffer.readData(&addr_, sizeof(addr_));
 }
@@ -56,32 +65,29 @@ AAAA::AAAA(const AAAA& other) : Rdata() {
 }
 
 void
-AAAA::toWire(OutputBuffer& buffer) const
-{
+AAAA::toWire(OutputBuffer& buffer) const {
     buffer.writeData(&addr_, sizeof(addr_));
 }
 
 void
-AAAA::toWire(MessageRenderer& renderer) const
-{
+AAAA::toWire(MessageRenderer& renderer) const {
     renderer.writeData(&addr_, sizeof(addr_));
 }
 
 string
-AAAA::toText() const
-{
+AAAA::toText() const {
     char addr_string[sizeof("ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255")];
 
     if (inet_ntop(AF_INET6, &addr_, addr_string, sizeof(addr_string)) == NULL) {
-        isc_throw(Unexpected, "inet_ntop failed for an IPv6 address");
+        isc_throw(Unexpected,
+                  "Failed to convert IN/AAAA RDATA to textual IPv6 address");
     }
 
     return (string(addr_string));
 }
 
 int
-AAAA::compare(const Rdata& other) const
-{
+AAAA::compare(const Rdata& other) const {
     const AAAA& other_a = dynamic_cast<const AAAA&>(other);
     return (memcmp(&addr_, &other_a.addr_, sizeof(addr_)));
 }

src/lib/dns/tests/rdata_txt_unittest.cc

@@ -15,6 +15,7 @@
 // $Id$
 
 #include <dns/buffer.h>
+#include <dns/exceptions.h>
 #include <dns/messagerenderer.h>
 #include <dns/rdata.h>
 #include <dns/rdataclass.h>
@@ -32,39 +33,135 @@ using namespace isc::dns;
 using namespace isc::dns::rdata;
 
 namespace {
-class Rdata_TXT_Test : public RdataTest {
-    // there's nothing to specialize
-};
-
 const generic::TXT rdata_txt("Test String");
+const generic::TXT rdata_txt_empty("");
 const generic::TXT rdata_txt_quoated("\"Test String\"");
 const uint8_t wiredata_txt[] = {
     sizeof("Test String") - 1,
     'T', 'e', 's', 't', ' ', 'S', 't', 'r', 'i', 'n', 'g'
 };
+const uint8_t wiredata_nulltxt[] = { 0 };
+vector<uint8_t> wiredata_longesttxt(256, 'a');
+
+class Rdata_TXT_Test : public RdataTest {
+protected:
+    Rdata_TXT_Test() {
+        wiredata_longesttxt[0] = 255; // adjust length
+    }
+};
 
-TEST_F(RdataTest, createFromText)
-{
+TEST_F(Rdata_TXT_Test, createFromText) {
+    // normal case is covered in toWireBuffer.
+
+    // surrounding double-quotes shouldn't change the result.
     EXPECT_EQ(0, rdata_txt.compare(rdata_txt_quoated));
+
+    // Null character-string.
+    obuffer.clear();
+    generic::TXT(string("")).toWire(obuffer);
+    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData,
+                        obuffer.getData(), obuffer.getLength(),
+                        wiredata_nulltxt, sizeof(wiredata_nulltxt));
+
+    // Longest possible character-string.
+    obuffer.clear();
+    generic::TXT(string(255, 'a')).toWire(obuffer);
+    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData,
+                        obuffer.getData(), obuffer.getLength(),
+                        &wiredata_longesttxt[0], wiredata_longesttxt.size());
+
+    // Too long text for a valid character-string.
+    EXPECT_THROW(generic::TXT(string(256, 'a')), CharStringTooLong);
+
+    // The escape character makes the double quote a part of character-string,
+    // so this is invalid input and should be rejected.
+    EXPECT_THROW(generic::TXT("\"Test String\\\""), InvalidRdataText);
+
+    // Terminating double-quote is provided, so this is valid, but in this
+    // version of implementation we reject escaped characters.
+    EXPECT_THROW(generic::TXT("\"Test String\\\"\""), InvalidRdataText);
 }
 
-TEST_F(Rdata_TXT_Test, createFromWire)
-{
+void
+makeLargest(vector<uint8_t>& data) {
+    uint8_t ch = 0;
+
+    // create 255 sets of character-strings, each of which has the longest
+    // length (255bytes string + 1-byte length field)
+    for (int i = 0; i < 255; ++i, ++ch) {
+        data.push_back(255);
+        data.insert(data.end(), 255, ch);
+    }
+    // the last character-string should be 255 bytes (including the one-byte
+    // length field) in length so that the total length should be in the range
+    // of 16-bit integers.
+    data.push_back(254);
+    data.insert(data.end(), 254, ch);
+
+    assert(data.size() == 65535);
+}
+
+TEST_F(Rdata_TXT_Test, createFromWire) {
     EXPECT_EQ(0, rdata_txt.compare(
                   *rdataFactoryFromFile(RRType("TXT"), RRClass("IN"),
-                                        "testdata/rdata_txt_fromWire")));
+                                        "testdata/rdata_txt_fromWire1")));
+
+    // Empty character string
+    EXPECT_EQ(0, rdata_txt_empty.compare(
+                  *rdataFactoryFromFile(RRType("TXT"), RRClass("IN"),
+                                        "testdata/rdata_txt_fromWire2")));
+
+    // Multiple character strings
+    obuffer.clear();
+    rdataFactoryFromFile(RRType("TXT"), RRClass("IN"),
+                         "testdata/rdata_txt_fromWire3")->toWire(obuffer);
+    // the result should be 'wiredata_txt' repeated twice
+    vector<uint8_t> expected_data(wiredata_txt, wiredata_txt +
+                                  sizeof(wiredata_txt));
+    expected_data.insert(expected_data.end(), wiredata_txt,
+                         wiredata_txt + sizeof(wiredata_txt));
+    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData,
+                        obuffer.getData(), obuffer.getLength(),
+                        &expected_data[0], expected_data.size());
+
+    // Largest length of data.  There's nothing special, but should be
+    // constructed safely, and the content should be identical to the original
+    // data.
+    vector<uint8_t> largest_txt_data;
+    makeLargest(largest_txt_data);
+    InputBuffer ibuffer(&largest_txt_data[0], largest_txt_data.size());
+    generic::TXT largest_txt(ibuffer, largest_txt_data.size());
+    obuffer.clear();
+    largest_txt.toWire(obuffer);
+    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData,
+                        obuffer.getData(), obuffer.getLength(),
+                        &largest_txt_data[0], largest_txt_data.size());
+
+    // rdlen parameter is out of range.  This is a rare event because we'd
+    // normally call the constructor via a polymorphic wrapper, where the
+    // length is validated.  But this should be checked explicitly.
+    InputBuffer ibuffer2(&largest_txt_data[0], largest_txt_data.size());
+    EXPECT_THROW(generic::TXT(ibuffer2, 65536), InvalidRdataLength);
+
+    // RDATA is empty, which is invalid for TXT.
+    EXPECT_THROW(rdataFactoryFromFile(RRType("TXT"), RRClass("IN"),
+                                      "testdata/rdata_txt_fromWire4"),
+                 DNSMessageFORMERR);
+
+    // character-string length is too large, which could cause overrun.
+    EXPECT_THROW(rdataFactoryFromFile(RRType("TXT"), RRClass("IN"),
+                                      "testdata/rdata_txt_fromWire5"),
+                 DNSMessageFORMERR);
 }
 
-TEST_F(Rdata_TXT_Test, toWireBuffer)
-{
+TEST_F(Rdata_TXT_Test, toWireBuffer) {
     rdata_txt.toWire(obuffer);
     EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData,
                         obuffer.getData(), obuffer.getLength(),
                         wiredata_txt, sizeof(wiredata_txt));
 }
 
-TEST_F(Rdata_TXT_Test, toText)
-{
+TEST_F(Rdata_TXT_Test, toText) {
     EXPECT_EQ("\"Test String\"", rdata_txt.toText());
 }
 }