From: Razvan Becheriu Date: Tue, 27 Jun 2023 14:47:05 +0000 (+0300) Subject: [#2942] clone options only once X-Git-Tag: Kea-2.4.0~43 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=feb55de164b639c40a3aed9283c8da82deb84559;p=thirdparty%2Fkea.git [#2942] clone options only once --- diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index ef800f3a32..235b051766 100644 --- 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::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 6f2555f352..f1b7e1b6c5 100644 --- 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(opt->getType(), opt)); diff --git a/src/lib/dhcp/pkt.h b/src/lib/dhcp/pkt.h index 3a2a35a301..06dcab6bc2 100644 --- 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 00bd99ad79..41ef6b8666 100644 --- 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 c89bf0f932..e38ad59a37 100644 --- 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));