Merge #1534

Conflicts:
	src/bin/sockcreator/Makefile.am

src/bin/sockcreator/sockcreator.cc

@@ -157,7 +157,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);
@@ -186,6 +186,52 @@ handleRequest(const int input_fd, const int output_fd,
     }
 }
 
+// Sets the MTU related flags for IPv6 UDP sockets.
+// It is borrowed from bind-9 lib/isc/unix/socket.c and modified
+// to compile here.
+//
+// The function returns -2 if it fails or the socket file descriptor
+// on success (for convenience, so the result can be just returned).
+int
+mtu(int fd) {
+#ifdef IPV6_USE_MIN_MTU        /* RFC 3542, not too common yet*/
+    const int on(1);
+    // use minimum MTU
+    if (setsockopt(fd, IPPROTO_IPV6, IPV6_USE_MIN_MTU, &on, sizeof(on)) < 0) {
+        return (-2);
+    }
+#else // Try the following as fallback
+#ifdef IPV6_MTU
+    // Use minimum MTU on systems that don't have the IPV6_USE_MIN_MTU
+    const int mtu = 1280;
+    if (setsockopt(fd, IPPROTO_IPV6, IPV6_MTU, &mtu, sizeof(mtu)) < 0) {
+        return (-2);
+    }
+#endif
+#if defined(IPV6_MTU_DISCOVER) && defined(IPV6_PMTUDISC_DONT)
+    // Turn off Path MTU discovery on IPv6/UDP sockets.
+    const int action = IPV6_PMTUDISC_DONT;
+    if (setsockopt(fd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, &action,
+                   sizeof(action)) < 0) {
+
+        return (-2);
+    }
+#endif
+#endif
+    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 {
@@ -193,7 +239,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);
@@ -201,15 +248,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 (maybeClose(mtu(sock), sock, close_fun));
     }
     return (sock);
 }

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.
 ///

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>
@@ -105,17 +106,47 @@ typedef void (*socket_check_t)(const int);
 // The other argument is the socket descriptor number.
 
 // IPv4 check
-void addressFamilySpecificCheck(const sockaddr_in*, const int) {
+void addressFamilySpecificCheck(const sockaddr_in*, const int, const int) {
 }
 
 // IPv6 check
-void addressFamilySpecificCheck(const sockaddr_in6*, const int socknum) {
+void addressFamilySpecificCheck(const sockaddr_in6*, const int socknum,
+                                const int socket_type)
+{
     int options;
     socklen_t len = sizeof(options);
-    EXPECT_EQ(0, getsockopt(socknum, IPPROTO_IPV6, IPV6_V6ONLY, &options, &len));
+    EXPECT_EQ(0, getsockopt(socknum, IPPROTO_IPV6, IPV6_V6ONLY, &options,
+                            &len));
     EXPECT_NE(0, options);
+    if (socket_type == SOCK_DGRAM) {
+    // Some more checks for UDP - MTU
+#ifdef IPV6_USE_MIN_MTU        /* RFC 3542, not too common yet*/
+        // use minimum MTU
+        EXPECT_EQ(getsockopt(socknum, IPPROTO_IPV6, IPV6_USE_MIN_MTU, &options,
+                             &len)) << strerror(errno);
+        EXPECT_NE(0, options);
+#else
+        // 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,
+                                &options, &len)) << strerror(errno);
+        EXPECT_EQ(IPV6_PMTUDISC_DONT, options);
+#endif
+#endif
+    }
 }
 
+// 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
@@ -133,7 +164,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);
 
@@ -147,7 +179,7 @@ void testAnyCreate(int socket_type, socket_check_t socket_check) {
     EXPECT_NE(0, options);
 
     // ...and the address-family specific tests.
-    addressFamilySpecificCheck(&addr, socket);
+    addressFamilySpecificCheck(&addr, socket, socket_type);
 
     // Tidy up and exit.
     EXPECT_EQ(0, close(socket));
@@ -171,12 +203,40 @@ TEST(get_sock, tcp6_create) {
     testAnyCreate<sockaddr_in6>(SOCK_STREAM, tcpCheck);
 }
 
+bool close_called(false);
+
+// You can use it as a close mockup. If you care about checking if it was really
+// called, you can use the close_called variable. But set it to false before the
+// test.
+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
@@ -198,7 +258,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;
 
@@ -253,12 +314,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()