diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index c3bd53b8802f..abf18cc938d9 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -830,7 +830,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, }; @@ -881,11 +881,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; @@ -905,7 +918,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; @@ -1103,6 +1127,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. @@ -1333,30 +1379,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, @@ -1473,14 +1514,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. @@ -1533,9 +1582,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; diff --git a/lib/std/os.zig b/lib/std/os.zig index bd6719ec8f51..e077d181ada8 100644 --- a/lib/std/os.zig +++ b/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, diff --git a/lib/std/os/windows.zig b/lib/std/os/windows.zig index fe0ebc49517c..60bf718043e1 100644 --- a/lib/std/os/windows.zig +++ b/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, diff --git a/lib/std/os/windows/kernel32.zig b/lib/std/os/windows/kernel32.zig index d3bfeaaf2c3b..7a05e810ac83 100644 --- a/lib/std/os/windows/kernel32.zig +++ b/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; diff --git a/lib/std/std.zig b/lib/std/std.zig index 4a6d33000313..fa0f46d7449a 100644 --- a/lib/std/std.zig +++ b/lib/std/std.zig @@ -53,6 +53,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"); diff --git a/test/standalone.zig b/test/standalone.zig index 965139235c35..b515ac89054b 100644 --- a/test/standalone.zig +++ b/test/standalone.zig @@ -65,6 +65,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) { diff --git a/test/standalone/childprocess_extrapipe/build.zig b/test/standalone/childprocess_extrapipe/build.zig new file mode 100644 index 000000000000..a936e8c231e8 --- /dev/null +++ b/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); +} diff --git a/test/standalone/childprocess_extrapipe/child.zig b/test/standalone/childprocess_extrapipe/child.zig new file mode 100644 index 000000000000..593652656d70 --- /dev/null +++ b/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"); +} diff --git a/test/standalone/childprocess_extrapipe/parent.zig b/test/standalone/childprocess_extrapipe/parent.zig new file mode 100644 index 000000000000..6834ec1ca0d5 --- /dev/null +++ b/test/standalone/childprocess_extrapipe/parent.zig @@ -0,0 +1,50 @@ +const builtin = @import("builtin"); +const std = @import("std"); +const ChildProcess = std.ChildProcess; +const math = std.math; +const windows = std.os.windows; +const os = std.os; +const testing = std.testing; +const child_process = std.child_process; +const pipe_rd = child_process.pipe_rd; +const pipe_wr = child_process.pipe_wr; + +pub fn main() !void { + var gpa_state = std.heap.GeneralPurposeAllocator(.{}){}; + defer if (gpa_state.deinit()) @panic("found memory leaks"); + const gpa = gpa_state.allocator(); + + var it = try std.process.argsWithAllocator(gpa); + defer it.deinit(); + _ = it.next() orelse unreachable; // skip binary name + const child_path = it.next() orelse unreachable; + + var pipe = try child_process.portablePipe(); + defer os.close(pipe[pipe_wr]); + var child_proc: ChildProcess = undefined; + // spawn block ensures read end of pipe always closed + shortly closed after spawn(). + { + defer os.close(pipe[pipe_rd]); + + var buf: [os.handleCharSize]u8 = comptime [_]u8{0} ** os.handleCharSize; + const s_handle = os.handleToString(pipe[pipe_rd], &buf) catch unreachable; + child_proc = ChildProcess.init( + &.{ child_path, s_handle }, + gpa, + ); + + // less time to leak read end of pipe => better + try os.enableInheritance(pipe[pipe_rd]); + try child_proc.spawn(); + } + + // check that inheritance was disabled for the handle the whole time + const is_inheritable = try os.isInheritable(pipe[pipe_wr]); + std.debug.assert(!is_inheritable); + + var file_out = std.fs.File{ .handle = pipe[pipe_wr] }; + const file_out_writer = file_out.writer(); + try file_out_writer.writeAll("test123\x17"); // ETB = \x17 + const ret_val = try child_proc.wait(); + try testing.expectEqual(ret_val, .{ .Exited = 0 }); +}