[#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 1e923da3cd1a3d54bd844f7640d63918c6a00093..b50df37601cc77a9c763b4730182cbfa820e0877 100644 (file)
--- 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 cd67ec12fe537f06d1e71e9f4c74b29a068789e4..fb9707440f11b1900b88ad1b0bcd9af659fe3e6f 100644 (file)
--- a/src/lib/dhcp/tests/option_unittest.cc
+++ b/src/lib/dhcp/tests/option_unittest.cc
@@ -13,6 +13,7 @@
 #include <dhcp/option_space.h>
 #include <exceptions/exceptions.h>
 #include <util/buffer.h>
+#include <testutils/gtest_utils.h>
 
 #include <boost/shared_ptr.hpp>
 #include <boost/scoped_ptr.hpp>
@@ -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 ba88b0081aef050c8d92a9142d589e97dcc85094..de1378586e0e412cabd574887de4879b1ad69124 100644 (file)
--- 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 7fca6b799c382950b5cc9ce9e6f59325650440c1..ab377da597673d1e1f1c0edcba9aba717abafd07 100644 (file)
--- 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<OptionCustom>
-                          (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 637665aa1c0ec20bf0cc630560e42440bbca1a9b..b79a032bccc582c6ba24c2f1aa775807cc15f810 100644 (file)
--- 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<HostReservationIdsParser6>(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