[3618] addressed comments: fix docs, improved unpack{,Vendor}Options[46], fixed/improved tests

src/lib/dhcp/libdhcp++.cc

@@ -249,6 +249,7 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
                                size_t* relay_msg_len /* = 0 */) {
     size_t offset = 0;
     size_t length = buf.size();
+    size_t last_offset = 0;
 
     // Get the list of standard option definitions.
     OptionDefContainer option_defs;
@@ -265,7 +266,17 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
 
     // 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) {
+        // Save the current offset for backtracking
+        last_offset = offset;
+
+        // Check if there is room for another option
+        if (offset + 4 > length) {
+            // Still something but smaller than an option
+            return (last_offset);
+        }
+
+        // Parse the option header
         uint16_t opt_type = isc::util::readUint16(&buf[offset], 2);
         offset += 2;
 
@@ -276,12 +287,12 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
             // 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
+            // it. Therefore we revert back by those 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);
+            return (last_offset);
         }
 
         if (opt_type == D6O_RELAY_MSG && relay_msg_offset && relay_msg_len) {
@@ -299,7 +310,7 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
                 // Truncated vendor-option. We expect at least
                 // 4 bytes for the enterprise-id field. Let's roll back
                 // option code + option length (4 bytes) and return.
-                return (offset - 4);
+                return (last_offset);
             }
 
             // Parse this as vendor option
@@ -359,6 +370,7 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
                                const std::string& option_space,
                                isc::dhcp::OptionCollection& options) {
     size_t offset = 0;
+    size_t last_offset = 0;
 
     // Get the list of standard option definitions.
     OptionDefContainer option_defs;
@@ -375,12 +387,20 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
 
     // 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()) {
+    while (offset < buf.size()) {
+        // Save the current offset for backtracking
+        last_offset = offset;
+
+        // Get the option type
         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
+        if (opt_type == DHO_END) {
+            // 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.
+            return (last_offset);
+        }
 
         // DHO_PAD is just a padding after DHO_END. Let's continue parsing
         // in case we receive a message without DHO_END.
@@ -391,12 +411,11 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
             // 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).
+            // it. Therefore we revert back (as if we never parsed it).
             //
             // @note it is the responsability of the caller to throw
             // an exception on partial parsing
-            return (offset - 1);
+            return (last_offset);
         }
 
         uint8_t opt_len =  buf[offset++];
@@ -404,9 +423,8 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
             // 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);
+            // it. Therefore we revert back (as if we never parsed it).
+            return (last_offset);
         }
 
         // Get all definitions with the particular option code. Note
@@ -470,7 +488,8 @@ size_t LibDHCP::unpackVendorOptions6(const uint32_t vendor_id,
     // a two-byte type code and a two-byte length field.
     while (offset < length) {
         if (offset + 4 > length) {
-            isc_throw(OutOfRange, "Option parse failed: truncated header");
+            isc_throw(OutOfRange,
+                      "Vendor option parse failed: truncated header");
         }
 
         uint16_t opt_type = isc::util::readUint16(&buf[offset], 2);
@@ -480,7 +499,7 @@ size_t LibDHCP::unpackVendorOptions6(const uint32_t vendor_id,
         offset += 2;
 
         if (offset + opt_len > length) {
-            isc_throw(OutOfRange, "Option parse failed. Tried to parse "
+            isc_throw(OutOfRange, "Vendor option parse failed. Tried to parse "
                           << offset + opt_len << " bytes from " << length
                           << "-byte long buffer.");
         }
@@ -558,7 +577,7 @@ 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()) {
+    while (offset < 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)
@@ -572,12 +591,12 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe
         uint8_t offset_end = offset + data_len;
 
         // beginning of data-chunk parser
-        while (offset + 1 <= offset_end) {
+        while (offset < offset_end) {
             uint8_t opt_type = buf[offset++];
 
             // No DHO_END or DHO_PAD in vendor options
 
-            if (offset + 1 > buf.size()) {
+            if (offset + 1 > offset_end) {
                 // opt_type must be cast to integer so as it is not
                 // treated as unsigned char value (a number is
                 // presented in error message).
@@ -587,7 +606,7 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe
             }
 
             uint8_t opt_len =  buf[offset++];
-            if (offset + opt_len > buf.size()) {
+            if (offset + opt_len > offset_end) {
                 isc_throw(OutOfRange, "Option parse failed. Tried to parse "
                           << offset + opt_len << " bytes from " << buf.size()
                           << "-byte long buffer.");
@@ -640,6 +659,7 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe
 
         } // end of data-chunk
 
+        break; // end of the vendor block.
     }
     return (offset);
 }

src/lib/dhcp/libdhcp++.h

@@ -137,30 +137,17 @@ public:
     static void packOptions(isc::util::OutputBuffer& buf,
                             const isc::dhcp::OptionCollection& options);
 
-    /// @brief Parses provided buffer as DHCPv4 options and creates Option objects.
-    ///
-    /// Parses provided buffer and stores created Option objects
-    /// in options container.
-    ///
-    /// @param buf Buffer to be parsed.
-    /// @param option_space A name of the option space which holds definitions
-    ///        to be used to parse options in the packets.
-    /// @param options Reference to option container. Options will be
-    ///        put here.
-    /// @return offset to the first byte after the last successfully parsed option
-    static size_t unpackOptions4(const OptionBuffer& buf,
-                                 const std::string& option_space,
-                                 isc::dhcp::OptionCollection& options);
-
-    /// @brief Parses provided buffer as DHCPv6 options and creates Option objects.
-    ///
-    /// Parses provided buffer and stores created Option objects in options
-    /// container. The last two parameters are optional and are used in
-    /// relay parsing. If they are specified, relay-msg option is not created,
-    /// but rather those two parameters are specified to point out where
-    /// the relay-msg option resides and what is its length. This is a performance
-    /// optimization that avoids unnecessary copying of potentially large
-    /// relay-msg option. It is not used for anything, except in the next
+    /// @brief Parses provided buffer as DHCPv6 options and creates
+    /// Option objects.
+    ///
+    /// Parses provided buffer and stores created Option objects in
+    /// options container. The last two parameters are optional and
+    /// are used in relay parsing. If they are specified, relay-msg
+    /// option is not created, but rather those two parameters are
+    /// specified to point out where the relay-msg option resides and
+    /// what is its length. This is a performance optimization that
+    /// avoids unnecessary copying of potentially large relay-msg
+    /// option. It is not used for anything, except in the next
     /// iteration its content will be treated as buffer to be parsed.
     ///
     /// @param buf Buffer to be parsed.
@@ -172,13 +159,38 @@ public:
     ///        offset to beginning of relay_msg option will be stored in it.
     /// @param relay_msg_len reference to a size_t structure. If specified,
     ///        length of the relay_msg option will be stored in it.
-    /// @return offset to the first byte after the last successfully parsed option
+    /// @return offset to the first byte after the last successfully
+    /// parsed option
+    ///
+    /// @note This function throws when an option type is defined more
+    /// than once, and it calls option building routines which can throw.
+    /// Partial parsing does not throw: it is the responsibility of the
+    /// caller to handle this condition.
     static size_t unpackOptions6(const OptionBuffer& buf,
                                  const std::string& option_space,
                                  isc::dhcp::OptionCollection& options,
                                  size_t* relay_msg_offset = 0,
                                  size_t* relay_msg_len = 0);
 
+    /// @brief Parses provided buffer as DHCPv4 options and creates
+    /// Option objects.
+    ///
+    /// Parses provided buffer and stores created Option objects
+    /// in options container.
+    ///
+    /// @param buf Buffer to be parsed.
+    /// @param option_space A name of the option space which holds definitions
+    ///        to be used to parse options in the packets.
+    /// @param options Reference to option container. Options will be
+    ///        put here.
+    /// @return offset to the first byte after the last successfully
+    /// parsed option or the offset of the DHO_END option type.
+    ///
+    /// The unpackOptions6 note applies too.
+    static size_t unpackOptions4(const OptionBuffer& buf,
+                                 const std::string& option_space,
+                                 isc::dhcp::OptionCollection& options);
+
     /// Registers factory method that produces options of specific option types.
     ///
     /// @throw isc::BadValue if provided the type is already registered, has
@@ -215,9 +227,13 @@ public:
     ///
     /// @param vendor_id enterprise-id of the vendor
     /// @param buf Buffer to be parsed.
-    /// @param options Reference to option container. Options will be
+    /// @param options Reference to option container. Suboptions will be
     ///        put here.
-    /// @return offset to the first byte after the last successfully parsed option
+    /// @return offset to the first byte after the last successfully
+    /// parsed suboption
+    ///
+    /// @note unpackVendorOptions6 throws when it fails to parse a suboption
+    /// so the return value is currently always the buffer length.
     static size_t unpackVendorOptions6(const uint32_t vendor_id,
                                        const OptionBuffer& buf,
                                        isc::dhcp::OptionCollection& options);
@@ -230,10 +246,14 @@ public:
     ///
     /// @param vendor_id enterprise-id of the vendor
     /// @param buf Buffer to be parsed.
-    /// @param options Reference to option container. Options will be
+    /// @param options Reference to option container. Suboptions will be
     ///        put here.
-    /// @return offset to the first byte after the last successfully parsed option
-    static size_t unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffer& buf,
+    /// @return offset to the first byte after the last successfully
+    /// parsed suboption
+    ///
+    /// The unpackVendorOptions6 note applies
+    static size_t unpackVendorOptions4(const uint32_t vendor_id,
+                                       const OptionBuffer& buf,
                                        isc::dhcp::OptionCollection& options);
 
 private:

src/lib/dhcp/pkt4.cc

@@ -223,9 +223,10 @@ Pkt4::unpack() {
         offset = callback_(opts_buffer, "dhcp4", options_, NULL, NULL);
     }
 
-    // If offset is not equal to the size, then something is wrong here. We
-    // either parsed past input buffer (bug in our code) or we haven't parsed
-    // everything (received trailing garbage or truncated option).
+    // If offset is not equal to the size and there is no DHO_END,
+    // then something is wrong here. We either parsed past input
+    // buffer (bug in our code) or we haven't parsed everything
+    // (received trailing garbage or truncated option).
     //
     // Invoking Jon Postel's law here: be conservative in what you send, and be
     // liberal in what you accept. There's no easy way to log something from
@@ -233,7 +234,7 @@ Pkt4::unpack() {
     // bytes. We also need to quell compiler warning about unused offset
     // variable.
     //
-    // if (offset != size) {
+    // if ((offset != size) && (opts_buffer[offset] != DHO_END)) {
     //        isc_throw(BadValue, "Received DHCPv6 buffer of size " << size
     //                  << ", were able to parse " << offset << " bytes.");
     // }

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

@@ -949,11 +949,19 @@ TEST_F(LibDhcpTest, stdOptionDefs4) {
         3, 1, 2, 3  // first byte is opaque data length, the rest is opaque data
     };
     std::vector<uint8_t> vivco_buf(vivco_data, vivco_data + sizeof(vivco_data));
+    const char vivsio_data[] = {
+        1, 2, 3, 4, // enterprise id
+        4,          // first byte is vendor block length
+        1, 2, 3, 4  // option type=1 length=2
+    };
+    std::vector<uint8_t> vivsio_buf(vivsio_data, vivsio_data + sizeof(vivsio_data));
+
     LibDhcpTest::testStdOptionDefs4(DHO_VIVCO_SUBOPTIONS, vivco_buf.begin(),
                                     vivco_buf.end(), typeid(OptionVendorClass));
 
-    LibDhcpTest::testStdOptionDefs4(DHO_VIVSO_SUBOPTIONS, begin, end,
-                                    typeid(OptionVendor));
+
+    LibDhcpTest::testStdOptionDefs4(DHO_VIVSO_SUBOPTIONS, vivsio_buf.begin(),
+                                    vivsio_buf.end(), typeid(OptionVendor));
 }
 
 // Test that definitions of standard options have been initialized

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -735,7 +735,7 @@ TEST_F(Pkt4Test, unpackVendorMalformed) {
     Pkt4Ptr success(new Pkt4(&orig[0], orig.size()));
     EXPECT_NO_THROW(success->unpack());
 
-    // Data-len must match but it doesn't throw
+    // Data-len must match
     vector<uint8_t> baddatalen = orig;
     baddatalen.resize(orig.size() - 5);
     baddatalen[full_len_index] = 10;

src/lib/dhcp/tests/pkt6_unittest.cc

@@ -376,6 +376,21 @@ TEST_F(Pkt6Test, unpackMalformed) {
 
     // ... but there should be no option 123 as it was malformed.
     EXPECT_FALSE(trunc_option->getOption(123));
+
+    // Check with truncated length field
+    Pkt6Ptr trunc_length(new Pkt6(&malform2[0], malform2.size() - 1));
+    EXPECT_NO_THROW(trunc_length->unpack());
+    EXPECT_FALSE(trunc_length->getOption(123));
+
+    // Check with missing length field
+    Pkt6Ptr no_length(new Pkt6(&malform2[0], malform2.size() - 2));
+    EXPECT_NO_THROW(no_length->unpack());
+    EXPECT_FALSE(no_length->getOption(123));
+
+    // Check with truncated type field
+    Pkt6Ptr trunc_type(new Pkt6(&malform2[0], malform2.size() - 3));
+    EXPECT_NO_THROW(trunc_type->unpack());
+    EXPECT_FALSE(trunc_type->getOption(123));
 }
 
 // Checks if the code is able to handle a malformed vendor option