From: Thomas Markwalder Date: Sun, 5 Jul 2026 04:47:15 +0000 (-0400) Subject: [#4626] Addressed review comments X-Git-Tag: Kea-3.3.0~89 X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=e7227024ceeb09fcbc9910a3d48862eb047d792f;p=thirdparty%2Fkea.git [#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 0d17053ec0..969f331005 100644 --- 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 3168b113b5..a819707e29 100644 --- 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 >opt(new OptionInt(Option::V4, 224, 123)); + OptionPtr opt(new OptionInt(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 5a39f0edbe..94a3eda28f 100644 --- 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(exp_len) << " for data type " << OptionDataTypeUtil::getDataTypeName(def->getType())); diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h index 8703540223..8d3ecfd40b 100644 --- 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 e2ee5fd8b0..c719cd6a2e 100644 --- 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' };