authorgravatar for thatlemon@gmail.comLemonBoy <thatlemon@gmail.com> 2020-10-20 08:51:21+02:00
committergravatar for johnnymarler@gmail.comJonathan Marler <johnnymarler@gmail.com> 2021-06-17 17:39:32-06:00
log34c00ecf57a50e19b31fce420044311c4f2e9c7a
tree4e12d9cbd5de56979506d1f14050adde2722231b
parent6f0cfdb8206026f239ca079a9f3eebae20bd5310

std: Avoid deadlocking in ChildProcess.exec

Reading stdin&stderr at different times may lead to nasty deadlocks (eg. when stdout is read before stderr and the child process doesn't write anything onto stdout). Implement a polling mechanism to make sure this won't happen: we read data from stderr/stdout as it becomes ready and then it's copied into an ArrayList provided by the user, avoiding any kind of blocking read.

3 files changed, 167 insertions(+), 2 deletions(-)

lib/std/child_process.zig+121-2
......@@ -257,6 +257,68 @@ pub const ChildProcess = struct {
257257 }
258258 }
259259
260 fn collectOutputWindows(child: *const ChildProcess, stdout: *std.ArrayList(u8), stderr: *std.ArrayList(u8), max_output_bytes: usize) !void {
261 // The order of the objects here is important, WaitForMultipleObjects
262 // uses the same order when scanning the events.
263 var wait_objects = [_]windows.kernel32.HANDLE{
264 child.handle, child.stdout.?.handle, child.stderr.?.handle,
265 };
266 // XXX: Calling zeroes([2]windows.OVERLAPPED) causes the stage1 compiler
267 // to crash and burn.
268 var overlapped = [_]windows.OVERLAPPED{
269 mem.zeroes(windows.OVERLAPPED),
270 mem.zeroes(windows.OVERLAPPED),
271 };
272 var temp_buf: [2][4096]u8 = undefined;
273
274 // Kickstart the loop by issuing two async reads.
275 // ReadFile returns false and GetLastError returns ERROR_IO_PENDING if
276 // everything is ok.
277 _ = windows.kernel32.ReadFile(wait_objects[1], &temp_buf[0], temp_buf[0].len, null, &overlapped[0]);
278 _ = windows.kernel32.ReadFile(wait_objects[2], &temp_buf[1], temp_buf[1].len, null, &overlapped[1]);
279
280 while (true) {
281 const status = windows.kernel32.WaitForMultipleObjects(wait_objects.len, &wait_objects, 0, windows.INFINITE);
282 std.debug.print("status {x}\n", .{status});
283 switch (status) {
284 windows.WAIT_OBJECT_0 + 0 => {
285 // The child process was terminated.
286 break;
287 },
288 windows.WAIT_OBJECT_0 + 1 => {
289 // stdout is ready.
290 var read_bytes: u32 = undefined;
291 if (windows.kernel32.GetOverlappedResult(wait_objects[1], &overlapped[0], &read_bytes, 0) == 0) {
292 switch (windows.kernel32.GetLastError()) {
293 else => |err| return windows.unexpectedError(err),
294 }
295 }
296 try stdout.appendSlice(temp_buf[0][0..read_bytes]);
297 _ = windows.kernel32.ReadFile(wait_objects[1], &temp_buf[0], temp_buf[0].len, null, &overlapped[0]);
298 },
299 windows.WAIT_OBJECT_0 + 2 => {
300 // stderr is ready.
301 var read_bytes: u32 = undefined;
302 if (windows.kernel32.GetOverlappedResult(wait_objects[2], &overlapped[1], &read_bytes, 0) == 0) {
303 switch (windows.kernel32.GetLastError()) {
304 else => |err| return windows.unexpectedError(err),
305 }
306 }
307 try stdout.appendSlice(temp_buf[1][0..read_bytes]);
308 _ = windows.kernel32.ReadFile(wait_objects[2], &temp_buf[1], temp_buf[1].len, null, &overlapped[1]);
309 },
310 windows.WAIT_FAILED => {
311 switch (windows.kernel32.GetLastError()) {
312 else => |err| return windows.unexpectedError(err),
313 }
314 },
315 // We're waiting with an infinite timeout
316 windows.WAIT_TIMEOUT => unreachable,
317 else => unreachable,
318 }
319 }
320 }
321
260322 /// Spawns a child process, waits for it, collecting stdout and stderr, and then returns.
261323 /// If it succeeds, the caller owns result.stdout and result.stderr memory.
262324 pub fn exec(args: struct {
......@@ -308,6 +370,14 @@ pub const ChildProcess = struct {
308370
309371 try collectOutputPosix(child, &stdout, &stderr, args.max_output_bytes);
310372
373 // XXX: Respect max_output_bytes
374 // XXX: Smarter reading logic, read directly into the ArrayList
375 if (builtin.os.tag == .windows) {
376 try collectOutputWindows(child, &stdout, &stderr, args.max_output_bytes);
377 } else {
378 try collectOutputPosix(child, &stdout, &stderr, args.max_output_bytes);
379 }
380
311381 return ExecResult{
312382 .term = try child.wait(),
313383 .stdout = stdout.toOwnedSlice(),
......@@ -644,7 +714,7 @@ pub const ChildProcess = struct {
644714 var g_hChildStd_OUT_Wr: ?windows.HANDLE = null;
645715 switch (self.stdout_behavior) {
646716 StdIo.Pipe => {
647 try windowsMakePipeOut(&g_hChildStd_OUT_Rd, &g_hChildStd_OUT_Wr, &saAttr);
717 try windowsMakePipe(&g_hChildStd_OUT_Rd, &g_hChildStd_OUT_Wr, &saAttr);
648718 },
649719 StdIo.Ignore => {
650720 g_hChildStd_OUT_Wr = nul_handle;
......@@ -664,7 +734,7 @@ pub const ChildProcess = struct {
664734 var g_hChildStd_ERR_Wr: ?windows.HANDLE = null;
665735 switch (self.stderr_behavior) {
666736 StdIo.Pipe => {
667 try windowsMakePipeOut(&g_hChildStd_ERR_Rd, &g_hChildStd_ERR_Wr, &saAttr);
737 try windowsMakePipe(&g_hChildStd_ERR_Rd, &g_hChildStd_ERR_Wr, &saAttr);
668738 },
669739 StdIo.Ignore => {
670740 g_hChildStd_ERR_Wr = nul_handle;
......@@ -897,6 +967,55 @@ fn windowsDestroyPipe(rd: ?windows.HANDLE, wr: ?windows.HANDLE) void {
897967 if (wr) |h| os.close(h);
898968}
899969
970var pipe_name_counter = std.atomic.Int(u32).init(1);
971
972fn windowsMakePipe(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const windows.SECURITY_ATTRIBUTES) !void {
973 var tmp_buf: [128]u8 = undefined;
974 // Forge a random path for the pipe.
975 const pipe_path = std.fmt.bufPrintZ(
976 &tmp_buf,
977 "\\\\.\\pipe\\zig-childprocess-{d}-{d}",
978 .{ windows.kernel32.GetCurrentProcessId(), pipe_name_counter.fetchAdd(1) },
979 ) catch unreachable;
980
981 // Create the read handle that can be used with overlapped IO ops.
982 const read_handle = windows.kernel32.CreateNamedPipeA(
983 pipe_path,
984 windows.PIPE_ACCESS_INBOUND | windows.FILE_FLAG_OVERLAPPED,
985 windows.PIPE_TYPE_BYTE,
986 1,
987 0x1000,
988 0x1000,
989 0,
990 sattr,
991 );
992 if (read_handle == windows.INVALID_HANDLE_VALUE) {
993 switch (windows.kernel32.GetLastError()) {
994 else => |err| return windows.unexpectedError(err),
995 }
996 }
997
998 const write_handle = windows.kernel32.CreateFileA(
999 pipe_path,
1000 windows.GENERIC_WRITE,
1001 0,
1002 sattr,
1003 windows.OPEN_EXISTING,
1004 windows.FILE_ATTRIBUTE_NORMAL,
1005 null,
1006 );
1007 if (write_handle == windows.INVALID_HANDLE_VALUE) {
1008 switch (windows.kernel32.GetLastError()) {
1009 else => |err| return windows.unexpectedError(err),
1010 }
1011 }
1012
1013 try windows.SetHandleInformation(read_handle, windows.HANDLE_FLAG_INHERIT, 0);
1014
1015 rd.* = read_handle;
1016 wr.* = write_handle;
1017}
1018
9001019fn windowsMakePipeIn(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const windows.SECURITY_ATTRIBUTES) !void {
9011020 var rd_h: windows.HANDLE = undefined;
9021021 var wr_h: windows.HANDLE = undefined;
lib/std/os/windows/bits.zig+13
......@@ -448,6 +448,19 @@ pub const SECURITY_ATTRIBUTES = extern struct {
448448pub const PSECURITY_ATTRIBUTES = *SECURITY_ATTRIBUTES;
449449pub const LPSECURITY_ATTRIBUTES = *SECURITY_ATTRIBUTES;
450450
451pub const PIPE_ACCESS_INBOUND = 0x00000001;
452pub const PIPE_ACCESS_OUTBOUND = 0x00000002;
453pub const PIPE_ACCESS_DUPLEX = 0x00000003;
454
455pub const PIPE_TYPE_BYTE = 0x00000000;
456pub const PIPE_TYPE_MESSAGE = 0x00000004;
457
458pub const PIPE_READMODE_BYTE = 0x00000000;
459pub const PIPE_READMODE_MESSAGE = 0x00000002;
460
461pub const PIPE_WAIT = 0x00000000;
462pub const PIPE_NOWAIT = 0x00000001;
463
451464pub const GENERIC_READ = 0x80000000;
452465pub const GENERIC_WRITE = 0x40000000;
453466pub const GENERIC_EXECUTE = 0x20000000;
lib/std/os/windows/kernel32.zig+33
......@@ -15,6 +15,29 @@ pub extern "kernel32" fn CloseHandle(hObject: HANDLE) callconv(WINAPI) BOOL;
1515pub extern "kernel32" fn CreateDirectoryW(lpPathName: [*:0]const u16, lpSecurityAttributes: ?*SECURITY_ATTRIBUTES) callconv(WINAPI) BOOL;
1616pub extern "kernel32" fn SetEndOfFile(hFile: HANDLE) callconv(WINAPI) BOOL;
1717
18pub extern "kernel32" fn GetCurrentProcessId() callconv(.Stdcall) DWORD;
19
20pub extern "kernel32" fn CreateNamedPipeA(
21 lpName: [*:0]const u8,
22 dwOpenMode: DWORD,
23 dwPipeMode: DWORD,
24 nMaxInstances: DWORD,
25 nOutBufferSize: DWORD,
26 nInBufferSize: DWORD,
27 nDefaultTimeOut: DWORD,
28 lpSecurityAttributes: ?*const SECURITY_ATTRIBUTES,
29) callconv(.Stdcall) HANDLE;
30pub extern "kernel32" fn CreateNamedPipeW(
31 lpName: LPCWSTR,
32 dwOpenMode: DWORD,
33 dwPipeMode: DWORD,
34 nMaxInstances: DWORD,
35 nOutBufferSize: DWORD,
36 nInBufferSize: DWORD,
37 nDefaultTimeOut: DWORD,
38 lpSecurityAttributes: ?*const SECURITY_ATTRIBUTES,
39) callconv(.Stdcall) HANDLE;
40
1841pub extern "kernel32" fn CreateEventExW(
1942 lpEventAttributes: ?*SECURITY_ATTRIBUTES,
2043 lpName: [*:0]const u16,
......@@ -32,6 +55,16 @@ pub extern "kernel32" fn CreateFileW(
3255 hTemplateFile: ?HANDLE,
3356) callconv(WINAPI) HANDLE;
3457
58pub extern "kernel32" fn CreateFileA(
59 lpFileName: [*:0]const u8,
60 dwDesiredAccess: DWORD,
61 dwShareMode: DWORD,
62 lpSecurityAttributes: ?*const SECURITY_ATTRIBUTES,
63 dwCreationDisposition: DWORD,
64 dwFlagsAndAttributes: DWORD,
65 hTemplateFile: ?HANDLE,
66) callconv(.Stdcall) HANDLE;
67
3568pub extern "kernel32" fn CreatePipe(
3669 hReadPipe: *HANDLE,
3770 hWritePipe: *HANDLE,