std.child_process: enable non-standard streams

This PR simplifies the setup code and handling to minimize leaking time
for both Windows and Posix and does not touch the list approach of
Windows. Portable pipes default to pipe2 with CLOEXEC on Posix and
disabled handle inhertence on Windows except shortly before
and after the `spawn`.

The potential time for leaking can be long due trying to spawn on
multiple PATH and PATHEXT entries on Windows.

Leaking on Posix
1. CLOEXEC does not take immediately effect with clone(), but after the
setup code for the child process was run and a exec* function is executed.
External code may at worst be long living and never do exec*.
2. File descriptors without CLOEXEC are designed to "leak to the child",
but every spawned process at the same time gets them as well.

File leaking on Windows can be fixed with an explicit list approach,
which might require runtime probing to not break on Kernel updates
for the parameter list.

Author: matu3ba

lib/std/child_process.zig

@@ -57,6 +57,12 @@ pub const ChildProcess = struct {
 
     expand_arg0: Arg0Expand,
 
+    /// Use this for additional posix spawn attributes.
+    /// Do not set spawn attribute flags and do not modify stdin, stdout, stderr
+    /// behavior, because those are set in the platform-specific spawn method.
+    posix_attr: if (builtin.target.isDarwin()) ?os.posix_spawn.Attr else void,
+    posix_action: if (builtin.target.isDarwin()) ?os.posix_spawn.Actions else void,
+
     /// Darwin-only. Disable ASLR for the child process.
     disable_aslr: bool = false,
 
@@ -100,6 +106,16 @@ pub const ChildProcess = struct {
         Close,
     };
 
+    pub const PipeDirection = enum {
+        parent_to_child,
+        child_to_parent,
+    };
+
+    pub const PosixData = union(enum) {
+        Attribute: os.posix_spawn.Attr,
+        Actions: os.posix_spawn.Actions,
+    };
+
     /// First argument in argv is the executable.
     pub fn init(argv: []const []const u8, allocator: mem.Allocator) ChildProcess {
         return .{
@@ -121,6 +137,8 @@ pub const ChildProcess = struct {
             .stdout_behavior = StdIo.Inherit,
             .stderr_behavior = StdIo.Inherit,
             .expand_arg0 = .no_expand,
+            .posix_attr = if (comptime builtin.target.isDarwin()) null else undefined,
+            .posix_action = if (comptime builtin.target.isDarwin()) null else undefined,
         };
     }
 
@@ -548,6 +566,10 @@ pub const ChildProcess = struct {
     }
 
     fn spawnMacos(self: *ChildProcess) SpawnError!void {
+        // cleanup user-initialization and initialization in this function
+        defer if (self.posix_attr != null) self.posix_attr.?.deinit();
+        defer if (self.posix_actions != null) self.posix_actions.?.deinit();
+
         const pipe_flags = if (io.is_async) os.O.NONBLOCK else 0;
         const stdin_pipe = if (self.stdin_behavior == StdIo.Pipe) try os.pipe2(pipe_flags) else undefined;
         errdefer if (self.stdin_behavior == StdIo.Pipe) destroyPipe(stdin_pipe);
@@ -575,28 +597,28 @@ pub const ChildProcess = struct {
             undefined;
         defer if (any_ignore) os.close(dev_null_fd);
 
-        var attr = try os.posix_spawn.Attr.init();
-        defer attr.deinit();
+        if (self.posix_attr == null)
+            self.posix_attr = try os.posix_spawn.Attr.init();
         var flags: u16 = os.darwin.POSIX_SPAWN_SETSIGDEF | os.darwin.POSIX_SPAWN_SETSIGMASK;
         if (self.disable_aslr) {
             flags |= os.darwin._POSIX_SPAWN_DISABLE_ASLR;
         }
         if (self.start_suspended) {
             flags |= os.darwin.POSIX_SPAWN_START_SUSPENDED;
         }
-        try attr.set(flags);
+        try self.posix_attr.?.set(flags);
 
-        var actions = try os.posix_spawn.Actions.init();
-        defer actions.deinit();
+        if (self.posix_actions == null)
+            self.posix_actions = try os.posix_spawn.Actions.init();
 
-        try setUpChildIoPosixSpawn(self.stdin_behavior, &actions, stdin_pipe, os.STDIN_FILENO, dev_null_fd);
-        try setUpChildIoPosixSpawn(self.stdout_behavior, &actions, stdout_pipe, os.STDOUT_FILENO, dev_null_fd);
-        try setUpChildIoPosixSpawn(self.stderr_behavior, &actions, stderr_pipe, os.STDERR_FILENO, dev_null_fd);
+        try setUpChildIoPosixSpawn(self.stdin_behavior, &self.posix_actions.?, stdin_pipe, os.STDIN_FILENO, dev_null_fd);
+        try setUpChildIoPosixSpawn(self.stdout_behavior, &self.posix_actions.?, stdout_pipe, os.STDOUT_FILENO, dev_null_fd);
+        try setUpChildIoPosixSpawn(self.stderr_behavior, &self.posix_actions.?, stderr_pipe, os.STDERR_FILENO, dev_null_fd);
 
         if (self.cwd_dir) |cwd| {
-            try actions.fchdir(cwd.fd);
+            try self.posix_actions.?.fchdir(cwd.fd);
         } else if (self.cwd) |cwd| {
-            try actions.chdir(cwd);
+            try self.posix_actions.?.chdir(cwd);
         }
 
         var arena_allocator = std.heap.ArenaAllocator.init(self.allocator);
@@ -611,7 +633,7 @@ pub const ChildProcess = struct {
             break :m envp_buf.ptr;
         } else std.c.environ;
 
-        const pid = try os.posix_spawn.spawnp(self.argv[0], actions, attr, argv_buf, envp);
+        const pid = try os.posix_spawn.spawnp(self.argv[0], self.posix_actions.?, self.posix_attr.?, argv_buf, envp);
 
         if (self.stdin_behavior == StdIo.Pipe) {
             self.stdin = File{ .handle = stdin_pipe[1] };
@@ -876,7 +898,12 @@ pub const ChildProcess = struct {
         var g_hChildStd_OUT_Wr: ?windows.HANDLE = null;
         switch (self.stdout_behavior) {
             StdIo.Pipe => {
-                try windowsMakeAsyncPipe(&g_hChildStd_OUT_Rd, &g_hChildStd_OUT_Wr, &saAttr);
+                try windowsMakeAsyncPipe(
+                    &g_hChildStd_OUT_Rd,
+                    &g_hChildStd_OUT_Wr,
+                    &saAttr,
+                    PipeDirection.child_to_parent,
+                );
             },
             StdIo.Ignore => {
                 g_hChildStd_OUT_Wr = nul_handle;
@@ -896,7 +923,12 @@ pub const ChildProcess = struct {
         var g_hChildStd_ERR_Wr: ?windows.HANDLE = null;
         switch (self.stderr_behavior) {
             StdIo.Pipe => {
-                try windowsMakeAsyncPipe(&g_hChildStd_ERR_Rd, &g_hChildStd_ERR_Wr, &saAttr);
+                try windowsMakeAsyncPipe(
+                    &g_hChildStd_ERR_Rd,
+                    &g_hChildStd_ERR_Wr,
+                    &saAttr,
+                    PipeDirection.child_to_parent,
+                );
             },
             StdIo.Ignore => {
                 g_hChildStd_ERR_Wr = nul_handle;
@@ -1324,23 +1356,18 @@ fn windowsCreateProcessPathExt(
 }
 
 fn windowsCreateProcess(app_name: [*:0]u16, cmd_line: [*:0]u16, envp_ptr: ?[*]u16, cwd_ptr: ?[*:0]u16, lpStartupInfo: *windows.STARTUPINFOW, lpProcessInformation: *windows.PROCESS_INFORMATION) !void {
-    // TODO the docs for environment pointer say:
-    // > A pointer to the environment block for the new process. If this parameter
-    // > is NULL, the new process uses the environment of the calling process.
-    // > ...
-    // > An environment block can contain either Unicode or ANSI characters. If
-    // > the environment block pointed to by lpEnvironment contains Unicode
-    // > characters, be sure that dwCreationFlags includes CREATE_UNICODE_ENVIRONMENT.
-    // > If this parameter is NULL and the environment block of the parent process
-    // > contains Unicode characters, you must also ensure that dwCreationFlags
-    // > includes CREATE_UNICODE_ENVIRONMENT.
-    // This seems to imply that we have to somehow know whether our process parent passed
-    // CREATE_UNICODE_ENVIRONMENT if we want to pass NULL for the environment parameter.
-    // Since we do not know this information that would imply that we must not pass NULL
-    // for the parameter.
-    // However this would imply that programs compiled with -DUNICODE could not pass
-    // environment variables to programs that were not, which seems unlikely.
-    // More investigation is needed.
+    // See https://stackoverflow.com/a/4207169/9306292
+    // One can manually write in unicode even if one doesn't compile in unicode
+    // (-DUNICODE).
+    // Thus CREATE_UNICODE_ENVIRONMENT, according to how one constructed the
+    // environment block, is necessary, since CreateProcessA and CreateProcessW may
+    // work with either Ansi or Unicode.
+    // * The environment variables can still be inherited from parent process,
+    //   if set to NULL
+    // * The OS can for an unspecified environment block not figure out,
+    //   if it is Unicode or ANSI.
+    // * Applications may break without specification of the environment variable
+    //   due to inability of Windows to check (+translate) the character encodings.
     return windows.CreateProcessW(
         app_name,
         cmd_line,
@@ -1471,7 +1498,16 @@ fn windowsMakePipeIn(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const w
 
 var pipe_name_counter = std.atomic.Atomic(u32).init(1);
 
-fn windowsMakeAsyncPipe(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const windows.SECURITY_ATTRIBUTES) !void {
+/// Create asynchronous pipe.
+/// To enable inheritance between parent and child process, set bInheritHandle = windows.TRUE
+/// in the security attributes.
+/// Direction defines which handle is updated (to enable inheritance by ChildProcess)
+pub fn windowsMakeAsyncPipe(
+    rd: *?windows.HANDLE,
+    wr: *?windows.HANDLE,
+    sattr: *const windows.SECURITY_ATTRIBUTES,
+    direction: ChildProcess.PipeDirection,
+) !void {
     var tmp_bufw: [128]u16 = undefined;
 
     // Anonymous pipes are built upon Named pipes.
@@ -1526,8 +1562,10 @@ fn windowsMakeAsyncPipe(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *cons
     }
     errdefer os.close(write_handle);
 
-    try windows.SetHandleInformation(read_handle, windows.HANDLE_FLAG_INHERIT, 0);
-
+    switch (direction) {
+        .child_to_parent => try windows.SetHandleInformation(read_handle, windows.HANDLE_FLAG_INHERIT, windows.HANDLE_FLAG_INHERIT),
+        .parent_to_child => try windows.SetHandleInformation(write_handle, windows.HANDLE_FLAG_INHERIT, windows.HANDLE_FLAG_INHERIT),
+    }
     rd.* = read_handle;
     wr.* = write_handle;
 }

lib/std/os.zig

@@ -4843,6 +4843,16 @@ pub fn lseek_CUR_get(fd: fd_t) SeekError!u64 {
     }
 }
 
+const UnsetFileInheritanceError = FcntlError || windows.SetHandleInformationError;
+
+pub inline fn disableFileInheritance(file_handle: fd_t) UnsetFileInheritanceError!void {
+    if (builtin.os.tag == .windows) {
+        try windows.SetHandleInformation(file_handle, windows.HANDLE_FLAG_INHERIT, 0);
+    } else {
+        _ = try fcntl(file_handle, F.SETFD, FD_CLOEXEC);
+    }
+}
+
 pub const FcntlError = error{
     PermissionDenied,
     FileBusy,

lib/std/os/windows.zig

@@ -227,6 +227,16 @@ pub fn DeviceIoControl(
     }
 }
 
+pub const GetHandleInformationError = error{Unexpected};
+
+pub fn GetHandleInformation(h: HANDLE, flags: *DWORD) GetHandleInformationError!void {
+    if (kernel32.GetHandleInformation(h, flags) == 0) {
+        switch (kernel32.GetLastError()) {
+            else => |err| return unexpectedError(err),
+        }
+    }
+}
+
 pub fn GetOverlappedResult(h: HANDLE, overlapped: *OVERLAPPED, wait: bool) !DWORD {
     var bytes: DWORD = undefined;
     if (kernel32.GetOverlappedResult(h, overlapped, &bytes, @boolToInt(wait)) == 0) {

lib/std/os/windows/kernel32.zig

@@ -227,6 +227,8 @@ pub extern "kernel32" fn GetFullPathNameW(
     lpFilePart: ?*?[*:0]u16,
 ) callconv(@import("std").os.windows.WINAPI) u32;
 
+pub extern "kernel32" fn GetHandleInformation(hObject: HANDLE, dwFlags: *DWORD) callconv(WINAPI) BOOL;
+
 pub extern "kernel32" fn GetOverlappedResult(hFile: HANDLE, lpOverlapped: *OVERLAPPED, lpNumberOfBytesTransferred: *DWORD, bWait: BOOL) callconv(WINAPI) BOOL;
 
 pub extern "kernel32" fn GetProcessHeap() callconv(WINAPI) ?HANDLE;

test/standalone.zig

@@ -64,6 +64,7 @@ pub fn addCases(cases: *tests.StandaloneContext) void {
         (builtin.os.tag != .windows or builtin.cpu.arch != .aarch64))
     {
         cases.addBuildFile("test/standalone/load_dynamic_library/build.zig", .{});
+        cases.addBuildFile("test/standalone/childprocess_extrapipe/build.zig", .{});
     }
 
     if (builtin.os.tag == .windows) {

test/standalone/childprocess_extrapipe/build.zig

@@ -0,0 +1,16 @@
+const Builder = @import("std").build.Builder;
+
+pub fn build(b: *Builder) void {
+    const mode = b.standardReleaseOptions();
+
+    const child = b.addExecutable("child", "child.zig");
+    child.setBuildMode(mode);
+
+    const parent = b.addExecutable("parent", "parent.zig");
+    parent.setBuildMode(mode);
+    const run_cmd = parent.run();
+    run_cmd.addArtifactArg(child);
+
+    const test_step = b.step("test", "Test it");
+    test_step.dependOn(&run_cmd.step);
+}

test/standalone/childprocess_extrapipe/child.zig

@@ -0,0 +1,39 @@
+const std = @import("std");
+const builtin = @import("builtin");
+const windows = std.os.windows;
+pub fn main() !void {
+    var general_purpose_allocator = std.heap.GeneralPurposeAllocator(.{}){};
+    defer if (general_purpose_allocator.deinit()) @panic("found memory leaks");
+    const gpa = general_purpose_allocator.allocator();
+
+    var it = try std.process.argsWithAllocator(gpa);
+    defer it.deinit();
+    _ = it.next() orelse unreachable; // skip binary name
+    const s_handle = it.next() orelse unreachable;
+
+    var file_handle: std.os.fd_t = file_handle: {
+        if (builtin.target.os.tag == .windows) {
+            var handle_int = try std.fmt.parseInt(usize, s_handle, 10);
+            break :file_handle @intToPtr(windows.HANDLE, handle_int);
+        } else {
+            break :file_handle try std.fmt.parseInt(std.os.fd_t, s_handle, 10);
+        }
+    };
+    if (builtin.target.os.tag == .windows) {
+        // windows.HANDLE_FLAG_INHERIT is enabled
+        var handle_flags: windows.DWORD = undefined;
+        try windows.GetHandleInformation(file_handle, &handle_flags);
+        try std.testing.expect(handle_flags & windows.HANDLE_FLAG_INHERIT != 0);
+    } else {
+        // FD_CLOEXEC is not set
+        var fcntl_flags = try std.os.fcntl(file_handle, std.os.F.GETFD, 0);
+        try std.testing.expect((fcntl_flags & std.os.FD_CLOEXEC) == 0);
+    }
+    try std.os.disableFileInheritance(file_handle);
+    var file_in = std.fs.File{ .handle = file_handle }; // read side of pipe
+    defer file_in.close();
+    const file_in_reader = file_in.reader();
+    const message = try file_in_reader.readUntilDelimiterAlloc(gpa, '\x17', 20_000);
+    defer gpa.free(message);
+    try std.testing.expectEqualSlices(u8, message, "test123");
+}