more cleanups, windows allows an explicit list of file handles

Thus use the allocating posix spawn logic within fork to close the child
end of the pipe.

Author: matu3ba

lib/std/child_process.zig

@@ -60,8 +60,8 @@ pub const ChildProcess = struct {
     /// 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_actions: if (builtin.target.isDarwin()) ?os.posix_spawn.Actions else void,
+    posix_attr: if (os.hasPosixSpawn) ?os.posix_spawn.Attr else void,
+    posix_actions: if (os.hasPosixSpawn) ?os.posix_spawn.Actions else void,
 
     /// Darwin-only. Disable ASLR for the child process.
     disable_aslr: bool = false,
@@ -111,11 +111,6 @@ pub const ChildProcess = struct {
         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 .{
@@ -137,8 +132,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_actions = if (comptime builtin.target.isDarwin()) null else undefined,
+            .posix_attr = if (os.hasPosixSpawn) null else undefined,
+            .posix_actions = if (os.hasPosixSpawn) null else undefined,
         };
     }
 
@@ -154,7 +149,7 @@ pub const ChildProcess = struct {
             @compileError("the target operating system cannot spawn processes");
         }
 
-        if (comptime builtin.target.isDarwin()) {
+        if (os.hasPosixSpawn) {
             return self.spawnMacos();
         }
 
@@ -486,7 +481,7 @@ pub const ChildProcess = struct {
     }
 
     fn waitUnwrapped(self: *ChildProcess) !void {
-        const res: os.WaitPidResult = if (comptime builtin.target.isDarwin())
+        const res: os.WaitPidResult = if (os.hasPosixSpawn)
             try os.posix_spawn.waitpid(self.pid, 0)
         else
             os.waitpid(self.pid, 0);
@@ -566,9 +561,9 @@ 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();
+        // dont cleanup structure owned == initialized by user
+        const user_attr: bool = self.posix_attr != null;
+        const user_actions: bool = self.posix_actions != null;
 
         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;
@@ -597,8 +592,8 @@ pub const ChildProcess = struct {
             undefined;
         defer if (any_ignore) os.close(dev_null_fd);
 
-        if (self.posix_attr == null)
-            self.posix_attr = try os.posix_spawn.Attr.init();
+        if (user_attr == false) self.posix_attr = try os.posix_spawn.Attr.init();
+        defer if (user_attr == false) self.posix_attr.?.deinit();
         var flags: u16 = os.darwin.POSIX_SPAWN_SETSIGDEF | os.darwin.POSIX_SPAWN_SETSIGMASK;
         if (self.disable_aslr) {
             flags |= os.darwin._POSIX_SPAWN_DISABLE_ASLR;
@@ -608,8 +603,8 @@ pub const ChildProcess = struct {
         }
         try self.posix_attr.?.set(flags);
 
-        if (self.posix_actions == null)
-            self.posix_actions = try os.posix_spawn.Actions.init();
+        if (user_actions == false) self.posix_actions = try os.posix_spawn.Actions.init();
+        defer if (user_actions == false) self.posix_actions.?.deinit();
 
         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);
@@ -947,6 +942,9 @@ pub const ChildProcess = struct {
         const cmd_line = try windowsCreateCommandLine(self.allocator, self.argv);
         defer self.allocator.free(cmd_line);
 
+        // TODO use explicit list of file handles
+        // https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873
+
         var siStartInfo = windows.STARTUPINFOW{
             .cb = @sizeOf(windows.STARTUPINFOW),
             .hStdError = g_hChildStd_ERR_Wr,

lib/std/os.zig

@@ -44,6 +44,9 @@ pub const windows = @import("os/windows.zig");
 pub const posix_spawn = @import("os/posix_spawn.zig");
 pub const ptrace = @import("os/ptrace.zig");
 
+// TODO where should we add our own target infos for things that we have bindings for?
+pub const hasPosixSpawn = builtin.target.isDarwin();
+
 comptime {
     assert(@import("std") == std); // std lib tests require --zig-lib-dir
 }
@@ -4843,6 +4846,18 @@ pub fn lseek_CUR_get(fd: fd_t) SeekError!u64 {
     }
 }
 
+const EnableFileInheritanceError = FcntlError || windows.SetHandleInformationError;
+
+pub inline fn enableFileInheritance(file_handle: fd_t) EnableFileInheritanceError!void {
+    if (builtin.os.tag == .windows) {
+        try windows.SetHandleInformation(file_handle, windows.HANDLE_FLAG_INHERIT, 1);
+    } else {
+        var flags = fcntl(file_handle, F.GETFD, 0);
+        flags &= ~@as(u32, FD_CLOEXEC);
+        _ = try fcntl(file_handle, F.SETFD, flags);
+    }
+}
+
 const DisableFileInheritanceError = FcntlError || windows.SetHandleInformationError;
 
 pub inline fn disableFileInheritance(file_handle: fd_t) DisableFileInheritanceError!void {

test/standalone/childprocess_extrapipe/child.zig

@@ -19,6 +19,8 @@ pub fn main() !void {
             break :file_handle try std.fmt.parseInt(std.os.fd_t, s_handle, 10);
         }
     };
+    // TODO: Is there a way on Windows to let the Kernel disable inheritance
+    // after it is inherited in CreateProcess???
     if (builtin.target.os.tag == .windows) {
         // windows.HANDLE_FLAG_INHERIT is enabled
         var handle_flags: windows.DWORD = undefined;

test/standalone/childprocess_extrapipe/parent.zig

@@ -39,17 +39,17 @@ pub fn main() !void {
             .bInheritHandle = windows.TRUE,
             .lpSecurityDescriptor = null,
         };
+        // create pipe and enable inheritance for the read end, which will be given to the child
         try child_process.windowsMakeAsyncPipe(&pipe[0], &pipe[1], &saAttr, .parent_to_child);
     } else {
-        pipe = try os.pipe(); // leaks on default
+        pipe = try os.pipe(); // leaks on default, but more portable, TODO: use pip2 and close earlier?
     }
 
-    // write pipe to string + add to command
+    // write read side of pipe to string + add to spawn command
     var buf: [handleCharSize]u8 = comptime [_]u8{0} ** handleCharSize;
-
     const s_handle =
         if (builtin.os.tag == .windows)
-        try handleToString(pipe[0].?, &buf) // read side of pipe
+        try handleToString(pipe[0].?, &buf)
     else
         try handleToString(pipe[0], &buf);
 
@@ -58,34 +58,24 @@ pub fn main() !void {
         gpa,
     );
     {
-        // close read side of pipe
-        if (comptime builtin.target.isDarwin()) {
-            {
-                // less time for leaking the file descriptor, if it is closed immediately
-                child_proc.posix_actions = try os.posix_spawn.Actions.init();
-                errdefer child_proc.posix_actions.?.deinit();
-                try child_proc.posix_actions.?.close(pipe[1]);
-                errdefer child_proc.posix_actions.?.deinit();
-            }
-        }
-        defer if (comptime !builtin.target.isDarwin()) {
-            if (builtin.os.tag == .windows) {
-                os.close(pipe[0].?);
-            } else {
-                os.close(pipe[0]);
-            }
-        };
+        // close read side of pipe, less time for leaking, if closed immediately
+        if (os.hasPosixSpawn) child_proc.posix_actions = try os.posix_spawn.Actions.init();
+        defer if (os.hasPosixSpawn) child_proc.posix_actions.?.deinit();
+        if (os.hasPosixSpawn) try child_proc.posix_actions.?.close(pipe[1]); // TODO: This is not closed in child
+        defer if (builtin.os.tag == .windows)
+            os.close(pipe[0].?)
+        else
+            os.close(pipe[0]);
 
         try child_proc.spawn();
     }
 
-    if (builtin.os.tag == .windows) {
-        try std.os.disableFileInheritance(pipe[1].?);
-    } else {
+    // call fcntl on Unixes to disable handle inheritance
+    if (builtin.os.tag != .windows) {
         try std.os.disableFileInheritance(pipe[1]);
     }
 
-    // check that disableFileInheritance was successful
+    // windows does have inheritance disabled on default, but we check to be sure
     if (builtin.target.os.tag == .windows) {
         var handle_flags: windows.DWORD = undefined;
         try windows.GetHandleInformation(pipe[1].?, &handle_flags);
@@ -95,13 +85,12 @@ pub fn main() !void {
         try std.testing.expect((fcntl_flags & os.FD_CLOEXEC) != 0);
     }
 
-    // do we want another extra prong for darwin?
     var file_out = if (builtin.target.os.tag == .windows)
         std.fs.File{ .handle = pipe[1].? }
     else
         std.fs.File{ .handle = pipe[1] };
 
-    defer if (comptime !builtin.target.isDarwin()) file_out.close();
+    defer file_out.close();
     const file_out_writer = file_out.writer();
     try file_out_writer.writeAll("test123\x17"); // ETB = \x17
     const ret_val = try child_proc.wait();