std.child_process: enable non-standard streams

This PR simplifies the setup code and handling of non-standard file
descriptors and handles for Windows and Posix with main focus on pipes.

Portable pipes default to pipe2 with CLOEXEC on Posix and disabled handle
inhertance on Windows except shortly before and after the creation of
the "child process".

Leaking of file desriptors 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.

Leaking of handles on Windows
1. File leaking on Windows can be fixed with an explicit list approach
as suggested in ziglang#14251, which might require runtime probing and
allocation of the necessary process setup list. Otherwise, it might
break on Kernel updates.
2. The potential time for leaking can be long due trying to spawn on
multiple PATH and PATHEXT entries on Windows.

Author: matu3ba

lib/std/child_process.zig

@@ -821,7 +821,7 @@ pub const ChildProcess = struct {
     fn spawnWindows(self: *ChildProcess) SpawnError!void {
         const saAttr = windows.SECURITY_ATTRIBUTES{
             .nLength = @sizeOf(windows.SECURITY_ATTRIBUTES),
-            .bInheritHandle = windows.TRUE,
+            .bInheritHandle = windows.FALSE,
             .lpSecurityDescriptor = null,
         };
 
@@ -872,11 +872,24 @@ pub const ChildProcess = struct {
             windowsDestroyPipe(g_hChildStd_IN_Rd, g_hChildStd_IN_Wr);
         };
 
+        var tmp_hChildStd_Rd: windows.HANDLE = undefined;
+        var tmp_hChildStd_Wr: windows.HANDLE = undefined;
         var g_hChildStd_OUT_Rd: ?windows.HANDLE = null;
         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(
+                    &tmp_hChildStd_Rd,
+                    &tmp_hChildStd_Wr,
+                    &saAttr,
+                );
+                errdefer {
+                    os.close(tmp_hChildStd_Rd);
+                    os.close(tmp_hChildStd_Wr);
+                }
+                try windows.SetHandleInformation(tmp_hChildStd_Wr, windows.HANDLE_FLAG_INHERIT, 1);
+                g_hChildStd_OUT_Rd = tmp_hChildStd_Rd;
+                g_hChildStd_OUT_Wr = tmp_hChildStd_Wr;
             },
             StdIo.Ignore => {
                 g_hChildStd_OUT_Wr = nul_handle;
@@ -896,7 +909,18 @@ 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(
+                    &tmp_hChildStd_Rd,
+                    &tmp_hChildStd_Wr,
+                    &saAttr,
+                );
+                errdefer {
+                    os.close(tmp_hChildStd_Rd);
+                    os.close(tmp_hChildStd_Wr);
+                }
+                try windows.SetHandleInformation(tmp_hChildStd_Wr, windows.HANDLE_FLAG_INHERIT, 1);
+                g_hChildStd_ERR_Rd = tmp_hChildStd_Rd;
+                g_hChildStd_ERR_Wr = tmp_hChildStd_Wr;
             },
             StdIo.Ignore => {
                 g_hChildStd_ERR_Wr = nul_handle;
@@ -1094,6 +1118,28 @@ pub const ChildProcess = struct {
     }
 };
 
+/// Pipe read side
+pub const pipe_rd = 0;
+/// Pipe write side
+pub const pipe_wr = 1;
+const PortPipeT = if (builtin.os.tag == .windows) [2]windows.HANDLE else [2]os.fd_t;
+
+/// Portable pipe creation with disabled inheritance
+pub inline fn portablePipe() !PortPipeT {
+    var pipe_new: PortPipeT = undefined;
+    if (builtin.os.tag == .windows) {
+        const saAttr = windows.SECURITY_ATTRIBUTES{
+            .nLength = @sizeOf(windows.SECURITY_ATTRIBUTES),
+            .bInheritHandle = windows.FALSE,
+            .lpSecurityDescriptor = null,
+        };
+        try windowsMakeAsyncPipe(&pipe_new[pipe_rd], &pipe_new[pipe_wr], &saAttr);
+    } else {
+        pipe_new = try os.pipe2(@as(u32, os.O.CLOEXEC));
+    }
+    return pipe_new;
+}
+
 /// Expects `app_buf` to contain exactly the app name, and `dir_buf` to contain exactly the dir path.
 /// After return, `app_buf` will always contain exactly the app name and `dir_buf` will always contain exactly the dir path.
 /// Note: `app_buf` should not contain any leading path separators.
@@ -1324,30 +1370,25 @@ 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,
         null,
         null,
         windows.TRUE,
-        windows.CREATE_UNICODE_ENVIRONMENT,
+        @enumToInt(windows.PROCESS_CREATION_FLAGS.CREATE_UNICODE_ENVIRONMENT),
         @ptrCast(?*anyopaque, envp_ptr),
         cwd_ptr,
         lpStartupInfo,
@@ -1464,14 +1505,22 @@ fn windowsMakePipeIn(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const w
     var wr_h: windows.HANDLE = undefined;
     try windows.CreatePipe(&rd_h, &wr_h, sattr);
     errdefer windowsDestroyPipe(rd_h, wr_h);
-    try windows.SetHandleInformation(wr_h, windows.HANDLE_FLAG_INHERIT, 0);
+    try windows.SetHandleInformation(rd_h, windows.HANDLE_FLAG_INHERIT, 1);
     rd.* = rd_h;
     wr.* = wr_h;
 }
 
 var pipe_name_counter = std.atomic.Atomic(u32).init(1);
 
-fn windowsMakeAsyncPipe(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const windows.SECURITY_ATTRIBUTES) !void {
+/// To enable/disable inheritance parent and child process, use
+/// os.enableInheritance() and os.disableInheritance() on the handle.
+/// convention: sattr uses bInheritHandle = windows.FALSE and only used pipe end
+/// is enabled.
+pub fn windowsMakeAsyncPipe(
+    rd: *windows.HANDLE,
+    wr: *windows.HANDLE,
+    sattr: *const windows.SECURITY_ATTRIBUTES,
+) !void {
     var tmp_bufw: [128]u16 = undefined;
 
     // Anonymous pipes are built upon Named pipes.
@@ -1524,9 +1573,6 @@ fn windowsMakeAsyncPipe(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *cons
             else => |err| return windows.unexpectedError(err),
         }
     }
-    errdefer os.close(write_handle);
-
-    try windows.SetHandleInformation(read_handle, windows.HANDLE_FLAG_INHERIT, 0);
 
     rd.* = read_handle;
     wr.* = write_handle;

lib/std/os.zig

@@ -288,6 +288,27 @@ pub fn close(fd: fd_t) void {
     }
 }
 
+pub const windowsPtrDigits = 19; // log10(max(usize))
+pub const unixoidPtrDigits = 10; // log10(max(u32)) + 1 for sign
+pub const handleCharSize = if (builtin.target.os.tag == .windows) windowsPtrDigits else unixoidPtrDigits;
+
+pub fn handleToString(handle: fd_t, buf: []u8) std.fmt.BufPrintError![]u8 {
+    var s_handle: []u8 = undefined;
+    const handle_int =
+        // handle is *anyopaque or an integer on unix-likes Kernels.
+        if (builtin.target.os.tag == .windows) @ptrToInt(handle) else handle;
+    s_handle = try std.fmt.bufPrint(buf[0..], "{d}", .{handle_int});
+    return s_handle;
+}
+
+pub fn stringToHandle(s_handle: []const u8) std.fmt.ParseIntError!std.os.fd_t {
+    var handle: std.os.fd_t = if (builtin.target.os.tag == .windows)
+        @intToPtr(windows.HANDLE, try std.fmt.parseInt(usize, s_handle, 10))
+    else
+        try std.fmt.parseInt(std.os.fd_t, s_handle, 10);
+    return handle;
+}
+
 pub const FChmodError = error{
     AccessDenied,
     InputOutput,
@@ -4866,6 +4887,44 @@ pub fn lseek_CUR_get(fd: fd_t) SeekError!u64 {
     }
 }
 
+const IsInheritableError = FcntlError || windows.GetHandleInformationError;
+
+/// Whether inheritence is enabled or CLOEXEC is not set.
+pub inline fn isInheritable(handle: fd_t) IsInheritableError!bool {
+    if (builtin.os.tag == .windows) {
+        var handle_flags: windows.DWORD = undefined;
+        try windows.GetHandleInformation(handle, &handle_flags);
+        return handle_flags & windows.HANDLE_FLAG_INHERIT != 0;
+    } else {
+        const fcntl_flags = try fcntl(handle, F.GETFD, 0);
+        return fcntl_flags & FD_CLOEXEC == 0;
+    }
+}
+
+const EnableInheritanceError = FcntlError || windows.SetHandleInformationError;
+
+/// Enables inheritence or sets CLOEXEC.
+pub inline fn enableInheritance(handle: fd_t) EnableInheritanceError!void {
+    if (builtin.os.tag == .windows) {
+        try windows.SetHandleInformation(handle, windows.HANDLE_FLAG_INHERIT, 1);
+    } else {
+        var flags = try fcntl(handle, F.GETFD, 0);
+        flags &= ~@as(u32, FD_CLOEXEC);
+        _ = try fcntl(handle, F.SETFD, flags);
+    }
+}
+
+const DisableInheritanceError = FcntlError || windows.SetHandleInformationError;
+
+/// Disables inheritence or unsets CLOEXEC.
+pub inline fn disableInheritance(handle: fd_t) DisableInheritanceError!void {
+    if (builtin.os.tag == .windows) {
+        try windows.SetHandleInformation(handle, windows.HANDLE_FLAG_INHERIT, 0);
+    } else {
+        _ = try fcntl(handle, F.SETFD, FD_CLOEXEC);
+    }
+}
+
 pub const FcntlError = error{
     PermissionDenied,
     FileBusy,

lib/std/os/windows.zig

@@ -228,6 +228,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) {
@@ -1590,6 +1600,45 @@ pub fn GetEnvironmentVariableW(lpName: LPWSTR, lpBuffer: [*]u16, nSize: DWORD) G
     return rc;
 }
 
+// zig fmt: off
+pub const PROCESS_CREATION_FLAGS = enum(u32) {
+    // <- gap here ->
+    DEBUG_PROCESS                       = 0x0000_0001,
+    DEBUG_ONLY_THIS_PROCESS             = 0x0000_0002,
+    CREATE_SUSPENDED                    = 0x0000_0004,
+    DETACHED_PROCESS                    = 0x0000_0008,
+    CREATE_NEW_CONSOLE                  = 0x0000_0010,
+    NORMAL_PRIORITY_CLASS               = 0x0000_0020,
+    IDLE_PRIORITY_CLASS                 = 0x0000_0040,
+    HIGH_PRIORITY_CLASS                 = 0x0000_0080,
+    REALTIME_PRIORITY_CLASS             = 0x0000_0100,
+    CREATE_NEW_PROCESS_GROUP            = 0x0000_0200,
+    CREATE_UNICODE_ENVIRONMENT          = 0x0000_0400,
+    CREATE_SEPARATE_WOW_VDM             = 0x0000_0800,
+    CREATE_SHARED_WOW_VDM               = 0x0000_1000,
+    CREATE_FORCEDOS                     = 0x0000_2000,
+    BELOW_NORMAL_PRIORITY_CLASS         = 0x0000_4000,
+    ABOVE_NORMAL_PRIORITY_CLASS         = 0x0000_8000,
+    INHERIT_PARENT_AFFINITY             = 0x0001_0000,
+    INHERIT_CALLER_PRIORITY             = 0x0002_0000,
+    CREATE_PROTECTED_PROCESS            = 0x0004_0000,
+    EXTENDED_STARTUPINFO_PRESENT        = 0x0008_0000,
+    PROCESS_MODE_BACKGROUND_BEGIN       = 0x0010_0000,
+    PROCESS_MODE_BACKGROUND_END         = 0x0020_0000,
+    CREATE_SECURE_PROCESS               = 0x0040_0000,
+    // <- gap here ->
+    CREATE_BREAKAWAY_FROM_JOB           = 0x0100_0000,
+    CREATE_PRESERVE_CODE_AUTHZ_LEVEL    = 0x0200_0000,
+    CREATE_DEFAULT_ERROR_MODE           = 0x0400_0000,
+    CREATE_NO_WINDOW                    = 0x0800_0000,
+    PROFILE_USER                        = 0x1000_0000,
+    PROFILE_KERNEL                      = 0x2000_0000,
+    PROFILE_SERVER                      = 0x4000_0000,
+    CREATE_IGNORE_SYSTEM_DEFAULT        = 0x8000_0000,
+    _,
+};
+// zig fmt: on
+
 pub const CreateProcessError = error{
     FileNotFound,
     AccessDenied,
@@ -2940,8 +2989,6 @@ pub const COORD = extern struct {
     Y: SHORT,
 };
 
-pub const CREATE_UNICODE_ENVIRONMENT = 1024;
-
 pub const TLS_OUT_OF_INDEXES = 4294967295;
 pub const IMAGE_TLS_DIRECTORY = extern struct {
     StartAddressOfRawData: usize,

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;

lib/std/std.zig

@@ -52,6 +52,7 @@ pub const base64 = @import("base64.zig");
 pub const bit_set = @import("bit_set.zig");
 pub const builtin = @import("builtin.zig");
 pub const c = @import("c.zig");
+pub const child_process = @import("child_process.zig");
 pub const coff = @import("coff.zig");
 pub const compress = @import("compress.zig");
 pub const crypto = @import("crypto.zig");

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,25 @@
+const Builder = @import("std").build.Builder;
+
+pub fn build(b: *Builder) void {
+    const target = b.standardTargetOptions(.{});
+    const optimize = b.standardOptimizeOption(.{});
+
+    const child = b.addExecutable(.{
+        .name = "child",
+        .root_source_file = .{ .path = "child.zig" },
+        .target = target,
+        .optimize = optimize,
+    });
+
+    const parent = b.addExecutable(.{
+        .name = "parent",
+        .root_source_file = .{ .path = "parent.zig" },
+        .target = target,
+        .optimize = optimize,
+    });
+    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,26 @@
+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 = try std.os.stringToHandle(s_handle);
+    defer std.os.close(file_handle);
+
+    // child inherited the handle, so inheritance must be enabled
+    const is_inheritable = try std.os.isInheritable(file_handle);
+    std.debug.assert(is_inheritable);
+
+    try std.os.disableInheritance(file_handle);
+    var file_in = std.fs.File{ .handle = file_handle }; // read side of pipe
+    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");
+}