[#608] Addressed review comments

src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
    TEST_F(CSVLeaseFile4Test, embeddedEscapes)  - new test

src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
    TEST_F(CSVLeaseFile6Test, embeddedEscapes) - new test

src/lib/util/csv_file.cc
    CSVRow::escapeCharacters() - now automatically also escapes
    first char of escape_tag to preserve input that happens to match
    escape tag

src/lib/util/tests/csv_file_unittest.cc
    TEST(CSVRowTest, escapeUnescape) - added embedded escape tests

plus misc clean up

diff --git a/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
index 57fef44090f0fd3b63624c97804c2a556c6ac3dc..cbb423da9c2b9df072babf66d7ec9c3fcebae48a 100644 (file)
--- a/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
+++ b/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
@@ -599,6 +599,48 @@ TEST_F(CSVLeaseFile4Test, embeddedCommas) {
     EXPECT_EQ(context_str, lease->getContext()->str());
 }
 
+// Verifies that it is possible to write and read a lease with
+// escape tags and sequences in hostname and user context.
+TEST_F(CSVLeaseFile4Test, embeddedEscapes) {
+    CSVLeaseFile4 lf(filename_);
+    ASSERT_NO_THROW(lf.recreate());
+    ASSERT_TRUE(io_.exists());
+
+    std::string hostname("host&#xexample&#x2ccom");
+    std::string context_str("{ \"&#xbar\": true, \"foo\": false, \"x\": \"fac&#x2ctor\" }");
+
+    // Create a lease with commas in the hostname.
+    Lease4Ptr lease(new Lease4(IOAddress("192.0.3.2"),
+                               hwaddr0_,
+                               NULL, 0,
+                               0xFFFFFFFF, time(0),
+                               8, true, true,
+                               hostname));
+
+    // Add the user context with commas.
+    lease->setContext(Element::fromJSON(context_str));
+
+    // Write this lease out to the lease file.
+    ASSERT_NO_THROW(lf.append(*lease));
+
+    // Close the lease file.
+    lf.close();
+
+    Lease4Ptr lease_read;
+
+    // Re-open the file for reading.
+    ASSERT_NO_THROW(lf.open());
+
+    // Read the lease and make sure it is successful.
+    EXPECT_TRUE(lf.next(lease_read));
+    ASSERT_TRUE(lease_read);
+
+    // Expect the hostname and user context to retain the commas
+    // they started with.
+    EXPECT_EQ(hostname, lease->hostname_);
+    EXPECT_EQ(context_str, lease->getContext()->str());
+}
+
 /// @todo Currently we don't check invalid lease attributes, such as invalid
 /// lease type, invalid preferred lifetime vs valid lifetime etc. The Lease6
 /// should be extended with the function that validates lease attributes. Once

diff --git a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
index 66220ba9b9ee827b7bac4a1955a632018e666d25..5eb5be7de276b29f95c910965e635caf1c078715 100644 (file)
--- a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
+++ b/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
@@ -658,6 +658,47 @@ TEST_F(CSVLeaseFile6Test, embeddedCommas) {
     EXPECT_EQ(context_str, lease->getContext()->str());
 }
 
+// Verifies that it is possible to write and read a lease with
+// escape tags and sequences in hostname and user context.
+TEST_F(CSVLeaseFile6Test, embeddedEscapes) {
+    CSVLeaseFile6 lf(filename_);
+    ASSERT_NO_THROW(lf.recreate());
+    ASSERT_TRUE(io_.exists());
+
+    std::string hostname("host&#xexample&#x2ccom");
+    std::string context_str("{ \"&#xbar\": true, \"foo\": false, \"x\": \"fac&#x2ctor\" }");
+
+    // Create a lease with commas in the hostname.
+    Lease6Ptr lease(new Lease6(Lease::TYPE_NA, IOAddress("2001:db8:1::1"),
+                               makeDUID(DUID0, sizeof(DUID0)),
+                               7, 100, 0xFFFFFFFF, 8, true, true,
+                               hostname));
+
+    // Add the user context with commas.
+    lease->setContext(Element::fromJSON(context_str));
+
+    // Write this lease out to the lease file.
+    ASSERT_NO_THROW(lf.append(*lease));
+
+    // Close the lease file.
+    lf.close();
+
+    Lease6Ptr lease_read;
+
+    // Re-open the file for reading.
+    ASSERT_NO_THROW(lf.open());
+
+    // Read the lease and make sure it is successful.
+    EXPECT_TRUE(lf.next(lease_read));
+    ASSERT_TRUE(lease_read);
+
+    // Expect the hostname and user context to retain the commas
+    // they started with.
+    EXPECT_EQ(hostname, lease->hostname_);
+    EXPECT_EQ(context_str, lease->getContext()->str());
+}
+
+
 
 
 /// @todo Currently we don't check invalid lease attributes, such as invalid

diff --git a/src/lib/util/csv_file.cc b/src/lib/util/csv_file.cc
index b6fd59131668b6491d167ebaf3e398ae761e8146..8f939537e057071cafd907c8980463af4e11a623 100644 (file)
--- a/src/lib/util/csv_file.cc
+++ b/src/lib/util/csv_file.cc
@@ -430,27 +430,32 @@ CSVRow::escapeCharacters(const std::string& orig_str, const std::string& charact
     size_t char_pos = 0;
     size_t prev_pos = 0;
 
-    // Check for a first occurance. If none, just return a
+    // We add the first character of the escape tag to the list of
+    // characters to escape.  This ensures input which happens to
+    // be valid esacpe sequences will be escaped.
+    std::string escape_chars(characters + escape_tag[0]);
+
+    // Check for a first occurence. If none, just return a
     // copy of the original.
-    char_pos = orig_str.find_first_of(characters, prev_pos);
+    char_pos = orig_str.find_first_of(escape_chars, prev_pos);
     if (char_pos == std::string::npos) {
         return(orig_str);
     }
 
     std::stringstream ss;
     while (char_pos < orig_str.size()) {
-        // Copy everything upto the charcater to escape.
+        // Copy everything upto the character to escape.
         ss << orig_str.substr(prev_pos, char_pos - prev_pos);
 
         // Copy the escape tag followed by the hex digits of the character.
         ss << escape_tag << std::hex << std::setw(2)
-           << (uint16_t)(orig_str[char_pos]);
+           << static_cast<uint16_t>(orig_str[char_pos]);
 
         ++char_pos;
         prev_pos = char_pos;
 
         // Find the next character to escape.
-        char_pos = orig_str.find_first_of(characters, prev_pos);
+        char_pos = orig_str.find_first_of(escape_chars, prev_pos);
 
         // If no more, copy the remainder of the string.
         if (char_pos == std::string::npos) {
@@ -492,9 +497,9 @@ CSVRow::unescapeCharacters(const std::string& escaped_str) {
                 uint8_t digit = escaped_str[dig_pos];
 
                 if (digit >= 'a' && digit <= 'f') {
-                    digit = (digit - 'a' + 10);
+                    digit = digit - 'a' + 10;
                 } else if (digit >= 'A' && digit <= 'F') {
-                    digit = (digit - 'A' + 10);
+                    digit = digit - 'A' + 10;
                 } else if (digit >= '0' && digit <= '9') {
                     digit -= '0';
                 } else {
@@ -503,7 +508,7 @@ CSVRow::unescapeCharacters(const std::string& escaped_str) {
                 }
 
                 if (i == 0) {
-                    escaped_char = (digit << 4);
+                    escaped_char = digit << 4;
                 } else {
                     escaped_char |= digit;
                 }

diff --git a/src/lib/util/csv_file.h b/src/lib/util/csv_file.h
index 1ffdc369f9a1de41e889a3072c8b21283f4d3dd0..9bf64f13d081c37adae203f4eb72de0131b3b6b0 100644 (file)
--- a/src/lib/util/csv_file.h
+++ b/src/lib/util/csv_file.h
@@ -113,7 +113,7 @@ public:
     /// characters.
     ///
     /// Returns a copy of the internal container value at the given index
-    /// which has had all escaped characters replaced with their unesacped
+    /// which has had all escaped characters replaced with their unescaped
     /// values. Escaped characters embedded using the following format:
     ///
     /// This function fetches the value at the given index and passes it
@@ -199,7 +199,7 @@ public:
         writeAt(at, value.c_str());
     }
 
-    /// @brief Replaces the value at specified index with a value that has
+    /// @brief Replaces the value at the specified index with a value that has
     /// had special characters escaped
     ///
     /// This function first calls @c CSVRow::esacpeCharacters to replace
@@ -274,7 +274,7 @@ public:
     /// escaping.
     /// @param characters list of characters which require escaping.
     ///
-    /// The escaped characters will use the followin format:
+    /// The escaped characters will use the following format:
     ///
     /// &#x{xx}
     ///

diff --git a/src/lib/util/tests/csv_file_unittest.cc b/src/lib/util/tests/csv_file_unittest.cc
index e5450a1230976fb4290539683eb8e66d9dd4a326..957bf681e91814fc82520c1ace8d51c4c689ac05 100644 (file)
--- a/src/lib/util/tests/csv_file_unittest.cc
+++ b/src/lib/util/tests/csv_file_unittest.cc
@@ -28,11 +28,19 @@ TEST(CSVRowTest, escapeUnescape) {
     std::string unescaped = CSVRow::unescapeCharacters(escaped);
     EXPECT_EQ (orig, unescaped);
 
-    // Make sure that an incident occurance of the escape tag
-    // characters is left intact.
+    // Make sure that an incident occurrence of just the escape tag
+    // is left intact.
     orig = ("no&#xescape");
+    escaped = CSVRow::escapeCharacters(orig, ",");
     unescaped = CSVRow::unescapeCharacters(orig);
     EXPECT_EQ (orig, unescaped);
+
+    // Make sure that an incidental occurrence of a valid
+    // escape tag sequence left intact.
+    orig = ("no&#x2cescape");
+    escaped = CSVRow::escapeCharacters(orig, ",");
+    unescaped = CSVRow::unescapeCharacters(escaped);
+    EXPECT_EQ (orig, unescaped);
 }
 
 // This test checks that the single data row is parsed.