From 0119b50f66d9c1ae139d663d08c99cbe895a53e7 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Thu, 14 Nov 2013 18:06:23 +0100 Subject: [PATCH 01/24] [2765] Applied patch for DHCPv4 silently failing when DHCP port in use. --- src/lib/dhcp/pkt_filter_lpf.cc | 22 ++++++++++++++- src/lib/dhcp/tests/iface_mgr_unittest.cc | 34 ++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/lib/dhcp/pkt_filter_lpf.cc b/src/lib/dhcp/pkt_filter_lpf.cc index e0964d5941..8616ad7c9b 100644 --- a/src/lib/dhcp/pkt_filter_lpf.cc +++ b/src/lib/dhcp/pkt_filter_lpf.cc @@ -103,9 +103,29 @@ namespace isc { namespace dhcp { int -PktFilterLPF::openSocket(const Iface& iface, const isc::asiolink::IOAddress&, +PktFilterLPF::openSocket(const Iface& iface, + const isc::asiolink::IOAddress& addr, const uint16_t port, const bool, const bool) { + // Let's check if a socket is already in use + int sock_check = socket(AF_INET, SOCK_DGRAM, 0); + if (sock_check < 0) { + isc_throw(SocketConfigError, "Failed to create dgram socket"); + } + + struct sockaddr_in addr4; + memset(& addr4, 0, sizeof(addr4)); + addr4.sin_family = AF_INET; + addr4.sin_addr.s_addr = htonl(addr); + addr4.sin_port = htons(port); + + if (bind(sock_check, (struct sockaddr *)& addr4, sizeof(addr4)) < 0) { + // We return negative, the proper error message will be displayed + // by the IfaceMgr ... + close(sock_check); + return (-1); + } + close(sock_check); int sock = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL)); if (sock < 0) { diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index b936b5c373..85ea491c09 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -986,6 +986,40 @@ TEST_F(IfaceMgrTest, setMatchingPacketFilter) { EXPECT_TRUE(iface_mgr->isDirectResponseSupported()); } +TEST_F(IfaceMgrTest, checkPacketFilterLPFSocket) { + IOAddress loAddr("127.0.0.1"); + int socket1 = 0, socket2 = 0; + // Create two instances of IfaceMgr. + boost::scoped_ptr iface_mgr1(new NakedIfaceMgr()); + ASSERT_TRUE(iface_mgr1); + boost::scoped_ptr iface_mgr2(new NakedIfaceMgr()); + ASSERT_TRUE(iface_mgr2); + + // Let IfaceMgr figure out which Packet Filter to use when + // direct response capability is not desired. It should pick + // PktFilterInet. + EXPECT_NO_THROW(iface_mgr1->setMatchingPacketFilter(false)); + // Let's open a loopback socket with handy unpriviliged port number + socket1 = iface_mgr1->openSocket(LOOPBACK, loAddr, + DHCP4_SERVER_PORT + 10000); + + EXPECT_GE(socket1, 0); + + // Then the second use PkFilterLPF mode + EXPECT_NO_THROW(iface_mgr2->setMatchingPacketFilter(true)); + // This socket opening attempt should not return positive value + // The first socket already opened same port + EXPECT_NO_THROW( + socket2 = iface_mgr2->openSocket(LOOPBACK, loAddr, + DHCP4_SERVER_PORT + 10000); + ); + + EXPECT_LE(socket2, 0); + + close(socket2); + close(socket1); +} + #else // This non-Linux specific test checks whether it is possible to use From 4e225668a62a8fde0edb28a2938d968989f3cbc4 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Mon, 25 Nov 2013 20:33:40 +0100 Subject: [PATCH 02/24] [2765] Added fallback socket descriptor to SocketInfo structure. --- src/bin/dhcp4/tests/dhcp4_test_utils.h | 8 ++-- src/lib/dhcp/iface_mgr.cc | 10 ++--- src/lib/dhcp/iface_mgr.h | 44 ++++++++++++++++--- src/lib/dhcp/pkt_filter.h | 32 +++++++++----- src/lib/dhcp/pkt_filter_inet.cc | 14 +++--- src/lib/dhcp/pkt_filter_inet.h | 24 +++++----- src/lib/dhcp/pkt_filter_lpf.cc | 7 +-- src/lib/dhcp/pkt_filter_lpf.h | 25 +++++------ src/lib/dhcp/tests/iface_mgr_unittest.cc | 36 ++++++++++----- .../dhcp/tests/pkt_filter_inet_unittest.cc | 8 ++-- src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc | 6 +-- 11 files changed, 134 insertions(+), 80 deletions(-) diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h index ee19728ce5..208d342901 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.h +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.h @@ -20,6 +20,7 @@ #define DHCP4_TEST_UTILS_H #include +#include #include #include #include @@ -52,9 +53,10 @@ public: } /// Does nothing. - virtual int openSocket(const Iface&, const isc::asiolink::IOAddress&, - const uint16_t, const bool, const bool) { - return (0); + virtual SocketInfo openSocket(const Iface&, + const isc::asiolink::IOAddress& addr, + const uint16_t port, const bool, const bool) { + return (SocketInfo(addr, port, 0)); } /// Does nothing. diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index 0ad35a508e..3b33671bcf 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -741,7 +741,7 @@ int IfaceMgr::openSocket6(Iface& iface, const IOAddress& addr, uint16_t port) { } } - SocketInfo info(sock, addr, port); + SocketInfo info(addr, port, sock); iface.addSocket(info); return (sock); @@ -754,13 +754,11 @@ int IfaceMgr::openSocket4(Iface& iface, const IOAddress& addr, // Skip checking if the packet_filter_ is non-NULL because this check // has been already done when packet filter object was set. - int sock = packet_filter_->openSocket(iface, addr, port, - receive_bcast, send_bcast); - - SocketInfo info(sock, addr, port); + SocketInfo info = packet_filter_->openSocket(iface, addr, port, + receive_bcast, send_bcast); iface.addSocket(info); - return (sock); + return (info.sockfd_); } bool diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index 2f48813cc2..bae67cc426 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -72,19 +72,49 @@ public: /// Holds information about socket. struct SocketInfo { - uint16_t sockfd_; /// socket descriptor + isc::asiolink::IOAddress addr_; /// bound address uint16_t port_; /// socket port uint16_t family_; /// IPv4 or IPv6 + /// @brief Socket descriptor (a.k.a. primary socket). + int sockfd_; + + /// @brief Fallback socket descriptor. + /// + /// This socket descriptor holds the handle to the fallback socket. + /// The fallback socket is created when there is a need for the regular + /// datagram socket to be bound to an IP address and port, besides + /// primary socket (sockfd_) which is actually used to receive and process + /// the DHCP messages. The fallback socket (if exists) is always associated + /// with the primary socket. In particular, the need for the fallback socket + /// arises when raw socket is a primary one. When primary socket is open, + /// it is bound to an interface not the address and port. The implications + /// include the possibility that the other process (e.g. the other instance + /// of DHCP server) will bind to the same address and port through which the + /// raw socket receives the DHCP messages.Another implication is that the + /// kernel, being unaware of the DHCP server operating through the raw + /// socket, will respond with the ICMP "Destination port unreachable" + /// messages when DHCP messages are only received through the raw socket. + /// In order to workaround the issues mentioned here, the fallback socket + /// should be opened so as/ the kernel is aware that the certain address + /// and port is in use. + /// + /// The fallback description is supposed to be set to a negative value if + /// the fallback socket is closed (not open). + int fallbackfd_; + /// @brief SocketInfo constructor. /// - /// @param sockfd socket descriptor - /// @param addr an address the socket is bound to - /// @param port a port the socket is bound to - SocketInfo(uint16_t sockfd, const isc::asiolink::IOAddress& addr, - uint16_t port) - :sockfd_(sockfd), addr_(addr), port_(port), family_(addr.getFamily()) { } + /// @param addr An address the socket is bound to. + /// @param port A port the socket is bound to. + /// @param sockfd Socket descriptor. + /// @param fallbackfd A descriptor of the fallback socket. + SocketInfo(const isc::asiolink::IOAddress& addr, const uint16_t port, + const int sockfd, const int fallbackfd = -1) + : addr_(addr), port_(port), family_(addr.getFamily()), + sockfd_(sockfd), fallbackfd_(fallbackfd) { } + }; diff --git a/src/lib/dhcp/pkt_filter.h b/src/lib/dhcp/pkt_filter.h index 204b25e6b6..f2a028d59b 100644 --- a/src/lib/dhcp/pkt_filter.h +++ b/src/lib/dhcp/pkt_filter.h @@ -67,20 +67,30 @@ public: /// @return true of the direct response is supported. virtual bool isDirectResponseSupported() const = 0; - /// @brief Open socket. + /// @brief Open primary and fallback socket. /// - /// @param iface interface descriptor - /// @param addr address on the interface to be used to send packets. - /// @param port port number. - /// @param receive_bcast configure socket to receive broadcast messages + /// A method implementation in the derived class may open one or two + /// sockets: + /// - a primary socket - used for communication with clients. DHCP messages + /// received using this socket are processed and the same socket is used + /// to send a response to the client. + /// - a fallback socket which is optionally opened if there is a need for + /// the presence of the socket which can be bound to a specific IP address + /// and UDP port (e.g. raw primary socket can't be). For the details, see + /// the documentation of @c isc::dhcp::SocketInfo. + /// + /// @param iface Interface descriptor. + /// @param addr Address on the interface to be used to send packets. + /// @param port Port number. + /// @param receive_bcast Configure socket to receive broadcast messages /// @param send_bcast configure socket to send broadcast messages. /// - /// @return created socket's descriptor - virtual int openSocket(const Iface& iface, - const isc::asiolink::IOAddress& addr, - const uint16_t port, - const bool receive_bcast, - const bool send_bcast) = 0; + /// @return A structure describing a primary and fallback socket. + virtual SocketInfo openSocket(const Iface& iface, + const isc::asiolink::IOAddress& addr, + const uint16_t port, + const bool receive_bcast, + const bool send_bcast) = 0; /// @brief Receive packet over specified socket. /// diff --git a/src/lib/dhcp/pkt_filter_inet.cc b/src/lib/dhcp/pkt_filter_inet.cc index 62695e5949..79f318d161 100644 --- a/src/lib/dhcp/pkt_filter_inet.cc +++ b/src/lib/dhcp/pkt_filter_inet.cc @@ -28,11 +28,12 @@ PktFilterInet::PktFilterInet() { } -int PktFilterInet::openSocket(const Iface& iface, - const isc::asiolink::IOAddress& addr, - const uint16_t port, - const bool receive_bcast, - const bool send_bcast) { +SocketInfo +PktFilterInet::openSocket(const Iface& iface, + const isc::asiolink::IOAddress& addr, + const uint16_t port, + const bool receive_bcast, + const bool send_bcast) { struct sockaddr_in addr4; memset(&addr4, 0, sizeof(sockaddr)); @@ -90,7 +91,8 @@ int PktFilterInet::openSocket(const Iface& iface, } #endif - return (sock); + SocketInfo sock_desc(addr, port, sock); + return (sock_desc); } diff --git a/src/lib/dhcp/pkt_filter_inet.h b/src/lib/dhcp/pkt_filter_inet.h index 95c9224847..0a506f0a0d 100644 --- a/src/lib/dhcp/pkt_filter_inet.h +++ b/src/lib/dhcp/pkt_filter_inet.h @@ -44,20 +44,20 @@ public: return (false); } - /// @brief Open socket. + /// @brief Open primary and fallback socket. /// - /// @param iface interface descriptor - /// @param addr address on the interface to be used to send packets. - /// @param port port number. - /// @param receive_bcast configure socket to receive broadcast messages - /// @param send_bcast configure socket to send broadcast messages. + /// @param iface Interface descriptor. + /// @param addr Address on the interface to be used to send packets. + /// @param port Port number. + /// @param receive_bcast Configure socket to receive broadcast messages + /// @param send_bcast Configure socket to send broadcast messages. /// - /// @return created socket's descriptor - virtual int openSocket(const Iface& iface, - const isc::asiolink::IOAddress& addr, - const uint16_t port, - const bool receive_bcast, - const bool send_bcast); + /// @return A structure describing a primary and fallback socket. + virtual SocketInfo openSocket(const Iface& iface, + const isc::asiolink::IOAddress& addr, + const uint16_t port, + const bool receive_bcast, + const bool send_bcast); /// @brief Receive packet over specified socket. /// diff --git a/src/lib/dhcp/pkt_filter_lpf.cc b/src/lib/dhcp/pkt_filter_lpf.cc index 8616ad7c9b..7faa341425 100644 --- a/src/lib/dhcp/pkt_filter_lpf.cc +++ b/src/lib/dhcp/pkt_filter_lpf.cc @@ -102,7 +102,7 @@ using namespace isc::util; namespace isc { namespace dhcp { -int +SocketInfo PktFilterLPF::openSocket(const Iface& iface, const isc::asiolink::IOAddress& addr, const uint16_t port, const bool, @@ -123,7 +123,7 @@ PktFilterLPF::openSocket(const Iface& iface, // We return negative, the proper error message will be displayed // by the IfaceMgr ... close(sock_check); - return (-1); + return (SocketInfo(addr, port, -1)); } close(sock_check); @@ -166,7 +166,8 @@ PktFilterLPF::openSocket(const Iface& iface, << "' to interface '" << iface.getName() << "'"); } - return (sock); + SocketInfo sock_desc(addr, port, sock); + return (sock_desc); } diff --git a/src/lib/dhcp/pkt_filter_lpf.h b/src/lib/dhcp/pkt_filter_lpf.h index d36719fbae..279c86451f 100644 --- a/src/lib/dhcp/pkt_filter_lpf.h +++ b/src/lib/dhcp/pkt_filter_lpf.h @@ -41,21 +41,20 @@ public: return (true); } - /// @brief Open socket. + /// @brief Open primary and fallback socket. /// - /// @param iface interface descriptor - /// @param addr address on the interface to be used to send packets. - /// @param port port number. - /// @param receive_bcast configure socket to receive broadcast messages - /// @param send_bcast configure socket to send broadcast messages. + /// @param iface Interface descriptor. + /// @param addr Address on the interface to be used to send packets. + /// @param port Port number. + /// @param receive_bcast Configure socket to receive broadcast messages + /// @param send_bcast Configure socket to send broadcast messages. /// - /// @throw isc::NotImplemented always - /// @return created socket's descriptor - virtual int openSocket(const Iface& iface, - const isc::asiolink::IOAddress& addr, - const uint16_t port, - const bool receive_bcast, - const bool send_bcast); + /// @return A structure describing a primary and fallback socket. + virtual SocketInfo openSocket(const Iface& iface, + const isc::asiolink::IOAddress& addr, + const uint16_t port, + const bool receive_bcast, + const bool send_bcast); /// @brief Receive packet over specified socket. /// diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index 85ea491c09..f2bb773488 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -84,13 +84,13 @@ public: /// (because real values are rather less than 255). Values greater /// than 255 are not recommended because they cause warnings to be /// reported by Valgrind when invoking close() on them. - virtual int openSocket(const Iface&, - const isc::asiolink::IOAddress&, - const uint16_t, - const bool, - const bool) { + virtual SocketInfo openSocket(const Iface&, + const isc::asiolink::IOAddress& addr, + const uint16_t port, + const bool, + const bool) { open_socket_called_ = true; - return (255); + return (SocketInfo(addr, port, 255)); } /// Does nothing @@ -1152,18 +1152,28 @@ TEST_F(IfaceMgrTest, iface_methods) { TEST_F(IfaceMgrTest, socketInfo) { // Check that socketinfo for IPv4 socket is functional - SocketInfo sock1(7, IOAddress("192.0.2.56"), DHCP4_SERVER_PORT + 7); + SocketInfo sock1(IOAddress("192.0.2.56"), DHCP4_SERVER_PORT + 7, 7); EXPECT_EQ(7, sock1.sockfd_); + EXPECT_EQ(-1, sock1.fallbackfd_); EXPECT_EQ("192.0.2.56", sock1.addr_.toText()); EXPECT_EQ(AF_INET, sock1.family_); EXPECT_EQ(DHCP4_SERVER_PORT + 7, sock1.port_); + // Check that non-default value of the fallback socket descriptor is set + SocketInfo sock2(IOAddress("192.0.2.53"), DHCP4_SERVER_PORT + 8, 8, 10); + EXPECT_EQ(8, sock2.sockfd_); + EXPECT_EQ(10, sock2.fallbackfd_); + EXPECT_EQ("192.0.2.53", sock2.addr_.toText()); + EXPECT_EQ(AF_INET, sock2.family_); + EXPECT_EQ(DHCP4_SERVER_PORT + 8, sock2.port_); + // Check that socketinfo for IPv6 socket is functional - SocketInfo sock2(9, IOAddress("2001:db8:1::56"), DHCP4_SERVER_PORT + 9); - EXPECT_EQ(9, sock2.sockfd_); - EXPECT_EQ("2001:db8:1::56", sock2.addr_.toText()); - EXPECT_EQ(AF_INET6, sock2.family_); - EXPECT_EQ(DHCP4_SERVER_PORT + 9, sock2.port_); + SocketInfo sock3(IOAddress("2001:db8:1::56"), DHCP4_SERVER_PORT + 9, 9); + EXPECT_EQ(9, sock3.sockfd_); + EXPECT_EQ(-1, sock3.fallbackfd_); + EXPECT_EQ("2001:db8:1::56", sock3.addr_.toText()); + EXPECT_EQ(AF_INET6, sock3.family_); + EXPECT_EQ(DHCP4_SERVER_PORT + 9, sock3.port_); // Now let's test if IfaceMgr handles socket info properly scoped_ptr ifacemgr(new NakedIfaceMgr()); @@ -1171,6 +1181,7 @@ TEST_F(IfaceMgrTest, socketInfo) { ASSERT_TRUE(loopback); loopback->addSocket(sock1); loopback->addSocket(sock2); + loopback->addSocket(sock3); Pkt6 pkt6(DHCPV6_REPLY, 123456); @@ -1225,6 +1236,7 @@ TEST_F(IfaceMgrTest, socketInfo) { EXPECT_NO_THROW( ifacemgr->getIface(LOOPBACK)->delSocket(7); + ifacemgr->getIface(LOOPBACK)->delSocket(8); ); // It should throw again, there's no usable socket anymore. diff --git a/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc b/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc index eaf2e62ef2..81ac0e740d 100644 --- a/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc @@ -112,7 +112,7 @@ TEST_F(PktFilterInetTest, openSocket) { // Try to open socket. PktFilterInet pkt_filter; - socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false); + socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false).sockfd_; // Check that socket has been opened. ASSERT_GE(socket_, 0); @@ -170,7 +170,7 @@ TEST_F(PktFilterInetTest, send) { // Open socket. We don't check that the socket has appropriate // options and family set because we have checked that in the // openSocket test already. - socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false); + socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false).sockfd_; ASSERT_GE(socket_, 0); // Send the packet over the socket. @@ -249,14 +249,14 @@ TEST_F(PktFilterInetTest, receive) { // Open socket. We don't check that the socket has appropriate // options and family set because we have checked that in the // openSocket test already. - socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false); + socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false).sockfd_; ASSERT_GE(socket_, 0); // Send the packet over the socket. ASSERT_NO_THROW(pkt_filter.send(iface, socket_, pkt)); // Receive the packet. - SocketInfo socket_info(socket_, IOAddress("127.0.0.1"), PORT); + SocketInfo socket_info(IOAddress("127.0.0.1"), PORT, socket_); Pkt4Ptr rcvd_pkt = pkt_filter.receive(iface, socket_info); // Check that the packet has been correctly received. ASSERT_TRUE(rcvd_pkt); diff --git a/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc b/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc index 742a7c98c3..1b5fc4b738 100644 --- a/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc @@ -124,7 +124,7 @@ TEST_F(PktFilterLPFTest, DISABLED_openSocket) { // Try to open socket. PktFilterLPF pkt_filter; - socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false); + socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false).sockfd_; // Check that socket has been opened. ASSERT_GE(socket_, 0); @@ -183,7 +183,7 @@ TEST_F(PktFilterLPFTest, DISABLED_send) { // Open socket. We don't check that the socket has appropriate // options and family set because we have checked that in the // openSocket test already. - socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false); + socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false).sockfd_; ASSERT_GE(socket_, 0); // Send the packet over the socket. @@ -273,7 +273,7 @@ TEST_F(PktFilterLPFTest, DISABLED_receive) { // Open socket. We don't check that the socket has appropriate // options and family set because we have checked that in the // openSocket test already. - socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false); + socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false).sockfd_; ASSERT_GE(socket_, 0); // Send the packet over the socket. From 721815f9c773352c738b5bcf3a7e88bb9ad4cbf8 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 26 Nov 2013 13:24:31 +0100 Subject: [PATCH 03/24] [2765] Fixed failing unit tests for the raw sockets using LPF. --- src/lib/dhcp/pkt_filter_lpf.cc | 13 ++++++++++--- src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc | 15 ++++++++++----- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/lib/dhcp/pkt_filter_lpf.cc b/src/lib/dhcp/pkt_filter_lpf.cc index 7faa341425..81d53f2e2f 100644 --- a/src/lib/dhcp/pkt_filter_lpf.cc +++ b/src/lib/dhcp/pkt_filter_lpf.cc @@ -230,9 +230,16 @@ PktFilterLPF::send(const Iface& iface, uint16_t sockfd, const Pkt4Ptr& pkt) { OutputBuffer buf(14); - HWAddrPtr hwaddr(new HWAddr(iface.getMac(), iface.getMacLen(), - iface.getHWType())); - pkt->setLocalHWAddr(hwaddr); + // Some interfaces may have no HW address - e.g. loopback interface. + // For these interfaces the HW address length is 0. If this is the case, + // then we will rely on the functions which construct the IP/UDP headers + // to provide a default HW addres. Otherwise, create the HW address + // object using the HW address of the interface. + if (iface.getMacLen() > 0) { + HWAddrPtr hwaddr(new HWAddr(iface.getMac(), iface.getMacLen(), + iface.getHWType())); + pkt->setLocalHWAddr(hwaddr); + } // Ethernet frame header. diff --git a/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc b/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc index 1b5fc4b738..3e12b5e761 100644 --- a/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc @@ -183,7 +183,11 @@ TEST_F(PktFilterLPFTest, DISABLED_send) { // Open socket. We don't check that the socket has appropriate // options and family set because we have checked that in the // openSocket test already. - socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false).sockfd_; + + SocketInfo sock_info = + pkt_filter.openSocket(iface, addr, PORT, false, false); + socket_ = sock_info.sockfd_; + ASSERT_GE(socket_, 0); // Send the packet over the socket. @@ -193,7 +197,7 @@ TEST_F(PktFilterLPFTest, DISABLED_send) { fd_set readfds; FD_ZERO(&readfds); FD_SET(socket_, &readfds); - + struct timeval timeout; timeout.tv_sec = 5; timeout.tv_usec = 0; @@ -273,15 +277,16 @@ TEST_F(PktFilterLPFTest, DISABLED_receive) { // Open socket. We don't check that the socket has appropriate // options and family set because we have checked that in the // openSocket test already. - socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false).sockfd_; + SocketInfo sock_info = + pkt_filter.openSocket(iface, addr, PORT, false, false); + socket_ = sock_info.sockfd_; ASSERT_GE(socket_, 0); // Send the packet over the socket. ASSERT_NO_THROW(pkt_filter.send(iface, socket_, pkt)); // Receive the packet. - SocketInfo socket_info(socket_, IOAddress("127.0.0.1"), PORT); - Pkt4Ptr rcvd_pkt = pkt_filter.receive(iface, socket_info); + Pkt4Ptr rcvd_pkt = pkt_filter.receive(iface, sock_info); // Check that the packet has been correctly received. ASSERT_TRUE(rcvd_pkt); From 67e3af3c8a57868712f85d86d47e1936ee834628 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 26 Nov 2013 14:18:55 +0100 Subject: [PATCH 04/24] [2765] Initialize and uninitialize fallback socket in the LPF tests. --- src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc | 68 ++++++++++--------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc b/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc index 3e12b5e761..df8898be85 100644 --- a/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc @@ -44,25 +44,28 @@ public: /// @brief Constructor /// - /// This constructor initializes socket_ member to a negative value. - /// Explcit initialization is performed here because some of the - /// tests do not initialize this value. In such cases, destructor - /// could invoke close() on uninitialized socket descriptor which - /// would result in errors being reported by Valgrind. + /// This constructor initializes sock_info_ structure to a default value. + /// The socket descriptors should be set to a negative value to indicate + /// that no socket has been opened. Specific tests will reinitialize this + /// structure with the values of the open sockets. For non-negative socket + /// descriptors, the class destructor will close associated sockets. PktFilterLPFTest() - : socket_(-1) { + : sock_info_(IOAddress("127.0.0.1"), PORT, -1, -1) { // Initialize ifname_ and ifindex_. loInit(); } /// @brief Destructor /// - /// Closes open socket (if any). + /// Closes open sockets (if any). ~PktFilterLPFTest() { // Cleanup after each test. This guarantees - // that the socket does not hang after a test. - if (socket_ >= 0) { - close(socket_); + // that the sockets do not hang after a test. + if (sock_info_.sockfd_ >= 0) { + close(sock_info_.sockfd_); + } + if (sock_info_.fallbackfd_ >=0) { + close(sock_info_.fallbackfd_); } } @@ -89,9 +92,9 @@ public: } } - std::string ifname_; ///< Loopback interface name - uint16_t ifindex_; ///< Loopback interface index. - int socket_; ///< Socket descriptor. + std::string ifname_; ///< Loopback interface name + uint16_t ifindex_; ///< Loopback interface index. + SocketInfo sock_info_; ///< A structure holding socket information. }; @@ -124,14 +127,18 @@ TEST_F(PktFilterLPFTest, DISABLED_openSocket) { // Try to open socket. PktFilterLPF pkt_filter; - socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false).sockfd_; - // Check that socket has been opened. - ASSERT_GE(socket_, 0); + sock_info_ = pkt_filter.openSocket(iface, addr, PORT, false, false); + + // Check that the primary socket has been opened. + ASSERT_GE(sock_info_.sockfd_, 0); + // Check that the fallback socket has been opened too. + ASSERT_GE(sock_info_.fallbackfd_, 0); // Verify that the socket belongs to AF_PACKET family. sockaddr_ll sock_address; socklen_t sock_address_len = sizeof(sock_address); - ASSERT_EQ(0, getsockname(socket_, reinterpret_cast(&sock_address), + ASSERT_EQ(0, getsockname(sock_info_.sockfd_, + reinterpret_cast(&sock_address), &sock_address_len)); EXPECT_EQ(AF_PACKET, sock_address.sll_family); @@ -141,7 +148,8 @@ TEST_F(PktFilterLPFTest, DISABLED_openSocket) { // Verify that the socket has SOCK_RAW type. int sock_type; socklen_t sock_type_len = sizeof(sock_type); - ASSERT_EQ(0, getsockopt(socket_, SOL_SOCKET, SO_TYPE, &sock_type, &sock_type_len)); + ASSERT_EQ(0, getsockopt(sock_info_.sockfd_, SOL_SOCKET, SO_TYPE, + &sock_type, &sock_type_len)); EXPECT_EQ(SOCK_RAW, sock_type); } @@ -184,30 +192,28 @@ TEST_F(PktFilterLPFTest, DISABLED_send) { // options and family set because we have checked that in the // openSocket test already. - SocketInfo sock_info = - pkt_filter.openSocket(iface, addr, PORT, false, false); - socket_ = sock_info.sockfd_; + sock_info_ = pkt_filter.openSocket(iface, addr, PORT, false, false); - ASSERT_GE(socket_, 0); + ASSERT_GE(sock_info_.sockfd_, 0); // Send the packet over the socket. - ASSERT_NO_THROW(pkt_filter.send(iface, socket_, pkt)); + ASSERT_NO_THROW(pkt_filter.send(iface, sock_info_.sockfd_, pkt)); // Read the data from socket. fd_set readfds; FD_ZERO(&readfds); - FD_SET(socket_, &readfds); + FD_SET(sock_info_.sockfd_, &readfds); struct timeval timeout; timeout.tv_sec = 5; timeout.tv_usec = 0; - int result = select(socket_ + 1, &readfds, NULL, NULL, &timeout); + int result = select(sock_info_.sockfd_ + 1, &readfds, NULL, NULL, &timeout); // We should receive some data from loopback interface. ASSERT_GT(result, 0); // Get the actual data. uint8_t rcv_buf[RECV_BUF_SIZE]; - result = recv(socket_, rcv_buf, RECV_BUF_SIZE, 0); + result = recv(sock_info_.sockfd_, rcv_buf, RECV_BUF_SIZE, 0); ASSERT_GT(result, 0); Pkt4Ptr dummy_pkt = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 0)); @@ -277,16 +283,14 @@ TEST_F(PktFilterLPFTest, DISABLED_receive) { // Open socket. We don't check that the socket has appropriate // options and family set because we have checked that in the // openSocket test already. - SocketInfo sock_info = - pkt_filter.openSocket(iface, addr, PORT, false, false); - socket_ = sock_info.sockfd_; - ASSERT_GE(socket_, 0); + sock_info_ = pkt_filter.openSocket(iface, addr, PORT, false, false); + ASSERT_GE(sock_info_.sockfd_, 0); // Send the packet over the socket. - ASSERT_NO_THROW(pkt_filter.send(iface, socket_, pkt)); + ASSERT_NO_THROW(pkt_filter.send(iface, sock_info_.sockfd_, pkt)); // Receive the packet. - Pkt4Ptr rcvd_pkt = pkt_filter.receive(iface, sock_info); + Pkt4Ptr rcvd_pkt = pkt_filter.receive(iface, sock_info_); // Check that the packet has been correctly received. ASSERT_TRUE(rcvd_pkt); From 4507742b539d4b992b184d3aefdc005264871973 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 26 Nov 2013 14:19:50 +0100 Subject: [PATCH 05/24] [2765] Added stub implementation for the function to open fallback socket. --- src/lib/dhcp/Makefile.am | 2 +- src/lib/dhcp/pkt_filter.cc | 29 +++++++++++++++++++++++++++++ src/lib/dhcp/pkt_filter.h | 24 ++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 src/lib/dhcp/pkt_filter.cc diff --git a/src/lib/dhcp/Makefile.am b/src/lib/dhcp/Makefile.am index 7a3a559294..eb9309ae29 100644 --- a/src/lib/dhcp/Makefile.am +++ b/src/lib/dhcp/Makefile.am @@ -41,7 +41,7 @@ libb10_dhcp___la_SOURCES += option_string.cc option_string.h libb10_dhcp___la_SOURCES += protocol_util.cc protocol_util.h libb10_dhcp___la_SOURCES += pkt6.cc pkt6.h libb10_dhcp___la_SOURCES += pkt4.cc pkt4.h -libb10_dhcp___la_SOURCES += pkt_filter.h +libb10_dhcp___la_SOURCES += pkt_filter.h pkt_filter.cc libb10_dhcp___la_SOURCES += pkt_filter_inet.cc pkt_filter_inet.h if OS_LINUX diff --git a/src/lib/dhcp/pkt_filter.cc b/src/lib/dhcp/pkt_filter.cc new file mode 100644 index 0000000000..1279c58e95 --- /dev/null +++ b/src/lib/dhcp/pkt_filter.cc @@ -0,0 +1,29 @@ +// Copyright (C) 2013 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 +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#include +#include + +namespace isc { +namespace dhcp { + +int +PktFilter::openFallbackSocket(const isc::asiolink::IOAddress&, + const uint16_t) { + return (0); +} + + +} // end of isc::dhcp namespace +} // end of isc namespace diff --git a/src/lib/dhcp/pkt_filter.h b/src/lib/dhcp/pkt_filter.h index f2a028d59b..1636abd316 100644 --- a/src/lib/dhcp/pkt_filter.h +++ b/src/lib/dhcp/pkt_filter.h @@ -110,6 +110,30 @@ public: /// @return result of sending the packet. It is 0 if successful. virtual int send(const Iface& iface, uint16_t sockfd, const Pkt4Ptr& pkt) = 0; + +protected: + + /// @brief Default implementation to open a fallback socket. + /// + /// This method provides a means to open a fallback socket and bind it + /// to a given IPv4 address and UDP port. This function may be used by the + /// derived classes to create a fallback socket. It can be overriden + /// in the derived classes if it happens to be insufficient on some + /// environments. + /// + /// The fallback socket is meant to be opened together with the other socket + /// (a.k.a. primary socket) used to receive and handle DHCPv4 traffic. The + /// traffic received through the fallback should be dropped. The reasoning + /// behind opening the fallback socket is explained in the documentation of + /// @s isc::dhcp::SocketInfo structure. + /// + /// @param addr An IPv4 address to bind the socket to. + /// @param port A port number to bind socket to. + /// + /// @return A fallback socket descriptor. This descriptor should be assigned + /// to the @c fallbackfd_ field of the @c isc::dhcp::SocketInfo structure. + virtual int openFallbackSocket(const isc::asiolink::IOAddress& addr, + const uint16_t port); }; /// Pointer to a PktFilter object. From 09811212553870d80e7958fb1161146a99e2d216 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 26 Nov 2013 18:36:09 +0100 Subject: [PATCH 06/24] [2765] Moved the PktFilter test fixture class to the common class. --- src/lib/dhcp/tests/Makefile.am | 1 + .../dhcp/tests/pkt_filter_inet_unittest.cc | 94 +++-------- src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc | 63 +------- src/lib/dhcp/tests/pkt_filter_test_utils.cc | 112 ++++++++++++++ src/lib/dhcp/tests/pkt_filter_test_utils.h | 146 ++++++++++++++++++ 5 files changed, 287 insertions(+), 129 deletions(-) create mode 100644 src/lib/dhcp/tests/pkt_filter_test_utils.cc create mode 100644 src/lib/dhcp/tests/pkt_filter_test_utils.h diff --git a/src/lib/dhcp/tests/Makefile.am b/src/lib/dhcp/tests/Makefile.am index 79bfde7811..a7bd5f5ba1 100644 --- a/src/lib/dhcp/tests/Makefile.am +++ b/src/lib/dhcp/tests/Makefile.am @@ -49,6 +49,7 @@ libdhcp___unittests_SOURCES += option_vendor_unittest.cc libdhcp___unittests_SOURCES += pkt4_unittest.cc libdhcp___unittests_SOURCES += pkt6_unittest.cc libdhcp___unittests_SOURCES += pkt_filter_inet_unittest.cc +libdhcp___unittests_SOURCES += pkt_filter_test_utils.h pkt_filter_test_utils.cc if OS_LINUX libdhcp___unittests_SOURCES += pkt_filter_lpf_unittest.cc diff --git a/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc b/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc index 81ac0e740d..424b1a2ff7 100644 --- a/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc @@ -17,6 +17,7 @@ #include #include #include +#include #include @@ -32,63 +33,12 @@ const uint16_t PORT = 10067; /// Size of the buffer holding received packets. const size_t RECV_BUF_SIZE = 2048; -/// This class handles has simple algorithm checking -/// presence of loopback interface and initializing -/// its index. -class PktFilterInetTest : public ::testing::Test { +// Test fixture class inherits from the class common for all packet +// filter tests. +class PktFilterInetTest : public isc::dhcp::test::PktFilterTest { public: - - /// @brief Constructor - /// - /// This constructor initializes socket_ member to a negative value. - /// Explcit initialization is performed here because some of the - /// tests do not initialize this value. In such cases, destructor - /// could invoke close() on uninitialized socket descriptor which - /// would result in errors being reported by Valgrind. - PktFilterInetTest() - : socket_(-1) { - // Initialize ifname_ and ifindex_. - loInit(); + PktFilterInetTest() : PktFilterTest(PORT) { } - - /// @brief Destructor - /// - /// Closes open socket (if any). - ~PktFilterInetTest() { - // Cleanup after each test. This guarantees - // that the socket does not hang after a test. - if (socket_ >= 0) { - close(socket_); - } - } - - /// @brief Detect loopback interface. - /// - /// @todo this function will be removed once cross-OS interface - /// detection is implemented - void loInit() { - if (if_nametoindex("lo") > 0) { - ifname_ = "lo"; - ifindex_ = if_nametoindex("lo"); - - } else if (if_nametoindex("lo0") > 0) { - ifname_ = "lo0"; - ifindex_ = if_nametoindex("lo0"); - - } else { - std::cout << "Failed to detect loopback interface. Neither " - << "lo nor lo0 worked. Giving up." << std::endl; - FAIL(); - - - - } - } - - std::string ifname_; ///< Loopback interface name - uint16_t ifindex_; ///< Loopback interface index. - int socket_; ///< Socket descriptor. - }; // This test verifies that the PktFilterInet class reports its lack @@ -112,14 +62,16 @@ TEST_F(PktFilterInetTest, openSocket) { // Try to open socket. PktFilterInet pkt_filter; - socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false).sockfd_; + sock_info_ = pkt_filter.openSocket(iface, addr, PORT, + false, false); // Check that socket has been opened. - ASSERT_GE(socket_, 0); + ASSERT_GE(sock_info_.sockfd_, 0); // Verify that the socket belongs to AF_INET family. sockaddr_in sock_address; socklen_t sock_address_len = sizeof(sock_address); - ASSERT_EQ(0, getsockname(socket_, reinterpret_cast(&sock_address), + ASSERT_EQ(0, getsockname(sock_info_.sockfd_, + reinterpret_cast(&sock_address), &sock_address_len)); EXPECT_EQ(AF_INET, sock_address.sin_family); @@ -133,7 +85,8 @@ TEST_F(PktFilterInetTest, openSocket) { // Verify that the socket has SOCK_DGRAM type. int sock_type; socklen_t sock_type_len = sizeof(sock_type); - ASSERT_EQ(0, getsockopt(socket_, SOL_SOCKET, SO_TYPE, &sock_type, &sock_type_len)); + ASSERT_EQ(0, getsockopt(sock_info_.sockfd_, SOL_SOCKET, SO_TYPE, + &sock_type, &sock_type_len)); EXPECT_EQ(SOCK_DGRAM, sock_type); } @@ -170,27 +123,27 @@ TEST_F(PktFilterInetTest, send) { // Open socket. We don't check that the socket has appropriate // options and family set because we have checked that in the // openSocket test already. - socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false).sockfd_; - ASSERT_GE(socket_, 0); + sock_info_ = pkt_filter.openSocket(iface, addr, PORT, false, false); + ASSERT_GE(sock_info_.sockfd_, 0); // Send the packet over the socket. - ASSERT_NO_THROW(pkt_filter.send(iface, socket_, pkt)); + ASSERT_NO_THROW(pkt_filter.send(iface, sock_info_.sockfd_, pkt)); // Read the data from socket. fd_set readfds; FD_ZERO(&readfds); - FD_SET(socket_, &readfds); - + FD_SET(sock_info_.sockfd_, &readfds); + struct timeval timeout; timeout.tv_sec = 5; timeout.tv_usec = 0; - int result = select(socket_ + 1, &readfds, NULL, NULL, &timeout); + int result = select(sock_info_.sockfd_ + 1, &readfds, NULL, NULL, &timeout); // We should receive some data from loopback interface. ASSERT_GT(result, 0); // Get the actual data. uint8_t rcv_buf[RECV_BUF_SIZE]; - result = recv(socket_, rcv_buf, RECV_BUF_SIZE, 0); + result = recv(sock_info_.sockfd_, rcv_buf, RECV_BUF_SIZE, 0); ASSERT_GT(result, 0); // Create the DHCPv4 packet from the received data. @@ -249,15 +202,14 @@ TEST_F(PktFilterInetTest, receive) { // Open socket. We don't check that the socket has appropriate // options and family set because we have checked that in the // openSocket test already. - socket_ = pkt_filter.openSocket(iface, addr, PORT, false, false).sockfd_; - ASSERT_GE(socket_, 0); + sock_info_ = pkt_filter.openSocket(iface, addr, PORT, false, false); + ASSERT_GE(sock_info_.sockfd_, 0); // Send the packet over the socket. - ASSERT_NO_THROW(pkt_filter.send(iface, socket_, pkt)); + ASSERT_NO_THROW(pkt_filter.send(iface, sock_info_.sockfd_, pkt)); // Receive the packet. - SocketInfo socket_info(IOAddress("127.0.0.1"), PORT, socket_); - Pkt4Ptr rcvd_pkt = pkt_filter.receive(iface, socket_info); + Pkt4Ptr rcvd_pkt = pkt_filter.receive(iface, sock_info_); // Check that the packet has been correctly received. ASSERT_TRUE(rcvd_pkt); diff --git a/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc b/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc index df8898be85..dd5d7f87da 100644 --- a/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -36,66 +37,12 @@ const uint16_t PORT = 10067; /// Size of the buffer holding received packets. const size_t RECV_BUF_SIZE = 2048; -/// This class handles has simple algorithm checking -/// presence of loopback interface and initializing -/// its index. -class PktFilterLPFTest : public ::testing::Test { +// Test fixture class inherits from the class common for all packet +// filter tests. +class PktFilterLPFTest : public isc::dhcp::test::PktFilterTest { public: - - /// @brief Constructor - /// - /// This constructor initializes sock_info_ structure to a default value. - /// The socket descriptors should be set to a negative value to indicate - /// that no socket has been opened. Specific tests will reinitialize this - /// structure with the values of the open sockets. For non-negative socket - /// descriptors, the class destructor will close associated sockets. - PktFilterLPFTest() - : sock_info_(IOAddress("127.0.0.1"), PORT, -1, -1) { - // Initialize ifname_ and ifindex_. - loInit(); + PktFilterLPFTest() : PktFilterTest(PORT) { } - - /// @brief Destructor - /// - /// Closes open sockets (if any). - ~PktFilterLPFTest() { - // Cleanup after each test. This guarantees - // that the sockets do not hang after a test. - if (sock_info_.sockfd_ >= 0) { - close(sock_info_.sockfd_); - } - if (sock_info_.fallbackfd_ >=0) { - close(sock_info_.fallbackfd_); - } - } - - /// @brief Detect loopback interface. - /// - /// @todo this function will be removed once cross-OS interface - /// detection is implemented - void loInit() { - if (if_nametoindex("lo") > 0) { - ifname_ = "lo"; - ifindex_ = if_nametoindex("lo"); - - } else if (if_nametoindex("lo0") > 0) { - ifname_ = "lo0"; - ifindex_ = if_nametoindex("lo0"); - - } else { - std::cout << "Failed to detect loopback interface. Neither " - << "lo nor lo0 worked. Giving up." << std::endl; - FAIL(); - - - - } - } - - std::string ifname_; ///< Loopback interface name - uint16_t ifindex_; ///< Loopback interface index. - SocketInfo sock_info_; ///< A structure holding socket information. - }; // This test verifies that the PktFilterLPF class reports its capability diff --git a/src/lib/dhcp/tests/pkt_filter_test_utils.cc b/src/lib/dhcp/tests/pkt_filter_test_utils.cc new file mode 100644 index 0000000000..8a626231e2 --- /dev/null +++ b/src/lib/dhcp/tests/pkt_filter_test_utils.cc @@ -0,0 +1,112 @@ +// Copyright (C) 2013 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 +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + + +#include +#include + +namespace isc { +namespace dhcp { +namespace test { + +PktFilterTest::PktFilterTest(const uint16_t port) + : port_(port), + sock_info_(isc::asiolink::IOAddress("127.0.0.1"), port, -1, -1) { + // Initialize ifname_ and ifindex_. + loInit(); +} + +PktFilterTest::~PktFilterTest() { + // Cleanup after each test. This guarantees + // that the sockets do not hang after a test. + if (sock_info_.sockfd_ >= 0) { + close(sock_info_.sockfd_); + } + if (sock_info_.fallbackfd_ >=0) { + close(sock_info_.fallbackfd_); + } +} + +void +PktFilterTest::loInit() { + if (if_nametoindex("lo") > 0) { + ifname_ = "lo"; + ifindex_ = if_nametoindex("lo"); + + } else if (if_nametoindex("lo0") > 0) { + ifname_ = "lo0"; + ifindex_ = if_nametoindex("lo0"); + + } else { + std::cout << "Failed to detect loopback interface. Neither " + << "lo nor lo0 worked. Giving up." << std::endl; + FAIL(); + + } +} + +void +PktFilterTest::testDgramSocket(const int sock) const { + // Check that socket has been opened. + ASSERT_GE(sock, 0); + + // Verify that the socket belongs to AF_INET family. + sockaddr_in sock_address; + socklen_t sock_address_len = sizeof(sock_address); + ASSERT_EQ(0, getsockname(sock, + reinterpret_cast(&sock_address), + &sock_address_len)); + EXPECT_EQ(AF_INET, sock_address.sin_family); + + // Verify that the socket is bound the appropriate address. + const std::string bind_addr(inet_ntoa(sock_address.sin_addr)); + EXPECT_EQ("127.0.0.1", bind_addr); + + // Verify that the socket is bound to appropriate port. + EXPECT_EQ(port_, ntohs(sock_address.sin_port)); + + // Verify that the socket has SOCK_DGRAM type. + int sock_type; + socklen_t sock_type_len = sizeof(sock_type); + ASSERT_EQ(0, getsockopt(sock, SOL_SOCKET, SO_TYPE, + &sock_type, &sock_type_len)); + EXPECT_EQ(SOCK_DGRAM, sock_type); +} + +bool +PktFilterStub::isDirectResponseSupported() const { + return (true); +} + +SocketInfo +PktFilterStub::openSocket(const Iface&, + const isc::asiolink::IOAddress& addr, + const uint16_t port, const bool, const bool) { + return (SocketInfo(addr, port, 0)); +} + +Pkt4Ptr +PktFilterStub::receive(const Iface&, const SocketInfo&) { + return Pkt4Ptr(); +} + +int +PktFilterStub::send(const Iface&, uint16_t, const Pkt4Ptr&) { + return (0); +} + + +} // end of isc::dhcp::test namespace +} // end of isc::dhcp namespace +} // end of isc namespace diff --git a/src/lib/dhcp/tests/pkt_filter_test_utils.h b/src/lib/dhcp/tests/pkt_filter_test_utils.h new file mode 100644 index 0000000000..dacec5cd91 --- /dev/null +++ b/src/lib/dhcp/tests/pkt_filter_test_utils.h @@ -0,0 +1,146 @@ +// Copyright (C) 2013 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 +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#ifndef PKT_FILTER_TEST_UTILS_H +#define PKT_FILTER_TEST_UTILS_H + +#include +#include +#include +#include + +namespace isc { +namespace dhcp { +namespace test { + +/// @brief Test fixture class for testing classes derived from PktFilter class. +/// +/// This class implements a simple algorithm checking presence of the loopback +/// interface and initializing its index. It assumes that the loopback interface +/// name is one of 'lo' or 'lo0'. If none of those interfaces is found, the +/// constructor will report a failure. +/// +/// @todo The interface detection algorithm should be more generic. This will +/// be possible once the cross-OS interface detection is implemented. +class PktFilterTest : public ::testing::Test { +public: + + /// @brief Constructor + /// + /// This constructor initializes sock_info_ structure to a default value. + /// The socket descriptors should be set to a negative value to indicate + /// that no socket has been opened. Specific tests will reinitialize this + /// structure with the values of the open sockets. For non-negative socket + /// descriptors, the class destructor will close associated sockets. + PktFilterTest(const uint16_t port); + + /// @brief Destructor + /// + /// Closes open sockets (if any). + virtual ~PktFilterTest(); + + /// @brief Detect loopback interface. + /// + /// @todo this function will be removed once cross-OS interface + /// detection is implemented + void loInit(); + + /// @brief Test that the datagram socket is opened correctly. + /// + /// This function is used by multiple tests. + /// + /// @param sock A descriptor of the open socket. + void testDgramSocket(const int sock) const; + + std::string ifname_; ///< Loopback interface name + uint16_t ifindex_; ///< Loopback interface index. + uint16_t port_; ///< A port number used for the test. + isc::dhcp::SocketInfo sock_info_; ///< A structure holding socket information. + +}; + +/// @brief A stub implementation of the PktFilter class. +/// +/// This class implements abstract methods of the @c isc::dhcp::test::PktFilter +/// class. It is used by unit tests, which test protected methods of the +/// @c isc::dhcp::test::PktFilter class. The implemented abstract methods are +/// no-op. +class PktFilterStub : public PktFilter { +public: + + /// @brief Checks if the direct DHCPv4 response is supported. + /// + /// This function checks if the direct response capability is supported, + /// i.e. if the server can respond to the client which doesn't have an + /// address yet. For this dummy class, the true is alaways returned. + /// + /// @return always true. + virtual bool isDirectResponseSupported() const; + + /// @brief Simulate opening of the socket. + /// + /// This function simulates openinga primary socket. In reality, it doesn't + /// open a socket but the socket descriptor returned in the SocketInfo + /// structure is always set to 0. + /// + /// @param iface An interface descriptor. + /// @param addr Address on the interface to be used to send packets. + /// @param port Port number to bind socket to. + /// @param receive_bcast A flag which indicates if socket should be + /// configured to receive broadcast packets (if true). + /// @param send_bcast A flag which indicates if the socket should be + /// configured to send broadcast packets (if true). + /// + /// @note All parameters are ignored. + /// + /// @return A SocketInfo structure with the socket descriptor set to 0. The + /// fallback socket descriptor is set to a negative value. + virtual SocketInfo openSocket(const Iface& iface, + const isc::asiolink::IOAddress& addr, + const uint16_t port, const bool, const bool); + + /// @brief Simulate reception of the DHCPv4 message. + /// + /// @param iface An interface to be used to receive DHCPv4 message. + /// @param sock_info A descriptor of the primary and fallback sockets. + /// + /// @note All parameters are ignored. + /// + /// @return always a NULL object. + virtual Pkt4Ptr receive(const Iface& iface, const SocketInfo& sock_info); + + /// @brief Simulates sending a DHCPv4 message. + /// + /// This function does nothing. + /// + /// @param iface An interface to be used to send DHCPv4 message. + /// @param port A port used to send a message. + /// @param pkt A DHCPv4 to be sent. + /// + /// @note All parameters are ignored. + /// + /// @return 0. + virtual int send(const Iface& iface, uint16_t port, const Pkt4Ptr& pkt); + + // Change the scope of the protected function so as they can be unit tested. + using PktFilter::openFallbackSocket; + +}; + + +}; // end of isc::dhcp::test namespace +}; // end of isc::dhcp namespace +}; // end of isc namespace + +#endif // PKT_FILTER_TEST_UTILS_H From d0506dcfcf7798859931851c3eb90956dd4bb03c Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 26 Nov 2013 18:37:02 +0100 Subject: [PATCH 07/24] [2765] Implemented the logic which opens fallback a socket. --- src/lib/dhcp/pkt_filter.cc | 28 +++++++- src/lib/dhcp/tests/Makefile.am | 1 + .../dhcp/tests/pkt_filter_inet_unittest.cc | 28 ++------ src/lib/dhcp/tests/pkt_filter_unittest.cc | 65 +++++++++++++++++++ 4 files changed, 96 insertions(+), 26 deletions(-) create mode 100644 src/lib/dhcp/tests/pkt_filter_unittest.cc diff --git a/src/lib/dhcp/pkt_filter.cc b/src/lib/dhcp/pkt_filter.cc index 1279c58e95..92834bf162 100644 --- a/src/lib/dhcp/pkt_filter.cc +++ b/src/lib/dhcp/pkt_filter.cc @@ -13,15 +13,37 @@ // PERFORMANCE OF THIS SOFTWARE. #include +#include #include namespace isc { namespace dhcp { int -PktFilter::openFallbackSocket(const isc::asiolink::IOAddress&, - const uint16_t) { - return (0); +PktFilter::openFallbackSocket(const isc::asiolink::IOAddress& addr, + const uint16_t port) { + // Create socket. + int sock = socket(AF_INET, SOCK_DGRAM, 0); + if (sock < 0) { + isc_throw(SocketConfigError, "failed to create fallback socket for address " + << addr.toText() << ", port " << port); + } + // Bind the socket to a specified address and port. + struct sockaddr_in addr4; + memset(&addr4, 0, sizeof(addr4)); + addr4.sin_family = AF_INET; + addr4.sin_addr.s_addr = htonl(addr); + addr4.sin_port = htons(port); + + if (bind(sock, reinterpret_cast(&addr4), sizeof(addr4)) < 0) { + // Remember to close the socket if we failed to bind it. + close(sock); + isc_throw(SocketConfigError, "failed to bind fallback socket to address " + << addr.toText() << ", port " << port << " - is another DHCP " + "server running?"); + } + // Successfully created and bound a fallback socket. Return a descriptor. + return (sock); } diff --git a/src/lib/dhcp/tests/Makefile.am b/src/lib/dhcp/tests/Makefile.am index a7bd5f5ba1..19678dd93f 100644 --- a/src/lib/dhcp/tests/Makefile.am +++ b/src/lib/dhcp/tests/Makefile.am @@ -48,6 +48,7 @@ libdhcp___unittests_SOURCES += option_string_unittest.cc libdhcp___unittests_SOURCES += option_vendor_unittest.cc libdhcp___unittests_SOURCES += pkt4_unittest.cc libdhcp___unittests_SOURCES += pkt6_unittest.cc +libdhcp___unittests_SOURCES += pkt_filter_unittest.cc libdhcp___unittests_SOURCES += pkt_filter_inet_unittest.cc libdhcp___unittests_SOURCES += pkt_filter_test_utils.h pkt_filter_test_utils.cc diff --git a/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc b/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc index 424b1a2ff7..bce89ad0cc 100644 --- a/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc @@ -64,30 +64,12 @@ TEST_F(PktFilterInetTest, openSocket) { PktFilterInet pkt_filter; sock_info_ = pkt_filter.openSocket(iface, addr, PORT, false, false); - // Check that socket has been opened. - ASSERT_GE(sock_info_.sockfd_, 0); + // For the packet filter in use, the fallback socket shouldn't be opened. + // Fallback is typically opened for raw sockets. + EXPECT_LT(sock_info_.fallbackfd_, 0); - // Verify that the socket belongs to AF_INET family. - sockaddr_in sock_address; - socklen_t sock_address_len = sizeof(sock_address); - ASSERT_EQ(0, getsockname(sock_info_.sockfd_, - reinterpret_cast(&sock_address), - &sock_address_len)); - EXPECT_EQ(AF_INET, sock_address.sin_family); - - // Verify that the socket is bound the appropriate address. - const std::string bind_addr(inet_ntoa(sock_address.sin_addr)); - EXPECT_EQ("127.0.0.1", bind_addr); - - // Verify that the socket is bound to appropriate port. - EXPECT_EQ(PORT, ntohs(sock_address.sin_port)); - - // Verify that the socket has SOCK_DGRAM type. - int sock_type; - socklen_t sock_type_len = sizeof(sock_type); - ASSERT_EQ(0, getsockopt(sock_info_.sockfd_, SOL_SOCKET, SO_TYPE, - &sock_type, &sock_type_len)); - EXPECT_EQ(SOCK_DGRAM, sock_type); + // Test the primary socket. + testDgramSocket(sock_info_.sockfd_); } // This test verifies that the packet is correctly sent over the INET diff --git a/src/lib/dhcp/tests/pkt_filter_unittest.cc b/src/lib/dhcp/tests/pkt_filter_unittest.cc new file mode 100644 index 0000000000..ab7f55a70d --- /dev/null +++ b/src/lib/dhcp/tests/pkt_filter_unittest.cc @@ -0,0 +1,65 @@ +// Copyright (C) 2013 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 +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#include +#include +#include +#include +#include + +using namespace isc::asiolink; +using namespace isc::dhcp; +using namespace isc::dhcp::test; + +namespace { + +/// Port number used by tests. +const uint16_t PORT = 10067; + +class PktFilterBaseClassTest : public isc::dhcp::test::PktFilterTest { +public: + /// @brief Constructor + /// + /// Does nothing but setting up the UDP port for the test. + PktFilterBaseClassTest() : PktFilterTest(PORT) { + } +}; + +// This test verifies that the fallback socket is successfuly opened and +// bound using the protected function of the PktFilter class. +TEST_F(PktFilterBaseClassTest, openFallbackSocket) { + // Open socket using the function under test. Note that, we don't have to + // close the socket on our own because the test fixture constructor + // will handle it. + PktFilterStub pkt_filter; + ASSERT_NO_THROW(sock_info_.fallbackfd_ = + pkt_filter.openFallbackSocket(IOAddress("127.0.0.1"), PORT) + ); + // Test that the socket has been successfully created. + testDgramSocket(sock_info_.fallbackfd_); + + // Now that we have the socket open, let's try to open another one. This + // should cause a binding error. + int another_sock; + EXPECT_THROW(another_sock = + pkt_filter.openFallbackSocket(IOAddress("127.0.0.1"), PORT), + isc::dhcp::SocketConfigError); + // Hard to believe we managed to open another socket. But if so, we have + // to close it to prevent a resource leak. + if (another_sock >= 0) { + close(another_sock); + } +} + +} // anonymous namespace From e4d1f908e14c82062aab9a391299eb0b2692bf9a Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 26 Nov 2013 19:00:47 +0100 Subject: [PATCH 08/24] [2765] Open fallback socket when LPF is used. --- src/lib/dhcp/pkt_filter_lpf.cc | 29 ++++++++--------------- src/lib/dhcp/tests/iface_mgr_unittest.cc | 30 +++++++++++++++++------- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/src/lib/dhcp/pkt_filter_lpf.cc b/src/lib/dhcp/pkt_filter_lpf.cc index 81d53f2e2f..7bb4dac908 100644 --- a/src/lib/dhcp/pkt_filter_lpf.cc +++ b/src/lib/dhcp/pkt_filter_lpf.cc @@ -107,28 +107,17 @@ PktFilterLPF::openSocket(const Iface& iface, const isc::asiolink::IOAddress& addr, const uint16_t port, const bool, const bool) { - // Let's check if a socket is already in use - int sock_check = socket(AF_INET, SOCK_DGRAM, 0); - if (sock_check < 0) { - isc_throw(SocketConfigError, "Failed to create dgram socket"); - } - struct sockaddr_in addr4; - memset(& addr4, 0, sizeof(addr4)); - addr4.sin_family = AF_INET; - addr4.sin_addr.s_addr = htonl(addr); - addr4.sin_port = htons(port); - - if (bind(sock_check, (struct sockaddr *)& addr4, sizeof(addr4)) < 0) { - // We return negative, the proper error message will be displayed - // by the IfaceMgr ... - close(sock_check); - return (SocketInfo(addr, port, -1)); - } - close(sock_check); + // Open fallback socket first. If it fails, it will give us an indication + // that there is another service (perhaps DHCP server) running. + // The function will throw an exception and effectivelly cease opening + // raw socket below. + int fallback = openFallbackSocket(addr, port); + // The fallback is open, so we are good to open primary socket. int sock = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL)); if (sock < 0) { + close(fallback); isc_throw(SocketConfigError, "Failed to create raw LPF socket"); } @@ -146,6 +135,7 @@ PktFilterLPF::openSocket(const Iface& iface, if (setsockopt(sock, SOL_SOCKET, SO_ATTACH_FILTER, &filter_program, sizeof(filter_program)) < 0) { close(sock); + close(fallback); isc_throw(SocketConfigError, "Failed to install packet filtering program" << " on the socket " << sock); } @@ -162,11 +152,12 @@ PktFilterLPF::openSocket(const Iface& iface, if (bind(sock, reinterpret_cast(&sa), sizeof(sa)) < 0) { close(sock); + close(fallback); isc_throw(SocketConfigError, "Failed to bind LPF socket '" << sock << "' to interface '" << iface.getName() << "'"); } - SocketInfo sock_desc(addr, port, sock); + SocketInfo sock_desc(addr, port, sock, fallback); return (sock_desc); } diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index f2bb773488..3d2ed014b6 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -117,7 +117,13 @@ public: IfaceCollection & getIfacesLst() { return ifaces_; } }; -// Dummy class for now, but this will be expanded when needed +/// @brief A test fixture class for IfaceMgr. +/// +/// @todo Sockets being opened by IfaceMgr tests should be managed by +/// the test fixture. In particular, the class should close sockets after +/// each test. Current approach where test cases are responsible for +/// closing sockets is resource leak prone, especially in case of the +/// test failure path. class IfaceMgrTest : public ::testing::Test { public: // These are empty for now, but let's keep them around @@ -1007,17 +1013,23 @@ TEST_F(IfaceMgrTest, checkPacketFilterLPFSocket) { // Then the second use PkFilterLPF mode EXPECT_NO_THROW(iface_mgr2->setMatchingPacketFilter(true)); - // This socket opening attempt should not return positive value - // The first socket already opened same port - EXPECT_NO_THROW( + + // The socket is open and bound. Another attempt to open socket and + // bind to the same address and port should result in an exception. + EXPECT_THROW( socket2 = iface_mgr2->openSocket(LOOPBACK, loAddr, - DHCP4_SERVER_PORT + 10000); + DHCP4_SERVER_PORT + 10000), + isc::dhcp::SocketConfigError ); + // Surprisingly we managed to open another socket. We have to close it + // to prevent resource leak. + if (socket2 >= 0) { + close(socket2); + } - EXPECT_LE(socket2, 0); - - close(socket2); - close(socket1); + if (socket1 >= 0) { + close(socket1); + } } #else From 183693547ce28671d56bc272859b401c47db8f33 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 26 Nov 2013 19:10:35 +0100 Subject: [PATCH 09/24] [2765] Close fallback sockets together with primary sockets. --- src/lib/dhcp/iface_mgr.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index 3b33671bcf..845fd21b7d 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -88,6 +88,10 @@ Iface::closeSockets(const uint16_t family) { // Close and delete the socket and move to the // next one. close(sock->sockfd_); + // Close fallback socket if open. + if (sock->fallbackfd_) { + close(sock->fallbackfd_); + } sockets_.erase(sock++); } else { @@ -148,6 +152,10 @@ bool Iface::delSocket(uint16_t sockfd) { while (sock!=sockets_.end()) { if (sock->sockfd_ == sockfd) { close(sockfd); + // Close fallback socket if open. + if (sock->fallbackfd_) { + close(sock->fallbackfd_); + } sockets_.erase(sock); return (true); //socket found } From ad0866f984b4a223234c4ba5ea1df55c5cf9bff1 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Wed, 27 Nov 2013 11:46:29 +0100 Subject: [PATCH 10/24] [2765] Implemented common utility functions for the PktFilter tests. --- .../dhcp/tests/pkt_filter_inet_unittest.cc | 82 ++--------------- src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc | 89 ++----------------- src/lib/dhcp/tests/pkt_filter_test_utils.cc | 86 +++++++++++++++++- src/lib/dhcp/tests/pkt_filter_test_utils.h | 25 +++++- 4 files changed, 125 insertions(+), 157 deletions(-) diff --git a/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc b/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc index bce89ad0cc..c21a9963ce 100644 --- a/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc @@ -75,27 +75,6 @@ TEST_F(PktFilterInetTest, openSocket) { // This test verifies that the packet is correctly sent over the INET // datagram socket. TEST_F(PktFilterInetTest, send) { - // Let's create a DHCPv4 packet. - Pkt4Ptr pkt(new Pkt4(DHCPOFFER, 0)); - ASSERT_TRUE(pkt); - - // Set required fields. - pkt->setLocalAddr(IOAddress("127.0.0.1")); - pkt->setRemoteAddr(IOAddress("127.0.0.1")); - pkt->setRemotePort(PORT); - pkt->setLocalPort(PORT + 1); - pkt->setIndex(ifindex_); - pkt->setIface(ifname_); - pkt->setHops(6); - pkt->setSecs(42); - pkt->setCiaddr(IOAddress("192.0.2.1")); - pkt->setSiaddr(IOAddress("192.0.2.2")); - pkt->setYiaddr(IOAddress("192.0.2.3")); - pkt->setGiaddr(IOAddress("192.0.2.4")); - - // Create the on-wire data. - ASSERT_NO_THROW(pkt->pack()); - // Packet will be sent over loopback interface. Iface iface(ifname_, ifindex_); IOAddress addr("127.0.0.1"); @@ -109,7 +88,7 @@ TEST_F(PktFilterInetTest, send) { ASSERT_GE(sock_info_.sockfd_, 0); // Send the packet over the socket. - ASSERT_NO_THROW(pkt_filter.send(iface, sock_info_.sockfd_, pkt)); + ASSERT_NO_THROW(pkt_filter.send(iface, sock_info_.sockfd_, test_message_)); // Read the data from socket. fd_set readfds; @@ -135,47 +114,16 @@ TEST_F(PktFilterInetTest, send) { // Parse the packet. ASSERT_NO_THROW(rcvd_pkt->unpack()); - // Verify that the received packet matches sent packet. - EXPECT_EQ(pkt->getHops(), rcvd_pkt->getHops()); - EXPECT_EQ(pkt->getOp(), rcvd_pkt->getOp()); - EXPECT_EQ(pkt->getSecs(), rcvd_pkt->getSecs()); - EXPECT_EQ(pkt->getFlags(), rcvd_pkt->getFlags()); - EXPECT_EQ(pkt->getCiaddr(), rcvd_pkt->getCiaddr()); - EXPECT_EQ(pkt->getSiaddr(), rcvd_pkt->getSiaddr()); - EXPECT_EQ(pkt->getYiaddr(), rcvd_pkt->getYiaddr()); - EXPECT_EQ(pkt->getGiaddr(), rcvd_pkt->getGiaddr()); - EXPECT_EQ(pkt->getTransid(), rcvd_pkt->getTransid()); - EXPECT_TRUE(pkt->getSname() == rcvd_pkt->getSname()); - EXPECT_TRUE(pkt->getFile() == rcvd_pkt->getFile()); - EXPECT_EQ(pkt->getHtype(), rcvd_pkt->getHtype()); - EXPECT_EQ(pkt->getHlen(), rcvd_pkt->getHlen()); + // Check if the received message is correct. + testRcvdMessage(rcvd_pkt); + } // This test verifies that the DHCPv4 packet is correctly received via // INET datagram socket and that it matches sent packet. TEST_F(PktFilterInetTest, receive) { - // Let's create a DHCPv4 packet. - Pkt4Ptr pkt(new Pkt4(DHCPOFFER, 0)); - ASSERT_TRUE(pkt); - // Set required fields. - pkt->setLocalAddr(IOAddress("127.0.0.1")); - pkt->setRemoteAddr(IOAddress("127.0.0.1")); - pkt->setRemotePort(PORT); - pkt->setLocalPort(PORT + 1); - pkt->setIndex(ifindex_); - pkt->setIface(ifname_); - pkt->setHops(6); - pkt->setSecs(42); - pkt->setCiaddr(IOAddress("192.0.2.1")); - pkt->setSiaddr(IOAddress("192.0.2.2")); - pkt->setYiaddr(IOAddress("192.0.2.3")); - pkt->setGiaddr(IOAddress("192.0.2.4")); - - // Create the on-wire data. - ASSERT_NO_THROW(pkt->pack()); - - // Packet will be sent over loopback interface. + // Packet will be received over loopback interface. Iface iface(ifname_, ifindex_); IOAddress addr("127.0.0.1"); @@ -187,8 +135,8 @@ TEST_F(PktFilterInetTest, receive) { sock_info_ = pkt_filter.openSocket(iface, addr, PORT, false, false); ASSERT_GE(sock_info_.sockfd_, 0); - // Send the packet over the socket. - ASSERT_NO_THROW(pkt_filter.send(iface, sock_info_.sockfd_, pkt)); + // Send a DHCPv4 message to the local loopback address and server's port. + sendMessage(); // Receive the packet. Pkt4Ptr rcvd_pkt = pkt_filter.receive(iface, sock_info_); @@ -198,20 +146,8 @@ TEST_F(PktFilterInetTest, receive) { // Parse the packet. ASSERT_NO_THROW(rcvd_pkt->unpack()); - // Verify that the received packet matches sent packet. - EXPECT_EQ(pkt->getHops(), rcvd_pkt->getHops()); - EXPECT_EQ(pkt->getOp(), rcvd_pkt->getOp()); - EXPECT_EQ(pkt->getSecs(), rcvd_pkt->getSecs()); - EXPECT_EQ(pkt->getFlags(), rcvd_pkt->getFlags()); - EXPECT_EQ(pkt->getCiaddr(), rcvd_pkt->getCiaddr()); - EXPECT_EQ(pkt->getSiaddr(), rcvd_pkt->getSiaddr()); - EXPECT_EQ(pkt->getYiaddr(), rcvd_pkt->getYiaddr()); - EXPECT_EQ(pkt->getGiaddr(), rcvd_pkt->getGiaddr()); - EXPECT_EQ(pkt->getTransid(), rcvd_pkt->getTransid()); - EXPECT_TRUE(pkt->getSname() == rcvd_pkt->getSname()); - EXPECT_TRUE(pkt->getFile() == rcvd_pkt->getFile()); - EXPECT_EQ(pkt->getHtype(), rcvd_pkt->getHtype()); - EXPECT_EQ(pkt->getHlen(), rcvd_pkt->getHlen()); + // Check if the received message is correct. + testRcvdMessage(rcvd_pkt); } } // anonymous namespace diff --git a/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc b/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc index dd5d7f87da..b0e5c58fef 100644 --- a/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc @@ -103,32 +103,6 @@ TEST_F(PktFilterLPFTest, DISABLED_openSocket) { // This test verifies correctness of sending DHCP packet through the raw // socket, whereby all IP stack headers are hand-crafted. TEST_F(PktFilterLPFTest, DISABLED_send) { - // Let's create a DHCPv4 packet. - Pkt4Ptr pkt(new Pkt4(DHCPOFFER, 0)); - ASSERT_TRUE(pkt); - - // Set required fields. - // By setting the local address to broadcast we simulate the - // typical scenario when client's request was send to broadcast - // address and server by default used it as a source address - // in its response. The send() function should be able to detect - // it and correct the source address. - pkt->setLocalAddr(IOAddress("255.255.255.255")); - pkt->setRemoteAddr(IOAddress("127.0.0.1")); - pkt->setRemotePort(PORT); - pkt->setLocalPort(PORT + 1); - pkt->setIndex(ifindex_); - pkt->setIface(ifname_); - pkt->setHops(6); - pkt->setSecs(42); - pkt->setCiaddr(IOAddress("192.0.2.1")); - pkt->setSiaddr(IOAddress("192.0.2.2")); - pkt->setYiaddr(IOAddress("192.0.2.3")); - pkt->setGiaddr(IOAddress("192.0.2.4")); - - // Create the on-wire data. - ASSERT_NO_THROW(pkt->pack()); - // Packet will be sent over loopback interface. Iface iface(ifname_, ifindex_); IOAddress addr("127.0.0.1"); @@ -144,7 +118,7 @@ TEST_F(PktFilterLPFTest, DISABLED_send) { ASSERT_GE(sock_info_.sockfd_, 0); // Send the packet over the socket. - ASSERT_NO_THROW(pkt_filter.send(iface, sock_info_.sockfd_, pkt)); + ASSERT_NO_THROW(pkt_filter.send(iface, sock_info_.sockfd_, test_message_)); // Read the data from socket. fd_set readfds; @@ -180,48 +154,15 @@ TEST_F(PktFilterLPFTest, DISABLED_send) { // Parse the packet. ASSERT_NO_THROW(rcvd_pkt->unpack()); - // Verify that the received packet matches sent packet. - EXPECT_EQ(pkt->getHops(), rcvd_pkt->getHops()); - EXPECT_EQ(pkt->getOp(), rcvd_pkt->getOp()); - EXPECT_EQ(pkt->getSecs(), rcvd_pkt->getSecs()); - EXPECT_EQ(pkt->getFlags(), rcvd_pkt->getFlags()); - EXPECT_EQ(pkt->getCiaddr(), rcvd_pkt->getCiaddr()); - EXPECT_EQ(pkt->getSiaddr(), rcvd_pkt->getSiaddr()); - EXPECT_EQ(pkt->getYiaddr(), rcvd_pkt->getYiaddr()); - EXPECT_EQ(pkt->getGiaddr(), rcvd_pkt->getGiaddr()); - EXPECT_EQ(pkt->getTransid(), rcvd_pkt->getTransid()); - EXPECT_TRUE(pkt->getSname() == rcvd_pkt->getSname()); - EXPECT_TRUE(pkt->getFile() == rcvd_pkt->getFile()); - EXPECT_EQ(pkt->getHtype(), rcvd_pkt->getHtype()); - EXPECT_EQ(pkt->getHlen(), rcvd_pkt->getHlen()); + // Check if the received message is correct. + testRcvdMessage(rcvd_pkt); } // This test verifies correctness of reception of the DHCP packet over // raw socket, whereby all IP stack headers are hand-crafted. TEST_F(PktFilterLPFTest, DISABLED_receive) { - // Let's create a DHCPv4 packet. - Pkt4Ptr pkt(new Pkt4(DHCPOFFER, 0)); - ASSERT_TRUE(pkt); - - // Set required fields. - pkt->setLocalAddr(IOAddress("127.0.0.1")); - pkt->setRemoteAddr(IOAddress("127.0.0.1")); - pkt->setRemotePort(PORT); - pkt->setLocalPort(PORT + 1); - pkt->setIndex(ifindex_); - pkt->setIface(ifname_); - pkt->setHops(6); - pkt->setSecs(42); - pkt->setCiaddr(IOAddress("192.0.2.1")); - pkt->setSiaddr(IOAddress("192.0.2.2")); - pkt->setYiaddr(IOAddress("192.0.2.3")); - pkt->setGiaddr(IOAddress("192.0.2.4")); - - // Create the on-wire data. - ASSERT_NO_THROW(pkt->pack()); - - // Packet will be sent over loopback interface. + // Packet will be received over loopback interface. Iface iface(ifname_, ifindex_); IOAddress addr("127.0.0.1"); @@ -233,10 +174,10 @@ TEST_F(PktFilterLPFTest, DISABLED_receive) { sock_info_ = pkt_filter.openSocket(iface, addr, PORT, false, false); ASSERT_GE(sock_info_.sockfd_, 0); - // Send the packet over the socket. - ASSERT_NO_THROW(pkt_filter.send(iface, sock_info_.sockfd_, pkt)); + // Send DHCPv4 message to the local loopback address and server's port. + sendMessage(); - // Receive the packet. + // Receive the packet using LPF packet filter. Pkt4Ptr rcvd_pkt = pkt_filter.receive(iface, sock_info_); // Check that the packet has been correctly received. ASSERT_TRUE(rcvd_pkt); @@ -244,20 +185,8 @@ TEST_F(PktFilterLPFTest, DISABLED_receive) { // Parse the packet. ASSERT_NO_THROW(rcvd_pkt->unpack()); - // Verify that the received packet matches sent packet. - EXPECT_EQ(pkt->getHops(), rcvd_pkt->getHops()); - EXPECT_EQ(pkt->getOp(), rcvd_pkt->getOp()); - EXPECT_EQ(pkt->getSecs(), rcvd_pkt->getSecs()); - EXPECT_EQ(pkt->getFlags(), rcvd_pkt->getFlags()); - EXPECT_EQ(pkt->getCiaddr(), rcvd_pkt->getCiaddr()); - EXPECT_EQ(pkt->getSiaddr(), rcvd_pkt->getSiaddr()); - EXPECT_EQ(pkt->getYiaddr(), rcvd_pkt->getYiaddr()); - EXPECT_EQ(pkt->getGiaddr(), rcvd_pkt->getGiaddr()); - EXPECT_EQ(pkt->getTransid(), rcvd_pkt->getTransid()); - EXPECT_TRUE(pkt->getSname() == rcvd_pkt->getSname()); - EXPECT_TRUE(pkt->getFile() == rcvd_pkt->getFile()); - EXPECT_EQ(pkt->getHtype(), rcvd_pkt->getHtype()); - EXPECT_EQ(pkt->getHlen(), rcvd_pkt->getHlen()); + // Check if the received message is correct. + testRcvdMessage(rcvd_pkt); } } // anonymous namespace diff --git a/src/lib/dhcp/tests/pkt_filter_test_utils.cc b/src/lib/dhcp/tests/pkt_filter_test_utils.cc index 8a626231e2..eb91b9f82a 100644 --- a/src/lib/dhcp/tests/pkt_filter_test_utils.cc +++ b/src/lib/dhcp/tests/pkt_filter_test_utils.cc @@ -12,19 +12,24 @@ // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR // PERFORMANCE OF THIS SOFTWARE. - +#include #include #include +using namespace isc::asiolink; + namespace isc { namespace dhcp { namespace test { PktFilterTest::PktFilterTest(const uint16_t port) : port_(port), - sock_info_(isc::asiolink::IOAddress("127.0.0.1"), port, -1, -1) { + sock_info_(isc::asiolink::IOAddress("127.0.0.1"), port, -1, -1), + send_msg_sock_(-1) { // Initialize ifname_ and ifindex_. loInit(); + // Initialize test_message_. + initTestMessage(); } PktFilterTest::~PktFilterTest() { @@ -36,6 +41,35 @@ PktFilterTest::~PktFilterTest() { if (sock_info_.fallbackfd_ >=0) { close(sock_info_.fallbackfd_); } + if (send_msg_sock_ >= 0) { + close(send_msg_sock_); + } +} + +void +PktFilterTest::initTestMessage() { + // Let's create a DHCPv4 message instance. + test_message_.reset(new Pkt4(DHCPOFFER, 0)); + + // Set required fields. + test_message_->setLocalAddr(IOAddress("127.0.0.1")); + test_message_->setRemoteAddr(IOAddress("127.0.0.1")); + test_message_->setRemotePort(port_); + test_message_->setLocalPort(port_ + 1); + test_message_->setIndex(ifindex_); + test_message_->setIface(ifname_); + test_message_->setHops(6); + test_message_->setSecs(42); + test_message_->setCiaddr(IOAddress("192.0.2.1")); + test_message_->setSiaddr(IOAddress("192.0.2.2")); + test_message_->setYiaddr(IOAddress("192.0.2.3")); + test_message_->setGiaddr(IOAddress("192.0.2.4")); + + try { + test_message_->pack(); + } catch (const isc::Exception& ex) { + ADD_FAILURE() << "failed to create test message for PktFilterTest"; + } } void @@ -56,6 +90,37 @@ PktFilterTest::loInit() { } } +void +PktFilterTest::sendMessage() { + + // Packet will be sent over loopback interface. + Iface iface(ifname_, ifindex_); + IOAddress addr("127.0.0.1"); + + struct sockaddr_in addr4; + memset(&addr4, 0, sizeof(sockaddr)); + addr4.sin_family = AF_INET; + addr4.sin_port = htons(port_ + 1); + + send_msg_sock_ = socket(AF_INET, SOCK_DGRAM, 0); + ASSERT_GE(send_msg_sock_, 0); + + ASSERT_GE(bind(send_msg_sock_, (struct sockaddr *)&addr4, + sizeof(addr4)), 0); + + struct sockaddr_in dest_addr4; + memset(&dest_addr4, 0, sizeof(sockaddr)); + dest_addr4.sin_family = AF_INET; + dest_addr4.sin_port = htons(port_); + ASSERT_EQ(sendto(send_msg_sock_, test_message_->getBuffer().getData(), + test_message_->getBuffer().getLength(), 0, + reinterpret_cast(&dest_addr4), + sizeof(sockaddr)), test_message_->getBuffer().getLength()); + close(send_msg_sock_); + send_msg_sock_ = -1; + +} + void PktFilterTest::testDgramSocket(const int sock) const { // Check that socket has been opened. @@ -84,6 +149,23 @@ PktFilterTest::testDgramSocket(const int sock) const { EXPECT_EQ(SOCK_DGRAM, sock_type); } +void +PktFilterTest::testRcvdMessage(const Pkt4Ptr& rcvd_msg) const { + EXPECT_EQ(test_message_->getHops(), rcvd_msg->getHops()); + EXPECT_EQ(test_message_->getOp(), rcvd_msg->getOp()); + EXPECT_EQ(test_message_->getSecs(), rcvd_msg->getSecs()); + EXPECT_EQ(test_message_->getFlags(), rcvd_msg->getFlags()); + EXPECT_EQ(test_message_->getCiaddr(), rcvd_msg->getCiaddr()); + EXPECT_EQ(test_message_->getSiaddr(), rcvd_msg->getSiaddr()); + EXPECT_EQ(test_message_->getYiaddr(), rcvd_msg->getYiaddr()); + EXPECT_EQ(test_message_->getGiaddr(), rcvd_msg->getGiaddr()); + EXPECT_EQ(test_message_->getTransid(), rcvd_msg->getTransid()); + EXPECT_TRUE(test_message_->getSname() == rcvd_msg->getSname()); + EXPECT_TRUE(test_message_->getFile() == rcvd_msg->getFile()); + EXPECT_EQ(test_message_->getHtype(), rcvd_msg->getHtype()); + EXPECT_EQ(test_message_->getHlen(), rcvd_msg->getHlen()); +} + bool PktFilterStub::isDirectResponseSupported() const { return (true); diff --git a/src/lib/dhcp/tests/pkt_filter_test_utils.h b/src/lib/dhcp/tests/pkt_filter_test_utils.h index dacec5cd91..1b93b06c38 100644 --- a/src/lib/dhcp/tests/pkt_filter_test_utils.h +++ b/src/lib/dhcp/tests/pkt_filter_test_utils.h @@ -50,12 +50,25 @@ public: /// Closes open sockets (if any). virtual ~PktFilterTest(); + /// @brief Initializes DHCPv4 message used by tests. + void initTestMessage(); + /// @brief Detect loopback interface. /// /// @todo this function will be removed once cross-OS interface /// detection is implemented void loInit(); + /// @brief Sends a single DHCPv4 message to the loopback address. + /// + /// This function opens a datagram socket and binds it to the local loopback + /// address and client port. The client's port is assumed to be port_ + 1. + /// The send_msg_sock_ member holds the socket descriptor so as the socket + /// is closed automatically in the destructor. If the function succeeds to + /// send a DHCPv4 message, the socket is closed so as the function can be + /// called again within the same test. + void sendMessage(); + /// @brief Test that the datagram socket is opened correctly. /// /// This function is used by multiple tests. @@ -63,10 +76,18 @@ public: /// @param sock A descriptor of the open socket. void testDgramSocket(const int sock) const; + /// @brief Checks if the received message matches the test_message_. + /// + /// @param rcvd_msg An instance of the message to be tested. + void testRcvdMessage(const Pkt4Ptr& rcvd_msg) const; + std::string ifname_; ///< Loopback interface name uint16_t ifindex_; ///< Loopback interface index. uint16_t port_; ///< A port number used for the test. - isc::dhcp::SocketInfo sock_info_; ///< A structure holding socket information. + isc::dhcp::SocketInfo sock_info_; ///< A structure holding socket info. + int send_msg_sock_; ///< Holds a descriptor of the socket used by + ///< sendMessage function. + Pkt4Ptr test_message_; ///< A DHCPv4 message used by tests. }; @@ -90,7 +111,7 @@ public: /// @brief Simulate opening of the socket. /// - /// This function simulates openinga primary socket. In reality, it doesn't + /// This function simulates opening a primary socket. In reality, it doesn't /// open a socket but the socket descriptor returned in the SocketInfo /// structure is always set to 0. /// From 8cd003e66527e9224b951bebd0c4c4e675e5827d Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Wed, 27 Nov 2013 13:50:32 +0100 Subject: [PATCH 11/24] [2765] Receive and discard data from the fallback socket. --- src/lib/dhcp/pkt_filter.cc | 12 ++++++++++++ src/lib/dhcp/pkt_filter_lpf.cc | 13 +++++++++++++ src/lib/dhcp/tests/pkt_filter_unittest.cc | 11 ++++++++++- 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/lib/dhcp/pkt_filter.cc b/src/lib/dhcp/pkt_filter.cc index 92834bf162..1dfa0a021e 100644 --- a/src/lib/dhcp/pkt_filter.cc +++ b/src/lib/dhcp/pkt_filter.cc @@ -16,6 +16,9 @@ #include #include +#include +#include + namespace isc { namespace dhcp { @@ -42,6 +45,15 @@ PktFilter::openFallbackSocket(const isc::asiolink::IOAddress& addr, << addr.toText() << ", port " << port << " - is another DHCP " "server running?"); } + + // Set socket to non-blocking mode. This is to prevent the read from the fallback + // socket to block message processing on the primary socket. + if (fcntl(sock, F_SETFL, O_NONBLOCK) != 0) { + close(sock); + isc_throw(SocketConfigError, "failed to set SO_NONBLOCK option on the" + " fallback socket, bound to " << addr.toText() << ", port " + << port); + } // Successfully created and bound a fallback socket. Return a descriptor. return (sock); } diff --git a/src/lib/dhcp/pkt_filter_lpf.cc b/src/lib/dhcp/pkt_filter_lpf.cc index 7bb4dac908..315fa7b44c 100644 --- a/src/lib/dhcp/pkt_filter_lpf.cc +++ b/src/lib/dhcp/pkt_filter_lpf.cc @@ -165,6 +165,19 @@ PktFilterLPF::openSocket(const Iface& iface, Pkt4Ptr PktFilterLPF::receive(const Iface& iface, const SocketInfo& socket_info) { uint8_t raw_buf[IfaceMgr::RCVBUFSIZE]; + // First let's get some data from the fallback socket. The data will be + // discarded but we don't want the socket buffer to bloat. We get the + // packets from the socket in loop but most of the time the loop will + // end after receiving one packet. The call to recv returns immediately + // when there is no data left on the socket because the socket is + // non-blocking. + int datalen; + do { + datalen = recv(socket_info.fallbackfd_, raw_buf, sizeof(raw_buf), 0); + } while (datalen >= 0); + + // Now that we finished getting data from the fallback socket, we + // have to get the data from the raw socket too. int data_len = read(socket_info.sockfd_, raw_buf, sizeof(raw_buf)); // If negative value is returned by read(), it indicates that an // error occured. If returned value is 0, no data was read from the diff --git a/src/lib/dhcp/tests/pkt_filter_unittest.cc b/src/lib/dhcp/tests/pkt_filter_unittest.cc index ab7f55a70d..bd48a63772 100644 --- a/src/lib/dhcp/tests/pkt_filter_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_unittest.cc @@ -45,16 +45,25 @@ TEST_F(PktFilterBaseClassTest, openFallbackSocket) { PktFilterStub pkt_filter; ASSERT_NO_THROW(sock_info_.fallbackfd_ = pkt_filter.openFallbackSocket(IOAddress("127.0.0.1"), PORT) + << "Failed to open fallback socket."; ); // Test that the socket has been successfully created. testDgramSocket(sock_info_.fallbackfd_); + // In addition, we should check that the fallback socket is non-blocking. + int sock_flags = fcntl(sock_info_.fallbackfd_, F_GETFL); + EXPECT_EQ(O_NONBLOCK, sock_flags & O_NONBLOCK) + << "Fallback socket is blocking, it should be non-blocking - check" + " fallback socket flags (fcntl)."; + // Now that we have the socket open, let's try to open another one. This // should cause a binding error. int another_sock; EXPECT_THROW(another_sock = pkt_filter.openFallbackSocket(IOAddress("127.0.0.1"), PORT), - isc::dhcp::SocketConfigError); + isc::dhcp::SocketConfigError) + << "it should be not allowed to open and bind two fallback sockets" + " to the same address and port. Surprisingly, the socket bound."; // Hard to believe we managed to open another socket. But if so, we have // to close it to prevent a resource leak. if (another_sock >= 0) { From c62586a2c2f8aecddbb2c1cfd0c00fb7cf41816a Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Thu, 28 Nov 2013 18:46:53 +0100 Subject: [PATCH 12/24] [2765] Gracefully handle socket opening errors in the IfaceMgr. Also, implemented missing unit tests for the Iface function which opens v4 sockets in IfaceMgr. --- src/lib/dhcp/iface_mgr.cc | 56 ++++- src/lib/dhcp/iface_mgr.h | 34 ++- src/lib/dhcp/tests/iface_mgr_unittest.cc | 284 +++++++++++++++++++++- src/lib/dhcp/tests/pkt_filter_unittest.cc | 6 +- 4 files changed, 351 insertions(+), 29 deletions(-) diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index 845fd21b7d..986aadf189 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -292,7 +292,9 @@ void IfaceMgr::stubDetectIfaces() { addInterface(iface); } -bool IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast) { +bool +IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast, + IfaceMgrErrorMsgCallback error_handler) { int sock; int count = 0; @@ -339,26 +341,39 @@ bool IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast) { // bind to INADDR_ANY address but we can do it only once. Thus, // if one socket has been bound we can't do it any further. if (!bind_to_device && bcast_num > 0) { - isc_throw(SocketConfigError, "SO_BINDTODEVICE socket option is" - << " not supported on this OS; therefore, DHCP" - << " server can only listen broadcast traffic on" - << " a single interface"); + handleSocketConfigError("SO_BINDTODEVICE socket option is" + " not supported on this OS;" + " therefore, DHCP server can only" + " listen broadcast traffic on a" + " single interface", + error_handler); } else { - // We haven't open any broadcast sockets yet, so we can - // open at least one more. - sock = openSocket(iface->getName(), *addr, port, true, true); - // Binding socket to an interface is not supported so we can't - // open any more broadcast sockets. Increase the number of - // opened broadcast sockets. + try { + // We haven't open any broadcast sockets yet, so we can + // open at least one more. + sock = openSocket(iface->getName(), *addr, port, + true, true); + } catch (const Exception& ex) { + handleSocketConfigError(ex.what(), error_handler); + + } + // Binding socket to an interface is not supported so we + // can't open any more broadcast sockets. Increase the + // number of open broadcast sockets. if (!bind_to_device) { ++bcast_num; } } } else { - // Not broadcast capable, do not set broadcast flags. - sock = openSocket(iface->getName(), *addr, port, false, false); + try { + // Not broadcast capable, do not set broadcast flags. + sock = openSocket(iface->getName(), *addr, port, + false, false); + } catch (const Exception& ex) { + handleSocketConfigError(ex.what(), error_handler); + } } if (sock < 0) { @@ -467,6 +482,21 @@ bool IfaceMgr::openSockets6(const uint16_t port) { return (count > 0); } +void +IfaceMgr::handleSocketConfigError(const std::string& errmsg, + IfaceMgrErrorMsgCallback handler) { + // If error handler is installed, we don't want to throw an exception, but + // rather call this handler. + if (handler != NULL) { + handler(errmsg); + + } else { + isc_throw(SocketConfigError, errmsg); + + } +} + + void IfaceMgr::printIfaces(std::ostream& out /*= std::cout*/) { for (IfaceCollection::const_iterator iface=ifaces_.begin(); diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index bae67cc426..77c9597bce 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -373,6 +374,13 @@ public: bool inactive6_; }; +/// @brief This type describes the callback function invoked when error occurs +/// in the IfaceMgr. +/// +/// @param errmsg An error message. +typedef +boost::function IfaceMgrErrorMsgCallback; + /// @brief Handles network interfaces, transmission and reception. /// /// IfaceMgr is an interface manager class that detects available network @@ -620,15 +628,22 @@ public: bool openSockets6(const uint16_t port = DHCP6_SERVER_PORT); /// Opens IPv4 sockets on detected interfaces. - /// Will throw exception if socket creation fails. /// /// @param port specifies port number (usually DHCP4_SERVER_PORT) /// @param use_bcast configure sockets to support broadcast messages. + /// @param errcb A pointer to a function which should be called everytime + /// a socket being opened failed. The presence of the callback function + /// (non NULL value) implies that an exception is not thrown when the + /// operation on the socket fails. The process of opening sockets will + /// continue after callback function returns. The socket which failed + /// to open will remain closed. /// - /// @throw SocketOpenFailure if tried and failed to open socket. + /// @throw SocketOpenFailure if tried and failed to open socket and callback + /// function hasn't been specified. /// @return true if any sockets were open bool openSockets4(const uint16_t port = DHCP4_SERVER_PORT, - const bool use_bcast = true); + const bool use_bcast = true, + IfaceMgrErrorMsgCallback errcb = NULL); /// @brief Closes all open sockets. /// Is used in destructor, but also from Dhcpv4Srv and Dhcpv6Srv classes. @@ -855,6 +870,19 @@ private: getLocalAddress(const isc::asiolink::IOAddress& remote_addr, const uint16_t port); + /// @brief Handles an error which occurs during operation on the socket. + /// + /// If the handler callback is specified (non-NULL), this handler is + /// called and the specified error message is passed to it. If the + /// handler is not specified, the @c isc::dhcpSocketConfigError exception + /// is thrown with the specified message. + /// + /// @param errmsg An error message to be passed to a handlder function or + /// to the @c isc::dhcp::SocketConfigError exception. + /// @param handler An error handler function or NULL. + void handleSocketConfigError(const std::string& errmsg, + IfaceMgrErrorMsgCallback handler); + /// Holds instance of a class derived from PktFilter, used by the /// IfaceMgr to open sockets and send/receive packets through these /// sockets. It is possible to supply custom object using diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index 3d2ed014b6..e7fb2bd606 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -78,17 +79,34 @@ public: return (false); } - /// Pretends to open socket. Only records a call to this function. - /// This function returns fake socket descriptor (always the same). - /// Note that the returned value has been selected to be unique - /// (because real values are rather less than 255). Values greater - /// than 255 are not recommended because they cause warnings to be - /// reported by Valgrind when invoking close() on them. - virtual SocketInfo openSocket(const Iface&, + /// @brief Pretend to open a socket. + /// + /// This function doesn't open a real socket. It always returns the + /// same fake socket descriptor. It also records the fact that it has + /// been called in the public open_socket_called_ member. + /// As in the case of opening a real socket, this function will check + /// if there is another fake socket "bound" to the same address and port. + /// If there is, it will throw an exception. This allows to simulate the + /// conditions when one of the sockets can't be open because there is + /// a socket already open and test how IfaceMgr will handle it. + /// + /// @param iface An interface on which the socket is to be opened. + /// @param addr An address to which the socket is to be bound. + /// @param port A port to which the socket is to be bound. + virtual SocketInfo openSocket(const Iface& iface, const isc::asiolink::IOAddress& addr, const uint16_t port, const bool, const bool) { + // Check if there is any other socket bound to the specified address + // and port on this interface. + const Iface::SocketCollection& sockets = iface.getSockets(); + for (Iface::SocketCollection::const_iterator socket = sockets.begin(); + socket != sockets.end(); ++socket) { + if ((socket->addr_ == addr) && (socket->port_ == port)) { + isc_throw(SocketConfigError, "test socket bind error"); + } + } open_socket_called_ = true; return (SocketInfo(addr, port, 255)); } @@ -112,9 +130,83 @@ public: class NakedIfaceMgr: public IfaceMgr { // "Naked" Interface Manager, exposes internal fields public: + + /// @brief Constructor. NakedIfaceMgr() { } - IfaceCollection & getIfacesLst() { return ifaces_; } + + /// @brief Returns the collection of existing interfaces. + IfaceCollection& getIfacesLst() { return (ifaces_); } + + /// @brief This function creates fictious interfaces with fictious + /// addresses. + /// + /// These interfaces can be used in tests that don't actually try + /// to open the sockets on these interfaces. Some tests use mock + /// objects to mimic sockets being open. These interfaces are + /// suitable for such tests. + void createIfaces() { + + ifaces_.clear(); + + // local loopback + ifaces_.push_back(createIface("lo", 0, "127.0.0.1")); + // eth0 + ifaces_.push_back(createIface("eth0", 1, "10.0.0.1")); + // eth1 + ifaces_.push_back(createIface("eth1", 2, "192.0.2.3")); + } + + /// @brief Create an object representing interface. + /// + /// Apart from creating an interface, this function also sets the + /// interface flags: + /// - loopback flag if interface name is "lo" + /// - up always true + /// - running always true + /// - inactive always to false + /// + /// If one needs to modify the default flag settings, the setIfaceFlags + /// function should be used. + /// + /// @param name A name of the interface to be created. + /// @param ifindex An index of the interface to be created. + /// @param addr An IP address to be assigned to the interface. + /// + /// @return An object representing interface. + static Iface createIface(const std::string& name, const int ifindex, + const std::string& addr) { + Iface iface(name, ifindex); + iface.addAddress(IOAddress(addr)); + if (name == "lo") { + iface.flag_loopback_ = true; + } + iface.flag_up_ = true; + iface.flag_running_ = true; + iface.inactive4_ = false; + return (iface); + } + + /// @brief Modified flags on the interface. + /// + /// @param name A name of the interface. + /// @param loopback A new value of the loopback flag. + /// @param up A new value of the up flag. + /// @param running A new value of the running flag. + /// @param inactive A new value of the inactive flag. + void setIfaceFlags(const std::string& name, const bool loopback, + const bool up, const bool running, + const bool inactive) { + for (IfaceMgr::IfaceCollection::iterator iface = ifaces_.begin(); + iface != ifaces_.end(); ++iface) { + if (iface->getName() == name) { + iface->flag_loopback_ = loopback; + iface->flag_up_ = up; + iface->flag_running_ = running; + iface->inactive4_ = inactive; + } + } + } }; /// @brief A test fixture class for IfaceMgr. @@ -126,8 +218,9 @@ public: /// test failure path. class IfaceMgrTest : public ::testing::Test { public: - // These are empty for now, but let's keep them around - IfaceMgrTest() { + /// @brief Constructor. + IfaceMgrTest() + : errors_count_(0) { } ~IfaceMgrTest() { @@ -172,6 +265,26 @@ public: return (NULL); } + /// @brief Implements an IfaceMgr error handler. + /// + /// This function can be installed as an error handler for the + /// IfaceMgr::openSockets4 function. The error handler is invoked + /// when an attempt to open a particular socket fails for any reason. + /// Typically, the error handler will log a warning. When the error + /// handler returns, the openSockets4 function should continue opening + /// sockets on other interfaces. + /// + /// @param errmsg An error string indicating the reason for failure. + void ifaceMgrErrorHandler(const std::string&) { + // Increase the counter of invocations to this function. By checking + // this number, a test amy check if the expected number of errors + // has occurred. + ++errors_count_; + } + + /// Holds the invocation counter for ifaceMgrErrorHandler. + int errors_count_; + }; // We need some known interface to work reliably. Loopback interface is named @@ -1091,6 +1204,157 @@ TEST_F(IfaceMgrTest, socket4) { close(socket1); } +// This test verifies that IPv4 sockets are open on all interfaces (except +// loopback), when interfaces are up, running and active (not disabled from +// the DHCP configuration). +TEST_F(IfaceMgrTest, openSockets4) { + NakedIfaceMgr ifacemgr; + + // Remove all real interfaces and create a set of dummy interfaces. + ifacemgr.createIfaces(); + + // Use the custom packet filter object. This object mimics the socket + // opening operation - the real socket is not open. + boost::shared_ptr custom_packet_filter(new TestPktFilter()); + ASSERT_TRUE(custom_packet_filter); + ASSERT_NO_THROW(ifacemgr.setPacketFilter(custom_packet_filter)); + + // Simulate opening sockets using the dummy packet filter. + ASSERT_NO_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, NULL)); + + // Expect that the sockets are open on both eth0 and eth1. + EXPECT_EQ(1, ifacemgr.getIface("eth0")->getSockets().size()); + EXPECT_EQ(1, ifacemgr.getIface("eth1")->getSockets().size()); + // Socket shouldn't have been opened on loopback. + EXPECT_TRUE(ifacemgr.getIface("lo")->getSockets().empty()); +} + +// This test verifies that the socket is not open on the interface which is +// down, but sockets are open on all other non-loopback interfaces. +TEST_F(IfaceMgrTest, openSockets4IfaceDown) { + NakedIfaceMgr ifacemgr; + + // Remove all real interfaces and create a set of dummy interfaces. + ifacemgr.createIfaces(); + + boost::shared_ptr custom_packet_filter(new TestPktFilter()); + ASSERT_TRUE(custom_packet_filter); + ASSERT_NO_THROW(ifacemgr.setPacketFilter(custom_packet_filter)); + + // Boolean parameters specify that eth0 is: + // - not a loopback + // - is "down" (not up) + // - is not running + // - is active (is not inactive) + ifacemgr.setIfaceFlags("eth0", false, false, true, false); + ASSERT_FALSE(ifacemgr.getIface("eth0")->flag_up_); + ASSERT_NO_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, NULL)); + + // There should be no socket on eth0 open, because interface was down. + EXPECT_TRUE(ifacemgr.getIface("eth0")->getSockets().empty()); + // Expecting that the socket is open on eth1 because it was up, running + // and active. + EXPECT_EQ(1, ifacemgr.getIface("eth1")->getSockets().size()); + // Never open socket on loopback interface. + EXPECT_TRUE(ifacemgr.getIface("lo")->getSockets().empty()); +} + +// This test verifies that the socket is not open on the interface which is +// disabled from the DHCP configuration, but sockets are open on all other +// non-loopback interfaces. +TEST_F(IfaceMgrTest, openSockets4IfaceInactive) { + NakedIfaceMgr ifacemgr; + + // Remove all real interfaces and create a set of dummy interfaces. + ifacemgr.createIfaces(); + + boost::shared_ptr custom_packet_filter(new TestPktFilter()); + ASSERT_TRUE(custom_packet_filter); + ASSERT_NO_THROW(ifacemgr.setPacketFilter(custom_packet_filter)); + + // Boolean parameters specify that eth1 is: + // - not a loopback + // - is up + // - is running + // - is inactive + ifacemgr.setIfaceFlags("eth1", false, true, true, true); + ASSERT_TRUE(ifacemgr.getIface("eth1")->inactive4_); + ASSERT_NO_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, NULL)); + + // The socket on eth0 should be open because interface is up, running and + // active (not disabled through DHCP configuration, for example). + EXPECT_EQ(1, ifacemgr.getIface("eth0")->getSockets().size()); + // There should be no socket open on eth1 because it was marked inactive. + EXPECT_TRUE(ifacemgr.getIface("eth1")->getSockets().empty()); + // Sockets are not open on loopback interfaces too. + EXPECT_TRUE(ifacemgr.getIface("lo")->getSockets().empty()); +} + +// Test that exception is thrown when trying to bind a new socket to the port +// and address which is already in use by another socket. +TEST_F(IfaceMgrTest, openSockets4NoErrorHandler) { + NakedIfaceMgr ifacemgr; + + // Remove all real interfaces and create a set of dummy interfaces. + ifacemgr.createIfaces(); + + boost::shared_ptr custom_packet_filter(new TestPktFilter()); + ASSERT_TRUE(custom_packet_filter); + ASSERT_NO_THROW(ifacemgr.setPacketFilter(custom_packet_filter)); + + // Open socket on eth1. The openSockets4 should detect that this + // socket has been already open and an attempt to open another socket + // and bind to this address and port should fail. + ASSERT_NO_THROW(ifacemgr.openSocket("eth1", IOAddress("192.0.2.3"), + DHCP4_SERVER_PORT)); + + // The function throws an exception when it tries to open a socket + // and bind it to the address in use. + EXPECT_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, NULL), + isc::dhcp::SocketConfigError); + +} + +// Test that the external error handler is called when trying to bind a new +// socket to the address and port being in use. The sockets on the other +// interfaces should open just fine.. +TEST_F(IfaceMgrTest, openSocket4ErrorHandler) { + NakedIfaceMgr ifacemgr; + + // Remove all real interfaces and create a set of dummy interfaces. + ifacemgr.createIfaces(); + + boost::shared_ptr custom_packet_filter(new TestPktFilter()); + ASSERT_TRUE(custom_packet_filter); + ASSERT_NO_THROW(ifacemgr.setPacketFilter(custom_packet_filter)); + + // Open socket on eth0. The openSockets4 should detect that this + // socket has been already open and an attempt to open another socket + // and bind to this address and port should fail. + ASSERT_NO_THROW(ifacemgr.openSocket("eth0", IOAddress("10.0.0.1"), + DHCP4_SERVER_PORT)); + + // Install an error handler before trying to open sockets. This handler + // should be called when the IfaceMgr fails to open socket on eth0. + isc::dhcp::IfaceMgrErrorMsgCallback error_handler = + boost::bind(&IfaceMgrTest::ifaceMgrErrorHandler, this, _1); + ASSERT_NO_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, error_handler)); + // We expect that an error occured when we tried to open a socket on + // eth0, but the socket on eth1 should open just fine. + EXPECT_EQ(1, errors_count_); + + // Reset errors count. + errors_count_ = 0; + + // Now that we have two sockets open, we can try this again but this time + // we should get two errors: one when opening a socket on eth0, another one + // when opening a socket on eth1. + ASSERT_NO_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, error_handler)); + EXPECT_EQ(2, errors_count_); + +} + + // Test the Iface structure itself TEST_F(IfaceMgrTest, iface) { boost::scoped_ptr iface; diff --git a/src/lib/dhcp/tests/pkt_filter_unittest.cc b/src/lib/dhcp/tests/pkt_filter_unittest.cc index bd48a63772..e677de9252 100644 --- a/src/lib/dhcp/tests/pkt_filter_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_unittest.cc @@ -44,9 +44,9 @@ TEST_F(PktFilterBaseClassTest, openFallbackSocket) { // will handle it. PktFilterStub pkt_filter; ASSERT_NO_THROW(sock_info_.fallbackfd_ = - pkt_filter.openFallbackSocket(IOAddress("127.0.0.1"), PORT) - << "Failed to open fallback socket."; - ); + pkt_filter.openFallbackSocket(IOAddress("127.0.0.1"), PORT)) + << "Failed to open fallback socket."; + // Test that the socket has been successfully created. testDgramSocket(sock_info_.fallbackfd_); From b52a45ce15d894e540d0c8011786a913630dbffc Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Thu, 28 Nov 2013 19:16:36 +0100 Subject: [PATCH 13/24] [2765] Improved commentary in the IfaceMgr. --- src/lib/dhcp/iface_mgr.h | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index 77c9597bce..731de413ee 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -619,7 +619,18 @@ public: /// Opens IPv6 sockets on detected interfaces. /// - /// Will throw exception if socket creation fails. + /// @todo This function will throw an exception immediately when a socket + /// fails to open. This is undersired behavior because it will preclude + /// other sockets from opening. We should strive to provide similar mechanism + /// that has been introduced for V4 sockets. If socket creation fails the + /// appropriate error handler is called and once the handler returns the + /// function contnues to open other sockets. The change in the IfaceMgr + /// is quite straight forward and it is proven to work for V4. However, + /// unit testing it is a bit involved, because for unit testing we need + /// a replacement of the openSocket6 function which will mimic the + /// behavior of the real socket opening. For the V4 we have the means to + /// to achieve that with the replaceable PktFilter class. For V6, the + /// implementation is hardcoded in the openSocket6. /// /// @param port specifies port number (usually DHCP6_SERVER_PORT) /// @@ -631,19 +642,19 @@ public: /// /// @param port specifies port number (usually DHCP4_SERVER_PORT) /// @param use_bcast configure sockets to support broadcast messages. - /// @param errcb A pointer to a function which should be called everytime - /// a socket being opened failed. The presence of the callback function - /// (non NULL value) implies that an exception is not thrown when the - /// operation on the socket fails. The process of opening sockets will - /// continue after callback function returns. The socket which failed - /// to open will remain closed. + /// @param error_handler A pointer to a callback function which is called + /// by the openSockets4 when it fails to open a socket. This parameter + /// can be NULL to indicate that the callback should not be used. In such + /// case the @c isc::dhcp::SocketConfigError exception is thrown instead. + /// When a callback is installed the function will continue when callback + /// returns control. /// /// @throw SocketOpenFailure if tried and failed to open socket and callback /// function hasn't been specified. /// @return true if any sockets were open bool openSockets4(const uint16_t port = DHCP4_SERVER_PORT, const bool use_bcast = true, - IfaceMgrErrorMsgCallback errcb = NULL); + IfaceMgrErrorMsgCallback error_handler = NULL); /// @brief Closes all open sockets. /// Is used in destructor, but also from Dhcpv4Srv and Dhcpv6Srv classes. From 3ca6b9a20ba4d9931a686fd5b684166d413d700f Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Fri, 29 Nov 2013 13:30:03 +0100 Subject: [PATCH 14/24] [2765] Invoke error handler if the interface opening failed. --- src/bin/dhcp4/dhcp4_messages.mes | 4 ++++ src/bin/dhcp4/dhcp4_srv.cc | 9 ++++++++- src/bin/dhcp4/dhcp4_srv.h | 9 +++++++++ src/bin/dhcp4/tests/dhcp4_srv_unittest.cc | 20 ++++++++++++++++++++ src/bin/dhcp4/tests/dhcp4_test_utils.cc | 1 + src/lib/dhcp/iface_mgr.h | 18 ++++++++++-------- 6 files changed, 52 insertions(+), 9 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 1bfce66e71..ae6c9bbae6 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -147,6 +147,10 @@ IPv4 DHCP server but it is not running. A debug message issued during startup, this indicates that the IPv4 DHCP server is about to open sockets on the specified port. +% DHCP4_OPEN_SOCKET_FAIL failed to create socket: %1 +A warning message issued when IfaceMgr fails to open and bind a socket. The reason +for the failure is appended as an argument of the log message. + % DHCP4_PACKET_PARSE_FAIL failed to parse incoming packet: %1 The IPv4 DHCP server has received a packet that it is unable to interpret. The reason why the packet is invalid is included in the message. diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 4ac24dd15b..8a73b1abe4 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -1241,7 +1241,9 @@ Dhcpv4Srv::openActiveSockets(const uint16_t port, // sockets are marked active or inactive. // @todo Optimization: we should not reopen all sockets but rather select // those that have been affected by the new configuration. - if (!IfaceMgr::instance().openSockets4(port, use_bcast)) { + isc::dhcp::IfaceMgrErrorMsgCallback error_handler = + boost::bind(&Dhcpv4Srv::ifaceMgrSocket4ErrorHandler, _1); + if (!IfaceMgr::instance().openSockets4(port, use_bcast, error_handler)) { LOG_WARN(dhcp4_logger, DHCP4_NO_SOCKETS_OPEN); } } @@ -1335,6 +1337,11 @@ Dhcpv4Srv::unpackOptions(const OptionBuffer& buf, return (offset); } +void +Dhcpv4Srv::ifaceMgrSocket4ErrorHandler(const std::string& errmsg) { + // Log the reason for socket opening failure and return. + LOG_WARN(dhcp4_logger, DHCP4_OPEN_SOCKET_FAIL).arg(errmsg); +} } // namespace dhcp } // namespace isc diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index 66fe3a6315..5bc015ed20 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -381,6 +381,15 @@ private: /// @return Option that contains netmask information static OptionPtr getNetmaskOption(const Subnet4Ptr& subnet); + /// @brief Implements the error handler for socket open failure. + /// + /// This callback function is installed on the @c isc::dhcp::IfaceMgr + /// when IPv4 sockets are being open. When socket fails to open for + /// any reason, this function is called. It simply logs the error message. + /// + /// @param errmsg An error message containing a cause of the failure. + static void ifaceMgrSocket4ErrorHandler(const std::string& errmsg); + /// @brief Allocation Engine. /// Pointer to the allocation engine that we are currently using /// It must be a pointer, because we will support changing engines diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index 1570ca917e..dcce78c992 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -191,6 +191,26 @@ TEST_F(Dhcpv4SrvTest, basic) { EXPECT_TRUE(naked_srv->getServerID()); } +// This test verifies that exception is not thrown when an error occurs during +// opening sockets. This test forces an error by adding a fictious interface +// to the IfaceMgr. An attempt to open socket on this interface must always +// fail. The DHCPv4 installs the error handler function to prevent exceptions +// being thrown from the openSockets4 function. +// @todo The server tests for socket should be extended but currently the +// ability to unit test the sockets code is somewhat limited. +TEST_F(Dhcpv4SrvTest, openActiveSockets) { + ASSERT_NO_THROW(CfgMgr::instance().activateAllIfaces()); + + Iface iface("bogusiface", 255); + iface.flag_loopback_ = false; + iface.flag_up_ = true; + iface.flag_running_ = true; + iface.inactive4_ = false; + iface.addAddress(IOAddress("192.0.0.0")); + IfaceMgr::instance().addInterface(iface); + ASSERT_NO_THROW(Dhcpv4Srv::openActiveSockets(DHCP4_SERVER_PORT, false)); +} + // This test verifies that the destination address of the response // message is set to giaddr, when giaddr is set to non-zero address // in the received message. diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index 56e96f18f7..e29c31d2f0 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc @@ -44,6 +44,7 @@ Dhcpv4SrvTest::Dhcpv4SrvTest() pool_ = Pool4Ptr(new Pool4(IOAddress("192.0.2.100"), IOAddress("192.0.2.110"))); subnet_->addPool(pool_); + CfgMgr::instance().deleteActiveIfaces(); CfgMgr::instance().deleteSubnets4(); CfgMgr::instance().addSubnet4(subnet_); diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index 731de413ee..d308d2a8a7 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -424,7 +424,7 @@ public: /// @return true if direct response is supported. bool isDirectResponseSupported() const; - /// @brief Returns interface with specified interface index + /// @brief Returns interfac specified interface index /// /// @param ifindex index of searched interface /// @@ -732,6 +732,15 @@ public: /// not having address assigned. void setMatchingPacketFilter(const bool direct_response_desired = false); + /// @brief Adds an interface to list of known interfaces. + /// + /// @param iface reference to Iface object. + /// @note This function must be public because it has to be callable + /// from unit tests. + void addInterface(const Iface& iface) { + ifaces_.push_back(iface); + } + /// A value of socket descriptor representing "not specified" state. static const int INVALID_SOCKET = -1; @@ -776,13 +785,6 @@ protected: /// @return socket descriptor int openSocket6(Iface& iface, const isc::asiolink::IOAddress& addr, uint16_t port); - /// @brief Adds an interface to list of known interfaces. - /// - /// @param iface reference to Iface object. - void addInterface(const Iface& iface) { - ifaces_.push_back(iface); - } - /// @brief Detects network interfaces. /// /// This method will eventually detect available interfaces. For now From 5b9c261d8b66d61f1df4a088e9a283855332c392 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Fri, 29 Nov 2013 13:57:18 +0100 Subject: [PATCH 15/24] [2765] Fixed error handling in the IfaceMgr::openSockets4. --- src/lib/dhcp/iface_mgr.cc | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index 986aadf189..bd13598404 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -295,7 +295,6 @@ void IfaceMgr::stubDetectIfaces() { bool IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast, IfaceMgrErrorMsgCallback error_handler) { - int sock; int count = 0; // This option is used to bind sockets to particular interfaces. @@ -332,6 +331,7 @@ IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast, continue; } + int sock = -1; // If selected interface is broadcast capable set appropriate // options on the socket so as it can receive and send broadcast // messages. @@ -347,6 +347,7 @@ IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast, " listen broadcast traffic on a" " single interface", error_handler); + continue; } else { try { @@ -356,6 +357,7 @@ IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast, true, true); } catch (const Exception& ex) { handleSocketConfigError(ex.what(), error_handler); + continue; } // Binding socket to an interface is not supported so we @@ -373,17 +375,19 @@ IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast, false, false); } catch (const Exception& ex) { handleSocketConfigError(ex.what(), error_handler); + continue; } } if (sock < 0) { const char* errstr = strerror(errno); - isc_throw(SocketConfigError, "failed to open IPv4 socket" - << " supporting broadcast traffic, reason:" - << errstr); + handleSocketConfigError(std::string("failed to open IPv4 socket," + " reason:") + errstr, + error_handler); + } else { + ++count; } - count++; } } return (count > 0); From 331b10dc0682b41939e9a184e3c4a2cc9b132803 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Fri, 29 Nov 2013 14:58:52 +0100 Subject: [PATCH 16/24] [2765] Fix an order of parameters passed to the SocketInfo constructor. --- tests/tools/perfdhcp/test_control.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tools/perfdhcp/test_control.cc b/tests/tools/perfdhcp/test_control.cc index d94f032509..7ffe9fa05e 100644 --- a/tests/tools/perfdhcp/test_control.cc +++ b/tests/tools/perfdhcp/test_control.cc @@ -47,7 +47,7 @@ namespace perfdhcp { bool TestControl::interrupted_ = false; TestControl::TestControlSocket::TestControlSocket(const int socket) : - SocketInfo(socket, asiolink::IOAddress("127.0.0.1"), 0), + SocketInfo(asiolink::IOAddress("127.0.0.1"), 0, socket), ifindex_(0), valid_(true) { try { initSocketData(); From 00d6ca046beb9f2e494043d4bc1397859d554f6f Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Fri, 29 Nov 2013 18:45:38 +0100 Subject: [PATCH 17/24] [2765] Initialize variable used in unit test to prevent compilation failure --- src/lib/dhcp/tests/pkt_filter_unittest.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/dhcp/tests/pkt_filter_unittest.cc b/src/lib/dhcp/tests/pkt_filter_unittest.cc index e677de9252..eef7fea13d 100644 --- a/src/lib/dhcp/tests/pkt_filter_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_unittest.cc @@ -58,7 +58,7 @@ TEST_F(PktFilterBaseClassTest, openFallbackSocket) { // Now that we have the socket open, let's try to open another one. This // should cause a binding error. - int another_sock; + int another_sock = -1; EXPECT_THROW(another_sock = pkt_filter.openFallbackSocket(IOAddress("127.0.0.1"), PORT), isc::dhcp::SocketConfigError) From 65238e01cb0390c23e4937ed7710e968514fb858 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Fri, 29 Nov 2013 18:54:48 +0100 Subject: [PATCH 18/24] [2765] Pass error handler when opening sockets in the constructor. --- src/bin/dhcp4/dhcp4_srv.cc | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 8a73b1abe4..0af9ec2f0c 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -108,10 +108,15 @@ Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const char* dbconfig, const bool use_bcast, // will be able to respond directly. IfaceMgr::instance().setMatchingPacketFilter(direct_response_desired); + // Open sockets only if port is non-zero. Port 0 is used + // for non-socket related testing. if (port) { - // open sockets only if port is non-zero. Port 0 is used - // for non-socket related testing. - IfaceMgr::instance().openSockets4(port_, use_bcast_); + // Create error handler. This handler will be called every time + // the socket opening operation fails. We use this handler to + // log a warning. + isc::dhcp::IfaceMgrErrorMsgCallback error_handler = + boost::bind(&Dhcpv4Srv::ifaceMgrSocket4ErrorHandler, _1); + IfaceMgr::instance().openSockets4(port_, use_bcast_, error_handler); } string srvid_file = CfgMgr::instance().getDataDir() + "/" + string(SERVER_ID_FILE); From fe717f9cfeebf48714de35b1c8f4b1ce323ca67f Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Fri, 29 Nov 2013 18:59:05 +0100 Subject: [PATCH 19/24] [2765] Fixed a typo in the doxygen documentation. --- src/lib/dhcp/pkt_filter.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/dhcp/pkt_filter.h b/src/lib/dhcp/pkt_filter.h index 1636abd316..e9b661b2d8 100644 --- a/src/lib/dhcp/pkt_filter.h +++ b/src/lib/dhcp/pkt_filter.h @@ -125,7 +125,7 @@ protected: /// (a.k.a. primary socket) used to receive and handle DHCPv4 traffic. The /// traffic received through the fallback should be dropped. The reasoning /// behind opening the fallback socket is explained in the documentation of - /// @s isc::dhcp::SocketInfo structure. + /// @c isc::dhcp::SocketInfo structure. /// /// @param addr An IPv4 address to bind the socket to. /// @param port A port number to bind socket to. From 9fdc1f5af7844126fdc5b578110ab91879666132 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Mon, 2 Dec 2013 11:58:18 +0100 Subject: [PATCH 20/24] [2765] Updated Developer's guide with the section about raw sockets use. --- doc/devel/mainpage.dox | 1 + src/lib/dhcp/libdhcp++.dox | 50 +++++++++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/doc/devel/mainpage.dox b/doc/devel/mainpage.dox index 2f4b339348..c85615de18 100644 --- a/doc/devel/mainpage.dox +++ b/doc/devel/mainpage.dox @@ -66,6 +66,7 @@ * - @subpage libdhcpIntro * - @subpage libdhcpRelay * - @subpage libdhcpIfaceMgr + * - @subpage libdhcpPktFilter * - @subpage libdhcpsrv * - @subpage leasemgr * - @subpage cfgmgr diff --git a/src/lib/dhcp/libdhcp++.dox b/src/lib/dhcp/libdhcp++.dox index eabc3926c7..34993afc65 100644 --- a/src/lib/dhcp/libdhcp++.dox +++ b/src/lib/dhcp/libdhcp++.dox @@ -1,4 +1,4 @@ -// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2013 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 @@ -137,4 +137,52 @@ Another useful methods are dedicated to transmission Note that receive4() and receive6() methods may return NULL, e.g. when timeout is reached or if dhcp daemon receives a signal. +@section libdhcpPktFilter Switchable Packet Filter objects used by Interface Manager + +The well known problem of DHCPv4 implementation is that it must be able to +provision devices which don't have an IPv4 address yet (the IPv4 address is +one of the configuration parameters provided by DHCP server to a client). +One way to communicate with such a device is to send server's response to +a broadcast address. An obvious drawback of this approach is that the server's +response will be received and processed by all clients in the particular +network. Therefore, the preferred approach is that the server unicasts its +response to a new address being assigned for the client. This client will +identify itself as a target of this message by checking chaddr and/or +Client Identifier value. In the same time, the other clients in the network +will not receive the unicast message. The major problem that arises with this +approach is that the client without an IP address doesn't respond to ARP +messages. As a result, server's response will not be sent over IP/UDP +socket because the system kernel will fail to resolve client's link-layer +address. + +Kea supports the use of raw sockets to create a complete Data-link/IP/UDP/DHCPv4 +stack. By creating each layer of the outgoing packet, the Kea logic has full +control over the frame contents and it may bypass the use of ARP to inject the +link layer address into the frame. The raw socket is bound to a specific interface, +not to the IP address/UDP port. Therefore, the system kernel doesn't have +means to verify that Kea is listening to the DHCP traffic on the specific address +and port. This has two major implications: +- It is possible to run another DHCPv4 sever instance which will bind socket to the +same address and port. +- An attempt to send a unicast message to the DHCPv4 server will result in ICMP +"Port Unreachable" message being sent by the kernel (which is unaware that the +DHCPv4 service is actually running). +In order to overcome these issues, the isc::dhcp::PktFilterLPF opens a +regular IP/UDP socket which coexists with the raw socket. The socket is referred +to as "fallback socket" in the Kea code. All packets received through this socket +are discarded. + +In general, the use of datagram sockets is preferred over raw sockets. +For convenience, the switchable Packet Filter objects are used to manage +sockets for different purposes. These objects implement the socket opening +operation and sending/receiving messages over this socket. For example: +the isc::dhcp::PktFilterLPF object opens a raw socket. +The isc::dhcp::PktFilterLPF::send and isc::dhcp::PktFilterLPF::receive +methods encode/decode full data-link/IP/UDP/DHCPv4 stack. The +isc::dhcp::PktFilterInet supports sending and receiving messages over +the regular IP/UDP socket. The isc::dhcp::PktFilterInet should be used in all +cases when an application using the libdhcp++ doesn't require sending +DHCP messages to a device which doesn't have an address yet. + + */ From ddf389fa6171e8b6b57bf8b359d5b2db45727dc1 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 3 Dec 2013 13:58:31 +0100 Subject: [PATCH 21/24] [2765] Addressed review comments. --- src/lib/dhcp/iface_mgr.cc | 4 +- src/lib/dhcp/iface_mgr.h | 70 +++++++++++++++++++++--- src/lib/dhcp/pkt_filter.cc | 21 ++++--- src/lib/dhcp/pkt_filter_inet.h | 8 +++ src/lib/dhcp/pkt_filter_lpf.cc | 13 ++++- src/lib/dhcp/tests/iface_mgr_unittest.cc | 4 +- 6 files changed, 96 insertions(+), 24 deletions(-) diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index bd13598404..44934ec564 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -89,7 +89,7 @@ Iface::closeSockets(const uint16_t family) { // next one. close(sock->sockfd_); // Close fallback socket if open. - if (sock->fallbackfd_) { + if (sock->fallbackfd_ >= 0) { close(sock->fallbackfd_); } sockets_.erase(sock++); @@ -153,7 +153,7 @@ bool Iface::delSocket(uint16_t sockfd) { if (sock->sockfd_ == sockfd) { close(sockfd); // Close fallback socket if open. - if (sock->fallbackfd_) { + if (sock->fallbackfd_ >= 0) { close(sock->fallbackfd_); } sockets_.erase(sock); diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index d308d2a8a7..0302e24fa6 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -617,7 +617,7 @@ public: const uint16_t port); - /// Opens IPv6 sockets on detected interfaces. + /// @brief Opens IPv6 sockets on detected interfaces. /// /// @todo This function will throw an exception immediately when a socket /// fails to open. This is undersired behavior because it will preclude @@ -638,16 +638,64 @@ public: /// @return true if any sockets were open bool openSockets6(const uint16_t port = DHCP6_SERVER_PORT); - /// Opens IPv4 sockets on detected interfaces. + /// @brief Opens IPv4 sockets on detected interfaces. + /// + /// This function attempts to open sockets on all interfaces which have been + /// detected by @c IfaceMgr and meet the following conditions: + /// - interface is not a local loopback, + /// - interface is running (connected), + /// - interface is up, + /// - interface is active, e.g. selected from the configuration to be used + /// to listen DHCPv4 messages, + /// - interface has an IPv4 address assigned. + /// + /// The type of the socket being open depends on the selected Packet Filter + /// represented by a class derived from @c isc::dhcp::PktFilter abstract + /// class. + /// + /// It is possible to specify whether sockets should be broadcast capable. + /// In most of the cases, the sockets should support broadcast traffic, e.g. + /// DHCPv4 server and relay need to listen to broadcast messages sent by + /// clients. If the socket has to be open on the particular interface, this + /// interface must have broadcast flag set. If this condition is not met, + /// the socket will be created in the unicast-only mode. If there are + /// multiple broadcast-capable interfaces present, they may be all open + /// in a broadcast mode only if the OS supports SO_BINDTODEVICE (bind socket + /// to a device) socket option. If this option is not supported, only the + /// first broadcast-capable socket will be opened in the broadcast mode. + /// The error will be reported for sockets being opened on other interfaces. + /// If the socket is bound to a device (interface), the broadcast traffic + /// sent to this interface will be received on this interface only. + /// This allows the DHCPv4 server or relay to detect the interface on which + /// the broadcast message has been received. This interface is later used + /// to send a response. + /// + /// On the systems with multiple interfaces, it is often desired that the + /// failure to open a socket on a particular interface doesn't cause a + /// fatal error and sockets should be opened on remaining interfaces. + /// However, the warning about the failure for the particular socket should + /// be communicated to the caller. The libdhcp++ is a common library with + /// no logger associated with it. Most of the functions in this library + /// communicate errors via exceptions. In case of openSockets4 function + /// exception must not be thrown if the function is supposed to continue + /// opening sockets, despite an error. Therefore, if such a behavior is + /// desired, the error handler function can be passed as a parameter. + /// This error handler is called (if present) with an error string. + /// Typically, error handler will simply log an error using an application + /// logger, but it can do more sophisticated error handling too. + /// + /// @todo It is possible that additional parameters will have to be added + /// to the error handler, e.g. Iface if it was really supposed to do + /// some more sophisticated error handling. + /// + /// If the error handler is not installed (is NULL), the exception is thrown + /// for each failure (default behavior). /// /// @param port specifies port number (usually DHCP4_SERVER_PORT) /// @param use_bcast configure sockets to support broadcast messages. - /// @param error_handler A pointer to a callback function which is called - /// by the openSockets4 when it fails to open a socket. This parameter - /// can be NULL to indicate that the callback should not be used. In such - /// case the @c isc::dhcp::SocketConfigError exception is thrown instead. - /// When a callback is installed the function will continue when callback - /// returns control. + /// @param error_handler A pointer to an error handler function which is + /// called by the openSockets4 when it fails to open a socket. This + /// parameter can be NULL to indicate that the callback should not be used. /// /// @throw SocketOpenFailure if tried and failed to open socket and callback /// function hasn't been specified. @@ -883,13 +931,17 @@ private: getLocalAddress(const isc::asiolink::IOAddress& remote_addr, const uint16_t port); - /// @brief Handles an error which occurs during operation on the socket. + /// @brief Handles an error which occurs during configuration of a socket. /// /// If the handler callback is specified (non-NULL), this handler is /// called and the specified error message is passed to it. If the /// handler is not specified, the @c isc::dhcpSocketConfigError exception /// is thrown with the specified message. /// + /// This function should be called to handle errors which occur during + /// socket opening, binding or configuration (e.g. setting socket options + /// etc). + /// /// @param errmsg An error message to be passed to a handlder function or /// to the @c isc::dhcp::SocketConfigError exception. /// @param handler An error handler function or NULL. diff --git a/src/lib/dhcp/pkt_filter.cc b/src/lib/dhcp/pkt_filter.cc index 1dfa0a021e..9c1995df1b 100644 --- a/src/lib/dhcp/pkt_filter.cc +++ b/src/lib/dhcp/pkt_filter.cc @@ -28,8 +28,9 @@ PktFilter::openFallbackSocket(const isc::asiolink::IOAddress& addr, // Create socket. int sock = socket(AF_INET, SOCK_DGRAM, 0); if (sock < 0) { - isc_throw(SocketConfigError, "failed to create fallback socket for address " - << addr.toText() << ", port " << port); + isc_throw(SocketConfigError, "failed to create fallback socket for" + " address " << addr.toText() << ", port " << port + << ", reason: " << strerror(errno)); } // Bind the socket to a specified address and port. struct sockaddr_in addr4; @@ -38,21 +39,23 @@ PktFilter::openFallbackSocket(const isc::asiolink::IOAddress& addr, addr4.sin_addr.s_addr = htonl(addr); addr4.sin_port = htons(port); - if (bind(sock, reinterpret_cast(&addr4), sizeof(addr4)) < 0) { + if (bind(sock, reinterpret_cast(&addr4), + sizeof(addr4)) < 0) { // Remember to close the socket if we failed to bind it. close(sock); - isc_throw(SocketConfigError, "failed to bind fallback socket to address " - << addr.toText() << ", port " << port << " - is another DHCP " - "server running?"); + isc_throw(SocketConfigError, "failed to bind fallback socket to" + " address " << addr.toText() << ", port " << port + << ", reason: " << strerror(errno) + << " - is another DHCP server running?"); } - // Set socket to non-blocking mode. This is to prevent the read from the fallback - // socket to block message processing on the primary socket. + // Set socket to non-blocking mode. This is to prevent the read from the + // fallback socket to block message processing on the primary socket. if (fcntl(sock, F_SETFL, O_NONBLOCK) != 0) { close(sock); isc_throw(SocketConfigError, "failed to set SO_NONBLOCK option on the" " fallback socket, bound to " << addr.toText() << ", port " - << port); + << port << ", reason: " << strerror(errno)); } // Successfully created and bound a fallback socket. Return a descriptor. return (sock); diff --git a/src/lib/dhcp/pkt_filter_inet.h b/src/lib/dhcp/pkt_filter_inet.h index 0a506f0a0d..690622ce28 100644 --- a/src/lib/dhcp/pkt_filter_inet.h +++ b/src/lib/dhcp/pkt_filter_inet.h @@ -53,6 +53,8 @@ public: /// @param send_bcast Configure socket to send broadcast messages. /// /// @return A structure describing a primary and fallback socket. + /// @throw isc::dhcp::SocketConfigError if error occurs when opening, + /// binding or configuring the socket. virtual SocketInfo openSocket(const Iface& iface, const isc::asiolink::IOAddress& addr, const uint16_t port, @@ -65,6 +67,10 @@ public: /// @param socket_info structure holding socket information /// /// @return Received packet + /// @throw isc::dhcp::SocketReadError if an error occurs during reception + /// of the packet. + /// @throw An execption thrown by the isc::dhcp::Pkt4 object if DHCPv4 + /// message parsing fails. virtual Pkt4Ptr receive(const Iface& iface, const SocketInfo& socket_info); /// @brief Send packet over specified socket. @@ -74,6 +80,8 @@ public: /// @param pkt packet to be sent /// /// @return result of sending a packet. It is 0 if successful. + /// @throw isc::dhcp::SocketWriteError if an error occures during sending + /// a DHCP message through the socket. virtual int send(const Iface& iface, uint16_t sockfd, const Pkt4Ptr& pkt); diff --git a/src/lib/dhcp/pkt_filter_lpf.cc b/src/lib/dhcp/pkt_filter_lpf.cc index 315fa7b44c..426d58ceef 100644 --- a/src/lib/dhcp/pkt_filter_lpf.cc +++ b/src/lib/dhcp/pkt_filter_lpf.cc @@ -157,8 +157,7 @@ PktFilterLPF::openSocket(const Iface& iface, << "' to interface '" << iface.getName() << "'"); } - SocketInfo sock_desc(addr, port, sock, fallback); - return (sock_desc); + return (SocketInfo(addr, port, sock, fallback)); } @@ -171,10 +170,18 @@ PktFilterLPF::receive(const Iface& iface, const SocketInfo& socket_info) { // end after receiving one packet. The call to recv returns immediately // when there is no data left on the socket because the socket is // non-blocking. + // @todo In the normal conditions, both the primary socket and the fallback + // socket are in sync as they are set to receive packets on the same + // address and port. The reception of packets on the fallback socket + // shouldn't cause significant lags in packet reception. If we find in the + // future that it does, the sort of threshold could be set for the maximum + // bytes received on the fallback socket in a single round. Further + // optimizations would include an asynchronous read from the fallback socket + // when the DHCP server is idle. int datalen; do { datalen = recv(socket_info.fallbackfd_, raw_buf, sizeof(raw_buf), 0); - } while (datalen >= 0); + } while (datalen > 0); // Now that we finished getting data from the fallback socket, we // have to get the data from the raw socket too. diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index e7fb2bd606..d020db81e6 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -1107,7 +1107,7 @@ TEST_F(IfaceMgrTest, setMatchingPacketFilter) { TEST_F(IfaceMgrTest, checkPacketFilterLPFSocket) { IOAddress loAddr("127.0.0.1"); - int socket1 = 0, socket2 = 0; + int socket1 = -1, socket2 = -1; // Create two instances of IfaceMgr. boost::scoped_ptr iface_mgr1(new NakedIfaceMgr()); ASSERT_TRUE(iface_mgr1); @@ -1138,6 +1138,8 @@ TEST_F(IfaceMgrTest, checkPacketFilterLPFSocket) { // to prevent resource leak. if (socket2 >= 0) { close(socket2); + ADD_FAILURE() << "Two sockets opened and bound to the same IP" + " address and UDP port"; } if (socket1 >= 0) { From a52c2327de24151a8a9f1d93f4bb6c3ee3962ac0 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 3 Dec 2013 14:02:21 +0100 Subject: [PATCH 22/24] [2765] Added throw tag to the openFallbackSocket function. --- src/lib/dhcp/pkt_filter.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/lib/dhcp/pkt_filter.h b/src/lib/dhcp/pkt_filter.h index e9b661b2d8..e35b75d495 100644 --- a/src/lib/dhcp/pkt_filter.h +++ b/src/lib/dhcp/pkt_filter.h @@ -132,6 +132,8 @@ protected: /// /// @return A fallback socket descriptor. This descriptor should be assigned /// to the @c fallbackfd_ field of the @c isc::dhcp::SocketInfo structure. + /// @throw isc::dhcp::SocketConfigError if socket opening, binding or + /// configuration fails. virtual int openFallbackSocket(const isc::asiolink::IOAddress& addr, const uint16_t port); }; From 68aae7c97b56de73918721193ec1ee29bee638e6 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 3 Dec 2013 14:35:37 +0100 Subject: [PATCH 23/24] [2765] Added unit test description in IfaceMgr unit tests. --- src/lib/dhcp/tests/iface_mgr_unittest.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index d020db81e6..0032433e83 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -1105,6 +1105,12 @@ TEST_F(IfaceMgrTest, setMatchingPacketFilter) { EXPECT_TRUE(iface_mgr->isDirectResponseSupported()); } +// This test checks that it is not possible to open two sockets: IP/UDP +// and raw (LPF) socket and bind to the same address and port. The +// raw socket should be opened together with the fallback IP/UDP socket. +// The fallback socket should fail to open when there is another IP/UDP +// socket bound to the same address and port. Failing to open the fallback +// socket should preclude the raw socket from being open. TEST_F(IfaceMgrTest, checkPacketFilterLPFSocket) { IOAddress loAddr("127.0.0.1"); int socket1 = -1, socket2 = -1; From 1a1447425c6acc5601ea59d18c3638a0e65f0aed Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Wed, 4 Dec 2013 16:13:55 +0100 Subject: [PATCH 24/24] [2765] Fixed typo in the libdhcp++ developer's guide. --- src/lib/dhcp/libdhcp++.dox | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/dhcp/libdhcp++.dox b/src/lib/dhcp/libdhcp++.dox index 34993afc65..6397a4b122 100644 --- a/src/lib/dhcp/libdhcp++.dox +++ b/src/lib/dhcp/libdhcp++.dox @@ -148,7 +148,7 @@ response will be received and processed by all clients in the particular network. Therefore, the preferred approach is that the server unicasts its response to a new address being assigned for the client. This client will identify itself as a target of this message by checking chaddr and/or -Client Identifier value. In the same time, the other clients in the network +Client Identifier value. At the same time, the other clients in the network will not receive the unicast message. The major problem that arises with this approach is that the client without an IP address doesn't respond to ARP messages. As a result, server's response will not be sent over IP/UDP