From 223fd9f4e536a6150f140cb82439a88049298470 Mon Sep 17 00:00:00 2001 From: Jan Philipp Hafer Date: Mon, 2 Jan 2023 00:06:50 +0100 Subject: [PATCH] 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 #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. --- lib/std/child_process.zig | 98 ++++++++++++++----- lib/std/os.zig | 59 +++++++++++ lib/std/os/windows.zig | 51 +++++++++- lib/std/os/windows/kernel32.zig | 2 + lib/std/std.zig | 1 + test/standalone.zig | 1 + .../childprocess_extrapipe/build.zig | 25 +++++ .../childprocess_extrapipe/child.zig | 26 +++++ .../childprocess_extrapipe/parent.zig | 50 ++++++++++ 9 files changed, 285 insertions(+), 28 deletions(-) create mode 100644 test/standalone/childprocess_extrapipe/build.zig create mode 100644 test/standalone/childprocess_extrapipe/child.zig create mode 100644 test/standalone/childprocess_extrapipe/parent.zig 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 }); +}