[#2942] clone options only once

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index ef800f3a3261db3e6d8aaecfcd6e5b71738531a2..235b0517660cabe44d72ccffed759577c18cd479 100644 (file)
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -1057,14 +1057,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 6f2555f352d6f1897691072beb62f5694e6a0a69..f1b7e1b6c51712f0001e06198e1afe1f23c4807e 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 3a2a35a30195341c29ee442b42759b2365463fc1..06dcab6bc25a91775757936d20bd73f9d3f67492 100644 (file)
--- a/src/lib/dhcp/pkt.h
+++ b/src/lib/dhcp/pkt.h
@@ -370,6 +370,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
@@ -924,6 +928,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 00bd99ad792350b4befd96e66b71a887b79016dc..41ef6b866637d7a97d125c5cbc86ab39963960bc 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 c89bf0f9324d573e9fa6a2b66378677f1888ea39..e38ad59a3722c8e42a74ef71c5b95931b8d4f433 100644 (file)
--- a/src/lib/dhcp/tests/protocol_util_unittest.cc
+++ b/src/lib/dhcp/tests/protocol_util_unittest.cc
@@ -540,9 +540,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));
@@ -552,7 +562,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));
@@ -576,9 +593,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));
@@ -588,7 +615,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));
@@ -615,6 +649,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));
@@ -625,6 +660,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));