From: Thomas Markwalder Date: Tue, 25 Jun 2019 16:30:18 +0000 (-0400) Subject: [#691,!395] Added bad socket purge to IfaceMgr X-Git-Tag: Kea-1.6.0-beta2~200 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=22ac8440eab215513c730b217db86c0b047b23ab;p=thirdparty%2Fkea.git [#691,!395] Added bad socket purge to IfaceMgr src/lib/dhcp/iface_mgr.cc IfaceMgr::purgeBadSockets() - new function to validate external sockets and unregister any that are invalid. IfaceMgr::receive4Indirect() IfaceMgr::receive4Direct() IfaceMgr::receive6Indirect() IfaceMgr::receive6Direct() - added logic to all purgeBadSockets() when select fails with EBADF src/lib/dhcp/tests/iface_mgr_unittest.cc TEST_F(IfaceMgrTest, purgeExternalSockets4) TEST_F(IfaceMgrTest, purgeExternalSockets6) - new tests to verify bad socket purging src/lib/http/client.cc Move close_callback_ reset back to Connection::close() --- diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index 50c041e20e..96eef3685e 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -346,6 +346,23 @@ IfaceMgr::deleteExternalSocket(int socketfd) { } } +int +IfaceMgr::purgeBadSockets() { + std::vector bad_fds; + BOOST_FOREACH(SocketCallbackInfo s, callbacks_) { + errno = 0; + if (fcntl(s.socket_, F_GETFD) < 0 && errno == EBADF) { + bad_fds.push_back(s.socket_); + } + } + + for (auto bad_fd : bad_fds) { + deleteExternalSocket(bad_fd); + } + + return (bad_fds.size()); +} + void IfaceMgr::deleteAllExternalSockets() { callbacks_.clear(); @@ -1042,6 +1059,11 @@ Pkt4Ptr IfaceMgr::receive4Indirect(uint32_t timeout_sec, uint32_t timeout_usec / // signal or for some other reason. if (errno == EINTR) { isc_throw(SignalInterruptOnSelect, strerror(errno)); + } else if (errno == EBADF) { + int cnt = purgeBadSockets(); + isc_throw(SocketReadError, + "SELECT interrupted by one invalid sockets, purged " + << cnt << " socket descriptors"); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1141,6 +1163,11 @@ Pkt4Ptr IfaceMgr::receive4Direct(uint32_t timeout_sec, uint32_t timeout_usec /* // signal or for some other reason. if (errno == EINTR) { isc_throw(SignalInterruptOnSelect, strerror(errno)); + } else if (errno == EBADF) { + int cnt = purgeBadSockets(); + isc_throw(SocketReadError, + "SELECT interrupted by one invalid sockets, purged " + << cnt << " socket descriptors"); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1265,6 +1292,11 @@ IfaceMgr::receive6Direct(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ ) // signal or for some other reason. if (errno == EINTR) { isc_throw(SignalInterruptOnSelect, strerror(errno)); + } else if (errno == EBADF) { + int cnt = purgeBadSockets(); + isc_throw(SocketReadError, + "SELECT interrupted by one invalid sockets, purged " + << cnt << " socket descriptors"); } else { isc_throw(SocketReadError, strerror(errno)); } @@ -1368,6 +1400,11 @@ IfaceMgr::receive6Indirect(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ // signal or for some other reason. if (errno == EINTR) { isc_throw(SignalInterruptOnSelect, strerror(errno)); + } else if (errno == EBADF) { + int cnt = purgeBadSockets(); + isc_throw(SocketReadError, + "SELECT interrupted by one invalid sockets, purged " + << cnt << " socket descriptors"); } else { isc_throw(SocketReadError, strerror(errno)); } diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index 1247b4fc21..fc308526ad 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -926,6 +926,16 @@ public: /// @brief Deletes external socket void deleteExternalSocket(int socketfd); + /// @brief Scans registered socket set and removes any that are invalid. + /// + /// Walks the list of regiseterd external sockets and test each for + /// validity. If any are found to be invalid they are removed. This + /// primarily a self-defense mechanism against hook libs or other users + /// of external sockets, leaving a closed socket registered by mistake. + /// + /// @return A count of the sockets purged. + int purgeBadSockets(); + /// @brief Deletes all external sockets. void deleteAllExternalSockets(); diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index 09ad40630b..56e71001d4 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -665,6 +665,172 @@ public: ASSERT_FALSE(ifacemgr->isDHCPReceiverRunning()); } + /// @brief Verifies that IfaceMgr DHCPv4 receive calls detect and + /// purge external sockets that have gone bad without affecting + /// affecting normal operations. It can be run with or without + /// packet queuing. + /// + /// @param use_queue determines if packet queuing is used or not. + void purgeExternalSockets4Test(bool use_queue = false) { + bool callback_ok = false; + bool callback2_ok = false; + + scoped_ptr ifacemgr(new NakedIfaceMgr()); + + if (use_queue) { + bool queue_enabled; + data::ConstElementPtr config = makeQueueConfig(PacketQueueMgr4::DEFAULT_QUEUE_TYPE4, 500); + ASSERT_NO_THROW(queue_enabled = ifacemgr->configureDHCPPacketQueue(AF_INET, config)); + ASSERT_TRUE(queue_enabled); + + // Thread should only start when there is a packet queue. + ASSERT_NO_THROW(ifacemgr->startDHCPReceiver(AF_INET)); + ASSERT_TRUE(ifacemgr->isDHCPReceiverRunning()); + } + + // Create first pipe and register it as extra socket + int pipefd[2]; + EXPECT_TRUE(pipe(pipefd) == 0); + EXPECT_NO_THROW(ifacemgr->addExternalSocket(pipefd[0], + [this, &callback_ok](){ callback_ok = true; })); + + + // Let's create a second pipe and register it as well + int secondpipe[2]; + EXPECT_TRUE(pipe(secondpipe) == 0); + EXPECT_NO_THROW(ifacemgr->addExternalSocket(secondpipe[0], + [this, &callback2_ok](){ callback2_ok = true; })); + + // Verify a call with no data and normal external sockets works ok. + Pkt4Ptr pkt4; + ASSERT_NO_THROW(pkt4 = ifacemgr->receive4(1)); + + // No callback invocations and no DHCPv4 pkt. + EXPECT_FALSE(callback_ok); + EXPECT_FALSE(callback2_ok); + EXPECT_FALSE(pkt4); + + // Now close the first pipe. This should make it's external socket invalid. + close(pipefd[1]); + close(pipefd[0]); + + // We call receive4() which should detect and remove the invalid socket. + try { + pkt4 = ifacemgr->receive4(1); + ADD_FAILURE() << "receive4 should have failed"; + } catch (const SocketReadError& ex) { + EXPECT_EQ(std::string("SELECT interrupted by one invalid sockets," + " purged 1 socket descriptors"), + std::string(ex.what())); + } catch (const std::exception& ex) { + ADD_FAILURE() << "wrong exception thrown: " << ex.what(); + } + + // No callback invocations and no DHCPv4 pkt. + EXPECT_FALSE(callback_ok); + EXPECT_FALSE(callback2_ok); + EXPECT_FALSE(pkt4); + + // Now check whether the second callback is still functional + EXPECT_EQ(38, write(secondpipe[1], "Hi, this is a message sent over a pipe", 38)); + + // Call recevie4 again, this should work. + ASSERT_NO_THROW(pkt4 = ifacemgr->receive4(1)); + + // Should have callback2 data only. + EXPECT_FALSE(callback_ok); + EXPECT_TRUE(callback2_ok); + EXPECT_FALSE(pkt4); + + // Stop the thread. This should be no harm/no foul if we're not + // queueuing. Either way, we should not have a thread afterwards. + ASSERT_NO_THROW(ifacemgr->stopDHCPReceiver()); + ASSERT_FALSE(ifacemgr->isDHCPReceiverRunning()); + } + + /// @brief Verifies that IfaceMgr DHCPv6 receive calls detect and + /// purge external sockets that have gone bad without affecting + /// affecting normal operations. It can be run with or without + /// packet queuing. + /// + /// @param use_queue determines if packet queuing is used or not. + void purgeExternalSockets6Test(bool use_queue = false) { + bool callback_ok = false; + bool callback2_ok = false; + + scoped_ptr ifacemgr(new NakedIfaceMgr()); + + if (use_queue) { + bool queue_enabled; + data::ConstElementPtr config = makeQueueConfig(PacketQueueMgr6::DEFAULT_QUEUE_TYPE6, 500); + ASSERT_NO_THROW(queue_enabled = ifacemgr->configureDHCPPacketQueue(AF_INET6, config)); + ASSERT_TRUE(queue_enabled); + + // Thread should only start when there is a packet queue. + ASSERT_NO_THROW(ifacemgr->startDHCPReceiver(AF_INET6)); + ASSERT_TRUE(ifacemgr->isDHCPReceiverRunning()); + } + + // Create first pipe and register it as extra socket + int pipefd[2]; + EXPECT_TRUE(pipe(pipefd) == 0); + EXPECT_NO_THROW(ifacemgr->addExternalSocket(pipefd[0], + [this, &callback_ok](){ callback_ok = true; })); + + + // Let's create a second pipe and register it as well + int secondpipe[2]; + EXPECT_TRUE(pipe(secondpipe) == 0); + EXPECT_NO_THROW(ifacemgr->addExternalSocket(secondpipe[0], + [this, &callback2_ok](){ callback2_ok = true; })); + + // Verify a call with no data and normal external sockets works ok. + Pkt6Ptr pkt6; + ASSERT_NO_THROW(pkt6 = ifacemgr->receive6(1)); + + // No callback invocations and no DHCPv6 pkt. + EXPECT_FALSE(callback_ok); + EXPECT_FALSE(callback2_ok); + EXPECT_FALSE(pkt6); + + // Now close the first pipe. This should make it's external socket invalid. + close(pipefd[1]); + close(pipefd[0]); + + // We call receive6() which should detect and remove the invalid socket. + try { + pkt6 = ifacemgr->receive6(1); + ADD_FAILURE() << "receive6 should have failed"; + } catch (const SocketReadError& ex) { + EXPECT_EQ(std::string("SELECT interrupted by one invalid sockets," + " purged 1 socket descriptors"), + std::string(ex.what())); + } catch (const std::exception& ex) { + ADD_FAILURE() << "wrong exception thrown: " << ex.what(); + } + + // No callback invocations and no DHCPv6 pkt. + EXPECT_FALSE(callback_ok); + EXPECT_FALSE(callback2_ok); + EXPECT_FALSE(pkt6); + + // Now check whether the second callback is still functional + EXPECT_EQ(38, write(secondpipe[1], "Hi, this is a message sent over a pipe", 38)); + + // Call recevie6 again, this should work. + ASSERT_NO_THROW(pkt6 = ifacemgr->receive6(1)); + + // Should have callback2 data only. + EXPECT_FALSE(callback_ok); + EXPECT_TRUE(callback2_ok); + EXPECT_FALSE(pkt6); + + // Stop the thread. This should be no harm/no foul if we're not + // queueuing. Either way, we should not have a thread afterwards. + ASSERT_NO_THROW(ifacemgr->stopDHCPReceiver()); + ASSERT_FALSE(ifacemgr->isDHCPReceiverRunning()); + } + /// Holds the invocation counter for ifaceMgrErrorHandler. int errors_count_; @@ -2760,6 +2926,18 @@ TEST_F(IfaceMgrTest, DeleteExternalSockets4) { close(secondpipe[0]); } +// Tests that an existing external socket that becomes invalid +// is detected and purged, without affecting other sockets. +// Tests uses receive4(). +TEST_F(IfaceMgrTest, purgeExternalSockets4) { + // Run purge test without packet queuing. + purgeExternalSockets4Test(); + + // Run purge test with packet queuing. + purgeExternalSockets4Test(true); +} + + // Tests if a single external socket and its callback can be passed and // it is supported properly by receive6() method. @@ -2929,6 +3107,15 @@ TEST_F(IfaceMgrTest, DeleteExternalSockets6) { close(secondpipe[0]); } +// Tests if existing external socket become invalid and be purged and not +// not affect any other existing sockets. Tests uses receive6() +TEST_F(IfaceMgrTest, purgeExternalSockets6) { + // Run purge test without packet queuing. + purgeExternalSockets6Test(); + + // Run purge test with packet queuing. + purgeExternalSockets6Test(true); +} // Test checks if the unicast sockets can be opened. // This test is now disabled, because there is no reliable way to test it. We diff --git a/src/lib/http/client.cc b/src/lib/http/client.cc index b9bafda0bb..c930e782b5 100644 --- a/src/lib/http/client.cc +++ b/src/lib/http/client.cc @@ -500,7 +500,6 @@ Connection::resetState() { current_response_.reset(); parser_.reset(); current_callback_ = HttpClient::RequestHandler(); - close_callback_ = HttpClient::CloseHandler(); } void @@ -571,6 +570,7 @@ void Connection::close() { if (close_callback_) { close_callback_(socket_.getNative()); + close_callback_ = HttpClient::CloseHandler(); } timer_.cancel();