[3618] decided between offset-x/throw and fixed vivso-suboptions

src/lib/dhcp/libdhcp++.cc

@@ -273,12 +273,14 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
         offset += 2;
 
         if (offset + opt_len > length) {
-            // @todo: consider throwing exception here.
-
-            // We peeked at the option header of the next option, but discovered
-            // that it would end up beyond buffer end, so the option is
-            // truncated. Hence we can't parse it. Therefore we revert
-            // back by those four bytes (as if we never parsed them).
+            // We peeked at the option header of the next option, but
+            // discovered that it would end up beyond buffer end, so
+            // the option is truncated. Hence we can't parse
+            // it. Therefore we revert back by those four bytes (as if
+            // we never parsed them).
+            //
+            // @note it is the responsability of the caller to throw
+            // an exception on partial parsing
             return (offset - 4);
         }
 
@@ -294,9 +296,9 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
 
         if (opt_type == D6O_VENDOR_OPTS) {
             if (offset + 4 > length) {
-                // Truncated vendor-option. There is expected at least 4 bytes
-                // long enterprise-id field. Let's roll back option code + option
-                // length (4 bytes) and return.
+                // Truncated vendor-option. There is expected at least
+                // 4 bytes long enterprise-id field. Let's roll back
+                // option code + option length (4 bytes) and return.
                 return (offset - 4);
             }
 
@@ -310,10 +312,11 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
         }
 
 
-        // Get all definitions with the particular option code. Note that option
-        // code is non-unique within this container however at this point we
-        // expect to get one option definition with the particular code. If more
-        // are returned we report an error.
+        // Get all definitions with the particular option code. Note
+        // that option code is non-unique within this container
+        // 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);
@@ -321,16 +324,18 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
         OptionPtr opt;
         if (num_defs > 1) {
             // Multiple options of the same code are not supported right now!
-            isc_throw(isc::Unexpected, "Internal error: multiple option definitions"
-                      " for option type " << opt_type << " returned. Currently it is not"
-                      " supported to initialize multiple option definitions"
-                      " for the same option code. This will be supported once"
-                      " support for option spaces is implemented");
+            isc_throw(isc::Unexpected, "Internal error: multiple option"
+                      " definitions for option type " << opt_type <<
+                      " returned. Currently it is not supported to initialize"
+                      " multiple option definitions for the same option code."
+                      " This will be supported once support for option spaces"
+                      " is implemented");
         } else if (num_defs == 0) {
-            // @todo Don't crash if definition does not exist because only a few
-            // option definitions are initialized right now. In the future
-            // we will initialize definitions for all options and we will
-            // remove this elseif. For now, return generic option.
+            // @todo Don't crash if definition does not exist because
+            // only a few option definitions are initialized right
+            // now. In the future we will initialize definitions for
+            // all options and we will remove this elseif. For now,
+            // return generic option.
             opt = OptionPtr(new Option(Option::V6, opt_type,
                                        buf.begin() + offset,
                                        buf.begin() + offset + opt_len));
@@ -356,7 +361,7 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
                                isc::dhcp::OptionCollection& options) {
     size_t offset = 0;
 
-    // Get the list of stdandard option definitions.
+    // Get the list of standard option definitions.
     OptionDefContainer option_defs;
     if (option_space == "dhcp4") {
         option_defs = LibDHCP::getOptionDefs(Option::V4);
@@ -384,30 +389,32 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
             continue;
 
         if (offset + 1 > buf.size()) {
-            // opt_type must be cast to integer so as it is not treated as
-            // unsigned char value (a number is presented in error message).
-            isc_throw(OutOfRange, "Attempt to parse truncated option "
-                      << static_cast<int>(opt_type));
+            // We peeked at the option header of the next option, but
+            // discovered that it would end up beyond buffer end, so
+            // the option is truncated. Hence we can't parse
+            // it. Therefore we revert back by that byte (as if
+            // we never parsed it).
+            //
+            // @note it is the responsability of the caller to throw
+            // an exception on partial parsing
+            return (offset - 1);
         }
 
         uint8_t opt_len =  buf[offset++];
         if (offset + opt_len > buf.size()) {
-
-            // We peeked at the option header of the next option, but discovered
-            // that it would end up beyond buffer end, so the option is
-            // truncated. Hence we can't parse it. Therefore we revert
-            // back by two bytes (as if we never parsed them).
+            // We peeked at the option header of the next option, but
+            // discovered that it would end up beyond buffer end, so
+            // the option is truncated. Hence we can't parse
+            // it. Therefore we revert back by two bytes (as if we
+            // never parsed them).
             return (offset - 2);
-
-            // isc_throw(OutOfRange, "Option parse failed. Tried to parse "
-            //          << offset + opt_len << " bytes from " << buf.size()
-            //          << "-byte long buffer.");
         }
 
-        // Get all definitions with the particular option code. Note that option code
-        // is non-unique within this container however at this point we expect
-        // to get one option definition with the particular code. If more are
-        // returned we report an error.
+        // Get all definitions with the particular option code. Note
+        // that option code is non-unique within this container
+        // 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);
@@ -415,12 +422,13 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
         OptionPtr opt;
         if (num_defs > 1) {
             // Multiple options of the same code are not supported right now!
-            isc_throw(isc::Unexpected, "Internal error: multiple option definitions"
-                      " for option type " << static_cast<int>(opt_type)
-                      << " returned. Currently it is not supported to initialize"
-                      << " multiple option definitions for the same option code."
-                      << " This will be supported once support for option spaces"
-                      << " is implemented");
+            isc_throw(isc::Unexpected, "Internal error: multiple option"
+                      " definitions for option type " <<
+                      static_cast<int>(opt_type) <<
+                      " returned. Currently it is not supported to initialize"
+                      " multiple option definitions for the same option code."
+                      " This will be supported once support for option spaces"
+                      " is implemented");
         } else if (num_defs == 0) {
             opt = OptionPtr(new Option(Option::V4, opt_type,
                                        buf.begin() + offset,
@@ -448,7 +456,8 @@ size_t LibDHCP::unpackVendorOptions6(const uint32_t vendor_id,
     size_t length = buf.size();
 
     // Get the list of option definitions for this particular vendor-id
-    const OptionDefContainer* option_defs = LibDHCP::getVendorOption6Defs(vendor_id);
+    const OptionDefContainer* option_defs =
+        LibDHCP::getVendorOption6Defs(vendor_id);
 
     // Get the search index #1. It allows to search for option definitions
     // using option code. If there's no such vendor-id space, we're out of luck
@@ -460,7 +469,11 @@ size_t LibDHCP::unpackVendorOptions6(const uint32_t vendor_id,
 
     // The buffer being read comprises a set of options, each starting with
     // a two-byte type code and a two-byte length field.
-    while (offset + 4 <= length) {
+    while (offset < length) {
+        if (offset + 4 > length) {
+            isc_throw(OutOfRange, "Option parse failed: truncated header");
+        }
+
         uint16_t opt_type = isc::util::readUint16(&buf[offset], 2);
         offset += 2;
 
@@ -468,13 +481,9 @@ size_t LibDHCP::unpackVendorOptions6(const uint32_t vendor_id,
         offset += 2;
 
         if (offset + opt_len > length) {
-            // @todo: consider throwing exception here.
-
-            // We peeked at the option header of the next option, but discovered
-            // that it would end up beyond buffer end, so the option is
-            // truncated. Hence we can't parse it. Therefore we revert
-            // back by those four bytes (as if we never parsed them).
-            return (offset - 4);
+            isc_throw(OutOfRange, "Option parse failed. Tried to parse "
+                          << offset + opt_len << " bytes from " << length
+                          << "-byte long buffer.");
         }
 
         OptionPtr opt;
@@ -482,20 +491,25 @@ size_t LibDHCP::unpackVendorOptions6(const uint32_t vendor_id,
 
         // If there is a definition for such a vendor option...
         if (idx) {
-            // Get all definitions with the particular option code. Note that option
-            // code is non-unique within this container 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.
+            // Get all definitions with the particular option
+            // code. Note that option code is non-unique within this
+            // container 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);
 
             if (num_defs > 1) {
-                // Multiple options of the same code are not supported right now!
-                isc_throw(isc::Unexpected, "Internal error: multiple option definitions"
-                          " for option type " << opt_type << " returned. Currently it is not"
-                          " supported to initialize multiple option definitions"
-                          " for the same option code. This will be supported once"
+                // Multiple options of the same code are not supported
+                // right now!
+                isc_throw(isc::Unexpected, "Internal error: multiple option"
+                          " definitions for option type " << opt_type <<
+                          " returned. Currently it is not supported to"
+                          " initialize multiple option definitions for the"
+                          " same option code. This will be supported once"
                           " support for option spaces is implemented");
             } else if (num_defs == 1) {
                 // The option definition has been found. Use it to create
@@ -510,7 +524,9 @@ size_t LibDHCP::unpackVendorOptions6(const uint32_t vendor_id,
 
         // This can happen in one of 2 cases:
         // 1. we do not have definitions for that vendor-space
-        // 2. we do have definitions, but that particular option was not defined
+        // 2. we do have definitions, but that particular option was
+        //    not defined
+
         if (!opt) {
             opt = OptionPtr(new Option(Option::V6, opt_type,
                                        buf.begin() + offset,
@@ -532,7 +548,8 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe
     size_t offset = 0;
 
     // Get the list of stdandard option definitions.
-    const OptionDefContainer* option_defs = LibDHCP::getVendorOption4Defs(vendor_id);
+    const OptionDefContainer* option_defs =
+        LibDHCP::getVendorOption4Defs(vendor_id);
     // Get the search index #1. It allows to search for option definitions
     // using option code.
     const OptionDefContainerTypeIndex* idx = NULL;
@@ -543,20 +560,14 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe
     // The buffer being read comprises a set of options, each starting with
     // a one-byte type code and a one-byte length field.
     while (offset + 1 <= buf.size()) {
-
-        // Note that Vendor-Specific info option (RFC3925) has a different option
-        // format than Vendor-Spec info for DHCPv6. (there's additional layer of
-        // data-length
+        // Note that Vendor-Specific info option (RFC3925) has a
+        // different option format than Vendor-Spec info for
+        // DHCPv6. (there's additional layer of data-length
         uint8_t data_len = buf[offset++];
 
         if (offset + data_len > buf.size()) {
             // The option is truncated.
-
-            // We peeked at the data_len, but discovered that it would end up
-            // beyond buffer end, so the data block is truncated. Hence we can't
-            // parse it. Therefore we revert back by one byte (as if we never
-            // parsed it).
-            return (offset - 1);
+            isc_throw(OutOfRange, "Attempt to parse truncated vendor option");
         }
 
         uint8_t offset_end = offset + data_len;
@@ -565,19 +576,14 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe
         while (offset + 1 <= offset_end) {
             uint8_t opt_type = buf[offset++];
 
-            // DHO_END is a special, one octet long option
-            if (opt_type == DHO_END)
-                return (offset); // just return. Don't need to add DHO_END option
-
-            // DHO_PAD is just a padding after DHO_END. Let's continue parsing
-            // in case we receive a message without DHO_END.
-            if (opt_type == DHO_PAD)
-                continue;
+            // No DHO_END or DHO_PAD in vendor options
 
             if (offset + 1 > buf.size()) {
-                // opt_type must be cast to integer so as it is not treated as
-                // unsigned char value (a number is presented in error message).
-                isc_throw(OutOfRange, "Attempt to parse truncated vendor option "
+                // opt_type must be cast to integer so as it is not
+                // treated as unsigned char value (a number is
+                // presented in error message).
+                isc_throw(OutOfRange,
+                          "Attempt to parse truncated vendor option "
                           << static_cast<int>(opt_type));
             }
 
@@ -592,22 +598,27 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe
             opt.reset();
 
             if (idx) {
-                // Get all definitions with the particular option code. Note that option code
-                // is non-unique within this container 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.
+                // Get all definitions with the particular option
+                // code. Note that option code is non-unique within
+                // this container 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);
 
                 if (num_defs > 1) {
-                    // Multiple options of the same code are not supported right now!
-                    isc_throw(isc::Unexpected, "Internal error: multiple option definitions"
-                              " for option type " << static_cast<int>(opt_type)
-                              << " returned. Currently it is not supported to initialize"
-                              << " multiple option definitions for the same option code."
-                              << " This will be supported once support for option spaces"
-                              << " is implemented");
+                    // Multiple options of the same code are not
+                    // supported right now!
+                    isc_throw(isc::Unexpected, "Internal error: multiple"
+                              " option definitions for option type "
+                              << opt_type << " returned. Currently it is"
+                              " not supported to initialize multiple option"
+                              " definitions for the same option code."
+                              " This will be supported once support for"
+                              " option spaces is implemented");
                 } else if (num_defs == 1) {
                     // The option definition has been found. Use it to create
                     // the option instance from the provided buffer chunk.

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

@@ -982,6 +982,16 @@ TEST_F(LibDhcpTest, stdOptionDefs6) {
     // Initialize a vector with the FQDN data.
     std::vector<uint8_t> fqdn_buf(data, data + sizeof(data));
 
+    // Prepare buffer holding a vendor option
+    const char vopt_data[] = {
+        1, 2, 3, 4,                               // enterprise=0x1020304
+        0, 100,                                   // type=100
+	0, 6,                                     // length=6
+	102, 111, 111, 98, 97, 114                // data="foobar"
+    };
+    // Initialize a vector with the suboption data.
+    std::vector<uint8_t> vopt_buf(vopt_data, vopt_data + sizeof(vopt_data));
+
     // The CLIENT_FQDN holds a uint8_t value and FQDN. We have
     // to add the uint8_t value to it and then append the buffer
     // holding some valid FQDN.
@@ -1038,7 +1048,8 @@ TEST_F(LibDhcpTest, stdOptionDefs6) {
                                     vclass_buf.end(),
                                     typeid(OptionVendorClass));
 
-    LibDhcpTest::testStdOptionDefs6(D6O_VENDOR_OPTS, begin, end,
+    LibDhcpTest::testStdOptionDefs6(D6O_VENDOR_OPTS, vopt_buf.begin(),
+				    vopt_buf.end(),
                                     typeid(OptionVendor),
                                     "vendor-opts-space");
 

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -682,13 +682,19 @@ TEST_F(Pkt4Test, unpackMalformed) {
     vector<uint8_t> nolength = orig;
     nolength.resize(orig.size() - 4);
     Pkt4Ptr no_length_pkt(new Pkt4(&nolength[0], nolength.size()));
-    EXPECT_THROW(no_length_pkt->unpack(), OutOfRange);
+    EXPECT_NO_THROW(no_length_pkt->unpack());
+
+    // The unpack() operation doesn't throw but there is no option 12
+    EXPECT_FALSE(no_length_pkt->getOption(12));
 
     // Truncated data is not accepted too but doesn't throw
     vector<uint8_t> shorty = orig;
     shorty.resize(orig.size() - 1);
     Pkt4Ptr too_short_pkt(new Pkt4(&shorty[0], shorty.size()));
     EXPECT_NO_THROW(too_short_pkt->unpack());
+
+    // The unpack() operation doesn't throw but there is no option 12
+    EXPECT_FALSE(no_length_pkt->getOption(12));
 }
 
 // Checks if the code is able to handle a malformed vendor option
@@ -734,7 +740,7 @@ TEST_F(Pkt4Test, unpackVendorMalformed) {
     baddatalen.resize(orig.size() - 5);
     baddatalen[full_len_index] = 10;
     Pkt4Ptr bad_data_len_pkt(new Pkt4(&baddatalen[0], baddatalen.size()));
-    EXPECT_NO_THROW(bad_data_len_pkt->unpack());
+    EXPECT_THROW(bad_data_len_pkt->unpack(), InvalidOptionValue);
 
     // At the exception of END and PAD, a suboption must have a length byte
     vector<uint8_t> nolength = orig;

src/lib/dhcp/tests/pkt6_unittest.cc

@@ -353,8 +353,7 @@ TEST_F(Pkt6Test, unpackMalformed) {
     Pkt6Ptr too_short_pkt(new Pkt6(&shorty[0], shorty.size()));
     EXPECT_THROW(too_short_pkt->unpack(), isc::BadValue);
 
-    // The code should complain about remaining bytes that can't
-    // be parsed.
+    // The code should complain about remaining bytes that can't be parsed.
     Pkt6Ptr trailing_garbage(new Pkt6(&malform1[0], malform1.size()));
     EXPECT_NO_THROW(trailing_garbage->unpack());
 
@@ -392,7 +391,7 @@ TEST_F(Pkt6Test, unpackVendorMalformed) {
     orig.push_back(17);
     orig.push_back(0);
     size_t len_index = orig.size();
-    orig.push_back(20); // length=18
+    orig.push_back(18); // length=18
     orig.push_back(1); // vendor_id=0x1020304
     orig.push_back(2);
     orig.push_back(3);
@@ -406,7 +405,7 @@ TEST_F(Pkt6Test, unpackVendorMalformed) {
     orig.push_back(111);
     orig.push_back(1); // suboption type=0x102
     orig.push_back(2);
-    orig.push_back(0); // suboption lenth=3
+    orig.push_back(0); // suboption length=3
     orig.push_back(3);
     orig.push_back(99); // data="bar'
     orig.push_back(98);
@@ -417,16 +416,23 @@ TEST_F(Pkt6Test, unpackVendorMalformed) {
 
     // Truncated vendor option is not accepted but doesn't throw
     vector<uint8_t> shortv = orig;
-    shortv.resize(orig.size() - 2);
+    shortv[len_index] = 20;
     Pkt6Ptr too_short_vendor_pkt(new Pkt6(&shortv[0], shortv.size()));
     EXPECT_NO_THROW(too_short_vendor_pkt->unpack());
     
-    // Truncated data is not accepted but doesn't throw
+    // Truncated option header is not accepted
+    vector<uint8_t> shorth = orig;
+    shorth.resize(orig.size() - 4);
+    shorth[len_index] = 12;
+    Pkt6Ptr too_short_header_pkt(new Pkt6(&shorth[0], shorth.size()));
+    EXPECT_THROW(too_short_header_pkt->unpack(), OutOfRange);
+
+    // Truncated option data is not accepted
     vector<uint8_t> shorto = orig;
     shorto.resize(orig.size() - 2);
-    shorto[len_index] = 18;
+    shorto[len_index] = 16;
     Pkt6Ptr too_short_option_pkt(new Pkt6(&shorto[0], shorto.size()));
-    EXPECT_NO_THROW(too_short_option_pkt->unpack());
+    EXPECT_THROW(too_short_option_pkt->unpack(), OutOfRange);
 }
 
 // This test verifies that it is possible to specify custom implementation of