[#2942] clone options only once

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index d11d680cc73ab09b200b89a5439bb2fc7fc11586..565e24a31f210eedba5fdaf6c880fe88859d6471 100644 (file)
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -936,14 +936,21 @@ LibDHCP::splitOptions4(OptionCollection& options,
                        uint32_t used) {
     bool result = false;
     // We need to loop until all options have been split.
-    for (;;) {
+    uint32_t tries = 0;
+    for (;; tries++) {
+        // Let's not do this forever if there is a bug hiding here somewhere...
+        // 65535 times should be enough for any packet load...
+        if (tries == std::numeric_limits<uint16_t>::max()) {
+            isc_throw(BadValue, "packet split failed after trying "
+                     << tries << " times.");
+        }
         bool found = false;
         // Make a copy of the options so we can safely iterate over the
         // old container.
         OptionCollection copy = options;
         // Iterate over all options in the container.
         for (auto const& option : options) {
-            OptionPtr candidate = option.second->clone();
+            OptionPtr candidate = option.second;
             OptionCollection& sub_options = candidate->getMutableOptions();
             // Split suboptions recursively, if any.
             OptionCollection distinct_options;

diff --git a/src/lib/dhcp/pkt.cc b/src/lib/dhcp/pkt.cc
index 4d214c7c257b43faeb2bd8d48004a20cf97b42cf..f2f9a49b3944dfa2d4f0cc595ced88669e0848cc 100644 (file)
--- a/src/lib/dhcp/pkt.cc
+++ b/src/lib/dhcp/pkt.cc
@@ -37,6 +37,15 @@ Pkt::Pkt(const uint8_t* buf, uint32_t len, const isc::asiolink::IOAddress& local
     }
 }
 
+void
+Pkt::cloneOptions() {
+    OptionCollection options;
+    for (auto const& option : options_) {
+        options.emplace(std::make_pair(option.second->getType(), option.second->clone()));
+    }
+    options_ = options;
+}
+
 void
 Pkt::addOption(const OptionPtr& opt) {
     options_.insert(std::pair<int, OptionPtr>(opt->getType(), opt));

diff --git a/src/lib/dhcp/pkt.h b/src/lib/dhcp/pkt.h
index 3436d2695b647f4e9466cdcd7de8991c4d967bcb..d17a210cb5b830570f1086d1fa5060497e107d97 100644 (file)
--- a/src/lib/dhcp/pkt.h
+++ b/src/lib/dhcp/pkt.h
@@ -328,6 +328,10 @@ protected:
 
 public:
 
+    /// @brief Clones all options so that they can be safely modified - some
+    /// options reference objects directly in the server running configuration.
+    void cloneOptions();
+
     /// @brief Returns the first option of specified type.
     ///
     /// Returns the first option of specified type. Note that in DHCPv6 several
@@ -862,6 +866,7 @@ public:
     ///
     /// @param pkt Pointer to the packet.
     ScopedPktOptionsCopy(PktType& pkt) : pkt_(pkt), options_(pkt.options_) {
+        pkt.cloneOptions();
     }
 
     /// @brief Destructor.

diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc
index 216e04c6d31bfeeca574a1b3b794cc1779a2d0da..1111e72dbb979913c883432eb4e363ab02986ef8 100644 (file)
--- a/src/lib/dhcp/pkt4.cc
+++ b/src/lib/dhcp/pkt4.cc
@@ -69,11 +69,12 @@ Pkt4::len() {
 
 void
 Pkt4::pack() {
-    ScopedPkt4OptionsCopy scoped_options(*this);
     if (!hwaddr_) {
         isc_throw(InvalidOperation, "Can't build Pkt4 packet. HWAddr not set.");
     }
 
+    ScopedPkt4OptionsCopy scoped_options(*this);
+
     // Clear the output buffer to make sure that consecutive calls to pack()
     // will not result in concatenation of multiple packet copies.
     buffer_out_.clear();

diff --git a/src/lib/dhcp/tests/protocol_util_unittest.cc b/src/lib/dhcp/tests/protocol_util_unittest.cc
index 669fefcc2f15873c492f93a373635f347110a194..f50639adc50762c200a37f0537a8066f7d2840a5 100644 (file)
--- a/src/lib/dhcp/tests/protocol_util_unittest.cc
+++ b/src/lib/dhcp/tests/protocol_util_unittest.cc
@@ -426,9 +426,19 @@ TEST(ScopedOptionsCopy, pkt4OptionsCopy) {
     size_t count = options.size();
     ASSERT_NE(0, count);
     ASSERT_EQ(option, pkt->getOption(DHO_BOOT_FILE_NAME));
+    std::string expected = pkt->toText();
+    pkt->pack();
+    auto buf = pkt->getBuffer();
     {
         ScopedPkt4OptionsCopy oc(*pkt);
-        ASSERT_EQ(pkt->options_, options);
+        ASSERT_NE(pkt->options_, options);
+        ASSERT_NE(option, pkt->getOption(DHO_BOOT_FILE_NAME));
+        pkt->pack();
+        ASSERT_EQ(buf.getLength(), pkt->getBuffer().getLength());
+        for (size_t index = 0; index < buf.getLength(); ++index) {
+            ASSERT_EQ(buf[index], pkt->getBuffer()[index]);
+        }
+        ASSERT_EQ(expected, pkt->toText());
         pkt->delOption(DHO_BOOT_FILE_NAME);
         ASSERT_EQ(pkt->options_.size(), count - 1);
         ASSERT_FALSE(pkt->getOption(DHO_BOOT_FILE_NAME));
@@ -438,7 +448,14 @@ TEST(ScopedOptionsCopy, pkt4OptionsCopy) {
     {
         try {
             ScopedPkt4OptionsCopy oc(*pkt);
-            ASSERT_EQ(pkt->options_, options);
+            ASSERT_NE(pkt->options_, options);
+            ASSERT_NE(option, pkt->getOption(DHO_BOOT_FILE_NAME));
+            pkt->pack();
+            ASSERT_EQ(buf.getLength(), pkt->getBuffer().getLength());
+            for (size_t index = 0; index < buf.getLength(); ++index) {
+                ASSERT_EQ(buf[index], pkt->getBuffer()[index]);
+            }
+            ASSERT_EQ(expected, pkt->toText());
             pkt->delOption(DHO_BOOT_FILE_NAME);
             ASSERT_EQ(pkt->options_.size(), count - 1);
             ASSERT_FALSE(pkt->getOption(DHO_BOOT_FILE_NAME));
@@ -462,9 +479,19 @@ TEST(ScopedOptionsCopy, pkt6OptionsCopy) {
     size_t count = options.size();
     ASSERT_NE(0, count);
     ASSERT_EQ(option, pkt->getOption(D6O_BOOTFILE_URL));
+    std::string expected = pkt->toText();
+    pkt->pack();
+    auto buf = pkt->getBuffer();
     {
         ScopedPkt6OptionsCopy oc(*pkt);
-        ASSERT_EQ(pkt->options_, options);
+        ASSERT_NE(pkt->options_, options);
+        ASSERT_NE(option, pkt->getOption(D6O_BOOTFILE_URL));
+        pkt->pack();
+        ASSERT_EQ(buf.getLength(), pkt->getBuffer().getLength());
+        for (size_t index = 0; index < buf.getLength(); ++index) {
+            ASSERT_EQ(buf[index], pkt->getBuffer()[index]);
+        }
+        ASSERT_EQ(expected, pkt->toText());
         pkt->delOption(D6O_BOOTFILE_URL);
         ASSERT_EQ(pkt->options_.size(), count - 1);
         ASSERT_FALSE(pkt->getOption(D6O_BOOTFILE_URL));
@@ -474,7 +501,14 @@ TEST(ScopedOptionsCopy, pkt6OptionsCopy) {
     {
         try {
             ScopedPkt6OptionsCopy oc(*pkt);
-            ASSERT_EQ(pkt->options_, options);
+            ASSERT_NE(pkt->options_, options);
+            ASSERT_NE(option, pkt->getOption(D6O_BOOTFILE_URL));
+            pkt->pack();
+            ASSERT_EQ(buf.getLength(), pkt->getBuffer().getLength());
+            for (size_t index = 0; index < buf.getLength(); ++index) {
+                ASSERT_EQ(buf[index], pkt->getBuffer()[index]);
+            }
+            ASSERT_EQ(expected, pkt->toText());
             pkt->delOption(D6O_BOOTFILE_URL);
             ASSERT_EQ(pkt->options_.size(), count - 1);
             ASSERT_FALSE(pkt->getOption(D6O_BOOTFILE_URL));
@@ -501,6 +535,7 @@ TEST(ScopedOptionsCopy, subOptionsCopy) {
     {
         ScopedSubOptionsCopy oc(initial);
         ASSERT_EQ(initial->getOptions(), options);
+        ASSERT_EQ(option, initial->getOption(DHO_BOOT_FILE_NAME));
         initial->delOption(DHO_BOOT_FILE_NAME);
         ASSERT_EQ(initial->getOptions().size(), count - 1);
         ASSERT_FALSE(initial->getOption(DHO_BOOT_FILE_NAME));
@@ -511,6 +546,7 @@ TEST(ScopedOptionsCopy, subOptionsCopy) {
         try {
             ScopedSubOptionsCopy oc(initial);
             ASSERT_EQ(initial->getOptions(), options);
+            ASSERT_EQ(option, initial->getOption(DHO_BOOT_FILE_NAME));
             initial->delOption(DHO_BOOT_FILE_NAME);
             ASSERT_EQ(initial->getOptions().size(), count - 1);
             ASSERT_FALSE(initial->getOption(DHO_BOOT_FILE_NAME));