BIND 10 trac1452, updated. f709af9e07ce8a0b700862fbc086e72c8f46f784 [1452] fixed another error handling issue: make sure the passed FD (if it succeeds) is closed if an exception is thrown in the middle of SocketSessionReceiver::pop(). Also clarified the responsibility of the caller of pop on which resource it should release which it shouldn't.
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue Dec 20 01:14:57 UTC 2011
The branch, trac1452 has been updated
via f709af9e07ce8a0b700862fbc086e72c8f46f784 (commit)
via 267a466b3ecac6a2ec07d7c873a3cbb9041f67cb (commit)
from 5050c9607584178e27d9caf196c25df01b3cd651 (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 f709af9e07ce8a0b700862fbc086e72c8f46f784
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon Dec 19 17:13:17 2011 -0800
[1452] fixed another error handling issue: make sure the passed FD (if
it succeeds) is closed if an exception is thrown in the middle of
SocketSessionReceiver::pop(). Also clarified the responsibility of
the caller of pop on which resource it should release which it shouldn't.
commit 267a466b3ecac6a2ec07d7c873a3cbb9041f67cb
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon Dec 19 15:06:05 2011 -0800
[1452] fixed some more corner cases in receiver::pop that have been missed:
- socket family mismatch for the remote endpoint
- cases where the given SA length is too small
corresponding tests were added, too.
-----------------------------------------------------------------------
Summary of changes:
src/lib/util/io/socketsession.cc | 43 ++++++++++++++++++++-----
src/lib/util/io/socketsession.h | 8 +++++
src/lib/util/tests/socketsession_unittest.cc | 40 +++++++++++++++++++++--
3 files changed, 78 insertions(+), 13 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/util/io/socketsession.cc b/src/lib/util/io/socketsession.cc
index 4bf5100..4c50949 100644
--- a/src/lib/util/io/socketsession.cc
+++ b/src/lib/util/io/socketsession.cc
@@ -33,6 +33,8 @@
#include <string>
#include <vector>
+#include <boost/noncopyable.hpp>
+
#include <exceptions/exceptions.h>
#include <util/buffer.h>
@@ -320,15 +322,33 @@ readFail(int actual_len, int expected_len) {
"SocketSessionForwarder: " << actual_len << "/" <<
expected_len);
}
+
+// A helper container for a (socket) file descriptor used in
+// SocketSessionReceiver::pop that ensures the socket is closed unless it
+// can be safely passed to the caller via release().
+struct ScopedSocket : boost::noncopyable {
+ ScopedSocket(int fd) : fd_(fd) {}
+ ~ScopedSocket() {
+ if (fd_ >= 0) {
+ close(fd_);
+ }
+ }
+ int release() {
+ const int fd = fd_;
+ fd_ = -1;
+ return (fd);
+ }
+ int fd_;
+};
}
SocketSession
SocketSessionReceiver::pop() {
- const int passed_fd = recv_fd(impl_->fd_);
- if (passed_fd == FD_SYSTEM_ERROR) {
+ ScopedSocket passed_sock(recv_fd(impl_->fd_));
+ if (passed_sock.fd_ == FD_SYSTEM_ERROR) {
isc_throw(SocketSessionError, "Receiving a forwarded FD failed: " <<
strerror(errno));
- } else if (passed_fd < 0) {
+ } else if (passed_sock.fd_ < 0) {
isc_throw(SocketSessionError, "No FD forwarded");
}
@@ -361,18 +381,23 @@ SocketSessionReceiver::pop() {
const int type = static_cast<int>(ibuffer.readUint32());
const int protocol = static_cast<int>(ibuffer.readUint32());
const socklen_t local_end_len = ibuffer.readUint32();
- if (local_end_len > sizeof(impl_->ss_local_)) {
- isc_throw(SocketSessionError, "Local SA length too large: " <<
+ const socklen_t endpoint_minlen = (family == AF_INET) ?
+ sizeof(struct sockaddr_in) : sizeof(struct sockaddr_in6);
+ if (local_end_len < endpoint_minlen ||
+ local_end_len > sizeof(impl_->ss_local_)) {
+ isc_throw(SocketSessionError, "Invalid local SA length: " <<
local_end_len);
}
ibuffer.readData(&impl_->ss_local_, local_end_len);
const socklen_t remote_end_len = ibuffer.readUint32();
- if (remote_end_len > sizeof(impl_->ss_remote_)) {
- isc_throw(SocketSessionError, "Remote SA length too large: " <<
+ if (remote_end_len < endpoint_minlen ||
+ remote_end_len > sizeof(impl_->ss_remote_)) {
+ isc_throw(SocketSessionError, "Invalid remote SA length: " <<
remote_end_len);
}
ibuffer.readData(&impl_->ss_remote_, remote_end_len);
- if (family != impl_->sa_local_->sa_family) {
+ if (family != impl_->sa_local_->sa_family ||
+ family != impl_->sa_remote_->sa_family) {
isc_throw(SocketSessionError, "SA family inconsistent: " <<
static_cast<int>(impl_->sa_local_->sa_family) << ", " <<
static_cast<int>(impl_->sa_remote_->sa_family) <<
@@ -393,7 +418,7 @@ SocketSessionReceiver::pop() {
readFail(cc_data, data_len);
}
- return (SocketSession(passed_fd, family, type, protocol,
+ return (SocketSession(passed_sock.release(), family, type, protocol,
impl_->sa_local_, impl_->sa_remote_,
&impl_->data_buf_[0], data_len));
} catch (const InvalidBufferPosition& ex) {
diff --git a/src/lib/util/io/socketsession.h b/src/lib/util/io/socketsession.h
index 04ba36f..77f18a3 100644
--- a/src/lib/util/io/socketsession.h
+++ b/src/lib/util/io/socketsession.h
@@ -421,6 +421,14 @@ public:
/// this method is called or until the \c SocketSessionReceiver object is
/// destructed.
///
+ /// The caller is responsible for closing the received socket (whose
+ /// file descriptor is accessible via \c SocketSession::getSocket()).
+ /// If the caller copies the returned \c SocketSession object, it's also
+ /// responsible for making sure the descriptor is closed at most once.
+ /// On the other hand, the caller is not responsible for freeing the
+ /// socket session data (accessible via \c SocketSession::getData());
+ /// the \c SocketSessionReceiver object will clean it up automatically.
+ ///
/// It ensures the following:
/// - The address family is either \c AF_INET or \c AF_INET6
/// - The address family (\c sa_family) member of the local and remote
diff --git a/src/lib/util/tests/socketsession_unittest.cc b/src/lib/util/tests/socketsession_unittest.cc
index 154333a..eade68a 100644
--- a/src/lib/util/tests/socketsession_unittest.cc
+++ b/src/lib/util/tests/socketsession_unittest.cc
@@ -25,6 +25,7 @@
#include <cerrno>
#include <cstring>
+#include <algorithm>
#include <string>
#include <utility>
#include <vector>
@@ -279,13 +280,14 @@ protected:
// but can be false for testing.
void pushSessionHeader(uint16_t hdrlen,
size_t hdrlen_len = sizeof(uint16_t),
- bool push_fd = true)
+ bool push_fd = true,
+ int fd = 0)
{
isc::util::OutputBuffer obuffer(0);
obuffer.clear();
dummy_forwarder_.reset(dummyConnect());
- if (push_fd && send_fd(dummy_forwarder_.fd, 0) != 0) {
+ if (push_fd && send_fd(dummy_forwarder_.fd, fd) != 0) {
isc_throw(isc::Unexpected, "Failed to pass FD");
}
obuffer.writeUint16(hdrlen);
@@ -318,9 +320,9 @@ protected:
obuffer.writeUint32(static_cast<uint32_t>(type));
obuffer.writeUint32(static_cast<uint32_t>(protocol));
obuffer.writeUint32(static_cast<uint32_t>(local_len));
- obuffer.writeData(&local, getSALength(local));
+ obuffer.writeData(&local, min(local_len, getSALength(local)));
obuffer.writeUint32(static_cast<uint32_t>(remote_len));
- obuffer.writeData(&remote, getSALength(remote));
+ obuffer.writeData(&remote, min(remote_len, getSALength(remote)));
obuffer.writeUint32(static_cast<uint32_t>(data_len));
pushSessionHeader(obuffer.getLength());
if (send(dummy_forwarder_.fd, obuffer.getData(), obuffer.getLength(),
@@ -751,6 +753,7 @@ TEST_F(ForwardTest, badPop) {
pushSession(AF_INET, SOCK_DGRAM, IPPROTO_UDP, sai_local.second,
*sai_local.first, sai6.second, *sai6.first);
dummy_forwarder_.reset(-1);
+ EXPECT_THROW(receiver_->pop(), SocketSessionError);
// Pass too big sa length for local
pushSession(AF_INET, SOCK_DGRAM, IPPROTO_UDP,
@@ -766,6 +769,20 @@ TEST_F(ForwardTest, badPop) {
dummy_forwarder_.reset(-1);
EXPECT_THROW(receiver_->pop(), SocketSessionError);
+ // Pass too small sa length for local
+ pushSession(AF_INET, SOCK_DGRAM, IPPROTO_UDP,
+ sizeof(struct sockaddr_in) - 1, *sai_local.first,
+ sai_remote.second, *sai_remote.first);
+ dummy_forwarder_.reset(-1);
+ EXPECT_THROW(receiver_->pop(), SocketSessionError);
+
+ // Same for remote
+ pushSession(AF_INET6, SOCK_DGRAM, IPPROTO_UDP,
+ sai6.second, *sai6.first, sizeof(struct sockaddr_in6) - 1,
+ *sai6.first);
+ dummy_forwarder_.reset(-1);
+ EXPECT_THROW(receiver_->pop(), SocketSessionError);
+
// Data length is too large
pushSession(AF_INET, SOCK_DGRAM, IPPROTO_UDP, sai_local.second,
*sai_local.first, sai_remote.second,
@@ -786,6 +803,21 @@ TEST_F(ForwardTest, badPop) {
*sai_remote.first, sizeof(TEST_DATA) + 1);
dummy_forwarder_.reset(-1);
EXPECT_THROW(receiver_->pop(), SocketSessionError);
+
+ // Check the forwarded FD is closed on failure
+ ScopedSocket sock(createSocket(AF_INET, SOCK_DGRAM, IPPROTO_UDP,
+ getSockAddr("127.0.0.1", TEST_PORT),
+ false));
+ pushSessionHeader(0, 1, true, sock.fd);
+ dummy_forwarder_.reset(-1);
+ EXPECT_THROW(receiver_->pop(), SocketSessionError);
+ // Close the original socket
+ sock.reset(-1);
+ // The passed one should have been closed, too, so we should be able
+ // to bind a new socket to the same port.
+ ScopedSocket(createSocket(AF_INET, SOCK_DGRAM, IPPROTO_UDP,
+ getSockAddr("127.0.0.1", TEST_PORT),
+ false));
}
TEST(SocketSessionTest, badValue) {
More information about the bind10-changes
mailing list