From fa698856b55f1060c7b0a9d8cc9dd4b958f27ec0 Mon Sep 17 00:00:00 2001 From: Matthew Lugg Date: Sun, 26 May 2024 09:40:49 -0700 Subject: [PATCH] std.process.Child: prevent racing children from inheriting one another's pipes The added comment explains the issue here relatively well. The new progress API made this bug obvious because it became visibly clear that certain Compile steps were seemingly "hanging" until other steps completed. As it turned out, these child processes had raced to spawn, and hence one had inherited the other's stdio pipes, meaning the `poll` call in `std.Build.Step.evalZigProcess` was not identifying the child stdout as closed until an unrelated process terminated. --- lib/std/process/Child.zig | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/lib/std/process/Child.zig b/lib/std/process/Child.zig index 527c31a3b3fc..48ab67800e97 100644 --- a/lib/std/process/Child.zig +++ b/lib/std/process/Child.zig @@ -526,7 +526,18 @@ fn statusToTerm(status: u32) Term { } fn spawnPosix(self: *ChildProcess) SpawnError!void { - const pipe_flags: posix.O = .{}; + // The child process does need to access (one end of) these pipes. However, + // we must initially set CLOEXEC to avoid a race condition. If another thread + // is racing to spawn a different child process, we don't want it to inherit + // these FDs in any scenario; that would mean that, for instance, calls to + // `poll` from the parent would not report the child's stdout as closing when + // expected, since the other child may retain a reference to the write end of + // the pipe. So, we create the pipes with CLOEXEC initially. After fork, we + // need to do something in the new child to make sure we preserve the reference + // we want. We could use `fcntl` to remove CLOEXEC from the FD, but as it + // turns out, we `dup2` everything anyway, so there's no need! + const pipe_flags: posix.O = .{ .CLOEXEC = true }; + const stdin_pipe = if (self.stdin_behavior == StdIo.Pipe) try posix.pipe2(pipe_flags) else undefined; errdefer if (self.stdin_behavior == StdIo.Pipe) { destroyPipe(stdin_pipe); @@ -614,19 +625,6 @@ fn spawnPosix(self: *ChildProcess) SpawnError!void { setUpChildIo(self.stdout_behavior, stdout_pipe[1], posix.STDOUT_FILENO, dev_null_fd) catch |err| forkChildErrReport(err_pipe[1], err); setUpChildIo(self.stderr_behavior, stderr_pipe[1], posix.STDERR_FILENO, dev_null_fd) catch |err| forkChildErrReport(err_pipe[1], err); - if (self.stdin_behavior == .Pipe) { - posix.close(stdin_pipe[0]); - posix.close(stdin_pipe[1]); - } - if (self.stdout_behavior == .Pipe) { - posix.close(stdout_pipe[0]); - posix.close(stdout_pipe[1]); - } - if (self.stderr_behavior == .Pipe) { - posix.close(stderr_pipe[0]); - posix.close(stderr_pipe[1]); - } - if (self.cwd_dir) |cwd| { posix.fchdir(cwd.fd) catch |err| forkChildErrReport(err_pipe[1], err); } else if (self.cwd) |cwd| {