BIND 10 trac1452, updated. f37822dcf51b014d5fa9f93f2e9ac85dec0d0ede [1452] added consideration for the case where the acceptor dies before push. we needed to ignore SIGPIPE in this case, so added that setup. also test of course.
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue Dec 13 03:03:37 UTC 2011
The branch, trac1452 has been updated
via f37822dcf51b014d5fa9f93f2e9ac85dec0d0ede (commit)
via 1007c575bbedf9bd07fef24de28d5c744b4c2293 (commit)
via f942fb1a8c2475bac2fd73e5fbf979fcacbd6f2f (commit)
from 6328a99430a833f72c7faa3515cde78e49b5de12 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit f37822dcf51b014d5fa9f93f2e9ac85dec0d0ede
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon Dec 12 19:02:33 2011 -0800
[1452] added consideration for the case where the acceptor dies before push.
we needed to ignore SIGPIPE in this case, so added that setup. also test
of course.
commit 1007c575bbedf9bd07fef24de28d5c744b4c2293
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon Dec 12 18:48:27 2011 -0800
[1452] added more parameter validations and corresponding tests.
commit f942fb1a8c2475bac2fd73e5fbf979fcacbd6f2f
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon Dec 12 13:47:31 2011 -0800
[1452] some documentation/comment cleanups
-----------------------------------------------------------------------
Summary of changes:
src/lib/util/io/fd_share.cc | 3 +
src/lib/util/io/fd_share.h | 3 +-
src/lib/util/io/socketsession.cc | 42 +++++-
src/lib/util/io/socketsession.h | 4 +
src/lib/util/tests/socketsession_unittest.cc | 179 ++++++++++++++++++++++----
5 files changed, 198 insertions(+), 33 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/util/io/fd_share.cc b/src/lib/util/io/fd_share.cc
index 92576e0..14a9947 100644
--- a/src/lib/util/io/fd_share.cc
+++ b/src/lib/util/io/fd_share.cc
@@ -18,6 +18,7 @@
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/uio.h>
+#include <errno.h>
#include <stdlib.h> // for malloc and free
#include "fd_share.h"
@@ -130,7 +131,9 @@ send_fd(const int sock, const int fd) {
*(int*)CMSG_DATA(cmsg) = fd;
const int ret = sendmsg(sock, &msghdr, 0);
+ const int e = errno;
free(msghdr.msg_control);
+ errno = e; // recover errno in case free() changed it
return (ret >= 0 ? 0 : FD_COMM_ERROR);
}
diff --git a/src/lib/util/io/fd_share.h b/src/lib/util/io/fd_share.h
index b74d4ef..63f8e7a 100644
--- a/src/lib/util/io/fd_share.h
+++ b/src/lib/util/io/fd_share.h
@@ -46,7 +46,8 @@ int recv_fd(const int sock);
* counterpart of recv_fd().
*
* \return FD_COMM_ERROR when there's error sending the socket, FD_OTHER_ERROR
- * for all other possible errors.
+ * for all other possible errors. The global 'errno' variable indicates
+ * the corresponding system error.
* \param sock The unix domain socket to send to. Tested and it does not
* work with a pipe.
* \param fd The file descriptor to send. It should work with any valid
diff --git a/src/lib/util/io/socketsession.cc b/src/lib/util/io/socketsession.cc
index 6837338..8682aed 100644
--- a/src/lib/util/io/socketsession.cc
+++ b/src/lib/util/io/socketsession.cc
@@ -19,6 +19,7 @@
#include <netinet/in.h>
#include <errno.h>
+#include <signal.h>
#include <stdint.h>
#include <string.h>
@@ -53,6 +54,11 @@ struct SocketSessionForwarder::ForwarderImpl {
SocketSessionForwarder::SocketSessionForwarder(const std::string& unix_file) :
impl_(NULL)
{
+ // We need to filter SIGPIPE for subsequent push(). See the description.
+ if (signal(SIGPIPE, SIG_IGN) == SIG_ERR) {
+ isc_throw(Unexpected, "Failed to filter SIGPIPE: " << strerror(errno));
+ }
+
ForwarderImpl impl;
if (sizeof(impl.sock_un_.sun_path) - 1 < unix_file.length()) {
isc_throw(SocketSessionError,
@@ -121,12 +127,28 @@ SocketSessionForwarder::push(int sock, int family, int sock_type, int protocol,
const struct sockaddr& remote_end,
const void* data, size_t data_len)
{
- // check state (fd must be valid)
- // family must be AF_INET or AF_INET6
- // sa_family should match
+ if (impl_->fd_ == -1) {
+ isc_throw(SocketSessionError, "Attempt of push before connect");
+ }
+ if ((local_end.sa_family != AF_INET && local_end.sa_family != AF_INET6) ||
+ (remote_end.sa_family != AF_INET && remote_end.sa_family != AF_INET6))
+ {
+ isc_throw(SocketSessionError, "Invalid address family: must be "
+ "AF_INET or AF_INET6; " <<
+ static_cast<int>(local_end.sa_family) << ", " <<
+ static_cast<int>(remote_end.sa_family) << " given");
+ }
+ if (family != local_end.sa_family || family != remote_end.sa_family) {
+ isc_throw(SocketSessionError, "Inconsistent address family: must be "
+ << static_cast<int>(family) << "; "
+ << static_cast<int>(local_end.sa_family) << ", "
+ << static_cast<int>(remote_end.sa_family) << " given");
+ }
- send_fd(impl_->fd_, sock);
- // TODO: error check
+ if (send_fd(impl_->fd_, sock) != 0) {
+ isc_throw(SocketSessionError, "FD passing failed: " <<
+ strerror(errno));
+ }
impl_->buf_.clear();
// Leave the space for the header length
@@ -162,7 +184,15 @@ SocketSession::SocketSession(int sock, int family, int type, int protocol,
local_end_(local_end), remote_end_(remote_end),
data_len_(data_len), data_(data)
{
- // TODO: local_end and remote_end must not be NULL; check it
+ if (local_end == NULL || remote_end == NULL) {
+ isc_throw(BadValue, "sockaddr must be non NULL for SocketSession");
+ }
+ if (data_len == 0) {
+ isc_throw(BadValue, "data_len must be non 0 for SocketSession");
+ }
+ if (data == NULL) {
+ isc_throw(BadValue, "data must be non NULL for SocketSession");
+ }
}
const size_t DEFAULT_HEADER_BUFLEN = sizeof(struct sockaddr_storage) * 2 +
diff --git a/src/lib/util/io/socketsession.h b/src/lib/util/io/socketsession.h
index 3d8de74..11aeb6c 100644
--- a/src/lib/util/io/socketsession.h
+++ b/src/lib/util/io/socketsession.h
@@ -33,7 +33,11 @@ public:
class SocketSessionForwarder : boost::noncopyable {
public:
+ // Note about SIGPIPE. Assuming this class is not often instantiated
+ // (so the overhead of signal setting should be marginal) and could also be
+ // instantiated by multiple threads, it always set the filter.
explicit SocketSessionForwarder(const std::string& unix_file);
+
~SocketSessionForwarder();
void connectToReceptor();
diff --git a/src/lib/util/tests/socketsession_unittest.cc b/src/lib/util/tests/socketsession_unittest.cc
index 2bb42b2..2266908 100644
--- a/src/lib/util/tests/socketsession_unittest.cc
+++ b/src/lib/util/tests/socketsession_unittest.cc
@@ -35,6 +35,7 @@
#include <util/io/sockaddr_util.h>
using namespace std;
+using namespace isc;
using namespace isc::util::io;
using namespace isc::util::io::internal;
@@ -99,12 +100,55 @@ setRecvDelay(int s) {
return (0);
}
+// A shortcut type that is convenient to be used for socket related
+// system calls, which generally require this pair
+typedef pair<const struct sockaddr*, socklen_t> SockAddrInfo;
+
+// A helper class to convert textual representation of IP address and port
+// to a pair of sockaddr and its length (in the form of a SockAddrInfo
+// pair). Its get method uses getaddrinfo(3) for the conversion and stores
+// the result in the addrinfo_list_ vector until the object is destructed.
+// The allocated resources will be automatically freed in an RAII manner.
+class SockAddrCreator {
+public:
+ ~SockAddrCreator() {
+ vector<struct addrinfo*>::const_iterator it;
+ for (it = addrinfo_list_.begin(); it != addrinfo_list_.end(); ++it) {
+ freeaddrinfo(*it);
+ }
+ }
+ SockAddrInfo get(const string& addr_str, const string& port_str) {
+ struct addrinfo hints, *res;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_flags = AI_NUMERICHOST | AI_NUMERICSERV;
+ hints.ai_family = AF_UNSPEC;
+ hints.ai_socktype = SOCK_DGRAM; // could be either DGRAM or STREAM here
+ const int error = getaddrinfo(addr_str.c_str(), port_str.c_str(),
+ &hints, &res);
+ if (error != 0) {
+ isc_throw(isc::Unexpected, "getaddrinfo failed for " <<
+ addr_str << ", " << port_str << ": " <<
+ gai_strerror(error));
+ }
+
+ // Technically, this is not entirely exception safe; if push_back
+ // throws, the resources allocated for 'res' will leak. We prefer
+ // brevity here and ignore the minor failure mode.
+ addrinfo_list_.push_back(res);
+
+ return (SockAddrInfo(res->ai_addr, res->ai_addrlen));
+ }
+private:
+ vector<struct addrinfo*> addrinfo_list_;
+};
+
class ForwarderTest : public ::testing::Test {
protected:
ForwarderTest() : listen_fd_(-1), forwarder_(TEST_UNIX_FILE),
large_text_(65535, 'a'),
test_un_len_(2 + strlen(TEST_UNIX_FILE))
{
+ unlink(TEST_UNIX_FILE);
test_un_.sun_family = AF_UNIX;
strncpy(test_un_.sun_path, TEST_UNIX_FILE, sizeof(test_un_.sun_path));
#ifdef HAVE_SA_LEN
@@ -117,11 +161,6 @@ protected:
close(listen_fd_);
}
unlink(TEST_UNIX_FILE);
-
- vector<struct addrinfo*>::const_iterator it;
- for (it = addrinfo_list_.begin(); it != addrinfo_list_.end(); ++it) {
- freeaddrinfo(*it);
- }
}
// Start an internal "socket session server".
@@ -159,22 +198,9 @@ protected:
return (s);
}
- typedef pair<const struct sockaddr*, socklen_t> SockAddrInfo;
+ // A convenient shortcut for the namespace-scope version of getSockAddr
SockAddrInfo getSockAddr(const string& addr_str, const string& port_str) {
- struct addrinfo hints, *res;
- memset(&hints, 0, sizeof(hints));
- hints.ai_flags = AI_NUMERICHOST | AI_NUMERICSERV;
- hints.ai_family = AF_UNSPEC;
- hints.ai_socktype = SOCK_DGRAM;
- const int error = getaddrinfo(addr_str.c_str(), port_str.c_str(),
- &hints, &res);
- if (error != 0) {
- isc_throw(isc::Unexpected, "getaddrinfo failed for " <<
- addr_str << ", " << port_str << ": " <<
- gai_strerror(error));
- }
- addrinfo_list_.push_back(res);
- return (SockAddrInfo(res->ai_addr, res->ai_addrlen));
+ return (addr_creator_.get(addr_str, port_str));
}
// A helper method that creates a specified type of socket that is
@@ -209,9 +235,10 @@ protected:
return (s);
}
+ // See below
void checkPushAndPop(int family, int type, int protocoal,
const SockAddrInfo& local,
- const SockAddrInfo& remote, const char* const data,
+ const SockAddrInfo& remote, const void* const data,
size_t data_len, bool new_connection);
protected:
@@ -223,7 +250,7 @@ protected:
private:
struct sockaddr_un test_un_;
const socklen_t test_un_len_;
- vector<struct addrinfo*> addrinfo_list_;
+ SockAddrCreator addr_creator_;
};
TEST_F(ForwarderTest, construct) {
@@ -292,11 +319,34 @@ checkSockAddrs(const sockaddr& expected, const sockaddr& actual) {
EXPECT_EQ(string(sbuf_expected), string(sbuf_actual));
}
+// This is a commonly used test case that confirms normal behavior of
+// session passing. It first creates a "local" socket (which is supposed
+// to act as a "server") bound to the 'local' parameter. It then forwards
+// the descriptor of the FD of the local socket along with given data.
+// Next, it creates an Receptor object to receive the forwarded FD itself,
+// receives the FD, and sends test data from the received FD. The
+// test finally checks if it can receive the test data from the local socket
+// at the Forwarder side. In the case of TCP it's a bit complicated because
+// it first needs to establish a new connection, but essentially the test
+// scenario is the same. See the diagram below for more details.
+//
+// UDP:
+// Forwarder Receptor
+// sock -- (pass) --> passed_sock
+// (check) <-------- send TEST_DATA
+//
+// TCP:
+// Forwarder Receptor
+// server_sock---(pass)--->passed_sock
+// ^ |
+// |(connect) |
+// client_sock |
+// (check)<---------send TEST_DATA
void
ForwarderTest::checkPushAndPop(int family, int type, int protocol,
const SockAddrInfo& local,
const SockAddrInfo& remote,
- const char* const data,
+ const void* const data,
size_t data_len, bool new_connection)
{
// Create an original socket to be passed
@@ -339,7 +389,7 @@ ForwarderTest::checkPushAndPop(int family, int type, int protocol,
// Pop the socket session we just pushed from a local receptor, and
// check the content
SocketSessionReceptor receptor(accept_sock_.fd);
- SocketSession sock_session = receptor.pop();
+ const SocketSession sock_session = receptor.pop();
const ScopedSocket passed_sock(sock_session.getSocket());
EXPECT_LE(0, passed_sock.fd);
// The passed FD should be different from the original FD
@@ -352,7 +402,7 @@ ForwarderTest::checkPushAndPop(int family, int type, int protocol,
ASSERT_EQ(data_len, sock_session.getDataLength());
EXPECT_EQ(0, memcmp(data, sock_session.getData(), data_len));
- // Check if the passed FD is usable by sending some data from it
+ // Check if the passed FD is usable by sending some data from it.
setNonBlock(passed_sock.fd, false);
if (protocol == IPPROTO_UDP) {
EXPECT_EQ(sizeof(TEST_DATA),
@@ -363,6 +413,10 @@ ForwarderTest::checkPushAndPop(int family, int type, int protocol,
EXPECT_EQ(sizeof(TEST_DATA),
send(passed_sock.fd, TEST_DATA, sizeof(TEST_DATA), 0));
}
+ // We don't use non blocking read below as it doesn't seem to be always
+ // reliable. Instead we impose some reasonably large upper time limit of
+ // blocking (normally it shouldn't even block at all; the limit is to
+ // force the test to stop even if there's some bug and recv fails).
char recvbuf[sizeof(TEST_DATA)];
sockaddr_storage ss;
socklen_t sa_len = sizeof(ss);
@@ -380,7 +434,7 @@ ForwarderTest::checkPushAndPop(int family, int type, int protocol,
EXPECT_EQ(string(TEST_DATA), string(recvbuf));
}
-TEST_F(ForwarderTest, pushAndPopUDP) {
+TEST_F(ForwarderTest, pushAndPop) {
// Pass a UDP/IPv6 session.
const SockAddrInfo sai_local6(getSockAddr("::1", TEST_PORT));
const SockAddrInfo sai_remote6(getSockAddr("2001:db8::1", "5300"));
@@ -389,6 +443,7 @@ TEST_F(ForwarderTest, pushAndPopUDP) {
checkPushAndPop(AF_INET6, SOCK_DGRAM, IPPROTO_UDP, sai_local6,
sai_remote6, TEST_DATA, sizeof(TEST_DATA), true);
}
+ // Pass a TCP/IPv6 session.
{
SCOPED_TRACE("Passing TCP/IPv6 session");
checkPushAndPop(AF_INET6, SOCK_STREAM, IPPROTO_TCP, sai_local6,
@@ -405,6 +460,7 @@ TEST_F(ForwarderTest, pushAndPopUDP) {
checkPushAndPop(AF_INET, SOCK_DGRAM, IPPROTO_UDP, sai_local4,
sai_remote4, TEST_DATA, sizeof(TEST_DATA), false);
}
+ // Pass a TCP/IPv4 session.
{
SCOPED_TRACE("Passing TCP/IPv4 session");
checkPushAndPop(AF_INET, SOCK_STREAM, IPPROTO_TCP, sai_local4,
@@ -438,4 +494,75 @@ TEST_F(ForwarderTest, pushAndPopUDP) {
}
}
+TEST_F(ForwarderTest, badPush) {
+ // push before connect
+ EXPECT_THROW(forwarder_.push(1, AF_INET, SOCK_DGRAM, IPPROTO_UDP,
+ *getSockAddr("192.0.2.1", "53").first,
+ *getSockAddr("192.0.2.2", "53").first,
+ TEST_DATA, sizeof(TEST_DATA)),
+ SocketSessionError);
+
+ // Now connect the forwarder for the rest of tests
+ startListen();
+ forwarder_.connectToReceptor();
+
+ // Invalid address family
+ struct sockaddr sockaddr_unspec;
+ sockaddr_unspec.sa_family = AF_UNSPEC;
+ EXPECT_THROW(forwarder_.push(1, AF_INET, SOCK_DGRAM, IPPROTO_UDP,
+ sockaddr_unspec,
+ *getSockAddr("192.0.2.2", "53").first,
+ TEST_DATA, sizeof(TEST_DATA)),
+ SocketSessionError);
+ EXPECT_THROW(forwarder_.push(1, AF_INET, SOCK_DGRAM, IPPROTO_UDP,
+ *getSockAddr("192.0.2.2", "53").first,
+ sockaddr_unspec, TEST_DATA,
+ sizeof(TEST_DATA)),
+ SocketSessionError);
+
+ // Inconsistent address family
+ EXPECT_THROW(forwarder_.push(1, AF_INET, SOCK_DGRAM, IPPROTO_UDP,
+ *getSockAddr("2001:db8::1", "53").first,
+ *getSockAddr("192.0.2.2", "53").first,
+ TEST_DATA, sizeof(TEST_DATA)),
+ SocketSessionError);
+ EXPECT_THROW(forwarder_.push(1, AF_INET6, SOCK_DGRAM, IPPROTO_UDP,
+ *getSockAddr("2001:db8::1", "53").first,
+ *getSockAddr("192.0.2.2", "53").first,
+ TEST_DATA, sizeof(TEST_DATA)),
+ SocketSessionError);
+
+ // Close the acceptor before push. It will result in SIGPIPE (should be
+ // ignored) and EPIPE, which will be converted to SocketSessionError.
+ const int receptor_fd = acceptForwarder();
+ close(receptor_fd);
+ EXPECT_THROW(forwarder_.push(1, AF_INET, SOCK_DGRAM, IPPROTO_UDP,
+ *getSockAddr("192.0.2.1", "53").first,
+ *getSockAddr("192.0.2.2", "53").first,
+ TEST_DATA, sizeof(TEST_DATA)),
+ SocketSessionError);
+}
+
+TEST(SocketSession, badValue) {
+ // normal cases are confirmed in ForwarderTest. We only check some
+ // abnormal cases here.
+
+ SockAddrCreator addr_creator;
+
+ EXPECT_THROW(SocketSession(42, AF_INET, SOCK_DGRAM, IPPROTO_UDP, NULL,
+ addr_creator.get("192.0.2.1", "53").first,
+ sizeof(TEST_DATA), TEST_DATA),
+ BadValue);
+ EXPECT_THROW(SocketSession(42, AF_INET6, SOCK_STREAM, IPPROTO_TCP,
+ addr_creator.get("2001:db8::1", "53").first,
+ NULL, sizeof(TEST_DATA), TEST_DATA), BadValue);
+ EXPECT_THROW(SocketSession(42, AF_INET, SOCK_DGRAM, IPPROTO_UDP,
+ addr_creator.get("192.0.2.1", "53").first,
+ addr_creator.get("192.0.2.2", "5300").first,
+ 0, TEST_DATA), BadValue);
+ EXPECT_THROW(SocketSession(42, AF_INET, SOCK_DGRAM, IPPROTO_UDP,
+ addr_creator.get("192.0.2.1", "53").first,
+ addr_creator.get("192.0.2.2", "5300").first,
+ sizeof(TEST_DATA), NULL), BadValue);
+}
}
More information about the bind10-changes
mailing list