[#539, !330] Addressed review comments

src/lib/util/strutil.*
    seekTrimmed() - new templated function

src/lib/util/tests/strutil_unittest.cc
   TEST(StringUtilTest, seekTrimmed) - new test

src/lib/dhcp/option_data_types.cc
src/lib/dhcp/option_string.c
    updated to use seekTrimmed()

diff --git a/src/lib/dhcp/option_data_types.cc b/src/lib/dhcp/option_data_types.cc
index a64707a9baff4341a016ebd8ad81a9858e063759..5365c3ef51e1ef78ef88c96e66e403a08129eafd 100644 (file)
--- a/src/lib/dhcp/option_data_types.cc
+++ b/src/lib/dhcp/option_data_types.cc
@@ -9,6 +9,7 @@
 #include <dhcp/option_data_types.h>
 #include <dns/labelsequence.h>
 #include <dns/name.h>
+#include <util/strutil.h>
 #include <util/encode/hex.h>
 #include <algorithm>
 #include <limits>
@@ -576,9 +577,7 @@ OptionDataTypeUtil::readString(const std::vector<uint8_t>& buf) {
     if (!buf.empty()) {
         // Per RFC 2132, section 2 we need to drop trailing NULLs
         auto begin = buf.begin();
-        auto end = buf.end();
-        for ( ; end != begin && *(end - 1) == 0x0; --end);
-
+        auto end = util::str::seekTrimmed(begin, buf.end(), 0x0);
         if (std::distance(begin, end) == 0) {
             isc_throw(isc::OutOfRange, "string value carried by the option "
                       << " contained only NULLs");

diff --git a/src/lib/dhcp/option_string.cc b/src/lib/dhcp/option_string.cc
index 2b185520a5af0078f40963bacef2bafb0d697aef..68b80a40f054149fc73c846410299ae676ceeef2 100644 (file)
--- a/src/lib/dhcp/option_string.cc
+++ b/src/lib/dhcp/option_string.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2019 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -7,6 +7,7 @@
 #include <config.h>
 
 #include <dhcp/option_string.h>
+#include <util/strutil.h>
 #include <sstream>
 
 namespace isc {
@@ -52,8 +53,7 @@ OptionString::setValue(const std::string& value) {
 
     // Trim off any trailing NULLs.
     auto begin = value.begin();
-    auto end = value.end();
-    for ( ; end != begin && *(end - 1) == 0x0; --end);
+    auto end = util::str::seekTrimmed(begin, value.end(), 0x0);
 
     if (std::distance(begin, end) == 0) {
         isc_throw(isc::OutOfRange, "string value carried by the option '"
@@ -86,8 +86,7 @@ void
 OptionString::unpack(OptionBufferConstIter begin,
                      OptionBufferConstIter end) {
     // Trim off trailing null(s)
-    for ( ; end != begin && *(end - 1) == 0x0; --end);
-
+    end = util::str::seekTrimmed(begin, end, 0x0);
     if (std::distance(begin, end) == 0) {
         isc_throw(isc::OutOfRange, "failed to parse an option '"
                   << getType() << "' holding string value"

diff --git a/src/lib/dhcp/option_string.h b/src/lib/dhcp/option_string.h
index c590fe1fe2b30fe5c81669d675ca66eac30b547e..784257d945d60cac6238ac2a760dae8b7e641518 100644 (file)
--- a/src/lib/dhcp/option_string.h
+++ b/src/lib/dhcp/option_string.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2019 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this

diff --git a/src/lib/util/strutil.h b/src/lib/util/strutil.h
index 8160bee8f61a14a99cdfa1604874740e1a5c501c..2ce16960b9fa3f99ac6177a4fa09e4e1b8289791 100644 (file)
--- a/src/lib/util/strutil.h
+++ b/src/lib/util/strutil.h
@@ -54,6 +54,26 @@ void normalizeSlash(std::string& name);
 /// \return String with leading and trailing spaces removed
 std::string trim(const std::string& instring);
 
+/// \brief Finds the "trimmed" end of a buffer
+///
+/// Works backward from the the end of the buffer, looking for the first
+/// character not equal to the trim value, and returns an iterator 
+/// pointing that that position.
+///
+/// \param begin - Forward iterator pointing to the beginning of the
+/// buffer to trim
+/// \param end - Forward iterator pointing to the untrimmed end of
+/// the buffer to trim
+/// \param val - byte value to trim off
+///
+/// \retrun Iterator pointing the first character from the end of the 
+/// buffer not equal to the  trim value
+template<typename Iterator>
+Iterator
+seekTrimmed(Iterator begin, Iterator end, uint8_t trim_val) {
+    for ( ; end != begin && *(end - 1) == trim_val; --end);
+    return(end);
+}
 
 /// \brief Split String into Tokens
 ///

diff --git a/src/lib/util/tests/strutil_unittest.cc b/src/lib/util/tests/strutil_unittest.cc
index 07f3d841214e3a1b1dc345f641ba0e1f29c08a53..125bf881d9f9c3750c7b4f5157cea69f34b52a73 100644 (file)
--- a/src/lib/util/tests/strutil_unittest.cc
+++ b/src/lib/util/tests/strutil_unittest.cc
@@ -551,4 +551,50 @@ TEST(StringUtilTest, stringSanitizer) {
                        "abc.123");
 }
 
+// Verifies templated buffer iterator trim() function
+TEST(StringUtilTest, seekTrimmed) {
+
+    // Empty buffer should be fine.
+    std::vector<uint8_t> buffer;
+    auto begin = buffer.end();
+    auto end = buffer.end();
+    ASSERT_NO_THROW(end = seekTrimmed(begin, end, 0));
+    EXPECT_EQ(0, std::distance(begin, end));
+
+    // Buffer of only trim values, should be fine.
+    buffer = { 1, 1 };
+    begin = buffer.begin();
+    end = buffer.end();
+    ASSERT_NO_THROW(end = seekTrimmed(begin, end, 1));
+    EXPECT_EQ(0, std::distance(begin, end));
+
+    // One trailing null should trim off.
+    buffer = {'o', 'n', 'e', 0 };
+    begin = buffer.begin();
+    end = buffer.end();
+    ASSERT_NO_THROW(end = seekTrimmed(begin, end, 0));
+    EXPECT_EQ(3, std::distance(begin, end));
+
+    // More than one trailing null should trim off.
+    buffer = { 't', 'h', 'r', 'e', 'e', 0, 0, 0 };
+    begin = buffer.begin();
+    end = buffer.end();
+    ASSERT_NO_THROW(end = seekTrimmed(begin, end, 0));
+    EXPECT_EQ(5, std::distance(begin, end));
+
+    // Embedded null should be left in place.
+    buffer = { 'e', 'm', 0, 'b', 'e', 'd' };
+    begin = buffer.begin();
+    end = buffer.end();
+    ASSERT_NO_THROW(end = seekTrimmed(begin, end, 0));
+    EXPECT_EQ(6, std::distance(begin, end));
+
+    // Leading null should be left in place.
+    buffer = { 0, 'l', 'e', 'a', 'd', 'i', 'n', 'g' };
+    begin = buffer.begin();
+    end = buffer.end();
+    ASSERT_NO_THROW(end = seekTrimmed(begin, end, 0));
+    EXPECT_EQ(8, std::distance(begin, end));
+}
+
 } // end of anonymous namespace