Stop leaking the /dev/null fd after spawning a subprocess.

[cubemap] / httpinput.cpp

diff --git a/httpinput.cpp b/httpinput.cpp
index 274cc2687b160d45e365fc7f4daafb665b9d2660..62ed663611402cef5d4517cef00d8033300391bc 100644 (file)
--- a/httpinput.cpp
+++ b/httpinput.cpp
@@ -75,11 +75,12 @@ HTTPInput::HTTPInput(const InputProto &serialized)
          http_header(serialized.http_header()),
          stream_header(serialized.stream_header()),
          has_metacube_header(serialized.has_metacube_header()),
-         sock(serialized.sock())
+         sock(serialized.sock()),
+         child_pid(serialized.child_pid())
 {
        // Set back the close-on-exec flag for the socket.
        // (This can't leak into a child, since we haven't been started yet.)
-       fcntl(sock, F_SETFD, 1);
+       fcntl(sock, F_SETFD, O_CLOEXEC);
 
        pending_data.resize(serialized.pending_data().size());
        memcpy(&pending_data[0], serialized.pending_data().data(), serialized.pending_data().size());
@@ -112,6 +113,13 @@ void HTTPInput::close_socket()
                safe_close(sock);
                sock = -1;
        }
+       if (child_pid != -1) {
+               // Kill the child process group, forcibly.
+               // TODO: Consider using a pidfd on newer kernels, so that we're guaranteed
+               // never to kill the wrong process.
+               kill(-child_pid, SIGKILL);
+       }
+       child_pid = -1;
 
        lock_guard<mutex> lock(stats_mutex);
        stats.connect_time = -1;
@@ -134,6 +142,7 @@ InputProto HTTPInput::serialize() const
        serialized.set_pending_data(string(pending_data.begin(), pending_data.end()));
        serialized.set_has_metacube_header(has_metacube_header);
        serialized.set_sock(sock);
+       serialized.set_child_pid(child_pid);
        serialized.set_bytes_received(stats.bytes_received);
        serialized.set_data_bytes_received(stats.data_bytes_received);
        if (isfinite(stats.latency_sec)) {
@@ -247,23 +256,33 @@ int HTTPInput::open_child_process(const string &cmdline)
        posix_spawn_file_actions_adddup2(&actions, devnullfd, 0);
        posix_spawn_file_actions_adddup2(&actions, pipefd[1], 1);
 
-       pid_t child_pid;
+       // Make the process a leader of its own process group, so that we can easily
+       // kill it and any of its child processes (unless it's started new process
+       // groups itself, of course).
+       posix_spawnattr_t attr;
+       posix_spawnattr_init(&attr);
+       posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETPGROUP);
+       posix_spawnattr_setpgroup(&attr, 0);
+
        char * const argv[] = {
                strdup("/bin/sh"),
                strdup("-c"),
                strdup(path.c_str()),
                nullptr
        };
-       int err = posix_spawn(&child_pid, "/bin/sh", &actions, /*attrp=*/nullptr, argv, /*envp=*/nullptr);
+       int err = posix_spawn(&child_pid, "/bin/sh", &actions, &attr, argv, /*envp=*/nullptr);
        posix_spawn_file_actions_destroy(&actions);
+       posix_spawnattr_destroy(&attr);
        free(argv[0]);
        free(argv[1]);
        free(argv[2]);
+       close(devnullfd);
        close(pipefd[1]);
 
        if (err == 0) {
                return pipefd[0];
        } else {
+               child_pid = -1;
                log_perror(cmdline.c_str());
                close(pipefd[0]);
                return -1;
@@ -383,8 +402,21 @@ void HTTPInput::do_work()
                switch (state) {
                case NOT_CONNECTED: {
                        // Reap any exited children.
-                       int wstatus;
-                       while (waitpid(-1, &wstatus, WNOHANG) != -1 || errno == EINTR) ;
+                       int wstatus, err;
+                       do {
+                               err = waitpid(-1, &wstatus, WNOHANG);
+                               if (err == -1) {
+                                       if (errno == EINTR) {
+                                               continue;
+                                       }
+                                       if (errno == ECHILD) {
+                                               break;
+                                       }
+                                       log_perror("waitpid");
+                                       break;
+                               }
+                       } while (err != 0);
+                       child_pid = -1;
 
                        request.clear();
                        request_bytes_sent = 0;