From: Thomas Markwalder Date: Sat, 8 Feb 2014 18:26:01 +0000 (-0500) Subject: [3221] Addressed review comments. X-Git-Tag: bind10-1.2.0beta1-release~58^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=18c4f954d1a7466f2b5bde33f855aa69707e2406;p=thirdparty%2Fkea.git [3221] Addressed review comments. Made behavior of dhcp_ddns::WatchSocket a bit more robust in handling programmatic abuse. Cleaned up addresses used in ncr_upd_unittest.cc, added specific test for client side address if 0.0.0.0/port 0. Updated copyrights as appropriate and other cosmetics. --- diff --git a/src/lib/dhcp_ddns/dhcp_ddns_messages.mes b/src/lib/dhcp_ddns/dhcp_ddns_messages.mes index c1361a9cb0..c5f6f7da8e 100644 --- a/src/lib/dhcp_ddns/dhcp_ddns_messages.mes +++ b/src/lib/dhcp_ddns/dhcp_ddns_messages.mes @@ -1,4 +1,4 @@ -# Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +# Copyright (C) 2013-2014 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 diff --git a/src/lib/dhcp_ddns/ncr_io.cc b/src/lib/dhcp_ddns/ncr_io.cc index 22fa7ac12b..52a72c9d21 100644 --- a/src/lib/dhcp_ddns/ncr_io.cc +++ b/src/lib/dhcp_ddns/ncr_io.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2014 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 @@ -338,8 +338,8 @@ NameChangeSender::peekAt(const size_t index) const { void -NameChangeSender::assumeQueue(NameChangeSender& sourceSender) { - if (sourceSender.amSending()) { +NameChangeSender::assumeQueue(NameChangeSender& source_sender) { + if (source_sender.amSending()) { isc_throw(NcrSenderError, "Cannot assume queue:" " source sender is actively sending"); } @@ -349,17 +349,17 @@ NameChangeSender::assumeQueue(NameChangeSender& sourceSender) { " target sender is actively sending"); } - if (getQueueMaxSize() < sourceSender.getQueueSize()) { + if (getQueueMaxSize() < source_sender.getQueueSize()) { isc_throw(NcrSenderError, "Cannot assume queue:" " source queue count exceeds target queue max"); } - if (send_queue_.size() != 0) { + if (!send_queue_.empty()) { isc_throw(NcrSenderError, "Cannot assume queue:" " target queue is not empty"); } - send_queue_.swap(sourceSender.getSendQueue()); + send_queue_.swap(source_sender.getSendQueue()); } int diff --git a/src/lib/dhcp_ddns/ncr_io.h b/src/lib/dhcp_ddns/ncr_io.h index 10d39d0a38..65b89290cb 100644 --- a/src/lib/dhcp_ddns/ncr_io.h +++ b/src/lib/dhcp_ddns/ncr_io.h @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2014 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 @@ -554,28 +554,27 @@ public: /// messages from one sender to another. This is useful for dealing with /// dynamic configuration changes. /// - /// @param Sender from whom the queued messages will be taken + /// @param source_sender from whom the queued messages will be taken /// /// @throw NcrSenderError if either sender is in send mode, if the number of /// messages in the source sender's queue is larger than this sender's /// maxium queue size, or if this sender's queue is not empty. - void assumeQueue(NameChangeSender& fromSender); + void assumeQueue(NameChangeSender& source_sender); - /// @brief Returns a file description suitable for use with select + /// @brief Returns a file descriptor suitable for use with select /// /// The value returned is an open file descriptor which can be used with /// select() system call to monitor the sender for IO events. This allows /// NameChangeSenders to be used in applications which use select, rather /// than IOService to wait for IO events to occur. /// - /// @note Attempting other use of this value may lead to unpredictable + /// @warning Attempting other use of this value may lead to unpredictable /// behavior in the sender. /// /// @return Returns an "open" file descriptor /// /// @throw NcrSenderError if the sender is not in send mode, - /// NotImplemented if the implementation does not support such an fd. - virtual int getSelectFd(); + virtual int getSelectFd() = 0; protected: /// @brief Dequeues and sends the next request on the send queue. diff --git a/src/lib/dhcp_ddns/ncr_udp.cc b/src/lib/dhcp_ddns/ncr_udp.cc index 29d6e9458e..3906c9b68e 100644 --- a/src/lib/dhcp_ddns/ncr_udp.cc +++ b/src/lib/dhcp_ddns/ncr_udp.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2014 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 diff --git a/src/lib/dhcp_ddns/ncr_udp.h b/src/lib/dhcp_ddns/ncr_udp.h index 03ac5bf33f..97f8316ef4 100644 --- a/src/lib/dhcp_ddns/ncr_udp.h +++ b/src/lib/dhcp_ddns/ncr_udp.h @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2014 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 @@ -504,6 +504,7 @@ public: /// asyncSend() method is called, passing in send_callback_ member's /// transfer buffer as the send buffer and the send_callback_ itself /// as the callback object. + /// @param ncr NameChangeRequest to send. virtual void doSend(NameChangeRequestPtr& ncr); /// @brief Implements the NameChangeRequest level send completion handler. @@ -526,14 +527,14 @@ public: void sendCompletionHandler(const bool successful, const UDPCallback* send_callback); - /// @brief Returns a file description suitable for use with select + /// @brief Returns a file descriptor suitable for use with select /// /// The value returned is an open file descriptor which can be used with /// select() system call to monitor the sender for IO events. This allows /// NameChangeUDPSenders to be used in applications which use select, /// rather than IOService to wait for IO events to occur. /// - /// @note Attempting other use of this value may lead to unpredictable + /// @warning Attempting other use of this value may lead to unpredictable /// behavior in the sender. /// /// @return Returns an "open" file descriptor @@ -572,6 +573,7 @@ private: /// @brief Flag which enables the reuse address socket option if true. bool reuse_address_; + /// @brief Pointer to WatchSocket instance supplying the "select-fd". WatchSocketPtr watch_socket_; }; diff --git a/src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc b/src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc index 5f438c3f75..7ea4b49c76 100644 --- a/src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc +++ b/src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2014 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 @@ -71,7 +71,6 @@ const char *valid_msgs[] = }; const char* TEST_ADDRESS = "127.0.0.1"; -//const char* TEST_ADDRESS = "192.0.2.10"; const uint32_t LISTENER_PORT = 5301; const uint32_t SENDER_PORT = LISTENER_PORT+1; const long TEST_TIMEOUT = 5 * 1000; @@ -315,8 +314,7 @@ TEST(NameChangeUDPSenderBasicTest, constructionTests) { /// @brief Tests NameChangeUDPSender basic send functionality /// This test verifies that: TEST(NameChangeUDPSenderBasicTest, basicSendTests) { - isc::asiolink::IOAddress ip_address("127.0.0.1"); - uint32_t port = 5301; + isc::asiolink::IOAddress ip_address(TEST_ADDRESS); isc::asiolink::IOService io_service; SimpleSendHandler ncr_handler; @@ -325,9 +323,9 @@ TEST(NameChangeUDPSenderBasicTest, basicSendTests) { // Create the sender, setting the queue max equal to the number of // messages we will have in the list. - isc::asiolink::IOAddress any("0.0.0.0"); - NameChangeUDPSender sender(any, 0, ip_address, port, - FMT_JSON, ncr_handler, num_msgs); + NameChangeUDPSender sender(ip_address, SENDER_PORT, ip_address, + LISTENER_PORT, FMT_JSON, ncr_handler, + num_msgs, true); // Verify that we can start sending. EXPECT_NO_THROW(sender.startSending(io_service)); @@ -426,6 +424,45 @@ TEST(NameChangeUDPSenderBasicTest, basicSendTests) { EXPECT_EQ(0, sender.getQueueSize()); } +/// @brief Tests NameChangeUDPSender basic send with INADDR_ANY and port 0. +TEST(NameChangeUDPSenderBasicTest, anyAddressSend) { + isc::asiolink::IOAddress ip_address(TEST_ADDRESS); + isc::asiolink::IOAddress any_address("0.0.0.0"); + isc::asiolink::IOService io_service; + SimpleSendHandler ncr_handler; + + // Tests are based on a list of messages, get the count now. + int num_msgs = sizeof(valid_msgs)/sizeof(char*); + + // Create the sender, setting the queue max equal to the number of + // messages we will have in the list. + NameChangeUDPSender sender(any_address, 0, ip_address, LISTENER_PORT, + FMT_JSON, ncr_handler, num_msgs); + + // Enter send mode. + ASSERT_NO_THROW(sender.startSending(io_service)); + EXPECT_TRUE(sender.amSending()); + + // Fetch the sender's select-fd. + int select_fd = sender.getSelectFd(); + + // Create and queue up a message. + NameChangeRequestPtr ncr; + ASSERT_NO_THROW(ncr = NameChangeRequest::fromJSON(valid_msgs[0])); + EXPECT_NO_THROW(sender.sendRequest(ncr)); + EXPECT_EQ(1, sender.getQueueSize()); + + // message and the queue count should decrement accordingly. + // Execute at one ready handler. + ASSERT_TRUE(selectCheck(select_fd) > 0); + ASSERT_NO_THROW(io_service.run_one()); + + // Verify that sender shows no IO ready. + // and that the queue is empty. + EXPECT_EQ(0, selectCheck(select_fd)); + EXPECT_EQ(0, sender.getQueueSize()); +} + /// @brief Test the NameChangeSender::assumeQueue method. TEST(NameChangeSender, assumeQueue) { isc::asiolink::IOAddress ip_address(TEST_ADDRESS); @@ -519,9 +556,9 @@ public: *this, true)); // Create our sender instance. Note that reuse_address is true. - sender_.reset( - new NameChangeUDPSender(addr, SENDER_PORT, addr, LISTENER_PORT, - FMT_JSON, *this, 100, true)); + sender_.reset( + new NameChangeUDPSender(addr, SENDER_PORT, addr, LISTENER_PORT, + FMT_JSON, *this, 100, true)); // Set the test timeout to break any running tasks if they hang. test_timer_.setup(boost::bind(&NameChangeUDPTest::testTimeoutHandler, diff --git a/src/lib/dhcp_ddns/tests/watch_socket_unittests.cc b/src/lib/dhcp_ddns/tests/watch_socket_unittests.cc index c54ec99749..82999f6b8c 100644 --- a/src/lib/dhcp_ddns/tests/watch_socket_unittests.cc +++ b/src/lib/dhcp_ddns/tests/watch_socket_unittests.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014 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 @@ -37,8 +37,8 @@ TEST(WatchSocketTest, basics) { /// Verify that post-construction the state the select-fd is valid. int select_fd = watch->getSelectFd(); EXPECT_NE(select_fd, WatchSocket::INVALID_SOCKET); - - /// Verify that isReady() is false and that a call to select agrees. + + /// Verify that isReady() is false and that a call to select agrees. EXPECT_FALSE(watch->isReady()); EXPECT_EQ(0, selectCheck(select_fd)); @@ -57,16 +57,151 @@ TEST(WatchSocketTest, basics) { EXPECT_FALSE(ioctl(select_fd, FIONREAD, &count)); EXPECT_EQ(sizeof(WatchSocket::MARKER), count); - /// Verify that isReady() is true and that a call to select agrees. + /// Verify that isReady() is true and that a call to select agrees. EXPECT_TRUE(watch->isReady()); EXPECT_EQ(1, selectCheck(select_fd)); /// Verify that the socket can be cleared. ASSERT_NO_THROW(watch->clearReady()); - /// Verify that isReady() is false and that a call to select agrees. + /// Verify that isReady() is false and that a call to select agrees. EXPECT_FALSE(watch->isReady()); EXPECT_EQ(0, selectCheck(select_fd)); } +/// @brief Checks behavior when select_fd is closed externally while in the +/// "cleared" state. +TEST(WatchSocketTest, closedWhileClear) { + WatchSocketPtr watch; + + /// Verify that we can construct a WatchSocket. + ASSERT_NO_THROW(watch.reset(new WatchSocket())); + ASSERT_TRUE(watch); + + /// Verify that post-construction the state the select-fd is valid. + int select_fd = watch->getSelectFd(); + ASSERT_NE(select_fd, WatchSocket::INVALID_SOCKET); + + // Verify that socket does not appear ready. + ASSERT_EQ(0, watch->isReady()); + + // Interfere by closing the fd. + ASSERT_EQ(0, close(select_fd)); + + // Verify that socket does not appear ready. + ASSERT_EQ(0, watch->isReady()); + + // Verify that clear does NOT throw. + ASSERT_NO_THROW(watch->clearReady()); + + // Verify that trying to mark it fails. + ASSERT_THROW(watch->markReady(), WatchSocketError); + + // Verify that clear does NOT throw. + ASSERT_NO_THROW(watch->clearReady()); + + // Verify that getSelectFd() returns invalid socket. + ASSERT_EQ(WatchSocket::INVALID_SOCKET, watch->getSelectFd()); +} + +/// @brief Checks behavior when select_fd has closed while in the "ready" +/// state. +TEST(WatchSocketTest, closedWhileReady) { + WatchSocketPtr watch; + + /// Verify that we can construct a WatchSocket. + ASSERT_NO_THROW(watch.reset(new WatchSocket())); + ASSERT_TRUE(watch); + + /// Verify that post-construction the state the select-fd is valid. + int select_fd = watch->getSelectFd(); + ASSERT_NE(select_fd, WatchSocket::INVALID_SOCKET); + + /// Verify that the socket can be marked ready. + ASSERT_NO_THROW(watch->markReady()); + EXPECT_EQ(1, selectCheck(select_fd)); + + // Interfere by closing the fd. + ASSERT_EQ(0, close(select_fd)); + + // Verify that trying to clear it does not throw. + ASSERT_NO_THROW(watch->clearReady()); + + // Verify the select_fd fails as socket is invalid/closed. + EXPECT_EQ(-1, selectCheck(select_fd)); + + // Verify that subsequent attempts to mark it will fail. + ASSERT_THROW(watch->markReady(), WatchSocketError); +} + +/// @brief Checks behavior when select_fd has been marked ready but then +/// emptied by an external read. +TEST(WatchSocketTest, emptyReadySelectFd) { + WatchSocketPtr watch; + + /// Verify that we can construct a WatchSocket. + ASSERT_NO_THROW(watch.reset(new WatchSocket())); + ASSERT_TRUE(watch); + + /// Verify that post-construction the state the select-fd is valid. + int select_fd = watch->getSelectFd(); + ASSERT_NE(select_fd, WatchSocket::INVALID_SOCKET); + + /// Verify that the socket can be marked ready. + ASSERT_NO_THROW(watch->markReady()); + EXPECT_EQ(1, selectCheck(select_fd)); + + // Interfere by reading the fd. This should empty the read pipe. + uint32_t buf = 0; + ASSERT_EQ((read (select_fd, &buf, sizeof(buf))), sizeof(buf)); + ASSERT_EQ(WatchSocket::MARKER, buf); + + // Really nothing that can be done to protect against this, but let's + // make sure we aren't in a weird state. + ASSERT_NO_THROW(watch->clearReady()); + + // Verify the select_fd fails as socket is invalid/closed. + EXPECT_EQ(0, selectCheck(select_fd)); + + // Verify that getSelectFd() returns is still good. + ASSERT_EQ(select_fd, watch->getSelectFd()); +} + +/// @brief Checks behavior when select_fd has been marked ready but then +/// contents have been "corrupted" by a partial read. +TEST(WatchSocketTest, badReadOnClear) { + WatchSocketPtr watch; + + /// Verify that we can construct a WatchSocket. + ASSERT_NO_THROW(watch.reset(new WatchSocket())); + ASSERT_TRUE(watch); + + /// Verify that post-construction the state the select-fd is valid. + int select_fd = watch->getSelectFd(); + ASSERT_NE(select_fd, WatchSocket::INVALID_SOCKET); + + /// Verify that the socket can be marked ready. + ASSERT_NO_THROW(watch->markReady()); + EXPECT_EQ(1, selectCheck(select_fd)); + + // Interfere by reading the fd. This should empty the read pipe. + uint32_t buf = 0; + ASSERT_EQ((read (select_fd, &buf, 1)), 1); + ASSERT_NE(WatchSocket::MARKER, buf); + + // Really nothing that can be done to protect against this, but let's + // make sure we aren't in a weird state. + /// @todo maybe clear should never throw, log only + ASSERT_THROW(watch->clearReady(), WatchSocketError); + + // Verify the select_fd fails as socket is invalid/closed. + EXPECT_EQ(-1, selectCheck(select_fd)); + + // Verify that getSelectFd() returns INVALID. + ASSERT_EQ(WatchSocket::INVALID_SOCKET, watch->getSelectFd()); + + // Verify that subsequent attempt to mark it fails. + ASSERT_THROW(watch->markReady(), WatchSocketError); +} + } // end of anonymous namespace diff --git a/src/lib/dhcp_ddns/watch_socket.cc b/src/lib/dhcp_ddns/watch_socket.cc index a123fa2ce7..0a437a638e 100644 --- a/src/lib/dhcp_ddns/watch_socket.cc +++ b/src/lib/dhcp_ddns/watch_socket.cc @@ -17,16 +17,19 @@ #include #include +#include #include +#include namespace isc { namespace dhcp_ddns { + const int WatchSocket::INVALID_SOCKET; const uint32_t WatchSocket::MARKER; -WatchSocket::WatchSocket() - : source_(INVALID_SOCKET), sink_(INVALID_SOCKET), ready_flag_(false) { +WatchSocket::WatchSocket() + : source_(INVALID_SOCKET), sink_(INVALID_SOCKET) { // Open the pipe. int fds[2]; if (pipe(fds)) { @@ -36,64 +39,109 @@ WatchSocket::WatchSocket() source_ = fds[1]; sink_ = fds[0]; -} -WatchSocket::~WatchSocket() { - // Close the pipe fds. Technically a close can fail (hugely unlikely) - // but there's no recovery for it either. If one does fail we log it - // and go on. Plus no likes destructors that throw. - if (source_ != INVALID_SOCKET) { - if (close(source_)) { - const char* errstr = strerror(errno); - LOG_ERROR(dhcp_ddns_logger, DHCP_DDNS_WATCH_SOURCE_CLOSE_ERROR) - .arg(errstr); - } + if (fcntl(sink_, F_SETFL, O_NONBLOCK)) { + const char* errstr = strerror(errno); + isc_throw(WatchSocketError, "Cannot set sink to non-blocking: " + << errstr); } +} - if (sink_ != INVALID_SOCKET) { - if (close(sink_)) { - const char* errstr = strerror(errno); - LOG_ERROR(dhcp_ddns_logger, DHCP_DDNS_WATCH_SINK_CLOSE_ERROR) - .arg(errstr); - } - } +WatchSocket::~WatchSocket() { + closeSocket(); } -void +void WatchSocket::markReady() { + // Make sure it hasn't been orphaned! Otherwise we may get SIGPIPE. + // We use fcntl to check as select() on some systems may show it as ready to read. + if (fcntl(sink_, F_GETFL) < 0) { + closeSocket(); + isc_throw(WatchSocketError, "WatchSocket markReady - select_fd was closed!"); + } + if (!isReady()) { int nbytes = write (source_, &MARKER, sizeof(MARKER)); if (nbytes != sizeof(MARKER)) { + // If there's an error get the error message than close + // the pipe. This should ensure any further use of the socket + // or testing the fd with select_fd will fail. const char* errstr = strerror(errno); + closeSocket(); isc_throw(WatchSocketError, "WatchSocket markReady failed:" << " bytes written: " << nbytes << " : " << errstr); } - - ready_flag_ = true; - } + } } -bool +bool WatchSocket::isReady() { - return (ready_flag_); + // Report it as not ready rather than error here. + if (sink_ == INVALID_SOCKET) { + return (false); + } + + fd_set read_fds; + FD_ZERO(&read_fds); + + // Add select_fd socket to listening set + FD_SET(sink_, &read_fds); + + // Set zero timeout (non-blocking). + struct timeval select_timeout; + select_timeout.tv_sec = 0; + select_timeout.tv_usec = 0; + + // Return true only if read ready, treat error same as not ready. + return (select(sink_ + 1, &read_fds, NULL, NULL, &select_timeout) > 0); } -void +void WatchSocket::clearReady() { if (isReady()) { - uint32_t buf; + uint32_t buf = 0; int nbytes = read (sink_, &buf, sizeof(buf)); - if (nbytes != sizeof(MARKER)) { + if ((nbytes != sizeof(MARKER) || (buf != MARKER))) { + // If there's an error get the error message than close + // the pipe. This should ensure any further use of the socket + // or testing the fd with select_fd will fail. const char* errstr = strerror(errno); + closeSocket(); isc_throw(WatchSocketError, "WatchSocket clearReady failed:" - << " bytes read: " << nbytes << " : " << errstr); + << " bytes read: " << nbytes << " : " + << " value read: " << buf << " error :" <getServerIp(), new_config->getServerPort(), new_config->getNcrFormat(), - *this, queueMax)); + *this, queue_max)); break; } default: diff --git a/src/lib/dhcpsrv/d2_client.h b/src/lib/dhcpsrv/d2_client.h index 9112abf318..7574fe1af8 100644 --- a/src/lib/dhcpsrv/d2_client.h +++ b/src/lib/dhcpsrv/d2_client.h @@ -232,7 +232,8 @@ boost::function #include -#include +//#include using namespace std; using namespace isc::dhcp;