[3695] Use the appropriate socket to send DHCPv4 response.

src/bin/dhcp4/dhcp4_srv.cc

@@ -522,10 +522,10 @@ Dhcpv4Srv::copyDefaultFields(const Pkt4Ptr& question, Pkt4Ptr& answer) {
     answer->setIface(question->getIface());
     answer->setIndex(question->getIndex());
 
-    answer->setSiaddr(IOAddress("0.0.0.0")); // explicitly set this to 0
+    answer->setSiaddr(IOAddress::IPV4_ZERO_ADDRESS()); // explicitly set this to 0
     // ciaddr is always 0, except for the Renew/Rebind state when it may
     // be set to the ciaddr sent by the client.
-    answer->setCiaddr(IOAddress("0.0.0.0"));
+    answer->setCiaddr(IOAddress::IPV4_ZERO_ADDRESS());
     answer->setHops(question->getHops());
 
     // copy MAC address
@@ -949,7 +949,7 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
             .arg(question->getRemoteAddr().toText())
             .arg(serverReceivedPacketName(question->getType()));
         answer->setType(DHCPNAK);
-        answer->setYiaddr(IOAddress("0.0.0.0"));
+        answer->setYiaddr(IOAddress::IPV4_ZERO_ADDRESS());
         return;
     }
 
@@ -981,11 +981,11 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
     // ciaddr.
     OptionCustomPtr opt_requested_address = boost::dynamic_pointer_cast<
         OptionCustom>(question->getOption(DHO_DHCP_REQUESTED_ADDRESS));
-    IOAddress hint("0.0.0.0");
+    IOAddress hint(0);
     if (opt_requested_address) {
         hint = opt_requested_address->readAddress();
 
-    } else if (question->getCiaddr() != IOAddress("0.0.0.0")) {
+    } else if (question->getCiaddr() != IOAddress::IPV4_ZERO_ADDRESS()) {
         hint = question->getCiaddr();
 
     }
@@ -1023,7 +1023,7 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
                 .arg(hwaddr ? hwaddr->toText():"(no hwaddr info)");
 
             answer->setType(DHCPNAK);
-            answer->setYiaddr(IOAddress("0.0.0.0"));
+            answer->setYiaddr(IOAddress::IPV4_ZERO_ADDRESS());
             return;
         }
         // Now check the second error case: unknown client.
@@ -1191,7 +1191,7 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
             .arg(hint.toText());
 
         answer->setType(DHCPNAK);
-        answer->setYiaddr(IOAddress("0.0.0.0"));
+        answer->setYiaddr(IOAddress::IPV4_ZERO_ADDRESS());
 
         answer->delOption(DHO_FQDN);
         answer->delOption(DHO_HOST_NAME);
@@ -1211,6 +1211,27 @@ Dhcpv4Srv::adjustIfaceData(const Pkt4Ptr& query, const Pkt4Ptr& response) {
     response->setRemotePort(query->isRelayed() ? DHCP4_SERVER_PORT :
                             DHCP4_CLIENT_PORT);
 
+
+    IOAddress local_addr = query->getLocalAddr();
+
+    // In many cases the query is sent to a broadcast address. This address
+    // apears as a local address in the query message. We can't simply copy
+    // this address to a response message and use it as a source address.
+    // Instead we will need to use the address assigned to the interface
+    // on which the query has been received. In other cases, we will just
+    // use this address as a source address for the response.
+    if (local_addr == IOAddress::IPV4_BCAST_ADDRESS()) {
+        SocketInfo sock_info = IfaceMgr::instance().getSocket(*query);
+        local_addr = sock_info.addr_;
+    }
+    // We assume that there is an appropriate socket bound to this address
+    // and that the address is correct. This is safe assumption because
+    // the local address of the query is set when the query is received.
+    // The query sent to an incorrect address wouldn't have been received.
+    // However, if socket is closed for this address between the reception
+    // of the query and sending a response, the IfaceMgr should detect it
+    // and return an error.
+    response->setLocalAddr(local_addr);
     // In many cases the query is sent to a broadcast address. This address
     // appears as a local address in the query message. Therefore we can't
     // simply copy local address from the query and use it as a source
@@ -1219,9 +1240,6 @@ Dhcpv4Srv::adjustIfaceData(const Pkt4Ptr& query, const Pkt4Ptr& response) {
     // may throw if for some reason the socket is closed.
     /// @todo Consider an optimization that we use local address from
     /// the query if this address is not broadcast.
-    SocketInfo sock_info = IfaceMgr::instance().getSocket(*query);
-    // Set local address, port and interface.
-    response->setLocalAddr(sock_info.addr_);
     response->setLocalPort(DHCP4_SERVER_PORT);
     response->setIface(query->getIface());
     response->setIndex(query->getIndex());
@@ -1229,13 +1247,6 @@ Dhcpv4Srv::adjustIfaceData(const Pkt4Ptr& query, const Pkt4Ptr& response) {
 
 void
 Dhcpv4Srv::adjustRemoteAddr(const Pkt4Ptr& question, const Pkt4Ptr& response) {
-    // Let's create static objects representing zeroed and broadcast
-    // addresses. We will use them further in this function to test
-    // other addresses against them. Since they are static, they will
-    // be created only once.
-    static const IOAddress zero_addr("0.0.0.0");
-    static const IOAddress bcast_addr("255.255.255.255");
-
     // The DHCPINFORM is slightly different than other messages in a sense
     // that the server should always unicast the response to the ciaddr.
     // It appears however that some clients don't set the ciaddr. We still
@@ -1244,7 +1255,7 @@ Dhcpv4Srv::adjustRemoteAddr(const Pkt4Ptr& question, const Pkt4Ptr& response) {
     if (question->getType() == DHCPINFORM) {
         // If client adheres to RFC2131 it will set the ciaddr and in this
         // case we always unicast our response to this address.
-        if (question->getCiaddr() != zero_addr) {
+        if (question->getCiaddr() != IOAddress::IPV4_ZERO_ADDRESS()) {
             response->setRemoteAddr(question->getCiaddr());
 
         // If we received DHCPINFORM via relay and the ciaddr is not set we
@@ -1275,24 +1286,24 @@ Dhcpv4Srv::adjustRemoteAddr(const Pkt4Ptr& question, const Pkt4Ptr& response) {
         // the message is relayed. Therefore, we set the BROADCAST flag so
         // as the relay can broadcast the packet.
         if ((question->getType() == DHCPINFORM) &&
-            (question->getCiaddr() == zero_addr)) {
+            (question->getCiaddr() == IOAddress::IPV4_ZERO_ADDRESS())) {
             response->setFlags(BOOTP_BROADCAST);
         }
         response->setRemoteAddr(question->getGiaddr());
 
     // If giaddr is 0 but client set ciaddr, server should unicast the
     // response to ciaddr.
-    } else if (question->getCiaddr() != zero_addr) {
+    } else if (question->getCiaddr() != IOAddress::IPV4_ZERO_ADDRESS()) {
         response->setRemoteAddr(question->getCiaddr());
 
     // We can't unicast the response to the client when sending NAK,
     // because we haven't allocated address for him. Therefore,
     // NAK is broadcast.
     } else if (response->getType() == DHCPNAK) {
-        response->setRemoteAddr(bcast_addr);
+        response->setRemoteAddr(IOAddress::IPV4_BCAST_ADDRESS());
 
     // If yiaddr is set it means that we have created a lease for a client.
-    } else if (response->getYiaddr() != zero_addr) {
+    } else if (response->getYiaddr() != IOAddress::IPV4_ZERO_ADDRESS()) {
         // If the broadcast bit is set in the flags field, we have to
         // send the response to broadcast address. Client may have requested it
         // because it doesn't support reception of messages on the interface
@@ -1301,7 +1312,7 @@ Dhcpv4Srv::adjustRemoteAddr(const Pkt4Ptr& question, const Pkt4Ptr& response) {
         // directly to a client without address assigned.
         const bool bcast_flag = ((question->getFlags() & Pkt4::FLAG_BROADCAST_MASK) != 0);
         if (!IfaceMgr::instance().isDirectResponseSupported() || bcast_flag) {
-            response->setRemoteAddr(bcast_addr);
+            response->setRemoteAddr(IOAddress::IPV4_BCAST_ADDRESS());
 
         // Client cleared the broadcast bit and we support direct responses
         // so we should unicast the response to a newly allocated address -
@@ -1356,7 +1367,7 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
     }
 
     // Adding any other options makes sense only when we got the lease.
-    if (offer->getYiaddr() != IOAddress("0.0.0.0")) {
+    if (offer->getYiaddr() != IOAddress::IPV4_ZERO_ADDRESS()) {
         appendRequestedOptions(discover, offer);
         appendRequestedVendorOptions(discover, offer);
         // There are a few basic options that we always want to
@@ -1408,7 +1419,7 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request) {
     }
 
     // Adding any other options makes sense only when we got the lease.
-    if (ack->getYiaddr() != IOAddress("0.0.0.0")) {
+    if (ack->getYiaddr() != IOAddress::IPV4_ZERO_ADDRESS()) {
         appendRequestedOptions(request, ack);
         appendRequestedVendorOptions(request, ack);
         // There are a few basic options that we always want to
@@ -1562,7 +1573,7 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) {
     // Also Relay Agent Options should be removed if present.
     if (ack->getRemoteAddr() != inform->getGiaddr()) {
         ack->setHops(0);
-        ack->setGiaddr(IOAddress("0.0.0.0"));
+        ack->setGiaddr(IOAddress::IPV4_ZERO_ADDRESS());
         ack->delOption(DHO_DHCP_AGENT_OPTIONS);
     }
 
@@ -1703,11 +1714,10 @@ Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const {
     // The source address must not be zero for the DHCPINFORM message from
     // the directly connected client because the server will not know where
     // to respond if the ciaddr was not present.
-    static const IOAddress zero_addr("0.0.0.0");
     try {
         if (pkt->getType() == DHCPINFORM) {
-            if ((pkt->getRemoteAddr() == zero_addr) &&
-                (pkt->getCiaddr() == zero_addr)) {
+            if ((pkt->getRemoteAddr() == IOAddress::IPV4_ZERO_ADDRESS()) &&
+                (pkt->getCiaddr() == IOAddress::IPV4_ZERO_ADDRESS())) {
                 return (false);
             }
         }
@@ -1717,8 +1727,7 @@ Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const {
         // we validate the message type prior to calling this function.
         return (false);
     }
-    static const IOAddress bcast("255.255.255.255");
-    return ((pkt->getLocalAddr() != bcast || selectSubnet(pkt)));
+    return ((pkt->getLocalAddr() != IOAddress::IPV4_BCAST_ADDRESS() || selectSubnet(pkt)));
 }
 
 bool
@@ -2034,7 +2043,7 @@ bool Dhcpv4Srv::classSpecificProcessing(const Pkt4Ptr& query, const Pkt4Ptr& rsp
     if (query->inClass(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_EROUTER)) {
 
         // Do not set TFTP server address for eRouter devices.
-        rsp->setSiaddr(IOAddress("0.0.0.0"));
+        rsp->setSiaddr(IOAddress::IPV4_ZERO_ADDRESS());
     }
 
     return (true);

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -81,7 +81,7 @@ TEST_F(Dhcpv4SrvTest, adjustIfaceDataRelay) {
     req->setFlags(0x0000);
 
     // Set local address, port and interface.
-    req->setLocalAddr(IOAddress("192.0.2.1"));
+    req->setLocalAddr(IOAddress("192.0.2.5"));
     req->setLocalPort(1001);
     req->setIface("eth1");
     req->setIndex(1);
@@ -104,7 +104,7 @@ TEST_F(Dhcpv4SrvTest, adjustIfaceDataRelay) {
     // The query has been relayed, so the response must be sent to the port 67.
     EXPECT_EQ(DHCP4_SERVER_PORT, resp->getRemotePort());
     // Local address should be the address assigned to interface eth1.
-    EXPECT_EQ("192.0.2.3", resp->getLocalAddr().toText());
+    EXPECT_EQ("192.0.2.5", resp->getLocalAddr().toText());
     // The local port is always DHCPv4 server port 67.
     EXPECT_EQ(DHCP4_SERVER_PORT, resp->getLocalPort());
     // We will send response over the same interface which was used to receive
@@ -184,7 +184,7 @@ TEST_F(Dhcpv4SrvTest, adjustIfaceDataRenew) {
     EXPECT_EQ(DHCP4_CLIENT_PORT, resp->getRemotePort());
     // The response should be sent from the unicast address on which the
     // query has been received.
-    EXPECT_EQ("192.0.2.3", resp->getLocalAddr().toText());
+    EXPECT_EQ("192.0.2.1", resp->getLocalAddr().toText());
     // The response should be sent from the DHCPv4 server port.
     EXPECT_EQ(DHCP4_SERVER_PORT, resp->getLocalPort());
     // The interface data should match the data in the query.

src/bin/dhcp4/tests/dora_unittest.cc

@@ -656,6 +656,8 @@ TEST_F(DORATest, reservationsWithConflicts) {
 
     // Client A performs 4-way exchange.
     client.setState(Dhcp4Client::SELECTING);
+    // Revert to the broadcast address for the selcting client.
+    client.setDestAddress(IOAddress::IPV4_BCAST_ADDRESS());
     // Obtain a lease from the server using the 4-way exchange.
     ASSERT_NO_THROW(client.doDORA(boost::shared_ptr<
                                   IOAddress>(new IOAddress("0.0.0.0"))));

src/lib/asiolink/io_address.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2010  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2010,2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -230,6 +230,23 @@ public:
     ///         network byte order
     operator uint32_t () const;
 
+    /// @name Methods returning @c IOAddress objects encapsulating typical addresses.
+    ///
+    //@{
+    /// @brief Returns an address set to all zeros.
+    static const IOAddress& IPV4_ZERO_ADDRESS() {
+        static IOAddress address(0);
+        return (address);
+    }
+
+    /// @brief Returns a "255.255.255.255" broadcast address.
+    static const IOAddress& IPV4_BCAST_ADDRESS() {
+        static IOAddress address(0xFFFFFFFF);
+        return (address);
+    }
+
+    //@}
+
 private:
     asio::ip::address asio_address_;
 };

src/lib/dhcp/iface_mgr.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2014 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -1165,7 +1165,7 @@ Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */
 uint16_t IfaceMgr::getSocket(const isc::dhcp::Pkt6& pkt) {
     Iface* iface = getIface(pkt.getIface());
     if (iface == NULL) {
-        isc_throw(BadValue, "Tried to find socket for non-existent interface");
+        isc_throw(IfaceNotFound, "Tried to find socket for non-existent interface");
     }
 
 
@@ -1214,7 +1214,7 @@ uint16_t IfaceMgr::getSocket(const isc::dhcp::Pkt6& pkt) {
         return (candidate->sockfd_);
     }
 
-    isc_throw(Unexpected, "Interface " << iface->getFullName()
+    isc_throw(SocketNotFound, "Interface " << iface->getFullName()
               << " does not have any suitable IPv6 sockets open.");
 }
 
@@ -1222,22 +1222,33 @@ SocketInfo
 IfaceMgr::getSocket(isc::dhcp::Pkt4 const& pkt) {
     Iface* iface = getIface(pkt.getIface());
     if (iface == NULL) {
-        isc_throw(BadValue, "Tried to find socket for non-existent interface");
+        isc_throw(IfaceNotFound, "Tried to find socket for non-existent interface");
     }
 
     const Iface::SocketCollection& socket_collection = iface->getSockets();
+    // A candidate being an end of the iterator marks that it is a begining of
+    // the socket search and that the candidate needs to be set to the first
+    // socket found.
+    Iface::SocketCollection::const_iterator candidate = socket_collection.end();
     Iface::SocketCollection::const_iterator s;
     for (s = socket_collection.begin(); s != socket_collection.end(); ++s) {
         if (s->family_ == AF_INET) {
-            return (*s);
+            if (s->addr_ == pkt.getLocalAddr()) {
+                return (*s);
+            }
+
+            if (candidate == socket_collection.end()) {
+                candidate = s;
+            }
         }
-        /// TODO: Add more checks here later. If remote address is
-        /// not link-local, we can't use link local bound socket
-        /// to send data.
     }
 
-    isc_throw(Unexpected, "Interface " << iface->getFullName()
-              << " does not have any suitable IPv4 sockets open.");
+    if (candidate == socket_collection.end()) {
+        isc_throw(SocketNotFound, "Interface " << iface->getFullName()
+                  << " does not have any suitable IPv4 sockets open.");
+    }
+
+    return (*candidate);
 }
 
 } // end of namespace isc::dhcp

src/lib/dhcp/iface_mgr.h

@@ -81,6 +81,20 @@ public:
         isc::Exception(file, line, what) { };
 };
 
+/// @brief IfaceMgr exception thrown when there is no suitable interface.
+class IfaceNotFound : public Exception {
+public:
+    IfaceNotFound(const char* file, size_t line, const char* what) :
+        isc::Exception(file, line, what) { };
+};
+
+/// @brief IfaceMgr exception thrown when there is no suitable socket found.
+class SocketNotFound : public Exception {
+public:
+    SocketNotFound(const char* file, size_t line, const char* what) :
+        isc::Exception(file, line, what) { };
+};
+
 
 /// Holds information about socket.
 struct SocketInfo {
@@ -613,26 +627,30 @@ public:
     ///
     /// This method takes Pkt6 (see overloaded implementation that takes
     /// Pkt4) and chooses appropriate socket to send it. This method
-    /// may throw BadValue if specified packet does not have outbound
-    /// interface specified, no such interface exists, or specified
-    /// interface does not have any appropriate sockets open.
+    /// may throw if specified packet does not have outbound interface specified,
+    /// no such interface exists, or specified interface does not have any
+    /// appropriate sockets open.
     ///
     /// @param pkt a packet to be transmitted
     ///
     /// @return a socket descriptor
+    /// @throw SocketNotFound If no suitable socket found.
+    /// @throw IfaceNotFound If interface is not set for the packet.
     uint16_t getSocket(const isc::dhcp::Pkt6& pkt);
 
     /// @brief Return most suitable socket for transmitting specified IPv4 packet.
     ///
-    /// This method takes Pkt4 (see overloaded implementation that takes
-    /// Pkt6) and chooses appropriate socket to send it. This method
-    /// may throw BadValue if specified packet does not have outbound
-    /// interface specified, no such interface exists, or specified
-    /// interface does not have any appropriate sockets open.
+    /// This method uses the local address assigned to the packet and tries
+    /// to match it with addresses to which sockets are bound for the particular
+    /// interface. If the match is not found, the method returns the first IPv4
+    /// socket found for the particular interface. In case, there are no IPv4
+    /// sockets assigned to the interface the exception is thrown.
     ///
-    /// @param pkt a packet to be transmitted
+    /// @param pkt A packet to be transmitted. It must hold a local address and
+    /// a valid pointer to the interface.
     ///
     /// @return A structure describing a socket.
+    /// @throw SocketNotFound if no suitable socket found.
     SocketInfo getSocket(const isc::dhcp::Pkt4& pkt);
 
     /// Debugging method that prints out all available interfaces.

src/lib/dhcp/tests/iface_mgr_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2014 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -2159,17 +2159,17 @@ TEST_F(IfaceMgrTest, socketInfo) {
 
     Pkt6 pkt6(DHCPV6_REPLY, 123456);
 
-    // pkt6 dos not have interface set yet
+    // pkt6 does not have interface set yet
     EXPECT_THROW(
         ifacemgr->getSocket(pkt6),
-        BadValue
+        IfaceNotFound
     );
 
     // Try to send over non-existing interface
     pkt6.setIface("nosuchinterface45");
     EXPECT_THROW(
         ifacemgr->getSocket(pkt6),
-        BadValue
+        IfaceNotFound
     );
 
     // This will work
@@ -2185,7 +2185,7 @@ TEST_F(IfaceMgrTest, socketInfo) {
     // It should throw again, there's no usable socket anymore
     EXPECT_THROW(
         ifacemgr->getSocket(pkt6),
-        Unexpected
+        SocketNotFound
     );
 
     // Repeat for pkt4
@@ -2194,20 +2194,30 @@ TEST_F(IfaceMgrTest, socketInfo) {
     // pkt4 does not have interface set yet.
     EXPECT_THROW(
         ifacemgr->getSocket(pkt4),
-        BadValue
+        IfaceNotFound
     );
 
     // Try to send over non-existing interface.
     pkt4.setIface("nosuchinterface45");
     EXPECT_THROW(
         ifacemgr->getSocket(pkt4),
-        BadValue
+        IfaceNotFound
     );
 
     // Socket info is set, packet has well defined interface. It should work.
     pkt4.setIface(LOOPBACK);
     EXPECT_EQ(7, ifacemgr->getSocket(pkt4).sockfd_);
 
+    // Set the local address to check if the socket for this address will
+    // be returned.
+    pkt4.setLocalAddr(IOAddress("192.0.2.56"));
+    EXPECT_EQ(7, ifacemgr->getSocket(pkt4).sockfd_);
+
+    // Modify the local address and expect that the other socket will be
+    // returned.
+    pkt4.setLocalAddr(IOAddress("192.0.2.53"));
+    EXPECT_EQ(8, ifacemgr->getSocket(pkt4).sockfd_);
+
     EXPECT_NO_THROW(
         ifacemgr->getIface(LOOPBACK)->delSocket(7);
         ifacemgr->getIface(LOOPBACK)->delSocket(8);
@@ -2216,7 +2226,7 @@ TEST_F(IfaceMgrTest, socketInfo) {
     // It should throw again, there's no usable socket anymore.
     EXPECT_THROW(
         ifacemgr->getSocket(pkt4),
-        Unexpected
+        SocketNotFound
     );
 }
 

src/lib/dhcp/tests/pkt_captures4.cc

@@ -62,9 +62,9 @@ Pkt4Ptr PktCaptures::packetFromCapture(const std::string& hex_string) {
 
 void PktCaptures::captureSetDefaultFields(const Pkt4Ptr& pkt) {
     pkt->setRemotePort(546);
-    pkt->setRemoteAddr(IOAddress("fe80::1"));
+    pkt->setRemoteAddr(IOAddress("10.0.0.2"));
     pkt->setLocalPort(0);
-    pkt->setLocalAddr(IOAddress("ff02::1:2"));
+    pkt->setLocalAddr(IOAddress("10.0.0.1"));
     pkt->setIndex(2);
     pkt->setIface("eth0");
 }