[3749] Relayed message with hops=0 is accepted by the DHCPv4 server.

src/bin/dhcp4/dhcp4_messages.mes

@@ -162,13 +162,6 @@ This debug message is issued when the client being in the INIT-REBOOT state
 requested an address which is not assigned to him. The server will respond
 to this client with DHCPNAK.
 
-% DHCP4_INVALID_RELAY_INFO malformed packet received from client-id %1, hwaddr %2: %3
-This message is logged when the client sends invalid combination of
-values in the giaddr and hops fields. If the packet is relayed it should
-contain non-zero values in both fields. If the packet is sent from the
-directly connected client, both values should be set to zero. All other
-combinations are invalid and trigger packet drop.
-
 % DHCP4_LEASE_ADVERT lease %1 advertised (client client-id %2, hwaddr %3)
 This debug message indicates that the server successfully advertised
 a lease. It is up to the client to choose one server out of other advertised

src/bin/dhcp4/dhcp4_srv.cc

@@ -1770,23 +1770,9 @@ Dhcpv4Srv::accept(const Pkt4Ptr& query) const {
 
 bool
 Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const {
-    try {
-        // This is the first call to the isRelayed function for this packet,
-        // so we have to catch exceptions which will be emitted if the
-        // packet contains invalid combination of hops and giaddr. For all
-        // other invocations of isRelayed function we will not catch
-        // exceptions because we eliminate malformed packets here.
-        if (pkt->isRelayed()) {
-            return (true);
-        }
-    } catch (const Exception& ex) {
-        OptionPtr client_id = pkt->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
-        HWAddrPtr hwaddr = pkt->getHWAddr();
-        LOG_INFO(dhcp4_logger, DHCP4_INVALID_RELAY_INFO)
-            .arg(client_id ? client_id->toText():"(no client-id)")
-            .arg(hwaddr ? hwaddr->toText():"(no hwaddr info)")
-            .arg(ex.what());
-        return (false);
+    // Accept all relayed messages.
+    if (pkt->isRelayed()) {
+        return (true);
     }
     // The source address must not be zero for the DHCPINFORM message from
     // the directly connected client because the server will not know where

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -342,45 +342,6 @@ TEST_F(Dhcpv4SrvTest, adjustIfaceDataBroadcast) {
 
 }
 
-// This test verifies that exception is thrown of the invalid combination
-// of giaddr and hops is specified in a client's message.
-TEST_F(Dhcpv4SrvTest, adjustIfaceDataInvalid) {
-    IfaceMgrTestConfig test_config(true);
-    IfaceMgr::instance().openSockets4();
-
-    boost::shared_ptr<Pkt4> req(new Pkt4(DHCPDISCOVER, 1234));
-
-    // The hops and giaddr values are used to determine if the client's
-    // message has been relayed or sent directly. The allowed combinations
-    // are (giaddr = 0 and hops = 0) or (giaddr != 0 and hops != 0). Any
-    // other combination is invalid and the adjustIfaceData should throw
-    // an exception. We will test that exception is indeed thrown.
-    req->setGiaddr(IOAddress("0.0.0.0"));
-    req->setHops(1);
-
-    // Clear client address as it hasn't got any address configured yet.
-    req->setCiaddr(IOAddress("0.0.0.0"));
-    // The query is sent to the broadcast address in the Select state.
-    req->setLocalAddr(IOAddress("255.255.255.255"));
-    // The query has been received on the DHCPv4 server port 67.
-    req->setLocalPort(DHCP4_SERVER_PORT);
-    // Set the interface. The response should be sent via the same interface.
-    req->setIface("eth0");
-    req->setIndex(1);
-
-    // Create the exchange using the req.
-    Dhcpv4Exchange ex = createExchange(req);
-    Pkt4Ptr resp = ex.getResponse();
-
-    // Assign some new address for this client.
-    resp->setYiaddr(IOAddress("192.0.2.13"));
-
-    // Clear the remote address.
-    resp->setRemoteAddr(IOAddress("0.0.0.0"));
-
-    EXPECT_THROW(NakedDhcpv4Srv::adjustIfaceData(ex), isc::BadValue);
-}
-
 // This test verifies that the server identifier option is appended to
 // a specified DHCPv4 message and the server identifier is correct.
 TEST_F(Dhcpv4SrvTest, appendServerID) {
@@ -3479,16 +3440,15 @@ TEST_F(Dhcpv4SrvTest, acceptDirectRequest) {
 
     Pkt4Ptr pkt(new Pkt4(DHCPDISCOVER, 1234));
     // Set Giaddr and local server's unicast address, but don't set hops.
-    // Hops value must be greater than 0, when giaddr is set. Otherwise,
-    // message is considered malformed and the accept() function should
-    // return false.
+    // Hops value should not matter. The server will treat the message
+    // with the hops value of 0 and non-zero giaddr as relayed.
     pkt->setGiaddr(IOAddress("192.0.10.1"));
     pkt->setRemoteAddr(IOAddress("0.0.0.0"));
     pkt->setLocalAddr(IOAddress("192.0.2.3"));
     pkt->setIface("eth1");
-    EXPECT_FALSE(srv.accept(pkt));
+    EXPECT_TRUE(srv.accept(pkt));
 
-    // Let's set hops and check that the message is now accepted as
+    // Let's set hops and check that the message is still accepted as
     // a relayed message.
     pkt->setHops(1);
     EXPECT_TRUE(srv.accept(pkt));

src/lib/dhcp/pkt4.cc

@@ -436,21 +436,7 @@ Pkt4::addOption(const OptionPtr& opt) {
 
 bool
 Pkt4::isRelayed() const {
-    static const IOAddress zero_addr("0.0.0.0");
-    // For non-relayed message both Giaddr and Hops are zero.
-    if (getGiaddr() == zero_addr && getHops() == 0) {
-        return (false);
-
-    // For relayed message, both Giaddr and Hops are non-zero.
-    } else if (getGiaddr() != zero_addr && getHops() > 0) {
-        return (true);
-    }
-    // In any other case, the packet is considered malformed.
-    isc_throw(isc::BadValue, "invalid combination of giaddr = "
-              << getGiaddr().toText() << " and hops = "
-              << static_cast<int>(getHops()) << ". Valid values"
-              " are: (giaddr = 0 and hops = 0) or (giaddr != 0 and"
-              "hops != 0)");
+    return (!giaddr_.isV4Zero() && !giaddr_.isV4Bcast());
 }
 
 } // end of namespace isc::dhcp

src/lib/dhcp/pkt4.h

@@ -332,17 +332,11 @@ public:
     /// This function returns a boolean value which indicates whether a DHCPv4
     /// message has been relayed (if true is returned) or not (if false).
     ///
-    /// This function uses a combination of Giaddr and Hops. It is expected that
-    /// if Giaddr is not 0, the Hops is greater than 0. In this case the message
-    /// is considered relayed. If Giaddr is 0, the Hops value must also be 0. In
-    /// this case the message is considered non-relayed. For any other
-    /// combination of Giaddr and Hops, an exception is thrown to indicate that
-    /// the message is malformed.
+    /// The message is considered relayed if the giaddr field is non-zero and
+    /// non-broadcast.
     ///
     /// @return Boolean value which indicates whether the message is relayed
     /// (true) or non-relayed (false).
-    /// @throw isc::BadValue if invalid combination of Giaddr and Hops values is
-    /// found.
     bool isRelayed() const;
 
     /// @brief That's the data of input buffer used in RX packet.

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -800,25 +800,25 @@ TEST_F(Pkt4Test, hwaddrSrcRemote) {
 }
 
 // This test verifies that the check for a message being relayed is correct.
-// It also checks that the exception is thrown if the combination of hops and
-// giaddr is invalid.
 TEST_F(Pkt4Test, isRelayed) {
     Pkt4 pkt(DHCPDISCOVER, 1234);
     // By default, the hops and giaddr should be 0.
-    ASSERT_EQ("0.0.0.0", pkt.getGiaddr().toText());
+    ASSERT_TRUE(pkt.getGiaddr().isV4Zero());
     ASSERT_EQ(0, pkt.getHops());
-    // For hops = 0 and giaddr = 0, the message is non-relayed.
+    // For zero giaddr the packet is non-relayed.
     EXPECT_FALSE(pkt.isRelayed());
-    // Set giaddr but leave hops = 0. This should result in exception.
+    // Set giaddr but leave hops = 0.
     pkt.setGiaddr(IOAddress("10.0.0.1"));
-    EXPECT_THROW(pkt.isRelayed(), isc::BadValue);
-    // Set hops. Now both hops and giaddr is set. The message is relayed.
+    EXPECT_TRUE(pkt.isRelayed());
+    // After setting hops the message should still be relayed.
     pkt.setHops(10);
     EXPECT_TRUE(pkt.isRelayed());
-    // Set giaddr to 0. For hops being set to non-zero value the function
-    // should throw an exception.
-    pkt.setGiaddr(IOAddress("0.0.0.0"));
-    EXPECT_THROW(pkt.isRelayed(), isc::BadValue);
+    // Set giaddr to 0. The message is now not-relayed.
+    pkt.setGiaddr(IOAddress(IOAddress::IPV4_ZERO_ADDRESS()));
+    EXPECT_FALSE(pkt.isRelayed());
+    // Setting the giaddr to 255.255.255.255 should not cause it to
+    // be relayed message.
+    pkt.setGiaddr(IOAddress(IOAddress::IPV4_BCAST_ADDRESS()));
 }
 
 // Tests whether a packet can be assigned to a class and later