[master] Merge branch 'trac4206' (empty DUID/client-id fix)

src/bin/dhcp4/dhcp4_messages.mes

@@ -457,6 +457,11 @@ This error message is issued when preparing an on-wire format of the packet
 has failed. The first argument identifies the client and the DHCP transaction.
 The second argument includes the error string.
 
+% DHCP4_PACKET_PROCESS_EXCEPTION exception occurred during packet processing: %1
+This error message indicates that an exception was raised during packet processing
+that was not caught by other, more specific exception handlers. This packet will
+be dropped and the server will continue operation.
+
 % DHCP4_PACKET_RECEIVED %1: %2 (type %3) received from %4 to %5 on interface %6
 A debug message noting that the server has received the specified type of
 packet on the specified interface. The first argument specifies the

src/bin/dhcp4/dhcp4_srv.cc

@@ -427,6 +427,8 @@ Dhcpv4Srv::run() {
         Pkt4Ptr query;
         Pkt4Ptr rsp;
 
+        try {
+
         try {
             uint32_t timeout = 1000;
             LOG_DEBUG(packet4_logger, DBG_DHCP4_DETAIL, DHCP4_BUFFER_WAIT).arg(timeout);
@@ -787,6 +789,19 @@ Dhcpv4Srv::run() {
                 .arg(rsp->getLabel())
                 .arg(e.what());
         }
+
+        } catch (const std::exception& e) {
+            // General catch-all exception that are not caught by more specific
+            // catches. This one is for exceptions derived from std::exception.
+            LOG_ERROR(packet4_logger, DHCP4_PACKET_PROCESS_EXCEPTION)
+                .arg(e.what());
+        } catch (...) {
+            // General catch-all exception that are not caught by more specific
+            // catches. This one is for other exceptions, not derived from
+            // std::exception.
+            LOG_ERROR(packet4_logger, DHCP4_PACKET_PROCESS_EXCEPTION)
+                .arg("an unknown exception not derived from std::exception");
+        }
     }
 
     return (true);

src/bin/dhcp4/tests/dhcp4_client.cc

@@ -212,6 +212,17 @@ Dhcp4Client::createMsg(const uint8_t msg_type) {
     return (msg);
 }
 
+void
+Dhcp4Client::appendExtraOptions() {
+    // If there are any custom options specified, add them all to the message.
+    if (!extra_options_.empty()) {
+        for (OptionCollection::iterator opt = extra_options_.begin();
+             opt != extra_options_.end(); ++opt) {
+            context_.query_->addOption(opt->second);
+        }
+    }
+}
+
 void
 Dhcp4Client::doDiscover(const boost::shared_ptr<IOAddress>& requested_addr) {
     context_.query_ = createMsg(DHCPDISCOVER);
@@ -228,6 +239,8 @@ Dhcp4Client::doDiscover(const boost::shared_ptr<IOAddress>& requested_addr) {
     if (ciaddr_.isSpecified()) {
         context_.query_->setCiaddr(ciaddr_.get());
     }
+    appendExtraOptions();
+
     // Send the message to the server.
     sendMsg(context_.query_);
     // Expect response.
@@ -485,6 +498,11 @@ Dhcp4Client::setHWAddress(const std::string& hwaddr_str) {
     }
 }
 
+void
+Dhcp4Client::addExtraOption(const OptionPtr& opt) {
+    extra_options_.insert(std::make_pair(opt->getType(), opt));
+}
+
 } // end of namespace isc::dhcp::test
 } // end of namespace isc::dhcp
 } // end of namespace isc

src/bin/dhcp4/tests/dhcp4_client.h

@@ -355,7 +355,16 @@ public:
     /// in the client's messages.
     isc::util::OptionalValue<asiolink::IOAddress> ciaddr_;
 
+    /// @brief Adds extra option (an option the client will always send)
+    ///
+    /// @param opt additional option to be sent
+    void addExtraOption(const OptionPtr& opt);
+
 private:
+    /// @brief Appends extra options, previously added with addExtraOption()
+    ///
+    /// @brief Copies options from extra_options_ into outgoing message
+    void appendExtraOptions();
 
     /// @brief Creates and adds Requested IP Address option to the client's
     /// query.
@@ -467,6 +476,8 @@ private:
     /// @brief Enable relaying messages to the server.
     bool use_relay_;
 
+    /// @brief Extra options the client will send.
+    OptionCollection extra_options_;
 };
 
 } // end of namespace isc::dhcp::test

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -19,6 +19,7 @@
 #include <cc/command_interpreter.h>
 #include <config/command_mgr.h>
 #include <dhcp4/tests/dhcp4_test_utils.h>
+#include <dhcp4/tests/dhcp4_client.h>
 #include <dhcp/tests/pkt_captures.h>
 #include <dhcp/dhcp4.h>
 #include <dhcp/iface_mgr.h>
@@ -2659,6 +2660,31 @@ TEST_F(Dhcpv4SrvTest, statisticsUnknownRcvd) {
     EXPECT_EQ(1, drop_stat->getInteger().first);
 }
 
+// This test verifies that the server is able to handle an empty client-id
+// in incoming client message.
+TEST_F(Dhcpv4SrvTest, emptyClientId) {
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
+    Dhcp4Client client;
+
+    EXPECT_NO_THROW(configure(CONFIGS[0], *client.getServer()));
+
+    // Tell the client to not send client-id on its own.
+    client.includeClientId("");
+
+    // Instead, tell him to send this extra option, which happens to be
+    // an empty client-id.
+    OptionPtr empty_client_id(new Option(Option::V4, DHO_DHCP_CLIENT_IDENTIFIER));
+    client.addExtraOption(empty_client_id);
+
+    // Let's check whether the server is able to process this packet without
+    // throwing any exceptions. We don't care whether the server sent any
+    // responses or not. The goal is to check that the server didn't throw
+    // any exceptions.
+    EXPECT_NO_THROW(client.doDORA());
+}
+
+
 /// @todo: Implement proper tests for MySQL lease/host database,
 ///        see ticket #4214.
 

src/bin/dhcp6/dhcp6_messages.mes

@@ -425,6 +425,11 @@ because packets of this type must be sent to multicast. The first argument
 specifies the client and transaction identification information, the
 second argument specifies packet type.
 
+% DHCP6_PACKET_PROCESS_EXCEPTION exception occurred during packet processing: %1
+This error message indicates that an exception was raised during packet processing
+that was not caught by other, more specific exception handlers. This packet will
+be dropped and the server will continue operation.
+
 % 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
 specified packet type from the indicated address failed.  The reason is given in the

src/bin/dhcp6/dhcp6_srv.cc

@@ -308,6 +308,8 @@ bool Dhcpv6Srv::run() {
         Pkt6Ptr query;
         Pkt6Ptr rsp;
 
+        try {
+
         try {
             uint32_t timeout = 1000;
             LOG_DEBUG(packet6_logger, DBG_DHCP6_DETAIL, DHCP6_BUFFER_WAIT).arg(timeout);
@@ -701,6 +703,18 @@ bool Dhcpv6Srv::run() {
                     .arg(e.what());
             }
         }
+
+        } catch (const std::exception& e) {
+            // General catch-all standard exceptions that are not caught by more
+            // specific catches.
+            LOG_ERROR(packet6_logger, DHCP6_PACKET_PROCESS_EXCEPTION)
+                .arg(e.what());
+        } catch (...) {
+            // General catch-all non-standard exception that are not caught
+            // by more specific catches.
+            LOG_ERROR(packet6_logger, DHCP6_PACKET_PROCESS_EXCEPTION)
+                .arg("an unknown exception not derived from std::exception");
+        }
     }
 
     return (true);

src/bin/dhcp6/tests/dhcp6_client.cc

@@ -369,6 +369,7 @@ Dhcp6Client::createMsg(const uint8_t msg_type) {
     if (use_client_id_) {
         msg->addOption(getClientId());
     }
+
     if (use_oro_) {
         OptionUint16ArrayPtr oro(new OptionUint16Array(Option::V6, D6O_ORO));
         oro->setValues(oro_);
@@ -376,6 +377,14 @@ Dhcp6Client::createMsg(const uint8_t msg_type) {
         msg->addOption(oro);
     };
 
+    // If there are any custom options specified, add them all to the message.
+    if (!extra_options_.empty()) {
+        for (OptionCollection::iterator opt = extra_options_.begin();
+             opt != extra_options_.end(); ++opt) {
+            msg->addOption(opt->second);
+        }
+    }
+
     return (msg);
 }
 
@@ -758,7 +767,10 @@ Dhcp6Client::useFQDN(const uint8_t flags, const std::string& fqdn_name,
     fqdn_.reset(new Option6ClientFqdn(flags, fqdn_name, fqdn_type));
 }
 
-
+void
+Dhcp6Client::addExtraOption(const OptionPtr& opt) {
+    extra_options_.insert(std::make_pair(opt->getType(), opt));
+}
 
 } // end of namespace isc::dhcp::test
 } // end of namespace isc::dhcp

src/bin/dhcp6/tests/dhcp6_client.h

@@ -579,6 +579,11 @@ public:
     void
     generateIAFromLeases(const Pkt6Ptr& query);
 
+    /// @brief Adds extra option (an option the client will always send)
+    ///
+    /// @param opt additional option to be sent
+    void addExtraOption(const OptionPtr& opt);
+
 private:
 
     /// @brief Applies the new leases for the client.
@@ -752,6 +757,9 @@ private:
     /// @brief forced (Overridden) value of the server-id option (may be NULL)
     OptionPtr forced_server_id_;
 
+    /// @brief Extra options the client will send.
+    OptionCollection extra_options_;
+
     /// @brief FQDN requested by the client.
     Option6ClientFqdnPtr fqdn_;
 

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

@@ -2734,6 +2734,77 @@ TEST_F(Dhcpv6SrvTest, receiveParseFailedStat) {
     EXPECT_EQ(1, recv_drop->getInteger().first);
 }
 
+// This test verifies that the server is able to handle an empty DUID (client-id)
+// in incoming client message.
+TEST_F(Dhcpv6SrvTest, emptyClientId) {
+    Dhcp6Client client;
+
+    // The following configuration enables RSOO options: 110 and 120.
+    // It also configures the server with option 120 which should
+    // "override" the option 120 sent in the RSOO by the relay.
+    string config =
+        "{"
+        "    \"preferred-lifetime\": 3000,"
+        "    \"rebind-timer\": 2000, "
+        "    \"renew-timer\": 1000, "
+        "    \"subnet6\": [ { "
+        "        \"pools\": [ { \"pool\": \"2001:db8::/64\" } ],"
+        "        \"subnet\": \"2001:db8::/48\" "
+        "     } ],"
+        "    \"valid-lifetime\": 4000"
+        "}";
+
+    EXPECT_NO_THROW(configure(config, *client.getServer()));
+
+    // Tell the client to not send client-id on its own.
+    client.useClientId(false);
+
+    // Instead, tell him to send this extra option, which happens to be
+    // an empty client-id.
+    OptionPtr empty_client_id(new Option(Option::V6, D6O_CLIENTID));
+    client.addExtraOption(empty_client_id);
+
+    // Let's check whether the server is able to process this packet without
+    // throwing any exceptions. We don't care whether the server sent any
+    // responses or not. The goal is to check that the server didn't throw
+    // any exceptions.
+    EXPECT_NO_THROW(client.doSARR());
+}
+
+// This test verifies that the server is able to handle an empty DUID (server-id)
+// in incoming client message.
+TEST_F(Dhcpv6SrvTest, emptyServerId) {
+    Dhcp6Client client;
+
+    // The following configuration enables RSOO options: 110 and 120.
+    // It also configures the server with option 120 which should
+    // "override" the option 120 sent in the RSOO by the relay.
+    string config =
+        "{"
+        "    \"preferred-lifetime\": 3000,"
+        "    \"rebind-timer\": 2000, "
+        "    \"renew-timer\": 1000, "
+        "    \"subnet6\": [ { "
+        "        \"pools\": [ { \"pool\": \"2001:db8::/64\" } ],"
+        "        \"subnet\": \"2001:db8::/48\" "
+        "     } ],"
+        "    \"valid-lifetime\": 4000"
+        "}";
+
+    EXPECT_NO_THROW(configure(config, *client.getServer()));
+
+    // Tell the client to use this specific server-id.
+    OptionPtr empty_server_id(new Option(Option::V6, D6O_SERVERID));
+    client.useServerId(empty_server_id);
+
+    // Let's check whether the server is able to process this packet without
+    // throwing any exceptions. We don't care whether the server sent any
+    // responses or not. The goal is to check that the server didn't throw
+    // any exceptions.
+    EXPECT_NO_THROW(client.doSARR());
+}
+
+
 /// @todo: Add more negative tests for processX(), e.g. extend sanityCheck() test
 /// to call processX() methods.