[4498] Avoid copying container with option definitions during parsing.

src/lib/dhcp/libdhcp++.cc

@@ -38,10 +38,10 @@ std::map<unsigned short, Option::Factory*> LibDHCP::v4factories_;
 std::map<unsigned short, Option::Factory*> 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

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_;

src/lib/dhcp/tests/libdhcp++_unittest.cc

@@ -23,6 +23,7 @@
 #include <dhcp/option_int.h>
 #include <dhcp/option_int_array.h>
 #include <dhcp/option_opaque_data_tuples.h>
+#include <dhcp/option_space.h>
 #include <dhcp/option_string.h>
 #include <dhcp/option_vendor.h>
 #include <dhcp/option_vendor_class.h>
@@ -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);

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.