BIND 10 trac1522, updated. d38b9068dde3191aef574fdda44f381f528b8146 [1522] added SIGPIPE filter and re-enabled tests that failed due to the signal.
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Dec 30 06:25:31 UTC 2011
The branch, trac1522 has been updated
via d38b9068dde3191aef574fdda44f381f528b8146 (commit)
via 3e87a325eb979710f55db75c87a70392edd9a3c8 (commit)
via 85bf91d5836306a39b471567952ad1e0af91d948 (commit)
via 0a867ce22896ee7c1075caf58c706cec120fed40 (commit)
from 182024bdd5343af59f6e4550eb977168d1745154 (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 d38b9068dde3191aef574fdda44f381f528b8146
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Thu Dec 29 22:25:11 2011 -0800
[1522] added SIGPIPE filter and re-enabled tests that failed due to the signal.
commit 3e87a325eb979710f55db75c87a70392edd9a3c8
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Thu Dec 29 22:15:35 2011 -0800
[1522] made sure sending socket token to the boss before receiving the FD.
updated the test to check the sent tokens, skipping them unless we can specify
read timeout so we can avoid deadlock in the worst case.
right now there's one remaining bug: once the boss side socket is closed
subsequent call to requestSocket() will result in SIGPIPE. I'll fix it
in the next commit. For now I disabled offending tests.
commit 85bf91d5836306a39b471567952ad1e0af91d948
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Thu Dec 29 22:14:58 2011 -0800
[1522] fixed a bug on dependency library: error handling was incorrect. also
made some style fixes.
commit 0a867ce22896ee7c1075caf58c706cec120fed40
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Thu Dec 29 14:06:10 2011 -0800
[1522] avoid trying read from unix domain socket once an unexpected error
happens.
-----------------------------------------------------------------------
Summary of changes:
src/lib/server_common/socket_request.cc | 29 +++-
.../server_common/tests/socket_requestor_test.cc | 191 ++++++++++++--------
src/lib/util/io/fd.cc | 20 +-
3 files changed, 155 insertions(+), 85 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/server_common/socket_request.cc b/src/lib/server_common/socket_request.cc
index 42198cd..8f85334 100644
--- a/src/lib/server_common/socket_request.cc
+++ b/src/lib/server_common/socket_request.cc
@@ -23,6 +23,7 @@
#include <sys/un.h>
#include <sys/socket.h>
#include <cerrno>
+#include <csignal>
#include <cstddef>
namespace isc {
@@ -182,7 +183,14 @@ createFdShareSocket(const std::string& path) {
// \exception SocketError if the socket cannot be read
// \return the socket fd that has been read
int
-getSocketFd(int sock_pass_fd) {
+getSocketFd(const std::string& token, int sock_pass_fd) {
+ // Tell the boss the socket token.
+ const std::string token_data = token + "\n";
+ if (!isc::util::io::write_data(sock_pass_fd, token_data.c_str(),
+ token_data.size())) {
+ isc_throw(SocketRequestor::SocketError, "Error writing socket token");
+ }
+
// Boss first sends some data to signal that getting the socket
// from its cache succeeded
char status[3]; // We need a space for trailing \0, hence 3
@@ -231,7 +239,19 @@ class SocketRequestorCCSession : public SocketRequestor {
public:
SocketRequestorCCSession(config::ModuleCCSession& session) :
session_(session)
- {}
+ {
+ // We need to filter SIGPIPE to prevent it from happening in
+ // getSocketFd() while writing to the UNIX domain socket after the
+ // remote end closed it. See lib/util/io/socketsession for more
+ // background details.
+ // Note: we should eventually unify this level of details into a single
+ // module. Setting a single filter here should be considered a short
+ // term workaround.
+ if (signal(SIGPIPE, SIG_IGN) == SIG_ERR) {
+ isc_throw(Unexpected, "Failed to filter SIGPIPE: " <<
+ strerror(errno));
+ }
+ }
~SocketRequestorCCSession() {
closeFdShareSockets();
@@ -240,7 +260,8 @@ public:
virtual SocketID requestSocket(Protocol protocol,
const std::string& address,
uint16_t port, ShareMode share_mode,
- const std::string& share_name) {
+ const std::string& share_name)
+ {
const isc::data::ConstElementPtr request_msg =
createRequestSocketMessage(protocol, address, port,
share_mode, share_name);
@@ -264,7 +285,7 @@ public:
const int sock_pass_fd = getFdShareSocket(path);
// and finally get the socket itself
- const int passed_sock_fd = getSocketFd(sock_pass_fd);
+ const int passed_sock_fd = getSocketFd(token, sock_pass_fd);
return (SocketID(passed_sock_fd, token));
}
diff --git a/src/lib/server_common/tests/socket_requestor_test.cc b/src/lib/server_common/tests/socket_requestor_test.cc
index 33c92bf..a32a53d 100644
--- a/src/lib/server_common/tests/socket_requestor_test.cc
+++ b/src/lib/server_common/tests/socket_requestor_test.cc
@@ -289,6 +289,22 @@ TEST_F(SocketRequestorTest, testBadSocketReleaseAnswers) {
SocketRequestor::SocketError);
}
+// A helper function to impose a read timeout for the server socket
+// in order to avoid deadlock when the client side has a bug and doesn't
+// send expected data.
+// It returns true when the timeout is set successfully; otherwise false.
+bool
+setRecvTimo(int s) {
+ const struct timeval timeo = { 10, 0 }; // 10sec, arbitrary choice
+ if (setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo)) == 0) {
+ return (true);
+ }
+ if (errno == ENOPROTOOPT) { // deviant OS, give up using it.
+ return (false);
+ }
+ isc_throw(isc::Unexpected, "set RCVTIMEO failed: " << strerror(errno));
+}
+
// Helper test class that creates a randomly named domain socket
// Upon init, it will only reserve the name (and place an empty file in its
// place).
@@ -315,7 +331,10 @@ public:
void
cleanup() {
unlink(path_);
- free(path_);
+ if (path_ != NULL) {
+ free(path_);
+ path_ = NULL;
+ }
if (fd_ != -1) {
close(fd_);
fd_ = -1;
@@ -327,23 +346,23 @@ public:
return (path_);
}
- // create socket, fork, and serve if child (child will exit when done)
- void run(const std::vector<int>& data) {
- try {
- create();
- const int child_pid = fork();
- if (child_pid == 0) {
- serve(data);
- exit(0);
- } else {
- // parent does not need fd anymore
- close(fd_);
- fd_ = -1;
- }
- } catch (const std::exception&) {
- cleanup();
- throw;
+ // create socket, fork, and serve if child (child will exit when done).
+ // If the underlying system doesn't allow to set read timeout, tell the
+ // caller that via a false return value so that the caller can avoid
+ // performing tests that could result in a dead lock.
+ bool run(const std::vector<std::pair<std::string, int> >& data) {
+ create();
+ const bool timo_ok = setRecvTimo(fd_);
+ const int child_pid = fork();
+ if (child_pid == 0) {
+ serve(data);
+ exit(0);
+ } else {
+ // parent does not need fd anymore
+ close(fd_);
+ fd_ = -1;
}
+ return (timo_ok);
}
private:
// Actually create the socket and listen on it
@@ -395,17 +414,34 @@ private:
// NOTE: client_fd could leak on exception. This should be cleaned up.
// See the note about SocketSessionReceiver in socket_request.cc.
void
- serve(const std::vector<int>& data) {
+ serve(const std::vector<std::pair<std::string, int> > data) {
const int client_fd = accept(fd_, NULL, NULL);
if (client_fd == -1) {
- isc_throw(Exception, "Error in accept(): " << strerror(errno));
+ isc_throw(Unexpected, "Error in accept(): " << strerror(errno));
+ }
+ if (!setRecvTimo(client_fd)) {
+ // In the loop below we do blocking read. To avoid deadlock
+ // when the parent is buggy we'll skip it unless we can
+ // set a read timeout on the socket.
+ return;
}
- BOOST_FOREACH(int cur_data, data) {
+ typedef std::pair<std::string, int> DataPair;
+ BOOST_FOREACH(DataPair cur_data, data) {
+ char buf[5];
+ memset(buf, 0, 5);
+ if (isc::util::io::read_data(client_fd, buf, 4) != 4) {
+ isc_throw(Unexpected, "unable to receive socket token");
+ }
+ if (cur_data.first != buf) {
+ isc_throw(Unexpected, "socket token mismatch: expected="
+ << cur_data.first << ", actual=" << buf);
+ }
+
bool result;
- if (cur_data == -1) {
+ if (cur_data.second == -1) {
// send 'CREATOR_SOCKET_UNAVAILABLE'
result = isc::util::io::write_data(client_fd, "0\n", 2);
- } else if (cur_data == -2) {
+ } else if (cur_data.second == -2) {
// send 'CREATOR_SOCKET_OK' first
result = isc::util::io::write_data(client_fd, "1\n", 2);
if (result) {
@@ -417,7 +453,8 @@ private:
// send 'CREATOR_SOCKET_OK' first
result = isc::util::io::write_data(client_fd, "1\n", 2);
if (result) {
- if (isc::util::io::send_fd(client_fd, cur_data) != 0) {
+ if (isc::util::io::send_fd(client_fd,
+ cur_data.second) != 0) {
result = false;
}
}
@@ -436,59 +473,72 @@ private:
TEST_F(SocketRequestorTest, testSocketPassing) {
TestSocket ts;
- std::vector<int> data;
- data.push_back(1);
- data.push_back(2);
- data.push_back(3);
- data.push_back(-1);
- data.push_back(-2);
- data.push_back(1);
- ts.run(data);
-
- // 1 should be ok
- addAnswer("foo", ts.getPath());
- SocketRequestor::SocketID socket_id = doRequest();
- ASSERT_EQ("foo", socket_id.second);
- ASSERT_EQ(0, close(socket_id.first));
-
- // 2 should be ok too
- addAnswer("bar", ts.getPath());
- socket_id = doRequest();
- ASSERT_EQ("bar", socket_id.second);
- ASSERT_EQ(0, close(socket_id.first));
-
- // 3 should be ok too (reuse earlier token)
- addAnswer("foo", ts.getPath());
- socket_id = doRequest();
- ASSERT_EQ("foo", socket_id.second);
- ASSERT_EQ(0, close(socket_id.first));
-
- // -1 should not
- addAnswer("foo", ts.getPath());
- ASSERT_THROW(doRequest(), SocketRequestor::SocketError);
-
- // -2 should not
- addAnswer("foo", ts.getPath());
- ASSERT_THROW(doRequest(), SocketRequestor::SocketError);
+ std::vector<std::pair<std::string, int> > data;
+ data.push_back(std::pair<std::string, int>("foo\n", 1));
+ data.push_back(std::pair<std::string, int>("bar\n", 2));
+ data.push_back(std::pair<std::string, int>("foo\n", 3));
+ data.push_back(std::pair<std::string, int>("foo\n", 1));
+ data.push_back(std::pair<std::string, int>("foo\n", -1));
+ data.push_back(std::pair<std::string, int>("foo\n", -2));
+
+ // run() returns true iff we can specify read timeout so we avoid a
+ // deadlock. Unless there's a bug the test should succeed even without the
+ // timeout, but we don't want to make the test hang up in case with an
+ // unexpected bug, so we'd rather skip most of the tests in that case.
+ const bool timo_ok = ts.run(data);
+ SocketRequestor::SocketID socket_id;
+ if (timo_ok) {
+ // 1 should be ok
+ addAnswer("foo", ts.getPath());
+ socket_id = doRequest();
+ ASSERT_EQ("foo", socket_id.second);
+ ASSERT_EQ(0, close(socket_id.first));
+
+ // 2 should be ok too
+ addAnswer("bar", ts.getPath());
+ socket_id = doRequest();
+ ASSERT_EQ("bar", socket_id.second);
+ ASSERT_EQ(0, close(socket_id.first));
+
+ // 3 should be ok too (reuse earlier token)
+ addAnswer("foo", ts.getPath());
+ socket_id = doRequest();
+ ASSERT_EQ("foo", socket_id.second);
+ ASSERT_EQ(0, close(socket_id.first));
+ }
// Create a second socket server, to test that multiple different
// domains sockets would work as well (even though we don't actually
// use that feature)
TestSocket ts2;
- std::vector<int> data2;
- data2.push_back(1);
- ts2.run(data2);
- // 1 should be ok
- addAnswer("foo", ts2.getPath());
- socket_id = doRequest();
- ASSERT_EQ("foo", socket_id.second);
- ASSERT_EQ(0, close(socket_id.first));
+ std::vector<std::pair<std::string, int> > data2;
+ data2.push_back(std::pair<std::string, int>("foo\n", 1));
+ const bool timo_ok2 = ts2.run(data2);
+
+ if (timo_ok2) {
+ // 1 should be ok
+ addAnswer("foo", ts2.getPath());
+ socket_id = doRequest();
+ ASSERT_EQ("foo", socket_id.second);
+ ASSERT_EQ(0, close(socket_id.first));
+ }
- // Now use first socket again
- addAnswer("foo", ts.getPath());
- socket_id = doRequest();
- ASSERT_EQ("foo", socket_id.second);
- ASSERT_EQ(0, close(socket_id.first));
+ if (timo_ok) {
+ // Now use first socket again
+ addAnswer("foo", ts.getPath());
+ socket_id = doRequest();
+ ASSERT_EQ("foo", socket_id.second);
+ ASSERT_EQ(0, close(socket_id.first));
+
+ // -1 is a "normal" error
+ addAnswer("foo", ts.getPath());
+ ASSERT_THROW(doRequest(), SocketRequestor::SocketError);
+
+ // -2 is an unexpected error. After this point it's not guaranteed the
+ // connection works as intended.
+ addAnswer("foo", ts.getPath());
+ ASSERT_THROW(doRequest(), SocketRequestor::SocketError);
+ }
// Vector is of first socket is now empty, so the socket should be gone
addAnswer("foo", ts.getPath());
@@ -500,5 +550,4 @@ TEST_F(SocketRequestorTest, testSocketPassing) {
ASSERT_THROW(doRequest(), SocketRequestor::SocketError);
}
-
}
diff --git a/src/lib/util/io/fd.cc b/src/lib/util/io/fd.cc
index 04e64dc..2797015 100644
--- a/src/lib/util/io/fd.cc
+++ b/src/lib/util/io/fd.cc
@@ -23,23 +23,23 @@ namespace io {
bool
write_data(const int fd, const void *buffer_v, const size_t length) {
- const unsigned char *buffer(static_cast<const unsigned char *>(buffer_v));
+ const unsigned char* buffer(static_cast<const unsigned char*>(buffer_v));
size_t rest(length);
// Just keep writing until all is written
while (rest) {
- ssize_t written(write(fd, buffer, rest));
- if (rest == -1) {
+ const int written = write(fd, buffer, rest);
+ if (written == -1) {
if (errno == EINTR) { // Just keep going
continue;
} else {
- return false;
+ return (false);
}
} else { // Wrote something
rest -= written;
buffer += written;
}
}
- return true;
+ return (true);
}
ssize_t
@@ -47,22 +47,22 @@ read_data(const int fd, void *buffer_v, const size_t length) {
unsigned char *buffer(static_cast<unsigned char *>(buffer_v));
size_t rest(length), already(0);
while (rest) { // Stil something to read
- ssize_t amount(read(fd, buffer, rest));
- if (rest == -1) {
+ const int amount = read(fd, buffer, rest);
+ if (amount == -1) {
if (errno == EINTR) { // Continue on interrupted call
continue;
} else {
- return -1;
+ return (-1);
}
} else if (amount) {
already += amount;
rest -= amount;
buffer += amount;
} else { // EOF
- return already;
+ return (already);
}
}
- return already;
+ return (already);
}
}
More information about the bind10-changes
mailing list