[3201] Test that suboptions are parsed correctly for fixed-size options.

src/lib/dhcp/option_custom.cc

@@ -322,6 +322,7 @@ OptionCustom::createBuffers(const OptionBuffer& data_buf) {
             }
             if (data_size > 0) {
                 buffers.push_back(OptionBuffer(data, data + data_size));
+                data += data_size;
             } else {
                 isc_throw(OutOfRange, "option buffer truncated");
             }

src/lib/dhcp/tests/option_custom_unittest.cc

@@ -32,6 +32,59 @@ public:
     /// @brief Constructor.
     OptionCustomTest() { }
 
+    /// @brief Appends DHCPv4 suboption in the on-wire format to the buffer.
+    ///
+    /// @param buf A buffer to which suboption is appended.
+    void appendV4Suboption(OptionBuffer& buf) {
+        const uint8_t subopt_data[] = {
+            0x01, 0x02, // Option type = 1, length = 2
+            0x01, 0x02  // Two bytes of data
+        };
+        buf.insert(buf.end(), subopt_data, subopt_data + sizeof(subopt_data));
+    }
+
+    /// @brief Check if the parsed option has a suboption.
+    ///
+    /// @param opt An option in which suboption is expected.
+    /// @return Assertion result indicating that the suboption is
+    /// present (success) or missing (failure).
+    ::testing::AssertionResult hasV4Suboption(OptionCustom* opt) {
+        OptionPtr subopt = opt->getOption(1);
+        if (!subopt) {
+            return (::testing::AssertionFailure(::testing::Message()
+                                                << "Suboption of OptionCustom"
+                                                " is missing"));
+        }
+        return (::testing::AssertionSuccess());
+    }
+
+    /// @brief Appends DHCPv6 suboption in the on-wire format to the buffer.
+    ///
+    /// @param buf A buffer to which suboption is appended.
+    void appendV6Suboption(OptionBuffer& buf) {
+        const uint8_t subopt_data[] = {
+            0x00, 0x01, // Option type = 1
+            0x00, 0x04, // Option length = 4
+            0x01, 0x02, 0x03, 0x04 // Four bytes of data
+        };
+        buf.insert(buf.end(), subopt_data, subopt_data + sizeof(subopt_data));
+    }
+
+    /// @brief Check if the parsed option has a suboption.
+    ///
+    /// @param opt An option in which suboption is expected.
+    /// @return Assertion result indicating that the suboption is
+    /// present (success) or missing (failure).
+    ::testing::AssertionResult hasV6Suboption(OptionCustom* opt) {
+        OptionPtr subopt = opt->getOption(1);
+        if (!subopt) {
+            return (::testing::AssertionFailure(::testing::Message()
+                                                << "Suboption of OptionCustom"
+                                                " is missing"));
+        }
+        return (::testing::AssertionSuccess());
+    }
+
     /// @brief Write IP address into a buffer.
     ///
     /// @param address address to be written.
@@ -114,23 +167,30 @@ TEST_F(OptionCustomTest, constructor) {
 // The purpose of this test is to verify that 'empty' option definition can
 // be used to create an instance of custom option.
 TEST_F(OptionCustomTest, emptyData) {
-    OptionDefinition opt_def("OPTION_FOO", 232, "empty");
+    OptionDefinition opt_def("option-foo", 232, "empty", "option-foo-space");
 
+    // Create a buffer holding 1 suboption.
     OptionBuffer buf;
+    appendV4Suboption(buf);
+
     boost::scoped_ptr<OptionCustom> option;
     ASSERT_NO_THROW(
-        option.reset(new OptionCustom(opt_def, Option::V4, buf.begin(), buf.end()));
+        option.reset(new OptionCustom(opt_def, Option::V4, buf.begin(),
+                                      buf.end()));
     );
     ASSERT_TRUE(option);
 
     // Option is 'empty' so no data fields are expected.
     EXPECT_EQ(0, option->getDataFieldsNum());
+
+    // Check that suboption has been parsed.
+    EXPECT_TRUE(hasV4Suboption(option.get()));
 }
 
 // The purpose of this test is to verify that the option definition comprising
 // a binary value can be used to create an instance of custom option.
 TEST_F(OptionCustomTest, binaryData) {
-    OptionDefinition opt_def("OPTION_FOO", 231, "binary");
+    OptionDefinition opt_def("option-foo", 231, "binary", "option-foo-space");
 
     // Create a buffer holding some binary data. This data will be
     // used as reference when we read back the data from a created
@@ -139,6 +199,11 @@ TEST_F(OptionCustomTest, binaryData) {
     for (int i = 0; i < 14; ++i) {
         buf_in[i] = i;
     }
+
+    // Append suboption data. This data should NOT be recognized when
+    // option has a binary format.
+    appendV4Suboption(buf_in);
+
     // Use scoped pointer because it allows to declare the option
     // in the function scope and initialize it under ASSERT.
     boost::scoped_ptr<OptionCustom> option;
@@ -169,20 +234,24 @@ TEST_F(OptionCustomTest, binaryData) {
                                       buf_in.end())),
         isc::OutOfRange
     );
+
+    // Suboptions are not recognized for the binary formats because as it is
+    // a variable length format. Therefore, we expect that there are no
+    // suboptions in the parsed option.
+    EXPECT_FALSE(option->getOption(1));
 }
 
 // The purpose of this test is to verify that an option definition comprising
 // a single boolean value can be used to create an instance of custom option.
 TEST_F(OptionCustomTest, booleanData) {
-    OptionDefinition opt_def("OPTION_FOO", 1000, "boolean");
+    OptionDefinition opt_def("option-foo", 1000, "boolean", "option-foo-space");
 
     OptionBuffer buf;
     // Push back the value that represents 'false'.
     buf.push_back(0);
-    // Push back the 'true' value. Note that this value should
-    // be ignored by custom option because it holds single boolean
-    // value (according to option definition).
-    buf.push_back(1);
+
+    // Append suboption. It should be present in the parsed packet.
+    appendV6Suboption(buf);
 
     boost::scoped_ptr<OptionCustom> option;
     ASSERT_NO_THROW(
@@ -202,10 +271,14 @@ TEST_F(OptionCustomTest, booleanData) {
     ASSERT_NO_THROW(value = option->readBoolean(0));
     EXPECT_FALSE(value);
 
+    // There should be one suboption present.
+    EXPECT_TRUE(hasV6Suboption(option.get()));
+
     // Check that the option with "no data" is rejected.
     buf.clear();
     EXPECT_THROW(
-        option.reset(new OptionCustom(opt_def, Option::V6, buf.begin(), buf.end())),
+        option.reset(new OptionCustom(opt_def, Option::V6, buf.begin(),
+                                      buf.end())),
         isc::OutOfRange
     );
 }
@@ -213,7 +286,7 @@ TEST_F(OptionCustomTest, booleanData) {
 // The purpose of this test is to verify that the data from a buffer
 // can be read as FQDN.
 TEST_F(OptionCustomTest, fqdnData) {
-    OptionDefinition opt_def("OPTION_FOO", 1000, "fqdn");
+    OptionDefinition opt_def("option-foo", 1000, "fqdn", "option-foo-space");
 
     const char data[] = {
         8, 109, 121, 100, 111, 109, 97, 105, 110, // "mydomain"
@@ -224,6 +297,10 @@ TEST_F(OptionCustomTest, fqdnData) {
 
     std::vector<uint8_t> buf(data, data + sizeof(data));
 
+    // The FQDN has a certain boundary. Right after FQDN it should be
+    // possible to append suboption and parse it correctly.
+    appendV6Suboption(buf);
+
     boost::scoped_ptr<OptionCustom> option;
     ASSERT_NO_THROW(
         option.reset(new OptionCustom(opt_def, Option::V6, buf.begin(), buf.end()));
@@ -235,6 +312,9 @@ TEST_F(OptionCustomTest, fqdnData) {
     std::string domain0 = option->readFqdn(0);
     EXPECT_EQ("mydomain.example.com.", domain0);
 
+    // This option should have one suboption.
+    EXPECT_TRUE(hasV6Suboption(option.get()));
+
     // Check that the option with truncated data can't be created.
     EXPECT_THROW(
         option.reset(new OptionCustom(opt_def, Option::V6,
@@ -246,12 +326,15 @@ TEST_F(OptionCustomTest, fqdnData) {
 // The purpose of this test is to verify that the option definition comprising
 // 16-bit signed integer value can be used to create an instance of custom option.
 TEST_F(OptionCustomTest, int16Data) {
-    OptionDefinition opt_def("OPTION_FOO", 1000, "int16");
+    OptionDefinition opt_def("option-foo", 1000, "int16", "option-foo-space");
 
     OptionBuffer buf;
     // Store signed integer value in the input buffer.
     writeInt<int16_t>(-234, buf);
 
+    // Append suboption.
+    appendV6Suboption(buf);
+
     // Create custom option.
     boost::scoped_ptr<OptionCustom> option;
     ASSERT_NO_THROW(
@@ -268,6 +351,9 @@ TEST_F(OptionCustomTest, int16Data) {
     ASSERT_NO_THROW(value = option->readInteger<int16_t>(0));
     EXPECT_EQ(-234, value);
 
+    // Parsed option should have one suboption.
+    EXPECT_TRUE(hasV6Suboption(option.get()));
+
     // Check that the option is not created when a buffer is
     // too short (1 byte instead of 2 bytes).
     EXPECT_THROW(
@@ -279,11 +365,13 @@ TEST_F(OptionCustomTest, int16Data) {
 // The purpose of this test is to verify that the option definition comprising
 // 32-bit signed integer value can be used to create an instance of custom option.
 TEST_F(OptionCustomTest, int32Data) {
-    OptionDefinition opt_def("OPTION_FOO", 1000, "int32");
+    OptionDefinition opt_def("option-foo", 1000, "int32", "option-foo-space");
 
     OptionBuffer buf;
     writeInt<int32_t>(-234, buf);
-    writeInt<int32_t>(100, buf);
+
+    // Append one suboption.
+    appendV6Suboption(buf);
 
     // Create custom option.
     boost::scoped_ptr<OptionCustom> option;
@@ -301,6 +389,9 @@ TEST_F(OptionCustomTest, int32Data) {
     ASSERT_NO_THROW(value = option->readInteger<int32_t>(0));
     EXPECT_EQ(-234, value);
 
+    // The parsed option should have one suboption.
+    EXPECT_TRUE(hasV6Suboption(option.get()));
+
     // Check that the option is not created when a buffer is
     // too short (3 bytes instead of 4 bytes).
     EXPECT_THROW(
@@ -312,12 +403,16 @@ TEST_F(OptionCustomTest, int32Data) {
 // The purpose of this test is to verify that the option definition comprising
 // single IPv4 address can be used to create an instance of custom option.
 TEST_F(OptionCustomTest, ipv4AddressData) {
-    OptionDefinition opt_def("OPTION_FOO", 231, "ipv4-address");
+    OptionDefinition opt_def("OPTION_FOO", 231, "ipv4-address",
+                             "option-foo-space");
 
     // Create input buffer.
     OptionBuffer buf;
     writeAddress(IOAddress("192.168.100.50"), buf);
 
+    // Append one suboption.
+    appendV4Suboption(buf);
+
     // Create custom option.
     boost::scoped_ptr<OptionCustom> option;
     ASSERT_NO_THROW(
@@ -334,6 +429,9 @@ TEST_F(OptionCustomTest, ipv4AddressData) {
 
     EXPECT_EQ("192.168.100.50", address.toText());
 
+    // Parsed option should have one suboption.
+    EXPECT_TRUE(hasV4Suboption(option.get()));
+
     // Check that option is not created if the provided buffer is
     // too short (use 3 bytes instead of 4).
     EXPECT_THROW(
@@ -345,12 +443,16 @@ TEST_F(OptionCustomTest, ipv4AddressData) {
 // The purpose of this test is to verify that the option definition comprising
 // single IPv6 address can be used to create an instance of custom option.
 TEST_F(OptionCustomTest, ipv6AddressData) {
-    OptionDefinition opt_def("OPTION_FOO", 1000, "ipv6-address");
+    OptionDefinition opt_def("option-foo", 1000, "ipv6-address",
+                             "option-foo-space");
 
     // Initialize input buffer.
     OptionBuffer buf;
     writeAddress(IOAddress("2001:db8:1::100"), buf);
 
+    // Append suboption.
+    appendV6Suboption(buf);
+
     // Create custom option using input buffer.
     boost::scoped_ptr<OptionCustom> option;
     ASSERT_NO_THROW(
@@ -369,6 +471,9 @@ TEST_F(OptionCustomTest, ipv6AddressData) {
 
     EXPECT_EQ("2001:db8:1::100", address.toText());
 
+    // Parsed option should have one suboption.
+    EXPECT_TRUE(hasV6Suboption(option.get()));
+
     // Check that option is not created if the provided buffer is
     // too short (use 15 bytes instead of 16).
     EXPECT_THROW(
@@ -382,12 +487,19 @@ TEST_F(OptionCustomTest, ipv6AddressData) {
 // The purpose of this test is to verify that the option definition comprising
 // string value can be used to create an instance of custom option.
 TEST_F(OptionCustomTest, stringData) {
-    OptionDefinition opt_def("OPTION_FOO", 1000, "string");
+    OptionDefinition opt_def("option-foo", 1000, "string", "option-foo-space");
 
     // Create an input buffer holding some string value.
     OptionBuffer buf;
     writeString("hello world!", buf);
 
+    // Append suboption. It should not be detected because the string field
+    //  has variable length.
+    appendV6Suboption(buf);
+
+    // Append suboption. Since the option has variable length string field,
+    // the suboption should not be recognized.
+
     // Create custom option using input buffer.
     boost::scoped_ptr<OptionCustom> option;
     ASSERT_NO_THROW(
@@ -403,7 +515,14 @@ TEST_F(OptionCustomTest, stringData) {
     std::string value;
     ASSERT_NO_THROW(value = option->readString(0));
 
-    EXPECT_EQ("hello world!", value);
+    // The initial part of the string should contain the actual string.
+    // The rest of it is a garbage from an attempt to decode suboption
+    // as a string.
+    ASSERT_EQ(20, value.size());
+    EXPECT_EQ("hello world!", value.substr(0, 12));
+
+    // No suboption should be present.
+    EXPECT_FALSE(option->getOption(1));
 
     // Check that option will not be created if empty buffer is provided.
     buf.clear();
@@ -416,7 +535,7 @@ TEST_F(OptionCustomTest, stringData) {
 // The purpose of this test is to verify that the option definition comprising
 // an array of boolean values can be used to create an instance of custom option.
 TEST_F(OptionCustomTest, booleanDataArray) {
-    OptionDefinition opt_def("OPTION_FOO", 1000, "boolean", true);
+    OptionDefinition opt_def("option-foo", 1000, "boolean", true);
 
     // Create a buffer with 5 values that represent array of
     // booleans.
@@ -472,7 +591,7 @@ TEST_F(OptionCustomTest, booleanDataArray) {
 // an array of 32-bit signed integer values can be used to create an instance
 // of custom option.
 TEST_F(OptionCustomTest, uint32DataArray) {
-    OptionDefinition opt_def("OPTION_FOO", 1000, "uint32", true);
+    OptionDefinition opt_def("option-foo", 1000, "uint32", true);
 
     // Create an input buffer that holds 4 uint32 values that
     // represent an array.
@@ -656,6 +775,47 @@ TEST_F(OptionCustomTest, fqdnDataArray) {
     EXPECT_EQ("example.com.", domain1);
 }
 
+// The purpose of this test is to verify that the opton definition comprising
+// a record of fixed-size fields can be used to create an option with a
+// suboption.
+TEST_F(OptionCustomTest, recordDataWithSuboption) {
+    OptionDefinition opt_def("option-foo", 1000, "record", "option-foo-space");
+    ASSERT_NO_THROW(opt_def.addRecordField("uint32"));
+    ASSERT_NO_THROW(opt_def.addRecordField("ipv4-address"));
+
+    // Create a buffer with two fields: 4-byte number and IPv4 address.
+    OptionBuffer buf;
+    writeInt<uint32_t>(0x01020304, buf);
+    writeAddress(IOAddress("192.168.0.1"), buf);
+
+    // Append a suboption. It should be correctly parsed because option fields
+    // preceding this option have fixed (known) size.
+    appendV6Suboption(buf);
+
+    boost::scoped_ptr<OptionCustom> option;
+    ASSERT_NO_THROW(
+         option.reset(new OptionCustom(opt_def, Option::V6, buf.begin(),
+                                       buf.end()));
+    );
+    ASSERT_TRUE(option);
+
+    // We should have two data fields parsed.
+    ASSERT_EQ(2, option->getDataFieldsNum());
+
+    // Validate values in fields.
+    uint32_t value0 = 0;
+    ASSERT_NO_THROW(value0 = option->readInteger<uint32_t>(0));
+    EXPECT_EQ(0x01020304, value0);
+
+    IOAddress value1 = 0;
+    ASSERT_NO_THROW(value1 = option->readAddress(1));
+    EXPECT_EQ("192.168.0.1", value1.toText());
+
+    // Parsed option should have one suboption.
+    EXPECT_TRUE(hasV6Suboption(option.get()));
+
+}
+
 // The purpose of this test is to verify that the option definition comprising
 // a record of various data fields can be used to create an instance of
 // custom option.

src/lib/dhcp/tests/option_definition_unittest.cc

@@ -439,7 +439,7 @@ TEST_F(OptionDefinitionTest, ipv4AddressArrayTokenized) {
     EXPECT_TRUE(std::equal(addrs.begin(), addrs.end(), addrs_returned.begin()));
 }
 
-// The purpose of thie test is to verify that option definition for
+// The purpose of this test is to verify that option definition for
 // 'empty' option can be created and that it returns 'empty' option.
 TEST_F(OptionDefinitionTest, empty) {
     OptionDefinition opt_def("OPTION_RAPID_COMMIT", D6O_RAPID_COMMIT, "empty");
@@ -464,6 +464,49 @@ TEST_F(OptionDefinitionTest, empty) {
     EXPECT_EQ(0, option_v4->getData().size());
 }
 
+// The purpose of this test is to verify that when the empty option encapsulates
+// some option space, an instance of the OptionCustom is returned and its
+// suboptions are decoded.
+TEST_F(OptionDefinitionTest, emptyWithSuboptions) {
+    // Create an instance of the 'empty' option definition. This option
+    // encapsulates 'option-foo-space' so when we create a new option
+    // with this definition the OptionCustom should be returned. The
+    // Option Custom is generic option which support variety of formats
+    // and supports decoding suboptions.
+    OptionDefinition opt_def("option-foo", 1024, "empty", "option-foo-space");
+
+    // Define a suboption.
+    const uint8_t subopt_data[] = {
+        0x04, 0x01,  // Option code 1025
+        0x00, 0x04,  // Option len = 4
+        0x01, 0x02, 0x03, 0x04 // Option data
+    };
+
+    // Create an option, having option code 1024 from the definition. Pass
+    // the option buffer containing suboption.
+    OptionPtr option_v6;
+    ASSERT_NO_THROW(
+        option_v6 = opt_def.optionFactory(Option::V6, 1024,
+                                          OptionBuffer(subopt_data,
+                                                       subopt_data +
+                                                       sizeof(subopt_data)))
+    );
+    // Returned option should be of the OptionCustom type.
+    ASSERT_TRUE(typeid(*option_v6) == typeid(OptionCustom));
+    // Sanity-check length, universe etc.
+    EXPECT_EQ(Option::V6, option_v6->getUniverse());
+    EXPECT_EQ(4, option_v6->getHeaderLen());
+    // This option should have one suboption with the code of 1025.
+    OptionPtr subopt_v6 = option_v6->getOption(1025);
+    EXPECT_TRUE(subopt_v6);
+    // Check that this suboption holds valid data.
+    EXPECT_EQ(1025, subopt_v6->getType());
+    EXPECT_EQ(Option::V6, subopt_v6->getUniverse());
+    EXPECT_EQ(0, memcmp(&subopt_v6->getData()[0], subopt_data + 4, 4));
+
+    // @todo consider having a similar test for V4.
+}
+
 // The purpose of this test is to verify that definition can be
 // creates for the option that holds binary data.
 TEST_F(OptionDefinitionTest, binary) {