From: Tomek Mrugalski Date: Fri, 11 Dec 2015 17:07:01 +0000 (+0100) Subject: [4206b_0.9.2] trac4206b ported to 0.9.2 X-Git-Tag: Kea-0.9.2-P1~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6c3fef817fbdd67e2e4305e6bbaa60f9cbe1e08c;p=thirdparty%2Fkea.git [4206b_0.9.2] trac4206b ported to 0.9.2 - fix for assert in getLabel when --enable-logger-checks is enabled --- diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index 17f1f17d00..5226a620b4 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -3434,4 +3434,30 @@ TEST_F(Dhcpv4SrvTest, emptyClientId) { EXPECT_NO_THROW(client.doDORA()); } +// This test verifies that the server is able to handle too long client-id +// in incoming client message. +TEST_F(Dhcpv4SrvTest, tooLongClientId) { + 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. + std::vector data(250, 250); + OptionPtr long_client_id(new Option(Option::V4, DHO_DHCP_CLIENT_IDENTIFIER, + data)); + client.addExtraOption(long_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()); +} + }; // end of anonymous namespace diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index bd82bd600a..67d174a006 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -2481,6 +2481,40 @@ TEST_F(Dhcpv6SrvTest, emptyServerId) { EXPECT_NO_THROW(client.doSARR()); } +// This test verifies that the server is able to handle a too large DUID (server-id) +// in incoming client message. +TEST_F(Dhcpv6SrvTest, tooLongServerId) { + 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. + std::vector data(250, 250); + OptionPtr long_server_id(new Option(Option::V6, D6O_SERVERID, data)); + client.useServerId(long_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. diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc index 44a96ca53c..2a82969139 100644 --- a/src/lib/dhcp/pkt4.cc +++ b/src/lib/dhcp/pkt4.cc @@ -343,15 +343,31 @@ std::string Pkt4::getLabel() const { /// @todo If and when client id is extracted into Pkt4, this method should - /// the instance member rather than fetch it every time. + /// use the instance member rather than fetch it every time. + std::string suffix; ClientIdPtr client_id; OptionPtr client_opt = getOption(DHO_DHCP_CLIENT_IDENTIFIER); - if (client_opt ) { - client_id = ClientIdPtr(new ClientId(client_opt->getData())); + if (client_opt) { + try { + client_id = ClientIdPtr(new ClientId(client_opt->getData())); + } catch (...) { + // ClientId may throw if the client-id is too short. + suffix = " (malformed client-id)"; + } } - return makeLabel(hwaddr_, client_id, transid_); - + std::ostringstream label; + try { + label << makeLabel(hwaddr_, client_id, transid_); + } catch (...) { + // This should not happen with the current code, but we may add extra + // sanity checks in the future that would possibly throw if + // the hwaddr length is 0. + label << " (malformed hw address)"; + } + + label << suffix; + return (label.str()); } std::string diff --git a/src/lib/dhcp/pkt4.h b/src/lib/dhcp/pkt4.h index 549be78da8..12af2cf2bd 100644 --- a/src/lib/dhcp/pkt4.h +++ b/src/lib/dhcp/pkt4.h @@ -103,6 +103,8 @@ public: /// wrapper around static makeLabel(). See this method for string /// content. /// + /// This method is exception safe. + /// /// @return string with text representation std::string getLabel() const; diff --git a/src/lib/dhcp/pkt6.cc b/src/lib/dhcp/pkt6.cc index 7881672476..d0fd5e505b 100644 --- a/src/lib/dhcp/pkt6.cc +++ b/src/lib/dhcp/pkt6.cc @@ -544,7 +544,18 @@ Pkt6::toText() const { DuidPtr Pkt6::getClientId() const { OptionPtr opt_duid = getOption(D6O_CLIENTID); - return (opt_duid ? DuidPtr(new DUID(opt_duid->getData())) : DuidPtr()); + try { + // This will throw if the DUID length is larger than 128 bytes + // or is too short. + return (opt_duid ? DuidPtr(new DUID(opt_duid->getData())) : DuidPtr()); + } catch (...) { + // Do nothing. This method is used only by getLabel(), which is + // used for logging purposes. We should not throw, but rather + // report no DUID. We should not log anything, as we're in the + // process of logging something for this packet. So the only + // choice left is to return an empty pointer. + } + return (DuidPtr()); } isc::dhcp::OptionCollection diff --git a/src/lib/dhcp/pkt6.h b/src/lib/dhcp/pkt6.h index febb92d35f..3228dadc56 100644 --- a/src/lib/dhcp/pkt6.h +++ b/src/lib/dhcp/pkt6.h @@ -217,6 +217,8 @@ public: /// @brief Retrieves the DUID from the Client Identifier option. /// + /// This method is exception safe. + /// /// @return Pointer to the DUID or NULL if the option doesn't exist. DuidPtr getClientId() const; diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc index 189b9c35fb..6636235c6f 100644 --- a/src/lib/dhcp/tests/pkt4_unittest.cc +++ b/src/lib/dhcp/tests/pkt4_unittest.cc @@ -1023,6 +1023,19 @@ TEST_F(Pkt4Test, getLabel) { } +// Test that empty client identifier option doesn't cause an exception from +// Pkt4::getLabel. +TEST_F(Pkt4Test, getLabelEmptyClientId) { + Pkt4 pkt(DHCPOFFER, 1234); + + // Create empty client identifier option. + OptionPtr empty_opt(new Option(Option::V4, DHO_DHCP_CLIENT_IDENTIFIER)); + pkt.addOption(empty_opt); + + EXPECT_EQ("[hwtype=1 ], cid=[no info], tid=0x4d2" + " (malformed client-id)", pkt.getLabel()); +} + // Tests that the correct DHCPv4 message name is returned for various // message types. TEST_F(Pkt4Test, getName) { diff --git a/src/lib/dhcp/tests/pkt6_unittest.cc b/src/lib/dhcp/tests/pkt6_unittest.cc index 12e80e10bd..6aff7a5f8d 100644 --- a/src/lib/dhcp/tests/pkt6_unittest.cc +++ b/src/lib/dhcp/tests/pkt6_unittest.cc @@ -1497,4 +1497,15 @@ TEST_F(Pkt6Test, getLabel) { } +// Test that empty client identifier option doesn't cause an exception from +// Pkt6::getLabel. +TEST_F(Pkt6Test, getLabelEmptyClientId) { + // Create a packet. + Pkt6 pkt(DHCPV6_SOLICIT, 0x2312); + + // Add empty client idenitifier option. + pkt.addOption(OptionPtr(new Option(Option::V6, D6O_CLIENTID))); + EXPECT_EQ("duid=[no info], tid=0x2312", pkt.getLabel()); +} + }