[master] Merged trac4074 (child process)

src/lib/config/command_socket_factory.cc

@@ -85,8 +85,17 @@ private:
         // shut down properly.
         static_cast<void>(remove(file_name.c_str()));
 
+        // Set this socket to be closed-on-exec.
+        if (fcntl(fd, F_SETFD, FD_CLOEXEC) != 0) {
+            const char* errmsg = strerror(errno);
+            ::close(fd);
+            isc_throw(SocketError, "Failed to set close-on-exec on unix socket\
+ "
+                      << fd << ": " << errmsg);
+        }
+
         // Set this socket to be non-blocking one.
-        if (fcntl(fd, F_SETFL, O_NONBLOCK) !=0 ) {
+        if (fcntl(fd, F_SETFL, O_NONBLOCK) != 0) {
             const char* errmsg = strerror(errno);
             ::close(fd);
             isc_throw(SocketError, "Failed to set non-block mode on unix socket "

src/lib/dhcp/iface_mgr_linux.cc

@@ -42,6 +42,7 @@
 #include <boost/array.hpp>
 #include <boost/static_assert.hpp>
 
+#include <fcntl.h>
 #include <stdint.h>
 #include <net/if.h>
 #include <linux/rtnetlink.h>
@@ -135,6 +136,10 @@ void Netlink::rtnl_open_socket() {
         isc_throw(Unexpected, "Failed to create NETLINK socket.");
     }
 
+    if (fcntl(fd_, F_SETFD, FD_CLOEXEC) < 0) {
+        isc_throw(Unexpected, "Failed to set close-on-exec in NETLINK socket.");
+    }
+
     if (setsockopt(fd_, SOL_SOCKET, SO_SNDBUF, &SNDBUF_SIZE, sizeof(SNDBUF_SIZE)) < 0) {
         isc_throw(Unexpected, "Failed to set send buffer in NETLINK socket.");
     }

src/lib/dhcp/pkt_filter.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013, 2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -32,6 +32,14 @@ PktFilter::openFallbackSocket(const isc::asiolink::IOAddress& addr,
                   " address " << addr << ", port " << port
                   << ", reason: " << strerror(errno));
     }
+    // Set the close-on-exec flag.
+    if (fcntl(sock, F_SETFD, FD_CLOEXEC) < 0) {
+        close(sock);
+        isc_throw(SocketConfigError, "Failed to set close-on-exec flag"
+                  << " on fallback socket for address " << addr
+                  << ", port " << port
+                  << ", reason: " << strerror(errno));
+    }
     // Bind the socket to a specified address and port.
     struct sockaddr_in addr4;
     memset(&addr4, 0, sizeof(addr4));

src/lib/dhcp/pkt_filter_bpf.cc

@@ -266,6 +266,14 @@ PktFilterBPF::openSocket(Iface& iface,
         }
     }
 
+    // Set the close-on-exec flag.
+    if (fcntl(sock, F_SETFD, FD_CLOEXEC) < 0) {
+        close(fallback);
+        close(sock);
+        isc_throw(SocketConfigError, "Failed to set close-on-exec flag"
+                  << " on BPF device with interface " << iface.getName());
+    }
+
     // The BPF device is now open. Now it needs to be configured.
 
     // Associate the device with the interface name.

src/lib/dhcp/pkt_filter_inet.cc

@@ -18,6 +18,7 @@
 #include <dhcp/pkt_filter_inet.h>
 #include <errno.h>
 #include <cstring>
+#include <fcntl.h>
 
 using namespace isc::asiolink;
 
@@ -55,6 +56,13 @@ PktFilterInet::openSocket(Iface& iface,
         isc_throw(SocketConfigError, "Failed to create UDP6 socket.");
     }
 
+    // Set the close-on-exec flag.
+    if (fcntl(sock, F_SETFD, FD_CLOEXEC) < 0) {
+        close(sock);
+        isc_throw(SocketConfigError, "Failed to set close-on-exec flag"
+                  << " on socket " << sock);
+    }
+
 #ifdef SO_BINDTODEVICE
     if (receive_bcast && iface.flag_broadcast_) {
         // Bind to device so as we receive traffic on a specific interface.

src/lib/dhcp/pkt_filter_inet6.cc

@@ -19,6 +19,7 @@
 #include <dhcp/pkt_filter_inet6.h>
 #include <util/io/pktinfo_utilities.h>
 
+#include <fcntl.h>
 #include <netinet/in.h>
 
 using namespace isc::asiolink;
@@ -66,6 +67,13 @@ PktFilterInet6::openSocket(const Iface& iface,
         isc_throw(SocketConfigError, "Failed to create UDP6 socket.");
     }
 
+    // Set the close-on-exec flag.
+    if (fcntl(sock, F_SETFD, FD_CLOEXEC) < 0) {
+        close(sock);
+        isc_throw(SocketConfigError, "Failed to set close-on-exec flag"
+                  << " on IPv6 socket.");
+    }
+
     // Set SO_REUSEADDR option.
     int flag = 1;
     if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR,

src/lib/dhcp/pkt_filter_lpf.cc

@@ -19,6 +19,7 @@
 #include <dhcp/pkt_filter_lpf.h>
 #include <dhcp/protocol_util.h>
 #include <exceptions/exceptions.h>
+#include <fcntl.h>
 #include <linux/filter.h>
 #include <linux/if_ether.h>
 #include <linux/if_packet.h>
@@ -154,6 +155,14 @@ PktFilterLPF::openSocket(Iface& iface,
         isc_throw(SocketConfigError, "Failed to create raw LPF socket");
     }
 
+    // Set the close-on-exec flag.
+    if (fcntl(sock, F_SETFD, FD_CLOEXEC) < 0) {
+        close(sock);
+        close(fallback);
+        isc_throw(SocketConfigError, "Failed to set close-on-exec flag"
+                  << " on the socket " << sock);
+    }
+
     // Create socket filter program. This program will only allow incoming UDP
     // traffic which arrives on the specific (DHCP) port). It will also filter
     // out all fragmented packets.

src/lib/util/Makefile.am

@@ -10,7 +10,6 @@ lib_LTLIBRARIES = libkea-util.la
 libkea_util_la_SOURCES  = boost_time_utils.h boost_time_utils.cc
 libkea_util_la_SOURCES += csv_file.h csv_file.cc
 libkea_util_la_SOURCES += filename.h filename.cc
-libkea_util_la_SOURCES += fork_detector.h
 libkea_util_la_SOURCES += strutil.h strutil.cc
 libkea_util_la_SOURCES += buffer.h io_utilities.h
 libkea_util_la_SOURCES += time_utilities.h time_utilities.cc

src/lib/util/fork_detector.h

@@ -1,62 +0,0 @@
-// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
-//
-// Permission to use, copy, modify, and/or distribute this software for any
-// purpose with or without fee is hereby granted, provided that the above
-// copyright notice and this permission notice appear in all copies.
-//
-// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
-// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
-// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
-// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
-// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
-// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
-// PERFORMANCE OF THIS SOFTWARE.
-
-#ifndef FORK_DETECTOR_H
-#define FORK_DETECTOR_H
-
-#include <sys/types.h>
-#include <unistd.h>
-
-namespace isc {
-namespace util {
-
-/// @brief Class which detects being child process.
-///
-/// This class detects if it is in the child process, by checking if the
-/// process PID matches the PID of the process which created instance of
-/// the class.
-///
-/// Detecting if we're in the child process is important when the application
-/// spawns new process using fork/exec. If exec step fails for any reason
-/// the child process exits. However, to exit gracefully the process may
-/// need to know if it is a child or parent and take a different path
-/// during destruction.
-class ForkDetector {
-public:
-
-    /// @brief Constructor.
-    ///
-    /// Stores the PID of the process creating this instance.
-    ForkDetector()
-        : creator_pid_(getpid()) {
-    }
-
-    /// @brief Check if the process is a parent process;
-    ///
-    /// @return true if the process is a parent process.
-    bool isParent() const {
-        return (getpid() == creator_pid_);
-    }
-
-private:
-
-    /// @brief PID of the process which created instance of this class.
-    pid_t creator_pid_;
-
-};
-
-} // namespace isc::util
-} // namespace isc
-
-#endif // FORK_DETECTOR_H

src/lib/util/process_spawn.cc

@@ -217,10 +217,10 @@ ProcessSpawnImpl::spawn() {
         if (execvp(executable_.c_str(), args_) != 0) {
             // We may end up here if the execvp failed, e.g. as a result
             // of issue with permissions or invalid executable name.
-            exit(EXIT_FAILURE);
+            _exit(EXIT_FAILURE);
         }
         // Process finished, exit the child process.
-        exit(EXIT_SUCCESS);
+        _exit(EXIT_SUCCESS);
     }
 
     // We're in the parent process.

src/lib/util/threads/tests/thread_unittest.cc

@@ -242,26 +242,13 @@ TEST_F(ThreadTest, sigmask) {
 }
 
 
-/// The @c ProcessSpawn class spawns new processes using the fork/exec
-/// scheme. If the call to exec fails, child process exits with the
-/// EXIT_FAILURE status code. The call to exit triggers destruction of
-/// all static objects, i.e. destructors of statically declared
-/// @c Thread objects are called in the child process. The call to
-/// fork doesn't clone threads existing in the main process. So, the
-/// @c Thread objects in the child process have invalid state, because
-/// they point to non-existing threads. As a result any attempts to
-/// detach or join the thread may result in errors or asserts.
-///
-/// This test verifies that the @c Thread class protects against such
-/// errors. It is supposed to detect that the @c Thread object is in
-/// the child process and not assert when the destruction fails.
+/// This test verifies using threads and spawning child processes
+/// work together.
 TEST_F(ThreadTest, spawnProcessWithThread) {
     // Initialize and run the stoppable thread. Note that the 'thread'
     // is a static variable, which will be 'cloned' into the child
-    // process. Its destructor will be called when the child process
-    // terminates with EXIT_FAILURE status. So in fact the destructor
-    // of the @c StoppableThread will be called twice: in the main
-    // process and in the child process.
+    // process. Its destructor must not be called when the child process
+    // terminates with EXIT_FAILURE status.
     thread.reset(new StoppableThread());
     thread->run();
 
@@ -273,14 +260,11 @@ TEST_F(ThreadTest, spawnProcessWithThread) {
     while (process_spawn.isRunning(pid)) {
         usleep(100);
     }
-    // When the child process terminates it will call destructors of
-    // static objects. This means that it will call the destructor of
-    // the 'thread' object too. The 'thread' object in the child
-    // process points to non-existing thread, but we expect the
-    // graceful destruction, i.e. the destructor should not assert.
-    // If the destructor asserts the exit code returned here will
-    // be 0 - same as if we aborted.
+    // When the child process terminates it will call _exit() so
+    // nothing bad should happen from the child.
     EXPECT_EQ(EXIT_FAILURE, process_spawn.getExitStatus(pid));
+    // The thread is still there.
+    EXPECT_TRUE(thread);
 }
 
 }

src/lib/util/threads/thread.cc

@@ -12,7 +12,6 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include <util/fork_detector.h>
 #include <util/threads/thread.h>
 #include <util/threads/sync.h>
 
@@ -75,8 +74,7 @@ public:
         // and the creating thread needs to release it.
         waiting_(2),
         main_(main),
-        exception_(false),
-        fork_detector_()
+        exception_(false)
     {}
     // Another of the waiting events is done. If there are no more, delete
     // impl.
@@ -127,8 +125,6 @@ public:
     Mutex mutex_;
     // Which thread are we talking about anyway?
     pthread_t tid_;
-    // Class to detect if we're in the child or parent process.
-    ForkDetector fork_detector_;
 };
 
 Thread::Thread(const boost::function<void ()>& main) :
@@ -152,15 +148,8 @@ Thread::Thread(const boost::function<void ()>& main) :
 
 Thread::~Thread() {
     if (impl_ != NULL) {
-
-        int result = pthread_detach(impl_->tid_);
-        // If the error indicates that thread doesn't exist but we're
-        // in child process (after fork) it is expected. We should
-        // not cause an assert.
-        if (result == ESRCH && !impl_->fork_detector_.isParent()) {
-            result = 0;
-        }
-
+        // In case we didn't call wait yet
+        const int result = pthread_detach(impl_->tid_);
         Impl::done(impl_);
         impl_ = NULL;
         // If the detach ever fails, something is screwed rather badly.
@@ -175,20 +164,13 @@ Thread::wait() {
                   "Wait called and no thread to wait for");
     }
 
-    // Was there an exception in the thread?
-    scoped_ptr<UncaughtException> ex;
-
     const int result = pthread_join(impl_->tid_, NULL);
     if (result != 0) {
-        // We will not throw exception if the error indicates that the
-        // thread doesn't exist and we are in the child process (forked).
-        // For the child process it is expected that the thread is not
-        // re-created when we fork.
-        if (result != ESRCH || impl_->fork_detector_.isParent()) {
-            isc_throw(isc::InvalidOperation, std::strerror(result));
-        }
+        isc_throw(isc::InvalidOperation, std::strerror(result));
     }
 
+    // Was there an exception in the thread?
+    scoped_ptr<UncaughtException> ex;
     // Something here could in theory throw. But we already terminated the thread, so
     // we need to make sure we are in consistent state even in such situation (like
     // releasing the mutex and impl_).

src/lib/util/watch_socket.cc

@@ -42,6 +42,18 @@ WatchSocket::WatchSocket()
     source_ = fds[1];
     sink_ = fds[0];
 
+    if (fcntl(source_, F_SETFD, FD_CLOEXEC)) {
+        const char* errstr = strerror(errno);
+        isc_throw(WatchSocketError, "Cannot set source to close-on-exec: "
+                                     << errstr);
+    }
+
+    if (fcntl(sink_, F_SETFD, FD_CLOEXEC)) {
+        const char* errstr = strerror(errno);
+        isc_throw(WatchSocketError, "Cannot set sink to close-on-exec: "
+                                     << errstr);
+    }
+
     if (fcntl(sink_, F_SETFL, O_NONBLOCK)) {
         const char* errstr = strerror(errno);
         isc_throw(WatchSocketError, "Cannot set sink to non-blocking: "