[950-dhcp-option-43-pad-end] Fixed the option 43 END bug

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index 6be24dbc54e4096fcaa22bec83e88ce82983a0e0..61d0f9f42a37767c541bd2f35a153ada20681341 100644 (file)
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2019 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2020 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -458,7 +458,8 @@ size_t
 LibDHCP::unpackOptions4(const OptionBuffer& buf,
                         const std::string& option_space,
                         isc::dhcp::OptionCollection& options,
-                        std::list<uint16_t>& deferred) {
+                       std::list<uint16_t>& deferred,
+                       bool flexible_pad_end) {
     size_t offset = 0;
     size_t last_offset = 0;
 
@@ -486,7 +487,11 @@ LibDHCP::unpackOptions4(const OptionBuffer& buf,
         uint8_t opt_type = buf[offset++];
 
         // DHO_END is a special, one octet long option
-        if (space_is_dhcp4 && (opt_type == DHO_END)) {
+        // Valid in dhcp4 space or when flexible_pad_end is true and
+        // there is a sub-option configured for this code.
+        if ((opt_type == DHO_END) &&
+            (space_is_dhcp4 ||
+             (flexible_pad_end && (runtime_idx.count(DHO_END) == 0)))) {
             // just return. Don't need to add DHO_END option
             // Don't return offset because it makes this condition
             // and partial parsing impossible to recognize.
@@ -495,7 +500,11 @@ LibDHCP::unpackOptions4(const OptionBuffer& buf,
 
         // DHO_PAD is just a padding after DHO_END. Let's continue parsing
         // in case we receive a message without DHO_END.
-        if (space_is_dhcp4 && (opt_type == DHO_PAD)) {
+        // Valid in dhcp4 space or when flexible_pad_end is true and
+        // there is a sub-option configured for this code.
+        if ((opt_type == DHO_PAD) &&
+            (space_is_dhcp4 ||
+             (flexible_pad_end && (runtime_idx.count(DHO_PAD) == 0)))) {
             continue;
         }
 

diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h
index 1b22fb547ee0ae761ac26e53f685e5d9c67a6577..469e95b42c4dc68f34da7d0c142496aa4b06d556 100644 (file)
--- a/src/lib/dhcp/libdhcp++.h
+++ b/src/lib/dhcp/libdhcp++.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2019 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -259,6 +259,8 @@ public:
     ///        put here.
     /// @param deferred Reference to an option code list. Options which
     ///        processing is deferred will be put here.
+    /// @param flexible_pad_end Parse options 0 and 255 as PAD and END
+    ///        when they are not defined in the option space.
     /// @return offset to the first byte after the last successfully
     /// parsed option or the offset of the DHO_END option type.
     ///
@@ -266,7 +268,8 @@ public:
     static size_t unpackOptions4(const OptionBuffer& buf,
                                  const std::string& option_space,
                                  isc::dhcp::OptionCollection& options,
-                                 std::list<uint16_t>& deferred);
+                                 std::list<uint16_t>& deferred,
+                                 bool flexible_pad_end);
 
     /// Registers factory method that produces options of specific option types.
     ///

diff --git a/src/lib/dhcp/option.cc b/src/lib/dhcp/option.cc
index c2485eb5ef8ab9daba1cfee57a295b6cfea31238..c404b2a878a0bd499f4c575c39d8672fb28ed584 100644 (file)
--- a/src/lib/dhcp/option.cc
+++ b/src/lib/dhcp/option.cc
@@ -5,8 +5,10 @@
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 #include <config.h>
+#include <dhcp/dhcp4.h>
 #include <dhcp/libdhcp++.h>
 #include <dhcp/option.h>
+#include <dhcp/option_space.h>
 #include <exceptions/exceptions.h>
 #include <util/encode/hex.h>
 #include <util/io_utilities.h>
@@ -165,7 +167,8 @@ Option::unpackOptions(const OptionBuffer& buf) {
     switch (universe_) {
     case V4:
         LibDHCP::unpackOptions4(buf, getEncapsulatedSpace(),
-                                options_, deferred);
+                                options_, deferred,
+                                getType() == DHO_VENDOR_ENCAPSULATED_OPTIONS);
         return;
     case V6:
         LibDHCP::unpackOptions6(buf, getEncapsulatedSpace(), options_);

diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc
index b01ec7b665b7365c4ade98da97140050672ff70a..dbd2b15704b797303a0f78e2db87eaee81d7095b 100644 (file)
--- a/src/lib/dhcp/pkt4.cc
+++ b/src/lib/dhcp/pkt4.cc
@@ -205,7 +205,7 @@ Pkt4::unpack() {
     // a vector as an input.
     buffer_in.readVector(opts_buffer, opts_len);
 
-    size_t offset = LibDHCP::unpackOptions4(opts_buffer, DHCP4_OPTION_SPACE, options_, deferred_options_);
+    size_t offset = LibDHCP::unpackOptions4(opts_buffer, DHCP4_OPTION_SPACE, options_, deferred_options_, false);
 
     // If offset is not equal to the size and there is no DHO_END,
     // then something is wrong here. We either parsed past input

diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index e63c45bd545d6c5258afff963d3feb92d42f9702..3c8d4f2a380206e659474ed9689f05371f70eea5 100644 (file)
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2019 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2020 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -779,7 +779,8 @@ TEST_F(LibDhcpTest, unpackOptions4) {
     list<uint16_t> deferred;
 
     ASSERT_NO_THROW(
-        LibDHCP::unpackOptions4(v4packed, DHCP4_OPTION_SPACE, options, deferred);
+        LibDHCP::unpackOptions4(v4packed, DHCP4_OPTION_SPACE, options,
+                               deferred false);
     );
 
     isc::dhcp::OptionCollection::const_iterator x = options.find(12);
@@ -924,7 +925,7 @@ TEST_F(LibDhcpTest, unpackEmptyOption4) {
     OptionCollection options;
     list<uint16_t> deferred;
     ASSERT_NO_THROW(LibDHCP::unpackOptions4(buf, DHCP4_OPTION_SPACE,
-                                            options, deferred));
+                                            options, deferred, false));
 
     // There should be one option.
     ASSERT_EQ(1, options.size());
@@ -983,7 +984,7 @@ TEST_F(LibDhcpTest, unpackSubOptions4) {
     OptionCollection options;
     list<uint16_t> deferred;
     ASSERT_NO_THROW(LibDHCP::unpackOptions4(buf, "space-foobar",
-                                            options, deferred));
+                                            options, deferred, false));
 
     // There should be one top level option.
     ASSERT_EQ(1, options.size());
@@ -1057,7 +1058,7 @@ TEST_F(LibDhcpTest, unpackPadEnd) {
     list<uint16_t> deferred;
     size_t offset = 0;
     ASSERT_NO_THROW(offset = LibDHCP::unpackOptions4(buf, DHCP4_OPTION_SPACE,
-                                                     options, deferred));
+                                                     options, deferred, false));
 
     // Returned offset should point to the END.
     EXPECT_EQ(0xff, buf[offset]);
@@ -1097,6 +1098,176 @@ TEST_F(LibDhcpTest, unpackPadEnd) {
     EXPECT_EQ("foo", sub->getValue());
 }
 
+// Verfies that option 0 (PAD) is handled as PAD in option 43 (so when
+// flexible pad end flag is true) only when option 0 (PAD) is not defined.
+TEST_F(LibDhcpTest, option43Pad) {
+    string space = "my-option43-space";
+
+    // Create option definition for option 1.
+    OptionDefinitionPtr opt_def1(new OptionDefinition("one", 1, "binary"));
+
+    // Create option definition for option 2.
+    OptionDefinitionPtr opt_def2(new OptionDefinition("two", 2, "uint8"));
+
+    // Register created option definitions as runtime option definitions.
+    OptionDefSpaceContainer defs;
+    ASSERT_NO_THROW(defs.addItem(opt_def1, space));
+    ASSERT_NO_THROW(defs.addItem(opt_def2, space));
+    LibDHCP::setRuntimeOptionDefs(defs);
+    LibDHCP::commitRuntimeOptionDefs();
+
+    // Create the buffer holding an option 43 content.
+    OptionBuffer buf = {
+        // Suboption 0,
+        0x00, 0x01, 0x00,             // code = 0, length = 1, content = 0
+                                      // or option code = 0 (PAD) followed by
+                                      // code = 1, length = 0
+        // Suboption 2.
+        0x02, 0x01, 0x01,             // code = 2, length = 1, content = 1
+    };
+
+    // Parse options.
+    OptionCollection options;
+    list<uint16_t> deferred;
+    size_t offset = 0;
+    ASSERT_NO_THROW(offset = LibDHCP::unpackOptions4(buf, space,
+                                                     options, deferred, true));
+
+    // There should be 2 suboptions (1 and 2).
+    ASSERT_EQ(2, options.size());
+
+    // Get suboption 1.
+    OptionPtr sub1 = options.begin()->second;
+    ASSERT_TRUE(sub1);
+    EXPECT_EQ(1, sub1->getType());
+    EXPECT_EQ(0, sub1->len() - sub1->getHeaderLen());
+
+    // Get suboption 2.
+    boost::shared_ptr<OptionInt<uint8_t> > sub2 =
+        boost::dynamic_pointer_cast<OptionInt<uint8_t> >
+            (options.rbegin()->second);
+    ASSERT_TRUE(sub2);
+    EXPECT_EQ(2, sub2->getType());
+    EXPECT_EQ(1, sub2->getValue());
+
+    // Create option definition for option 0 and register it.
+    OptionDefinitionPtr opt_def0(new OptionDefinition("zero", 0, "uint8"));
+    ASSERT_NO_THROW(defs.addItem(opt_def0, space));
+    LibDHCP::clearRuntimeOptionDefs();
+    LibDHCP::setRuntimeOptionDefs(defs);
+    LibDHCP::commitRuntimeOptionDefs();
+
+    options.clear();
+    offset = 0;
+    ASSERT_NO_THROW(offset = LibDHCP::unpackOptions4(buf, space,
+                                                     options, deferred, true));
+
+    // There should be 2 suboptions (0 and 1).
+    EXPECT_EQ(2, options.size());
+
+    // Get suboption 0
+    boost::shared_ptr<OptionInt<uint8_t> > sub0 =
+        boost::dynamic_pointer_cast<OptionInt<uint8_t> >
+            (options.begin()->second);
+    ASSERT_TRUE(sub0);
+    EXPECT_EQ(0, sub0->getType());
+    EXPECT_EQ(0, sub0->getValue());
+
+    // Get suboption 2.
+    sub2 =
+        boost::dynamic_pointer_cast<OptionInt<uint8_t> >
+            (options.rbegin()->second);
+    ASSERT_TRUE(sub2);
+    EXPECT_EQ(2, sub2->getType());
+    EXPECT_EQ(1, sub2->getValue());
+}
+
+// Verfies that option 255 (END) is handled as END in option 43 (so when
+//flexible pad end flag is true) only when option 255 (END) is not defined.
+TEST_F(LibDhcpTest, option43End) {
+    string space = "my-option43-space";
+
+    // Create the buffer holding an option 43 content.
+    OptionBuffer buf = {
+        // Suboption 255,
+        0xff, 0x01, 0x02              // code = 255, length = 1, content = 2
+    };
+
+    // Parse options.
+    OptionCollection options;
+    list<uint16_t> deferred;
+    size_t offset = 0;
+    ASSERT_NO_THROW(offset = LibDHCP::unpackOptions4(buf, space,
+                                                     options, deferred, true));
+
+    // Parsing should stop at the first byte.
+    EXPECT_EQ(0, offset);
+
+    // There should be 0 suboptions.
+    EXPECT_EQ(0, options.size());
+
+
+    // Create option definition for option 255.
+    OptionDefinitionPtr opt_def255(new OptionDefinition("max", 255, "uint8"));
+
+    // Register created option definition as runtime option definitions.
+    OptionDefSpaceContainer defs;
+    ASSERT_NO_THROW(defs.addItem(opt_def255, space));
+    LibDHCP::setRuntimeOptionDefs(defs);
+    LibDHCP::commitRuntimeOptionDefs();
+
+    options.clear();
+    offset = 0;
+    ASSERT_NO_THROW(offset = LibDHCP::unpackOptions4(buf, space,
+                                                     options, deferred, true));
+
+    // There should be 1 suboption.
+    ASSERT_EQ(1, options.size());
+
+    // Get suboption 255.
+    boost::shared_ptr<OptionInt<uint8_t> > sub255 =
+        boost::dynamic_pointer_cast<OptionInt<uint8_t> >
+            (options.begin()->second);
+    ASSERT_TRUE(sub255);
+    EXPECT_EQ(255, sub255->getType());
+    EXPECT_EQ(2, sub255->getValue());
+}
+
+// Verify the option 43 DEND bug is fixed.
+TEST_F(LibDhcpTest, option32Factory) {
+    // Create the buffer holding the structure of option 43 content.
+    OptionBuffer buf = {
+        // Suboption 1.
+        0x01, 0x00,                     // option code = 1, option length = 0
+        // END
+        0xff
+    };
+
+    // Get last resort definition.
+    OptionDefinitionPtr def =
+        LibDHCP::getLastResortOptionDef(DHCP4_OPTION_SPACE, 43);
+    ASSERT_TRUE(def);
+
+    // Apply the definition.
+    OptionPtr option;
+    ASSERT_NO_THROW(option = def->optionFactory(Option::V4, 43, buf));
+    ASSERT_TRUE(option);
+    EXPECT_EQ(DHO_VENDOR_ENCAPSULATED_OPTIONS, option->getType());
+    EXPECT_EQ(def->getEncapsulatedSpace(), option->getEncapsulatedSpace());
+
+    // There should be 1 suboption.
+    EXPECT_EQ(1, option->getOptions().size());
+
+    // Get suboption 1.
+    OptionPtr sub1 = option->getOption(1);
+    ASSERT_TRUE(sub1);
+    EXPECT_EQ(1, sub1->getType());
+    EXPECT_EQ(0, sub1->len() - sub1->getHeaderLen());
+
+    // Of course no suboption 255.
+    EXPECT_FALSE(option->getOption(255));
+}
+
 // Verifies that an Host Name (option 12), will be dropped when empty,
 // while subsequent options will still be unpacked.
 TEST_F(LibDhcpTest, emptyHostName) {
@@ -1111,7 +1282,7 @@ TEST_F(LibDhcpTest, emptyHostName) {
     list<uint16_t> deferred;
 
     ASSERT_NO_THROW(
-        LibDHCP::unpackOptions4(packed, "dhcp4", options, deferred);
+        LibDHCP::unpackOptions4(packed, "dhcp4", options, deferred, false);
     );
 
     // Host Name should not exist, we quietly drop it when empty.