From: Marcin Siodelski Date: Wed, 18 May 2016 15:06:54 +0000 (+0200) Subject: [4498] Avoid copying container with option definitions during parsing. X-Git-Tag: fdxhook_base~5^2~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c3eeb3cc60577ba4235a3d3c299754109f32f64c;p=thirdparty%2Fkea.git [4498] Avoid copying container with option definitions during parsing. --- diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index be9b61805d..50bb45ab29 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/src/lib/dhcp/libdhcp++.cc @@ -38,10 +38,10 @@ std::map LibDHCP::v4factories_; std::map LibDHCP::v6factories_; // Static container with DHCPv4 option definitions. -OptionDefContainer LibDHCP::v4option_defs_; +OptionDefContainerPtr LibDHCP::v4option_defs_(new OptionDefContainer()); // Static container with DHCPv6 option definitions. -OptionDefContainer LibDHCP::v6option_defs_; +OptionDefContainerPtr LibDHCP::v6option_defs_(new OptionDefContainer()); VendorOptionDefContainers LibDHCP::vendor4_defs_; @@ -65,17 +65,17 @@ void initOptionSpace(OptionDefContainer& defs, const OptionDefParams* params, size_t params_size); -const OptionDefContainer& +const OptionDefContainerPtr& LibDHCP::getOptionDefs(const Option::Universe u) { switch (u) { case Option::V4: - if (v4option_defs_.empty()) { + if (v4option_defs_->empty()) { initStdOptionDefs4(); initVendorOptsDocsis4(); } return (v4option_defs_); case Option::V6: - if (v6option_defs_.empty()) { + if (v6option_defs_->empty()) { initStdOptionDefs6(); initVendorOptsDocsis6(); } @@ -124,8 +124,8 @@ LibDHCP::getVendorOption6Defs(const uint32_t vendor_id) { OptionDefinitionPtr LibDHCP::getOptionDef(const Option::Universe u, const uint16_t code) { - const OptionDefContainer& defs = getOptionDefs(u); - const OptionDefContainerTypeIndex& idx = defs.get<1>(); + const OptionDefContainerPtr& defs = getOptionDefs(u); + const OptionDefContainerTypeIndex& idx = defs->get<1>(); const OptionDefContainerTypeRange& range = idx.equal_range(code); if (range.first != range.second) { return (*range.first); @@ -135,8 +135,8 @@ LibDHCP::getOptionDef(const Option::Universe u, const uint16_t code) { OptionDefinitionPtr LibDHCP::getOptionDef(const Option::Universe u, const std::string& name) { - const OptionDefContainer& defs = getOptionDefs(u); - const OptionDefContainerNameIndex& idx = defs.get<2>(); + const OptionDefContainerPtr defs = getOptionDefs(u); + const OptionDefContainerNameIndex& idx = defs->get<2>(); const OptionDefContainerNameRange& range = idx.equal_range(name); if (range.first != range.second) { return (*range.first); @@ -316,24 +316,19 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf, size_t last_offset = 0; // Get the list of standard option definitions. - OptionDefContainer option_defs; - if (option_space == "dhcp6") { - option_defs = LibDHCP::getOptionDefs(Option::V6); - } else { - OptionDefContainerPtr option_defs_ptr = - LibDHCP::getRuntimeOptionDefs(option_space); - if (option_defs_ptr) { - option_defs = *option_defs_ptr; - } - } + const OptionDefContainerPtr& option_defs = LibDHCP::getOptionDefs(Option::V6); + // Runtime option definitions for non standard option space and if + // the definition doesn't exist within the standard option definitions. + const OptionDefContainerPtr& runtime_option_defs = LibDHCP::getRuntimeOptionDefs(option_space); // @todo Once we implement other option spaces we should add else clause // here and gather option definitions for them. For now leaving option_defs // empty will imply creation of generic Option. - // Get the search index #1. It allows to search for option definitions + // Get the search indexes #1. It allows to search for option definitions // using option code. - const OptionDefContainerTypeIndex& idx = option_defs.get<1>(); + const OptionDefContainerTypeIndex& idx = option_defs->get<1>(); + const OptionDefContainerTypeIndex& runtime_idx = runtime_option_defs->get<1>(); // The buffer being read comprises a set of options, each starting with // a two-byte type code and a two-byte length field. @@ -398,9 +393,21 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf, // however at this point we expect to get one option // definition with the particular code. If more are returned // we report an error. - const OptionDefContainerTypeRange& range = idx.equal_range(opt_type); - // Get the number of returned option definitions for the option code. - size_t num_defs = distance(range.first, range.second); + OptionDefContainerTypeRange range; + // Number of option definitions returned. + size_t num_defs = 0; + if (option_space == DHCP6_OPTION_SPACE) { + range = idx.equal_range(opt_type); + num_defs = distance(range.first, range.second); + } + + // Standard option definitions do not include the definition for + // out option or we're searching for non-standard option. Try to + // find the definition among runtime option definitions. + if (num_defs == 0) { + range = runtime_idx.equal_range(opt_type); + num_defs = distance(range.first, range.second); + } OptionPtr opt; if (num_defs > 1) { @@ -445,24 +452,15 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, size_t last_offset = 0; // Get the list of standard option definitions. - OptionDefContainer option_defs; - if (option_space == "dhcp4") { - option_defs = LibDHCP::getOptionDefs(Option::V4); - } else { - OptionDefContainerPtr option_defs_ptr = - LibDHCP::getRuntimeOptionDefs(option_space); - if (option_defs_ptr) { - option_defs = *option_defs_ptr; - } - } - - // @todo Once we implement other option spaces we should add else clause - // here and gather option definitions for them. For now leaving option_defs - // empty will imply creation of generic Option. + const OptionDefContainerPtr& option_defs = LibDHCP::getOptionDefs(Option::V4); + // Runtime option definitions for non standard option space and if + // the definition doesn't exist within the standard option definitions. + const OptionDefContainerPtr& runtime_option_defs = LibDHCP::getRuntimeOptionDefs(option_space); - // Get the search index #1. It allows to search for option definitions + // Get the search indexes #1. It allows to search for option definitions // using option code. - const OptionDefContainerTypeIndex& idx = option_defs.get<1>(); + const OptionDefContainerTypeIndex& idx = option_defs->get<1>(); + const OptionDefContainerTypeIndex& runtime_idx = runtime_option_defs->get<1>(); // The buffer being read comprises a set of options, each starting with // a one-byte type code and a one-byte length field. @@ -511,9 +509,21 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, // however at this point we expect to get one option // definition with the particular code. If more are returned // we report an error. - const OptionDefContainerTypeRange& range = idx.equal_range(opt_type); - // Get the number of returned option definitions for the option code. - size_t num_defs = distance(range.first, range.second); + OptionDefContainerTypeRange range; + // Number of option definitions returned. + size_t num_defs = 0; + if (option_space == DHCP4_OPTION_SPACE) { + range = idx.equal_range(opt_type); + num_defs = distance(range.first, range.second); + } + + // Standard option definitions do not include the definition for + // out option or we're searching for non-standard option. Try to + // find the definition among runtime option definitions. + if (num_defs == 0) { + range = runtime_idx.equal_range(opt_type); + num_defs = distance(range.first, range.second); + } OptionPtr opt; if (num_defs > 1) { @@ -828,12 +838,12 @@ void LibDHCP::OptionFactoryRegister(Option::Universe u, void LibDHCP::initStdOptionDefs4() { - initOptionSpace(v4option_defs_, OPTION_DEF_PARAMS4, OPTION_DEF_PARAMS_SIZE4); + initOptionSpace(*v4option_defs_, OPTION_DEF_PARAMS4, OPTION_DEF_PARAMS_SIZE4); } void LibDHCP::initStdOptionDefs6() { - initOptionSpace(v6option_defs_, OPTION_DEF_PARAMS6, OPTION_DEF_PARAMS_SIZE6); + initOptionSpace(*v6option_defs_, OPTION_DEF_PARAMS6, OPTION_DEF_PARAMS_SIZE6); } void diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h index 51b68e4432..ea3838d182 100644 --- a/src/lib/dhcp/libdhcp++.h +++ b/src/lib/dhcp/libdhcp++.h @@ -36,8 +36,8 @@ public: /// /// @param u universe of the options (V4 or V6). /// - /// @return collection of option definitions. - static const OptionDefContainer& getOptionDefs(const Option::Universe u); + /// @return Pointer to a collection of option definitions. + static const OptionDefContainerPtr& getOptionDefs(const Option::Universe u); /// @brief Return the first option definition matching a /// particular option code. @@ -356,10 +356,10 @@ private: static FactoryMap v6factories_; /// Container with DHCPv4 option definitions. - static OptionDefContainer v4option_defs_; + static OptionDefContainerPtr v4option_defs_; /// Container with DHCPv6 option definitions. - static OptionDefContainer v6option_defs_; + static OptionDefContainerPtr v6option_defs_; /// Container for v4 vendor option definitions static VendorOptionDefContainers vendor4_defs_; diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc index 218ed39180..d4ad53186f 100644 --- a/src/lib/dhcp/tests/libdhcp++_unittest.cc +++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -233,10 +234,10 @@ private: // the definition for a particular option code. // We don't have to initialize option definitions here because they // are initialized in the class's constructor. - OptionDefContainer options = LibDHCP::getOptionDefs(u); + OptionDefContainerPtr options = LibDHCP::getOptionDefs(u); // Get the container index #1. This one allows for searching // option definitions using option code. - const OptionDefContainerTypeIndex& idx = options.get<1>(); + const OptionDefContainerTypeIndex& idx = options->get<1>(); // Get 'all' option definitions for a particular option code. // For standard options we expect that the range returned // will contain single option as their codes are unique. @@ -518,6 +519,42 @@ TEST_F(LibDhcpTest, unpackOptions6) { EXPECT_TRUE(x == options.end()); // option 32000 not found */ } +// Check parsing of an empty DHCPv6 option. +TEST_F(LibDhcpTest, unpackEmptyOption6) { + // Create option definition for the option code 1024 without fields. + OptionDefinitionPtr opt_def(new OptionDefinition("option-empty", 1024, + "empty", false)); + + // Use it as runtime option definition within standard options space. + // The tested code should find this option definition within runtime + // option definitions set when it detects that this definition is + // not a standard definition. + OptionDefSpaceContainer defs; + defs.addItem(opt_def, DHCP6_OPTION_SPACE); + LibDHCP::setRuntimeOptionDefs(defs); + LibDHCP::commitRuntimeOptionDefs(); + + // Create the buffer holding the structure of the empty option. + const uint8_t raw_data[] = { + 0x04, 0x00, // option code = 1024 + 0x00, 0x00 // option length = 0 + }; + size_t raw_data_len = sizeof(raw_data) / sizeof(uint8_t); + OptionBuffer buf(raw_data, raw_data + raw_data_len); + + // Parse options. + OptionCollection options; + ASSERT_NO_THROW(LibDHCP::unpackOptions6(buf, DHCP6_OPTION_SPACE, + options)); + + // There should be one option. + ASSERT_EQ(1, options.size()); + OptionPtr option_empty = options.begin()->second; + ASSERT_TRUE(option_empty); + EXPECT_EQ(1024, option_empty->getType()); + EXPECT_EQ(4, option_empty->len()); +} + // This test verifies that the following option structure can be parsed: // - option (option space 'foobar') // - sub option (option space 'foo') @@ -864,21 +901,24 @@ TEST_F(LibDhcpTest, unpackOptions4) { } -// Check parsing of an empty option +// Check parsing of an empty option. TEST_F(LibDhcpTest, unpackEmptyOption) { - // Create option definition for the option code 1 without fields. - OptionDefinitionPtr opt_def(new OptionDefinition("option-empty", 1, + // Create option definition for the option code 254 without fields. + OptionDefinitionPtr opt_def(new OptionDefinition("option-empty", 254, "empty", false)); - // Use it as runtime option definition. + // Use it as runtime option definition within standard options space. + // The tested code should find this option definition within runtime + // option definitions set when it detects that this definition is + // not a standard definition. OptionDefSpaceContainer defs; - defs.addItem(opt_def, "space-empty"); + defs.addItem(opt_def, DHCP4_OPTION_SPACE); LibDHCP::setRuntimeOptionDefs(defs); LibDHCP::commitRuntimeOptionDefs(); // Create the buffer holding the structure of the empty option. const uint8_t raw_data[] = { - 0x01, // option code = 1 + 0xFE, // option code = 254 0x00 // option length = 0 }; size_t raw_data_len = sizeof(raw_data) / sizeof(uint8_t); @@ -886,14 +926,14 @@ TEST_F(LibDhcpTest, unpackEmptyOption) { // Parse options. OptionCollection options; - ASSERT_NO_THROW(LibDHCP::unpackOptions4(buf, "space-empty", + ASSERT_NO_THROW(LibDHCP::unpackOptions4(buf, DHCP4_OPTION_SPACE, options)); // There should be one option. ASSERT_EQ(1, options.size()); OptionPtr option_empty = options.begin()->second; ASSERT_TRUE(option_empty); - EXPECT_EQ(1, option_empty->getType()); + EXPECT_EQ(254, option_empty->getType()); EXPECT_EQ(2, option_empty->len()); } @@ -1584,10 +1624,10 @@ TEST_F(LibDhcpTest, stdOptionDefs6) { // an option name. TEST_F(LibDhcpTest, getOptionDefByName6) { // Get all definitions. - const OptionDefContainer& defs = LibDHCP::getOptionDefs(Option::V6); + const OptionDefContainerPtr defs = LibDHCP::getOptionDefs(Option::V6); // For each definition try to find it using option name. - for (OptionDefContainer::const_iterator def = defs.begin(); - def != defs.end(); ++def) { + for (OptionDefContainer::const_iterator def = defs->begin(); + def != defs->end(); ++def) { OptionDefinitionPtr def_by_name = LibDHCP::getOptionDef(Option::V6, (*def)->getName()); ASSERT_TRUE(def_by_name); @@ -1600,10 +1640,10 @@ TEST_F(LibDhcpTest, getOptionDefByName6) { // an option name. TEST_F(LibDhcpTest, getOptionDefByName4) { // Get all definitions. - const OptionDefContainer& defs = LibDHCP::getOptionDefs(Option::V4); + const OptionDefContainerPtr defs = LibDHCP::getOptionDefs(Option::V4); // For each definition try to find it using option name. - for (OptionDefContainer::const_iterator def = defs.begin(); - def != defs.end(); ++def) { + for (OptionDefContainer::const_iterator def = defs->begin(); + def != defs->end(); ++def) { OptionDefinitionPtr def_by_name = LibDHCP::getOptionDef(Option::V4, (*def)->getName()); ASSERT_TRUE(def_by_name); diff --git a/src/lib/dhcp/tests/option_custom_unittest.cc b/src/lib/dhcp/tests/option_custom_unittest.cc index 678c7cf4aa..73516cfac8 100644 --- a/src/lib/dhcp/tests/option_custom_unittest.cc +++ b/src/lib/dhcp/tests/option_custom_unittest.cc @@ -170,6 +170,7 @@ TEST_F(OptionCustomTest, emptyData) { option.reset(new OptionCustom(opt_def, Option::V4, buf.begin(), buf.end())); ); + ASSERT_TRUE(option); // Option is 'empty' so no data fields are expected.