std: Avoid deadlocking in ChildProcess.exec

Reading stdin&stderr at different times may lead to nasty deadlocks (eg.
when stdout is read before stderr and the child process doesn't write
anything onto stdout).

Implement a polling mechanism to make sure this won't happen: we read
data from stderr/stdout as it becomes ready and then it's copied into an
ArrayList provided by the user, avoiding any kind of blocking read.

1 files changed, 162 insertions, 11 deletions

diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig
index b1dcc3737f..6e62063e26 100644
--- a/lib/std/child_process.zig
+++ b/lib/std/child_process.zig
@@ -186,6 +186,106 @@ pub const ChildProcess = struct {
 
     pub const exec2 = @compileError("deprecated: exec2 is renamed to exec");
 
+    fn collectOutputPosix(child: *const ChildProcess, stdout: *std.ArrayList(u8), stderr: *std.ArrayList(u8), max_output_bytes: usize) !void {
+        var poll_fds = [_]os.pollfd{
+            .{ .fd = child.stdout.?.handle, .events = os.POLLIN, .revents = undefined },
+            .{ .fd = child.stderr.?.handle, .events = os.POLLIN, .revents = undefined },
+        };
+
+        var dead_fds: usize = 0;
+        var loop_buf: [4096]u8 = undefined;
+
+        while (dead_fds < poll_fds.len) {
+            const events = try os.poll(&poll_fds, std.math.maxInt(i32));
+            if (events == 0) continue;
+
+            // Try reading whatever is available before checking the error
+            // conditions.
+            if (poll_fds[0].revents & os.POLLIN != 0) {
+                // stdout is ready.
+                const n = try os.read(poll_fds[0].fd, &loop_buf);
+                try stdout.appendSlice(loop_buf[0..n]);
+            }
+            if (poll_fds[1].revents & os.POLLIN != 0) {
+                // stderr is ready.
+                const n = try os.read(poll_fds[1].fd, &loop_buf);
+                try stderr.appendSlice(loop_buf[0..n]);
+            }
+
+            // Exclude the fds that signaled an error.
+            if (poll_fds[0].revents & (os.POLLERR | os.POLLNVAL | os.POLLHUP) != 0) {
+                poll_fds[0].fd = -1;
+                dead_fds += 1;
+            }
+            if (poll_fds[1].revents & (os.POLLERR | os.POLLNVAL | os.POLLHUP) != 0) {
+                poll_fds[1].fd = -1;
+                dead_fds += 1;
+            }
+        }
+    }
+
+    fn collectOutputWindows(child: *const ChildProcess, stdout: *std.ArrayList(u8), stderr: *std.ArrayList(u8), max_output_bytes: usize) !void {
+        // The order of the objects here is important, WaitForMultipleObjects
+        // uses the same order when scanning the events.
+        var wait_objects = [_]windows.kernel32.HANDLE{
+            child.handle, child.stdout.?.handle, child.stderr.?.handle,
+        };
+        // XXX: Calling zeroes([2]windows.OVERLAPPED) causes the stage1 compiler
+        // to crash and burn.
+        var overlapped = [_]windows.OVERLAPPED{
+            mem.zeroes(windows.OVERLAPPED),
+            mem.zeroes(windows.OVERLAPPED),
+        };
+        var temp_buf: [2][4096]u8 = undefined;
+
+        // Kickstart the loop by issuing two async reads.
+        // ReadFile returns false and GetLastError returns ERROR_IO_PENDING if
+        // everything is ok.
+        _ = windows.kernel32.ReadFile(wait_objects[1], &temp_buf[0], temp_buf[0].len, null, &overlapped[0]);
+        _ = windows.kernel32.ReadFile(wait_objects[2], &temp_buf[1], temp_buf[1].len, null, &overlapped[1]);
+
+        while (true) {
+            const status = windows.kernel32.WaitForMultipleObjects(wait_objects.len, &wait_objects, 0, windows.INFINITE);
+            std.debug.print("status {x}\n", .{status});
+            switch (status) {
+                windows.WAIT_OBJECT_0 + 0 => {
+                    // The child process was terminated.
+                    break;
+                },
+                windows.WAIT_OBJECT_0 + 1 => {
+                    // stdout is ready.
+                    var read_bytes: u32 = undefined;
+                    if (windows.kernel32.GetOverlappedResult(wait_objects[1], &overlapped[0], &read_bytes, 0) == 0) {
+                        switch (windows.kernel32.GetLastError()) {
+                            else => |err| return windows.unexpectedError(err),
+                        }
+                    }
+                    try stdout.appendSlice(temp_buf[0][0..read_bytes]);
+                    _ = windows.kernel32.ReadFile(wait_objects[1], &temp_buf[0], temp_buf[0].len, null, &overlapped[0]);
+                },
+                windows.WAIT_OBJECT_0 + 2 => {
+                    // stderr is ready.
+                    var read_bytes: u32 = undefined;
+                    if (windows.kernel32.GetOverlappedResult(wait_objects[2], &overlapped[1], &read_bytes, 0) == 0) {
+                        switch (windows.kernel32.GetLastError()) {
+                            else => |err| return windows.unexpectedError(err),
+                        }
+                    }
+                    try stdout.appendSlice(temp_buf[1][0..read_bytes]);
+                    _ = windows.kernel32.ReadFile(wait_objects[2], &temp_buf[1], temp_buf[1].len, null, &overlapped[1]);
+                },
+                windows.WAIT_FAILED => {
+                    switch (windows.kernel32.GetLastError()) {
+                        else => |err| return windows.unexpectedError(err),
+                    }
+                },
+                // We're waiting with an infinite timeout
+                windows.WAIT_TIMEOUT => unreachable,
+                else => unreachable,
+            }
+        }
+    }
+
     /// Spawns a child process, waits for it, collecting stdout and stderr, and then returns.
     /// If it succeeds, the caller owns result.stdout and result.stderr memory.
     pub fn exec(args: struct {
@@ -210,19 +310,21 @@ pub const ChildProcess = struct {
 
         try child.spawn();
 
-        const stdout_in = child.stdout.?.reader();
-        const stderr_in = child.stderr.?.reader();
+        var stdout = std.ArrayList(u8).init(args.allocator);
+        var stderr = std.ArrayList(u8).init(args.allocator);
 
-        // TODO https://github.com/ziglang/zig/issues/6343
-        const stdout = try stdout_in.readAllAlloc(args.allocator, args.max_output_bytes);
-        errdefer args.allocator.free(stdout);
-        const stderr = try stderr_in.readAllAlloc(args.allocator, args.max_output_bytes);
-        errdefer args.allocator.free(stderr);
+        // XXX: Respect max_output_bytes
+        // XXX: Smarter reading logic, read directly into the ArrayList
+        if (builtin.os.tag == .windows) {
+            try collectOutputWindows(child, &stdout, &stderr, args.max_output_bytes);
+        } else {
+            try collectOutputPosix(child, &stdout, &stderr, args.max_output_bytes);
+        }
 
         return ExecResult{
             .term = try child.wait(),
-            .stdout = stdout,
-            .stderr = stderr,
+            .stdout = stdout.toOwnedSlice(),
+            .stderr = stderr.toOwnedSlice(),
         };
     }
 
@@ -555,7 +657,7 @@ pub const ChildProcess = struct {
         var g_hChildStd_OUT_Wr: ?windows.HANDLE = null;
         switch (self.stdout_behavior) {
             StdIo.Pipe => {
-                try windowsMakePipeOut(&g_hChildStd_OUT_Rd, &g_hChildStd_OUT_Wr, &saAttr);
+                try windowsMakePipe(&g_hChildStd_OUT_Rd, &g_hChildStd_OUT_Wr, &saAttr);
             },
             StdIo.Ignore => {
                 g_hChildStd_OUT_Wr = nul_handle;
@@ -575,7 +677,7 @@ pub const ChildProcess = struct {
         var g_hChildStd_ERR_Wr: ?windows.HANDLE = null;
         switch (self.stderr_behavior) {
             StdIo.Pipe => {
-                try windowsMakePipeOut(&g_hChildStd_ERR_Rd, &g_hChildStd_ERR_Wr, &saAttr);
+                try windowsMakePipe(&g_hChildStd_ERR_Rd, &g_hChildStd_ERR_Wr, &saAttr);
             },
             StdIo.Ignore => {
                 g_hChildStd_ERR_Wr = nul_handle;
@@ -808,6 +910,55 @@ fn windowsDestroyPipe(rd: ?windows.HANDLE, wr: ?windows.HANDLE) void {
     if (wr) |h| os.close(h);
 }
 
+var pipe_name_counter = std.atomic.Int(u32).init(1);
+
+fn windowsMakePipe(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const windows.SECURITY_ATTRIBUTES) !void {
+    var tmp_buf: [128]u8 = undefined;
+    // Forge a random path for the pipe.
+    const pipe_path = std.fmt.bufPrintZ(
+        &tmp_buf,
+        "\\\\.\\pipe\\zig-childprocess-{d}-{d}",
+        .{ windows.kernel32.GetCurrentProcessId(), pipe_name_counter.fetchAdd(1) },
+    ) catch unreachable;
+
+    // Create the read handle that can be used with overlapped IO ops.
+    const read_handle = windows.kernel32.CreateNamedPipeA(
+        pipe_path,
+        windows.PIPE_ACCESS_INBOUND | windows.FILE_FLAG_OVERLAPPED,
+        windows.PIPE_TYPE_BYTE,
+        1,
+        0x1000,
+        0x1000,
+        0,
+        sattr,
+    );
+    if (read_handle == windows.INVALID_HANDLE_VALUE) {
+        switch (windows.kernel32.GetLastError()) {
+            else => |err| return windows.unexpectedError(err),
+        }
+    }
+
+    const write_handle = windows.kernel32.CreateFileA(
+        pipe_path,
+        windows.GENERIC_WRITE,
+        0,
+        sattr,
+        windows.OPEN_EXISTING,
+        windows.FILE_ATTRIBUTE_NORMAL,
+        null,
+    );
+    if (write_handle == windows.INVALID_HANDLE_VALUE) {
+        switch (windows.kernel32.GetLastError()) {
+            else => |err| return windows.unexpectedError(err),
+        }
+    }
+
+    try windows.SetHandleInformation(read_handle, windows.HANDLE_FLAG_INHERIT, 0);
+
+    rd.* = read_handle;
+    wr.* = write_handle;
+}
+
 fn windowsMakePipeIn(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const windows.SECURITY_ATTRIBUTES) !void {
     var rd_h: windows.HANDLE = undefined;
     var wr_h: windows.HANDLE = undefined;