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

[cubemap] / httpinput.cpp

diff --git a/httpinput.cpp b/httpinput.cpp
index 1b09abf6ce8ad550b6c371853fdbcd0bc0bb8e6f..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;
@@ -397,6 +416,7 @@ void HTTPInput::do_work()
                                        break;
                                }
                        } while (err != 0);
+                       child_pid = -1;
 
                        request.clear();
                        request_bytes_sent = 0;