[3546] Pkt6::unpack now handles problems with exceptions, not return false

src/bin/dhcp6/dhcp6_messages.mes

@@ -319,8 +319,11 @@ for the failure is appended as an argument of the log message.
 A debug message noting that server has received message with server identifier
 option that not matching server identifier that server is using.
 
-% DHCP6_PACKET_PARSE_FAIL failed to parse incoming packet
-The IPv6 DHCP server has received a packet that it is unable to interpret.
+% DHCP6_PACKET_PARSE_FAIL failed to parse incoming packet: %1
+The DHCPv6 server has received a packet that it is unable to interpret.
+There may be many causes: truncated header, truncated or malformed option,
+trailing padding that contains garbage etc. More information is specified
+as a parameter.
 
 % DHCP6_PACKET_PROCESS_FAIL processing of %1 message received from %2 failed: %3
 This is a general catch-all message indicating that the processing of the

src/bin/dhcp6/dhcp6_srv.cc

@@ -311,9 +311,11 @@ bool Dhcpv6Srv::run() {
         // Unpack the packet information unless the buffer6_receive callouts
         // indicated they did it
         if (!skip_unpack) {
-            if (!query->unpack()) {
+            try {
+                query->unpack();
+            } catch (const std::exception &e) {
                 LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL,
-                          DHCP6_PACKET_PARSE_FAIL);
+                          DHCP6_PACKET_PARSE_FAIL).arg(e.what());
                 continue;
             }
         }

src/bin/perfdhcp/test_control.cc

@@ -1167,6 +1167,9 @@ TestControl::receivePackets(const TestControlSocket& socket) {
                 if ((received > 1) && testDiags('i')) {
                     stats_mgr4_->incrementCounter("multircvd");
                 }
+
+                /// @todo: Add packet exception handling here. Right now any
+                /// malformed packet will cause perfdhcp to abort.
                 pkt4->unpack();
                 processReceivedPacket4(socket, pkt4);
             }
@@ -1185,9 +1188,11 @@ TestControl::receivePackets(const TestControlSocket& socket) {
                 if ((received > 1) && testDiags('i')) {
                     stats_mgr6_->incrementCounter("multircvd");
                 }
-                if (pkt6->unpack()) {
-                    processReceivedPacket6(socket, pkt6);
-                }
+
+                /// @todo: Add packet exception handling here. Right now any
+                /// malformed packet will cause perfdhcp to abort.
+                pkt6->unpack();
+                processReceivedPacket6(socket, pkt6);
             }
         }
     }

src/lib/dhcp/libdhcp++.cc

@@ -239,7 +239,8 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
 
         if (offset + opt_len > length) {
             // @todo: consider throwing exception here.
-            return (offset);
+            // Let's pretend we never parsed those 4 bytes
+            return (offset - 4);
         }
 
         if (opt_type == D6O_RELAY_MSG && relay_msg_offset && relay_msg_len) {

src/lib/dhcp/pkt.cc

@@ -45,7 +45,9 @@ Pkt::Pkt(const uint8_t* buf, uint32_t len, const isc::asiolink::IOAddress& local
      buffer_out_(0)
 {
     data_.resize(len);
-    memcpy(&data_[0], buf, len);
+    if (len) {
+        memcpy(&data_[0], buf, len);
+    }
 }
 
 void

src/lib/dhcp/pkt.h

@@ -152,13 +152,8 @@ public:
     ///
     /// Method will throw exception if packet parsing fails.
     ///
-    /// @todo Pkt4 throws exceptions when unpacking fails, while Pkt6
-    ///       catches exceptions and returns false. We need to unify that
-    ///       behavior one day (most likely using exceptions and turning
-    ///       return type to void).
-    ///
-    /// @return true if unpack was successful
-    virtual bool unpack() = 0;
+    /// @throw tbd
+    virtual void unpack() = 0;
 
     /// @brief Returns reference to output buffer.
     ///

src/lib/dhcp/pkt4.cc

@@ -155,7 +155,7 @@ Pkt4::pack() {
     }
 }
 
-bool
+void
 Pkt4::unpack() {
 
     // input buffer (used during message reception)
@@ -224,8 +224,6 @@ Pkt4::unpack() {
     // @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();
-
-    return (true);
 }
 
 void Pkt4::check() {

src/lib/dhcp/pkt4.h

@@ -93,8 +93,7 @@ public:
     /// be stored in options_ container.
     ///
     /// Method with throw exception if packet parsing fails.
-    /// @return true if unpack was successful
-    virtual bool unpack();
+    virtual void unpack();
 
     /// @brief performs sanity check on a packet.
     ///

src/lib/dhcp/pkt6.cc

@@ -253,7 +253,7 @@ Pkt6::packTCP() {
               "not implemented yet.");
 }
 
-bool
+void
 Pkt6::unpack() {
     switch (proto_) {
     case UDP:
@@ -263,15 +263,13 @@ Pkt6::unpack() {
     default:
         isc_throw(BadValue, "Invalid protocol specified (non-TCP, non-UDP)");
     }
-    return (false); // never happens
 }
 
-bool
+void
 Pkt6::unpackUDP() {
     if (data_.size() < 4) {
-        // @todo: throw exception here informing that packet is truncated
-        // once we turn this function to void.
-        return (false);
+        isc_throw(BadValue, "Received truncated UDP DHCPv6 packet of size "
+                  << data_.size() << ", DHCPv6 header alone has 4 bytes.");
     }
     msg_type_ = data_[0];
     switch (msg_type_) {
@@ -295,12 +293,14 @@ Pkt6::unpackUDP() {
     }
 }
 
-bool
+void
 Pkt6::unpackMsg(OptionBuffer::const_iterator begin,
                 OptionBuffer::const_iterator end) {
-    if (std::distance(begin, end) < 4) {
+    size_t size = std::distance(begin, end);
+    if (size < 4) {
         // truncated message (less than 4 bytes)
-        return (false);
+        isc_throw(BadValue, "Received truncated UDP DHCPv6 packet of size "
+                  << data_.size() << ", DHCPv6 header alone has 4 bytes.");
     }
 
     msg_type_ = *begin++;
@@ -309,27 +309,33 @@ Pkt6::unpackMsg(OptionBuffer::const_iterator begin,
         ((*begin++) << 8) + (*begin++);
     transid_ = transid_ & 0xffffff;
 
-    try {
-        OptionBuffer opt_buffer(begin, end);
+    size -= sizeof(uint32_t); // We just parsed 4 bytes header
 
-        // If custom option parsing function has been set, use this function
-        // to parse options. Otherwise, use standard function from libdhcp.
-        if (callback_.empty()) {
-            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);
+    OptionBuffer opt_buffer(begin, end);
+
+    // If custom option parsing function has been set, use this function
+    // to parse options. Otherwise, use standard function from libdhcp.
+    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.");
         }
-    } catch (const Exception& e) {
-        // @todo: throw exception here once we turn this function to void.
-        return (false);
+    } 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);
     }
-    return (true);
 }
 
-bool
+void
 Pkt6::unpackRelayMsg() {
 
     // we use offset + bufsize, because we want to avoid creating unnecessary
@@ -355,67 +361,60 @@ Pkt6::unpackRelayMsg() {
         offset += isc::asiolink::V6ADDRESS_LEN;
         bufsize -= DHCPV6_RELAY_HDR_LEN; // 34 bytes (1+1+16+16)
 
-        try {
-            // parse the rest as options
-            OptionBuffer opt_buffer(&data_[offset], &data_[offset+bufsize]);
-
-            // If custom option parsing function has been set, use this function
-            // to parse options. Otherwise, use standard function from libdhcp.
-            if (callback_.empty()) {
-                LibDHCP::unpackOptions6(opt_buffer, "dhcp6", relay.options_,
-                                        &relay_msg_offset, &relay_msg_len);
-            } else {
-                callback_(opt_buffer, "dhcp6", relay.options_,
-                          &relay_msg_offset, &relay_msg_len);
-            }
+        // parse the rest as options
+        OptionBuffer opt_buffer(&data_[offset], &data_[offset+bufsize]);
 
-            /// @todo: check that each option appears at most once
-            //relay.interface_id_ = options->getOption(D6O_INTERFACE_ID);
-            //relay.subscriber_id_ = options->getOption(D6O_SUBSCRIBER_ID);
-            //relay.remote_id_ = options->getOption(D6O_REMOTE_ID);
+        // If custom option parsing function has been set, use this function
+        // to parse options. Otherwise, use standard function from libdhcp.
+        if (callback_.empty()) {
+            LibDHCP::unpackOptions6(opt_buffer, "dhcp6", relay.options_,
+                                    &relay_msg_offset, &relay_msg_len);
+        } else {
+            callback_(opt_buffer, "dhcp6", relay.options_,
+                      &relay_msg_offset, &relay_msg_len);
+        }
 
-            if (relay_msg_offset == 0 || relay_msg_len == 0) {
-                isc_throw(BadValue, "Mandatory relay-msg option missing");
-            }
+        /// @todo: check that each option appears at most once
+        //relay.interface_id_ = options->getOption(D6O_INTERFACE_ID);
+        //relay.subscriber_id_ = options->getOption(D6O_SUBSCRIBER_ID);
+        //relay.remote_id_ = options->getOption(D6O_REMOTE_ID);
 
-            // store relay information parsed so far
-            addRelayInfo(relay);
+        if (relay_msg_offset == 0 || relay_msg_len == 0) {
+            isc_throw(BadValue, "Mandatory relay-msg option missing");
+        }
 
-            /// @todo: implement ERO here
+        // store relay information parsed so far
+        addRelayInfo(relay);
 
-            if (relay_msg_len >= bufsize) {
-                // length of the relay_msg option extends beyond end of the message
-                isc_throw(Unexpected, "Relay-msg option is truncated.");
-                return false;
-            }
-            uint8_t inner_type = data_[offset + relay_msg_offset];
-            offset += relay_msg_offset; // offset is relative
-            bufsize = relay_msg_len;    // length is absolute
-
-            if ( (inner_type != DHCPV6_RELAY_FORW) &&
-                 (inner_type != DHCPV6_RELAY_REPL)) {
-                // Ok, the inner message is not encapsulated, let's decode it
-                // directly
-                return (unpackMsg(data_.begin() + offset, data_.begin() + offset
-                                  + relay_msg_len));
-            }
+        /// @todo: implement ERO here
 
-            // Oh well, there's inner relay-forw or relay-repl inside. Let's
-            // unpack it as well. The next loop iteration will take care
-            // of that.
-        } catch (const Exception& e) {
-            /// @todo: throw exception here once we turn this function to void.
-            return (false);
+        if (relay_msg_len >= bufsize) {
+            // length of the relay_msg option extends beyond end of the message
+            isc_throw(Unexpected, "Relay-msg option is truncated.");
         }
+        uint8_t inner_type = data_[offset + relay_msg_offset];
+        offset += relay_msg_offset; // offset is relative
+        bufsize = relay_msg_len;    // length is absolute
+
+        if ( (inner_type != DHCPV6_RELAY_FORW) &&
+             (inner_type != DHCPV6_RELAY_REPL)) {
+            // Ok, the inner message is not encapsulated, let's decode it
+            // directly
+            return (unpackMsg(data_.begin() + offset, data_.begin() + offset
+                              + relay_msg_len));
+        }
+
+        // Oh well, there's inner relay-forw or relay-repl inside. Let's
+        // unpack it as well. The next loop iteration will take care
+        // of that.
     }
 
     if ( (offset == data_.size()) && (bufsize == 0) ) {
         // message has been parsed completely
-        return (true);
+        return;
     }
 
     /// @todo: log here that there are additional unparsed bytes
-    return (true);
 }
 
 void
@@ -428,7 +427,7 @@ Pkt6::addRelayInfo(const RelayInfo& relay) {
     relay_info_.push_back(relay);
 }
 
-bool
+void
 Pkt6::unpackTCP() {
     isc_throw(Unexpected, "DHCPv6 over TCP (bulk leasequery and failover) "
               "not implemented yet.");

src/lib/dhcp/pkt6.h

@@ -142,8 +142,8 @@ public:
     /// This method calls appropriate dispatch function (unpackUDP or
     /// unpackTCP).
     ///
-    /// @return true if parsing was successful
-    virtual bool unpack();
+    /// @throw tbd
+    virtual void unpack();
 
     /// @brief Returns protocol of this packet (UDP or TCP).
     ///
@@ -300,8 +300,8 @@ protected:
     ///
     /// @todo This function is not implemented yet.
     ///
-    /// @return true, if build was successful
-    bool unpackTCP();
+    /// @throw tbd
+    void unpackTCP();
 
     /// @brief Parses on-wire form of UDP DHCPv6 packet.
     ///
@@ -310,8 +310,8 @@ protected:
     /// Will create a collection of option objects that will
     /// be stored in options_ container.
     ///
-    /// @return true, if build was successful
-    bool unpackUDP();
+    /// @throw tbd
+    void unpackUDP();
 
     /// @brief Unpacks direct (non-relayed) message.
     ///
@@ -321,8 +321,8 @@ protected:
     ///
     /// @param begin start of the buffer
     /// @param end end of the buffer
-    /// @return true if parsing was successful and there are no leftover bytes
-    bool unpackMsg(OptionBuffer::const_iterator begin,
+    /// @throw tbd
+    void unpackMsg(OptionBuffer::const_iterator begin,
                    OptionBuffer::const_iterator end);
 
     /// @brief Unpacks relayed message (RELAY-FORW or RELAY-REPL).
@@ -331,8 +331,8 @@ protected:
     /// is detected to be relay-message. It goes iteratively over
     /// all relays (if there are multiple encapsulation levels).
     ///
-    /// @return true if parsing was successful
-    bool unpackRelayMsg();
+    /// @throw tbd
+    void unpackRelayMsg();
 
     /// @brief Calculates overhead introduced in specified relay.
     ///

src/lib/dhcp/tests/pkt6_unittest.cc

@@ -279,7 +279,7 @@ Pkt6* capture2() {
 TEST_F(Pkt6Test, unpack_solicit1) {
     scoped_ptr<Pkt6> sol(capture1());
 
-    ASSERT_EQ(true, sol->unpack());
+    ASSERT_NO_THROW(sol->unpack());
 
     // Check for length
     EXPECT_EQ(98, sol->len() );
@@ -305,7 +305,7 @@ TEST_F(Pkt6Test, packUnpack) {
     Pkt6Ptr clone = packAndClone();
 
     // Now recreate options list
-    EXPECT_TRUE(clone->unpack());
+    ASSERT_NO_THROW(clone->unpack());
 
     // transid, message-type should be the same as before
     EXPECT_EQ(0x020304, clone->getTransid());
@@ -317,6 +317,65 @@ TEST_F(Pkt6Test, packUnpack) {
     EXPECT_FALSE(clone->getOption(4));
 }
 
+// Checks if the code is able to handle malformed packet
+TEST_F(Pkt6Test, unpackMalformed) {
+    // Get a packet. We're really interested in its on-wire representation only.
+    scoped_ptr<Pkt6> donor(capture1());
+
+    // That's our original content. It should be sane.
+    OptionBuffer orig = donor->data_;
+
+    Pkt6Ptr success(new Pkt6(&orig[0], orig.size()));
+    EXPECT_NO_THROW(success->unpack());
+
+    // Insert trailing garbage.
+    OptionBuffer malform1 = orig;
+    malform1.push_back(123);
+
+    // Let's check a truncated packet. Moderately sane DHCPv6 packet should at
+    // least have four bytes header. Zero bytes is definitely not a valid one.
+    OptionBuffer empty(1); // Let's allocate one byte, so we won't be
+                           // dereferencing and empty buffer.
+
+    Pkt6Ptr empty_pkt(new Pkt6(&empty[0], 0));
+    EXPECT_THROW(empty_pkt->unpack(), isc::BadValue);
+
+    // Neither is 3 bytes long.
+    OptionBuffer shorty;
+    shorty.push_back(DHCPV6_SOLICIT);
+    shorty.push_back(1);
+    shorty.push_back(2);
+    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.
+    Pkt6Ptr trailing_garbage(new Pkt6(&malform1[0], malform1.size()));
+    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.
+
+    // Add an option that is truncated
+    OptionBuffer malform2 = orig;
+    malform2.push_back(0);
+    malform2.push_back(123); // 0, 123 - option code = 123
+    malform2.push_back(0);
+    malform2.push_back(1);   // 0, 1 - option length = 1
+    // Option content would go here, but it's missing
+
+    Pkt6Ptr trunc_option(new Pkt6(&malform2[0], malform2.size()));
+
+    // The unpack() operation should succeed...
+    EXPECT_NO_THROW(trunc_option->unpack());
+
+    // ... 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
 // the option parsing algorithm by installing a callback function.
 TEST_F(Pkt6Test, packUnpackWithCallback) {
@@ -334,7 +393,7 @@ TEST_F(Pkt6Test, packUnpackWithCallback) {
     ASSERT_FALSE(cb.executed_);
 
     // Now recreate options list
-    EXPECT_TRUE(clone->unpack());
+    ASSERT_NO_THROW(clone->unpack());
 
     // An object which holds a callback should now have a flag set which
     // indicates that callback has been called.
@@ -354,7 +413,7 @@ TEST_F(Pkt6Test, packUnpackWithCallback) {
     // By setting the callback to NULL we effectively uninstall the callback.
     clone->setCallback(NULL);
     // Do another unpack.
-    EXPECT_TRUE(clone->unpack());
+    ASSERT_NO_THROW(clone->unpack());
     // Callback should not be executed.
     EXPECT_FALSE(cb.executed_);
 }
@@ -644,7 +703,7 @@ TEST_F(Pkt6Test, relayPack) {
                                     parent->getBuffer().getLength()));
 
     // Now recreate options list
-    EXPECT_TRUE( clone->unpack() );
+    EXPECT_NO_THROW( clone->unpack() );
 
     // transid, message-type should be the same as before
     EXPECT_EQ(parent->getTransid(), parent->getTransid());