[3546] Another changes after review:

 - (offset - 4) explained better
 - Modified Dhcpv6Srv::unpackOptions()
 - setHWAddrMember() mac_addr renamed (this time for real)
 - Commented out unused variables
 - RelayInfo now uses DEFAULT_ADDRESS6 in ctor
 - LibDHCP::unpackOptions6 and callback now used uniformly
 - Clarified that ERO is Echo Request Option (RFC4994)
 - unpackOptions4 in libdhcp and Dhcp4Srv no longer throw when
   truncated option is received.

src/bin/dhcp4/dhcp4_srv.cc

@@ -1839,9 +1839,16 @@ Dhcpv4Srv::unpackOptions(const OptionBuffer& buf,
 
         uint8_t opt_len =  buf[offset++];
         if (offset + opt_len > buf.size()) {
-            isc_throw(OutOfRange, "Option parse failed. Tried to parse "
-                      << offset + opt_len << " bytes from " << buf.size()
-                      << "-byte long buffer.");
+
+            // 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

src/bin/dhcp6/dhcp6_srv.cc

@@ -2470,7 +2470,12 @@ Dhcpv6Srv::unpackOptions(const OptionBuffer& buf,
 
         if (offset + opt_len > length) {
             // @todo: consider throwing exception here.
-            return (offset);
+
+            // 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
+            // by by those four bytes (as if we never parsed them).
+            return (offset - 4);
         }
 
         if (opt_type == D6O_RELAY_MSG && relay_msg_offset && relay_msg_len) {

src/lib/dhcp/libdhcp++.cc

@@ -239,7 +239,11 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
 
         if (offset + opt_len > length) {
             // @todo: consider throwing exception here.
-            // Let's pretend we never parsed those 4 bytes
+
+            // 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);
         }
 
@@ -352,9 +356,16 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
 
         uint8_t opt_len =  buf[offset++];
         if (offset + opt_len > buf.size()) {
-            isc_throw(OutOfRange, "Option parse failed. Tried to parse "
-                      << offset + opt_len << " bytes from " << buf.size()
-                      << "-byte long buffer.");
+
+            // 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

src/lib/dhcp/pkt.cc

@@ -116,10 +116,10 @@ Pkt::setRemoteHWAddr(const HWAddrPtr& hw_addr) {
 
 void
 Pkt::setHWAddrMember(const uint8_t htype, const uint8_t,
-                      const std::vector<uint8_t>& mac_addr,
-                      HWAddrPtr& hw_addr) {
+                      const std::vector<uint8_t>& hw_addr,
+                      HWAddrPtr& storage) {
 
-    hw_addr.reset(new HWAddr(mac_addr, htype));
+    storage.reset(new HWAddr(hw_addr, htype));
 }
 
 HWAddrPtr

src/lib/dhcp/pkt.h

@@ -56,31 +56,31 @@ public:
 
     /// Extracted from DUID-LL or DUID-LLT (not 100% reliable as the client
     /// can send fake DUID).
-    static const uint32_t MAC_SOURCE_DUID = 0x0002;
+    //static const uint32_t MAC_SOURCE_DUID = 0x0002;
 
     /// Extracted from IPv6 link-local address. Not 100% reliable, as the
     /// client can use different IID other than EUI-64, e.g. Windows supports
     /// RFC4941 and uses random values instead of EUI-64.
-    static const uint32_t MAC_SOURCE_IPV6_LINK_LOCAL = 0x0004;
+    //static const uint32_t MAC_SOURCE_IPV6_LINK_LOCAL = 0x0004;
 
     /// Get it from RFC6939 option. (A relay agent can insert client link layer
     /// address option). Note that a skilled attacker can fake that by sending
     /// his request relayed, so the legitimate relay will think it's a second
     /// relay.
-    static const uint32_t MAC_SOURCE_CLIENT_ADDR_RELAY_OPTION = 0x0008;
+    //static const uint32_t MAC_SOURCE_CLIENT_ADDR_RELAY_OPTION = 0x0008;
 
     /// A relay can insert remote-id. In some deployments it contains a MAC
     /// address (RFC4649).
-    static const uint32_t MAC_SOURCE_REMOTE_ID = 0x0010;
+    //static const uint32_t MAC_SOURCE_REMOTE_ID = 0x0010;
 
     /// A relay can insert a subscriber-id option. In some deployments it
     /// contains a MAC address (RFC4580).
-    static const uint32_t MAC_SOURCE_SUBSCRIBER_ID = 0x0020;
+    //static const uint32_t MAC_SOURCE_SUBSCRIBER_ID = 0x0020;
 
     /// A CMTS (acting as DHCP relay agent) that supports DOCSIS standard
     /// can insert DOCSIS options that contain client's MAC address.
     /// Client in this context would be a cable modem.
-    static const uint32_t MAC_SOURCE_DOCSIS_OPTIONS = 0x0040;
+    //static const uint32_t MAC_SOURCE_DOCSIS_OPTIONS = 0x0040;
 
     /// @}
 
@@ -572,13 +572,13 @@ private:
     ///
     /// @param htype hardware type.
     /// @param hlen hardware length.
-    /// @param mac_addr pointer to actual hardware address.
-    /// @param [out] hw_addr pointer to a class member to be modified.
+    /// @param hw_addr pointer to actual hardware address.
+    /// @param [out] storage pointer to a class member to be modified.
     ///
     /// @trow isc::OutOfRange if invalid HW address specified.
     virtual void setHWAddrMember(const uint8_t htype, const uint8_t hlen,
-                                 const std::vector<uint8_t>& mac_addr,
-                                 HWAddrPtr& hw_addr);
+                                 const std::vector<uint8_t>& hw_addr,
+                                 HWAddrPtr& storage);
 };
 
 }; // namespace isc::dhcp

src/lib/dhcp/pkt4.cc

@@ -210,17 +210,34 @@ Pkt4::unpack() {
 
     // Use readVector because a function which parses option requires
     // a vector as an input.
+    size_t offset;
     buffer_in.readVector(opts_buffer, opts_len);
     if (callback_.empty()) {
-        LibDHCP::unpackOptions4(opts_buffer, "dhcp4", options_);
+        offset = LibDHCP::unpackOptions4(opts_buffer, "dhcp4", options_);
     } else {
         // The last two arguments are set to NULL because they are
         // specific to DHCPv6 options parsing. They are unused for
         // DHCPv4 case. In DHCPv6 case they hold are the relay message
         // offset and length.
-        callback_(opts_buffer, "dhcp4", options_, NULL, NULL);
+        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).
+    //
+    // 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
+    // libdhcp++ library, so we just choose to be silent about remaining
+    // bytes. We also need to quell compiler warning about unused offset
+    // variable.
+    //
+    // if (offset != size) {
+    //        isc_throw(BadValue, "Received DHCPv6 buffer of size " << size
+    //                  << ", were able to parse " << offset << " bytes.");
+    // }
+    (void)offset;
+
     // @todo check will need to be called separately, so hooks can be called
     // after the packet is parsed, but before its content is verified
     check();

src/lib/dhcp/pkt6.cc

@@ -31,10 +31,8 @@ namespace isc {
 namespace dhcp {
 
 Pkt6::RelayInfo::RelayInfo()
-    :msg_type_(0), hop_count_(0), linkaddr_("::"), peeraddr_("::"),
-    relay_msg_len_(0) {
-    // interface_id_, subscriber_id_, remote_id_ initialized to NULL
-    // echo_options_ initialized to empty collection
+    :msg_type_(0), hop_count_(0), linkaddr_(DEFAULT_ADDRESS6),
+    peeraddr_(DEFAULT_ADDRESS6), relay_msg_len_(0) {
 }
 
 Pkt6::Pkt6(const uint8_t* buf, uint32_t buf_len, DHCPv6Proto proto /* = UDP */)
@@ -315,24 +313,31 @@ Pkt6::unpackMsg(OptionBuffer::const_iterator begin,
 
     // If custom option parsing function has been set, use this function
     // to parse options. Otherwise, use standard function from libdhcp.
+    size_t offset;
     if (callback_.empty()) {
-        size_t offset = LibDHCP::unpackOptions6(opt_buffer, "dhcp6", options_);
-        if (offset != size) {
-            // Something is wrong here. We either parsed past input buffer
-            // (impossible, our code is bug-free ;) 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.
-            //isc_throw(BadValue, "Received DHCPv6 buffer of size " << size
-            //          << ", were able to parse " << offset << " bytes.");
-        }
+        offset = LibDHCP::unpackOptions6(opt_buffer, "dhcp6", options_);
     } else {
         // The last two arguments hold the DHCPv6 Relay message offset and
         // length. Setting them to NULL because we are dealing with the
         // not-relayed message.
-        callback_(opt_buffer, "dhcp6", options_, NULL, NULL);
+        offset = callback_(opt_buffer, "dhcp6", 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).
+    //
+    // 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
+    // libdhcp++ library, so we just choose to be silent about remaining
+    // bytes. We also need to quell compiler warning about unused offset
+    // variable.
+    //
+    // if (offset != size) {
+    //        isc_throw(BadValue, "Received DHCPv6 buffer of size " << size
+    //                  << ", were able to parse " << offset << " bytes.");
+    // }
+    (void)offset;
 }
 
 void
@@ -386,7 +391,7 @@ Pkt6::unpackRelayMsg() {
         // store relay information parsed so far
         addRelayInfo(relay);
 
-        /// @todo: implement ERO here
+        /// @todo: implement ERO (Echo Request Option, RFC 4994) here
 
         if (relay_msg_len >= bufsize) {
             // length of the relay_msg option extends beyond end of the message

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -856,7 +856,7 @@ TEST_F(Pkt4Test, clientClasses) {
 TEST_F(Pkt4Test, getMAC) {
     Pkt4 pkt(DHCPOFFER, 1234);
 
-    // DHCPv6 packet by default doens't have MAC address specified.
+    // DHCPv4 packet by default doens't have MAC address specified.
     EXPECT_FALSE(pkt.getMAC(Pkt::MAC_SOURCE_ANY));
     EXPECT_FALSE(pkt.getMAC(Pkt::MAC_SOURCE_RAW));
 

src/lib/dhcp/tests/pkt6_unittest.cc

@@ -354,7 +354,8 @@ TEST_F(Pkt6Test, unpackMalformed) {
     EXPECT_NO_THROW(trailing_garbage->unpack());
 
     // A strict approach would assume the code will reject the whole packet,
-    // but we decided to follow Jon Postel's law.
+    // but we decided to follow Jon Postel's law and be silent about
+    // received malformed or truncated options.
 
     // Add an option that is truncated
     OptionBuffer malform2 = orig;
@@ -371,9 +372,6 @@ TEST_F(Pkt6Test, unpackMalformed) {
 
     // ... but there should be no option 123 as it was malformed.
     EXPECT_FALSE(trunc_option->getOption(123));
-
-    // A strict approach would assume the code will reject the whole packet,
-    // but we decided to follow Jon Postel's law.
 }
 
 // This test verifies that it is possible to specify custom implementation of