BIND 10 trac1534, updated. bc4fc1865977c94d4ad6eddf0cf96a7881a71536 [1534] Close sockets on errors
BIND 10 source code commits
bind10-changes at lists.isc.org
Thu Feb 23 11:59:22 UTC 2012
The branch, trac1534 has been updated
via bc4fc1865977c94d4ad6eddf0cf96a7881a71536 (commit)
via f43af5ae2a1a904d1fa091d227f6d5cb1c580fa3 (commit)
from 5758984e174f25954af41dcf07979e2dfca12763 (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 bc4fc1865977c94d4ad6eddf0cf96a7881a71536
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Thu Feb 23 12:58:52 2012 +0100
[1534] Close sockets on errors
commit f43af5ae2a1a904d1fa091d227f6d5cb1c580fa3
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Thu Feb 23 12:34:27 2012 +0100
[1534] Remove the failing test
Because it's the test which is failing, not the code. Unfortunately, it
can't be reliably tested :-(.
-----------------------------------------------------------------------
Summary of changes:
src/bin/sockcreator/sockcreator.cc | 24 +++++++--
src/bin/sockcreator/sockcreator.h | 15 +++--
src/bin/sockcreator/tests/sockcreator_tests.cc | 65 +++++++++++++++--------
3 files changed, 69 insertions(+), 35 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/sockcreator/sockcreator.cc b/src/bin/sockcreator/sockcreator.cc
index 38547e4..e87a310 100644
--- a/src/bin/sockcreator/sockcreator.cc
+++ b/src/bin/sockcreator/sockcreator.cc
@@ -126,7 +126,7 @@ handleRequest(const int input_fd, const int output_fd,
}
// Obtain the socket
- const int result = get_sock(sock_type, addr, addr_len);
+ const int result = get_sock(sock_type, addr, addr_len, close_fun);
if (result >= 0) {
// Got the socket, send it to the client.
writeMessage(output_fd, "S", 1);
@@ -198,6 +198,17 @@ mtu(int fd) {
return (fd);
}
+// This one closes the socket if result is negative. Used not to leak socket
+// on error.
+int maybeClose(const int result, const int socket, const close_t close_fun) {
+ if (result < 0) {
+ if (close_fun(socket) == -1) {
+ isc_throw(InternalError, "Error closing socket");
+ }
+ }
+ return (result);
+}
+
} // Anonymous namespace
namespace isc {
@@ -205,7 +216,8 @@ namespace socket_creator {
// Get the socket and bind to it.
int
-getSock(const int type, struct sockaddr* bind_addr, const socklen_t addr_len) {
+getSock(const int type, struct sockaddr* bind_addr, const socklen_t addr_len,
+ const close_t close_fun) {
const int sock = socket(bind_addr->sa_family, type, 0);
if (sock == -1) {
return (-1);
@@ -213,19 +225,19 @@ getSock(const int type, struct sockaddr* bind_addr, const socklen_t addr_len) {
const int on = 1;
if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)) == -1) {
// This is part of the binding process, so it's a bind error
- return (-2);
+ return (maybeClose(-2, sock, close_fun));
}
if (bind_addr->sa_family == AF_INET6 &&
setsockopt(sock, IPPROTO_IPV6, IPV6_V6ONLY, &on, sizeof(on)) == -1) {
// This is part of the binding process, so it's a bind error
- return (-2);
+ return (maybeClose(-2, sock, close_fun));
}
if (bind(sock, bind_addr, addr_len) == -1) {
- return (-2);
+ return (maybeClose(-2, sock, close_fun));
}
if (type == SOCK_DGRAM && bind_addr->sa_family == AF_INET6) {
// Set some MTU flags on IPv6 UDP sockets.
- return (mtu(sock));
+ return (maybeClose(mtu(sock), sock, close_fun));
}
return (sock);
}
diff --git a/src/bin/sockcreator/sockcreator.h b/src/bin/sockcreator/sockcreator.h
index fd6975d..2491d2c 100644
--- a/src/bin/sockcreator/sockcreator.h
+++ b/src/bin/sockcreator/sockcreator.h
@@ -73,6 +73,9 @@ public:
};
+// Type of the close() function, so it can be passed as a parameter.
+// Argument is the same as that for close(2).
+typedef int (*close_t)(int);
/// \short Create a socket and bind it.
///
@@ -82,13 +85,16 @@ public:
/// \param type The type of socket to create (SOCK_STREAM, SOCK_DGRAM, etc).
/// \param bind_addr The address to bind.
/// \param addr_len The actual length of bind_addr.
+/// \param close_fun The furction used to close a socket if there's an error
+/// after the creation.
///
/// \return The file descriptor of the newly created socket, if everything
/// goes well. A negative number is returned if an error occurs -
/// -1 if the socket() call fails or -2 if bind() fails. In case of
/// error, errno is set (or left intact from socket() or bind()).
int
-getSock(const int type, struct sockaddr* bind_addr, const socklen_t addr_len);
+getSock(const int type, struct sockaddr* bind_addr, const socklen_t addr_len,
+ const close_t close_fun);
// Define some types for functions used to perform socket-related operations.
// These are typedefed so that alternatives can be passed through to the
@@ -96,16 +102,13 @@ getSock(const int type, struct sockaddr* bind_addr, const socklen_t addr_len);
// Type of the function to get a socket and to pass it as parameter.
// Arguments are those described above for getSock().
-typedef int (*get_sock_t)(const int, struct sockaddr *, const socklen_t);
+typedef int (*get_sock_t)(const int, struct sockaddr *, const socklen_t,
+ const close_t close_fun);
// Type of the send_fd() function, so it can be passed as a parameter.
// Arguments are the same as those of the send_fd() function.
typedef int (*send_fd_t)(const int, const int);
-// Type of the close() function, so it can be passed as a parameter.
-// Argument is the same as that for close(2).
-typedef int (*close_t)(int);
-
/// \brief Infinite loop parsing commands and returning the sockets.
///
diff --git a/src/bin/sockcreator/tests/sockcreator_tests.cc b/src/bin/sockcreator/tests/sockcreator_tests.cc
index 9f55009..a100cc2 100644
--- a/src/bin/sockcreator/tests/sockcreator_tests.cc
+++ b/src/bin/sockcreator/tests/sockcreator_tests.cc
@@ -22,6 +22,7 @@
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
+#include <arpa/inet.h>
#include <unistd.h>
#include <iostream>
@@ -125,20 +126,12 @@ void addressFamilySpecificCheck(const sockaddr_in6*, const int socknum,
&len)) << strerror(errno);
EXPECT_NE(0, options);
#endif
-#ifdef IPV6_MTU
- struct sockaddr_in6 addr;
- memset(&addr, 0, sizeof(addr));
- addr.sin6_family = AF_INET6;
- addr.sin6_port = htons(53);
- addr.sin6_addr = in6addr_loopback;
- EXPECT_EQ(0, connect(socknum,
- reinterpret_cast<struct sockaddr*>(&addr),
- sizeof(addr)));
- // Use minimum MTU on systems that don't have the IPV6_USE_MIN_MTU
- EXPECT_EQ(0, getsockopt(socknum, IPPROTO_IPV6, IPV6_MTU, &options,
- &len)) << strerror(errno);
- EXPECT_EQ(1280, options);
-#endif
+ // We do not check for the IPV6_MTU, because while setting works (eg.
+ // the packets are fragmented correctly), the getting does not. If
+ // we try to getsockopt it, an error complaining it can't be accessed
+ // on unconnected socket is returned. If we try to connect it, the
+ // MTU of the interface is returned, not the one we set. So we live
+ // in belief it works because we examined the packet dump.
#if defined(IPV6_MTU_DISCOVER) && defined(IPV6_PMTUDISC_DONT)
// Turned off Path MTU discovery on IPv6/UDP sockets?
EXPECT_EQ(0, getsockopt(socknum, IPPROTO_IPV6, IPV6_MTU_DISCOVER,
@@ -148,6 +141,11 @@ void addressFamilySpecificCheck(const sockaddr_in6*, const int socknum,
}
}
+// Just ignore the fd and pretend success. We close invalid fds in the tests.
+int
+closeIgnore(int) {
+ return (0);
+}
// Generic version of the socket test. It creates the socket and checks that
// it is a valid descriptor. The family-specific check functions are called
@@ -165,7 +163,8 @@ void testAnyCreate(int socket_type, socket_check_t socket_check) {
memset(&addr, 0, sizeof(addr));
setAddressFamilyFields(&addr);
sockaddr* addr_ptr = reinterpret_cast<sockaddr*>(&addr);
- const int socket = getSock(socket_type, addr_ptr, sizeof(addr));
+ const int socket = getSock(socket_type, addr_ptr, sizeof(addr),
+ closeIgnore);
ASSERT_GE(socket, 0) << "Couldn't create socket: failed with " <<
"return code " << socket << " and error " << strerror(errno);
@@ -203,12 +202,37 @@ TEST(get_sock, tcp6_create) {
testAnyCreate<sockaddr_in6>(SOCK_STREAM, tcpCheck);
}
+bool close_called(false);
+
+int closeCall(int socket) {
+ close(socket);
+ close_called = true;
+ return (0);
+}
+
// Ask the get_sock function for some nonsense and test if it is able to report
// an error.
TEST(get_sock, fail_with_nonsense) {
sockaddr addr;
memset(&addr, 0, sizeof(addr));
- ASSERT_LT(getSock(0, &addr, sizeof addr), 0);
+ close_called = false;
+ ASSERT_EQ(-1, getSock(0, &addr, sizeof addr, closeCall));
+ ASSERT_FALSE(close_called); // The "socket" call should have failed already
+}
+
+// Bind should have failed here
+TEST(get_sock, fail_with_bind) {
+ sockaddr_in addr;
+ memset(&addr, 0, sizeof(addr));
+ addr.sin_family = AF_INET;
+ addr.sin_port = 1;
+ // No host should have this address on the interface, so it should not be
+ // possible to bind it.
+ addr.sin_addr.s_addr = inet_addr("192.0.2.1");
+ close_called = false;
+ ASSERT_EQ(-2, getSock(SOCK_STREAM, reinterpret_cast<sockaddr*>(&addr),
+ sizeof addr, closeCall));
+ ASSERT_TRUE(close_called); // The "socket" call should have failed already
}
// The main run() function in the socket creator takes three functions to
@@ -230,7 +254,8 @@ TEST(get_sock, fail_with_nonsense) {
// -1: The simulated bind() call has failed
// -2: The simulated socket() call has failed
int
-getSockDummy(const int type, struct sockaddr* addr, const socklen_t) {
+getSockDummy(const int type, struct sockaddr* addr, const socklen_t,
+ const close_t) {
int result = 0;
int port = 0;
@@ -285,12 +310,6 @@ send_FdDummy(const int destination, const int what) {
return (status ? 0 : -1);
}
-// Just ignore the fd and pretend success. We close invalid fds in the tests.
-int
-closeIgnore(int) {
- return (0);
-}
-
// Generic test that it works, with various inputs and outputs.
// It uses different functions to create the socket and send it and pass
// data to it and check it returns correct data back, to see if the run()
More information about the bind10-changes
mailing list