[#4626] Addressed review comments

Added UT scenarios, minor clean up

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 0d17053ec02346e96d77176d170f5e5476766758..969f3310058d6808073fcf05a0aeb0e26865bfbc 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -5238,8 +5238,10 @@ Dhcpv4Srv::deferredUnpack(Pkt4Ptr& query) {
             continue;
         }
 
-        // Remove the packed option.
-        while (query->delOption(code));
+        // Remove the packed options.
+        while (query->delOption(code)) {
+            // continue;
+        };
 
         // Add the unpacked option.
         query->addOption(opt);

diff --git a/src/bin/dhcp4/tests/vendor_opts_unittest.cc b/src/bin/dhcp4/tests/vendor_opts_unittest.cc
index 3168b113b5c18fd1632be5acdf47501b7f64886f..a819707e29528210ca43017101095bd4222f06df 100644 (file)
--- a/src/bin/dhcp4/tests/vendor_opts_unittest.cc
+++ b/src/bin/dhcp4/tests/vendor_opts_unittest.cc
@@ -3003,7 +3003,7 @@ TEST_F(VendorOptsTest, sanityCheckDeferredOptions) {
     query->setIndex(ETH1_INDEX);
 
     // Add an option that is too long.
-    boost::shared_ptr<OptionInt<int16_t> >opt(new OptionInt<int16_t>(Option::V4, 224, 123));
+    OptionPtr opt(new OptionInt<int16_t>(Option::V4, 224, 123));
     ASSERT_TRUE(opt);
     query->addOption(opt);
 

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index 5a39f0edbe1de162a32df711de08f5a994a24e4c..94a3eda28fed13e8c8957b86817b4398fa608573 100644 (file)
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -726,8 +726,8 @@ LibDHCP::sanityCheckScalarLength(const OptionDefinitionPtr& def, uint16_t opt_le
     // lengths as this is done in option factories and would break v4 option
     // fusing.
     size_t exp_len = OptionDataTypeUtil::getDataTypeLen(def->getType());
-    if ((exp_len > 0 && opt_len > exp_len) && (!def->getArrayType()) &&
-        (def->getEncapsulatedSpace().empty())) {
+    if ((exp_len > 0) && (opt_len > exp_len) && !def->getArrayType() &&
+        def->getEncapsulatedSpace().empty()) {
         isc_throw(BadValue, "opt_len does not match defined option length "
                              << static_cast<uint16_t>(exp_len) << " for data type "
                              << OptionDataTypeUtil::getDataTypeName(def->getType()));

diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h
index 870354022349cdf65aaf6a3a4ab8f516b6f75773..8d3ecfd40b7706d016f73b0bbb9bdabd1955722a 100644 (file)
--- a/src/lib/dhcp/libdhcp++.h
+++ b/src/lib/dhcp/libdhcp++.h
@@ -337,12 +337,12 @@ public:
 
     /// @brief Validates a scalar option's length against it's defined length.
     ///
-    /// If the given definition is a scalar option that is not part of an array,
-    /// or record, the given length (length value from the option data) must
-    /// not exceed the defined length.  If it exceeds the defined length then
-    /// throw the appropriate exception based on Option::lenient_parsing_.
-    /// It does not check for truncated lengths as this is done in option
-    /// factories and would break option fusing of split scalar options.
+    /// If the given definition is a scalar option that is not part of an
+    /// array, the given length (length value from the option data) must not
+    /// exceed the defined length. If it exceeds the defined length then throw
+    /// the appropriate exception based on Option::lenient_parsing_. It does
+    /// not check for truncated lengths as this is done in option factories and
+    /// would break option fusing of split scalar options.
     ///
     /// @param def option's definition
     /// @param opt_len length of the option stated in the option data.

diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index e2ee5fd8b04ab1ec341efbf82b3b3466f1401f0c..c719cd6a2ede27dda36ee68507ee14ddcfb867f8 100644 (file)
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc
@@ -4267,6 +4267,15 @@ TEST_F(LibDhcpTest, invalidScalarLengthLenientDisabled4) {
             { DHO_SUBNET_MASK, 3, 1, 2, 3 },
             "opt_type: 1, opt_len: 3, error: option buffer truncated"
         },
+        {
+            // OPT_INT16_TYPE - Integer followed by a sub-option.
+            // This amounts to an undeclared record. With lenient
+            // parsing disabled it should not parse.
+            __LINE__,
+            { DHO_BOOT_SIZE, 6, 1, 2, DHO_BOOT_SIZE, 2, 3, 4 },
+            "opt_type: 13, opt_len: 6, error: opt_len does not"
+            " match defined option length 2 for data type uint16"
+        }
     };
 
     for (auto const& scenario : scenarios) {
@@ -4391,6 +4400,15 @@ TEST_F(LibDhcpTest, invalidScalarLengthLenientEnabled4) {
             false,
             "opt_type: 1, opt_len: 3, error: option buffer truncated"
         },
+        {
+            // OPT_INT16_TYPE - Integer followed by a sub-option.
+            // This amounts to an undeclared record. With lenient
+            // parsing enabled it should parse.
+            __LINE__,
+            { DHO_BOOT_SIZE, 6, 1, 2, DHO_BOOT_SIZE, 2,  3, 4 },
+            true,
+            "type=013, len=006: 258 (uint16),\noptions:\n  type=013, len=002: 03:04"
+        }
     };
 
     for (auto const& scenario : scenarios) {
@@ -4403,8 +4421,7 @@ TEST_F(LibDhcpTest, invalidScalarLengthLenientEnabled4) {
         if (scenario.should_parse_) {
             // Should not throw.
             ASSERT_NO_THROW_LOG(
-                LibDHCP::unpackOptions4(scenario.buf_, DHCP4_OPTION_SPACE, options,
-                                        deferred_options));
+                LibDHCP::unpackOptions4(scenario.buf_, DHCP4_OPTION_SPACE, options, deferred_options));
             ASSERT_EQ(1U, options.size());
             auto it = options.begin();
             ASSERT_TRUE(it != options.end());
@@ -4559,6 +4576,17 @@ TEST_F(LibDhcpTest, invalidScalarLengthLenientDisabled6) {
             { 0x00, D6O_LINK_ADDRESS, 0x00, 0x03, 1, 2, 3 },
             "opt_type: 80, opt_len: 3, error: option buffer truncated"
         },
+        {
+            // OPT_INT16_TYPE - Integer followed by a sub-option.
+            // This amounts to an undeclared record. With lenient
+            // parsing disabled it should not parse.
+            __LINE__,
+            { 0x00, D6O_ELAPSED_TIME, 0x00, 0x08, 0x01, 0x02,
+              0x00, D6O_ELAPSED_TIME, 0x00, 0x02, 0x03, 0x04,
+            },
+            "opt_type: 8, opt_len: 8, error: opt_len does not"
+            " match defined option length 2 for data type uint16"
+        },
     };
 
     for (auto const& scenario : scenarios) {
@@ -4716,6 +4744,17 @@ TEST_F(LibDhcpTest, invalidScalarLengthLenientEnabled6) {
             false,
             "opt_type: 80, opt_len: 3, error: option buffer truncated"
         },
+        {
+            // OPT_INT16_TYPE - Integer followed by a sub-option.
+            // This amounts to an undeclared record. With lenient
+            // parsing enabled it should parse.
+            __LINE__,
+            { 0x00, D6O_ELAPSED_TIME, 0x00, 0x08, 0x01, 0x02,
+              0x00, D6O_ELAPSED_TIME, 0x00, 0x02, 0x03, 0x04,
+            },
+            true,
+            "type=00008, len=00008: 258 (uint16),\noptions:\n  type=00008, len=00002: 03:04"
+        },
     };
 
     OptionBuffer good_option = { 0x00, D6O_BOOTFILE_URL, 0x00, 0x03, 'f', 'o', 'o' };