From: Thomas Markwalder Date: Thu, 9 Feb 2023 17:15:51 +0000 (-0500) Subject: [#2538] Addressed review comments X-Git-Tag: Kea-2.3.5~47 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=2c1d2ac45e7d96fa1b58b64d77c8fc3ae8517d0a;p=thirdparty%2Fkea.git [#2538] Addressed review comments src/lib/dhcp/option.cc Option::addOption(OptionPtr opt) - now throws rather than silently ignoring attempted self-adds src/lib/dhcp/tests/option_unittest.cc TEST_F(OptionTest, optionsCannotContainThemselves) - new test src/lib/dhcpsrv/cfg_option.cc CfgOption::encapsulateInternal(const OptionPtr& option) - removed self-add check, relies on Option::addOption() to detect src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc TEST_F(ParseConfigTest, selfEncapsulationTest) - altered to expect throw src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc TEST_F(HostReservationParserTest, selfEncapsulation) - deleted test --- diff --git a/src/lib/dhcp/option.cc b/src/lib/dhcp/option.cc index 1e923da3cd..b50df37601 100644 --- a/src/lib/dhcp/option.cc +++ b/src/lib/dhcp/option.cc @@ -329,10 +329,11 @@ Option::getHeaderLen() const { } void Option::addOption(OptionPtr opt) { - // Do not allow options to be added to themselves as this - // can lead to infinite recursion. if (this == opt.get()) { - return; + // Do not allow options to be added to themselves as this + // can lead to infinite recursion. + isc_throw(InvalidOperation, "option cannot be added to itself: " << toText()); + // return; } options_.insert(make_pair(opt->getType(), opt)); diff --git a/src/lib/dhcp/tests/option_unittest.cc b/src/lib/dhcp/tests/option_unittest.cc index cd67ec12fe..fb9707440f 100644 --- a/src/lib/dhcp/tests/option_unittest.cc +++ b/src/lib/dhcp/tests/option_unittest.cc @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -634,4 +635,17 @@ TEST_F(OptionTest, createPayload) { EXPECT_EQ(buf_, option->getData()); } +// Verify that options cannot be added to themselves as suboptions. +TEST_F(OptionTest, optionsCannotContainThemselves) { + OptionBuffer buf1 {0xaa, 0xbb}; + OptionBuffer buf2 {0xcc, 0xdd}; + OptionPtr option = Option::create(Option::V4, 123, buf1); + OptionPtr option2 = Option::create(Option::V4, 124, buf2); + ASSERT_TRUE(option); + ASSERT_NO_THROW(option->addOption(option2)); + EXPECT_THROW_MSG(option->addOption(option), InvalidOperation, + "option cannot be added to itself: type=123, len=006: aa:bb,\noptions:\n" + " type=124, len=002: cc:dd"); +} + } diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index ba88b0081a..de1378586e 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/src/lib/dhcpsrv/cfg_option.cc @@ -270,11 +270,6 @@ CfgOption::encapsulateInternal(const OptionPtr& option) { // Retrieve all options from the encapsulated option space. OptionContainerPtr encap_options = getAll(encap_space); for (auto const& encap_opt : *encap_options) { - if (option.get() == encap_opt.option_.get()) { - // Avoid recursion by not adding options to themselves. - continue; - } - // Add sub-option if there isn't one added already. if (!option->getOption(encap_opt.option_->getType())) { option->addOption(encap_opt.option_); diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 7fca6b799c..ab377da597 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -3378,8 +3378,8 @@ TEST_F(ParseConfigTest, invalidSubnetPdAllocator6) { // src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc). -// Verifies that an option which encapsulates its own option space -// doesn't cause infinite recursion. +// Verifies that parsing an option which encapsulates its own option space +// is detected. TEST_F(ParseConfigTest, selfEncapsulationTest) { // Verify that the option definition can be retrieved. OptionDefinitionPtr def = LibDHCP::getOptionDef(DHCP6_OPTION_SPACE, 45); @@ -3400,20 +3400,20 @@ TEST_F(ParseConfigTest, selfEncapsulationTest) { // Verify that the configuration string parses. family_ = AF_INET6; - int rcode = parseConfiguration(config, true, true); - ASSERT_EQ(0, rcode); - - // Verify that the option can be retrieved. - OptionCustomPtr opt = boost::dynamic_pointer_cast - (getOptionPtr(DHCP6_OPTION_SPACE, D6O_CLIENT_DATA)); - ASSERT_TRUE(opt); - - // Verify length is correct and doesn't infinitely recurse. - EXPECT_EQ(19, opt->len()); - // Check if it can be unparsed. - CfgOptionsTest cfg(CfgMgr::instance().getStagingCfg()); - cfg.runCfgOptionsTest(family_, config); + ElementPtr json = Element::fromJSON(config); + EXPECT_TRUE(json); + ConstElementPtr status = parseElementSet(json, false); + int rcode = 0; + ConstElementPtr comment = parseAnswer(rcode, status); + ASSERT_TRUE(comment); + ASSERT_EQ(comment->getType(), Element::string); + EXPECT_EQ(1, rcode); + std::string expected = + "Configuration parsing failed: " + "option cannot be added to itself: type=00045, len=00015:" + ",\noptions:\n type=00001, len=00011: 01:02:02:02:02:03:03:03:03:03:03"; + EXPECT_EQ(expected, comment->stringValue()); } } // Anonymous namespace diff --git a/src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc b/src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc index 637665aa1c..b79a032bcc 100644 --- a/src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc @@ -1483,50 +1483,4 @@ TEST_F(HostReservationIdsParserTest, dhcp6EmptyList) { testInvalidConfig(config); } -// Verify that options which encapsulate their own standard -// namespace don't cause recursion. -TEST_F(HostReservationParserTest, selfEncapsulation) { - // Create configuration with a client-data option for a host. - // This option encapsulates dhcp6 option space, and while it - // is not an option users would ever configure, let's make - // sure it won't do bad things if they do. - std::string config = "{ \"duid\": \"01:02:03:04:05:06:07:08:09:0A\"," - "\"option-data\": [" - "{" - " \"name\": \"client-data\"," - " \"csv-format\": false," - " \"data\": \"0001000B0102020202030303030303\"" - "}" - ",{ \"code\": 2, \"data\": \"778899\" }" - "]" - "}"; - - ElementPtr config_element = Element::fromJSON(config); - - HostPtr host; - HostReservationParser6 parser; - ASSERT_NO_THROW(host = parser.parse(SubnetID(10), config_element)); - ASSERT_TRUE(host); - - // One host should have been added to the configuration. - CfgHostsPtr cfg_hosts = CfgMgr::instance().getStagingCfg()->getCfgHosts(); - ASSERT_NO_THROW(cfg_hosts->add(host)); - - HostCollection hosts; - ASSERT_NO_THROW(hosts = cfg_hosts->getAll(Host::IDENT_DUID, - &duid_->getDuid()[0], - duid_->getDuid().size())); - ASSERT_EQ(1, hosts.size()); - - // Retrieve and sanity the client data option. - OptionCustomPtr opt = boost::dynamic_pointer_cast< - OptionCustom>(retrieveOption(*hosts[0], DHCP6_OPTION_SPACE, D6O_CLIENT_DATA)); - ASSERT_TRUE(opt); - - // The length should include the explicit data declared plus the length - // and data for option 2 - EXPECT_EQ(26, opt->len()); -} - - } // end of anonymous namespace