Commit 18c4f954 authored by Thomas Markwalder's avatar Thomas Markwalder
Browse files

[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.
parent 400bf9a1
# 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
......
// 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
......
// 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.
......
// 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
......
// 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_;
};
......
// 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,
......
// 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
......@@ -17,16 +17,19 @@
#include <dhcp_ddns/dhcp_ddns_log.h>
#include <dhcp_ddns/watch_socket.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/select.h>
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 :" <<errstr);
}
}
}
void
WatchSocket::closeSocket() {
// 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 this is called by the destructor and no one 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);
}
source_ = INVALID_SOCKET;
}
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);
}
ready_flag_ = false;
sink_ = INVALID_SOCKET;
}
}
int
int
WatchSocket::getSelectFd() {
return (sink_);
}
......
......@@ -44,11 +44,20 @@ public:
/// to the pipe. To clear the socket, the marker is read from the pipe. Note
/// that WatchSocket will only write the marker if it is not already marked.
/// This prevents the socket's pipe from filling endlessly.
///
/// @warning Because the read or "sink" side of the pipe is used as the select_fd,
/// it is possible for that fd to be interfered with, albeit only from within the
/// process space which owns it. Performing operations that may alter the fd's state
/// such as close, read, or altering behavior flags with fcntl or ioctl can have
/// unpredictable results. It is intended strictly use with functions such as select()
/// poll() or their variants.
class WatchSocket {
public:
/// @brief Value used to signify an invalid descriptor.
static const int INVALID_SOCKET = -1;
/// @brief Value written to the source when marking the socket as ready.
/// The value itself is arbitrarily chosen as one that is unlikely to occur
/// otherwise and easy to debug.
static const uint32_t MARKER = 0xDEADBEEF;
/// @brief Constructor
......@@ -65,16 +74,32 @@ public:
/// @brief Marks the select-fd as ready to read.
///
/// Marks the socket as ready to read, if is not already so marked.
/// If an error occurs, closeSocket is called. This will force any further
/// use of the select_fd to fail rather than show the fd as READY. Such
/// an error is almost surely a programmatic error which has corrupted the
/// select_fd.
///
/// @throw WatchSocketError if an error occurs marking the socket.
void markReady();
/// @brief Returns true the if socket is marked as ready.
///
/// This method uses a non-blocking call to select() to test read state of the
/// select_fd. Rather than track what the status "should be" it tests the status.
/// This should eliminate conditions where the select-fd appear to be perpetually
/// ready.
/// @return Returns true if select_fd is not INVALID_SOCKET and select() reports it
/// as !EWOULDBLOCK, otherwise it returns false.
/// This method is guaranteed NOT to throw.
bool isReady();
/// @brief Clears the socket's ready to read marker.
///
/// Clears the socket if it is currently marked as ready to read.
/// If an error occurs, closeSocket is called. This will force any further
/// use of the select_fd to fail rather than show the fd as READY. Such
/// an error is almost surely a programmatic error which has corrupted the
/// select_fd.
///
/// @throw WatchSocketError if an error occurs clearing the socket
/// marker.
......@@ -90,15 +115,18 @@ public:
int getSelectFd();
private:
/// @brief Closes the descriptors associated with the socket.
///
/// Used internally in the destructor and if an error occurs marking or
/// clearing the socket.