authorgravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2020-12-26 16:33:38-08:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2020-12-26 16:33:38-08:00
loge5894221f7a886c8b0cc21b8369e4d3bf11890b0
tree3697490677b01c796b62881adffa769f72560f0a
parent641bf4c46eb2d5c1f3e95898ed74848a56e0d999
parentcb290ed6c99681ade0bace286ae5040546542395
signaturebadge-question-mark Signed by PGP key 4AEE18F83AFDEB23

Merge pull request #7553 from ziglang/fix-the-damn-deadlock

Fix the damn deadlock

6 files changed, 201 insertions(+), 107 deletions(-)

ci/drone/linux_script+1-2
......@@ -17,8 +17,7 @@ git config core.abbrev 9
1717
1818mkdir build
1919cd build
20# TODO figure out why Drone CI is deadlocking and stop passing -DZIG_SINGLE_THREADED=ON
21cmake .. -DCMAKE_BUILD_TYPE=Release "-DCMAKE_INSTALL_PREFIX=$DISTDIR" -DZIG_STATIC=ON -DCMAKE_PREFIX_PATH=/deps/local -GNinja -DZIG_SINGLE_THREADED=ON
20cmake .. -DCMAKE_BUILD_TYPE=Release "-DCMAKE_INSTALL_PREFIX=$DISTDIR" -DZIG_STATIC=ON -DCMAKE_PREFIX_PATH=/deps/local -GNinja
2221
2322samu install
2423./zig build test -Dskip-release -Dskip-non-native
lib/std/child_process.zig+63-13
......@@ -19,6 +19,7 @@ const builtin = @import("builtin");
1919const Os = builtin.Os;
2020const TailQueue = std.TailQueue;
2121const maxInt = std.math.maxInt;
22const assert = std.debug.assert;
2223
2324pub const ChildProcess = struct {
2425 pid: if (builtin.os.tag == .windows) void else i32,
......@@ -376,19 +377,44 @@ pub const ChildProcess = struct {
376377 if (any_ignore) os.close(dev_null_fd);
377378 }
378379
379 var env_map_owned: BufMap = undefined;
380 var we_own_env_map: bool = undefined;
381 const env_map = if (self.env_map) |env_map| x: {
382 we_own_env_map = false;
383 break :x env_map;
384 } else x: {
385 we_own_env_map = true;
386 env_map_owned = try process.getEnvMap(self.allocator);
387 break :x &env_map_owned;
380 var arena_allocator = std.heap.ArenaAllocator.init(self.allocator);
381 defer arena_allocator.deinit();
382 const arena = &arena_allocator.allocator;
383
384 // The POSIX standard does not allow malloc() between fork() and execve(),
385 // and `self.allocator` may be a libc allocator.
386 // I have personally observed the child process deadlocking when it tries
387 // to call malloc() due to a heap allocation between fork() and execve(),
388 // in musl v1.1.24.
389 // Additionally, we want to reduce the number of possible ways things
390 // can fail between fork() and execve().
391 // Therefore, we do all the allocation for the execve() before the fork().
392 // This means we must do the null-termination of argv and env vars here.
393 const argv_buf = try arena.alloc(?[*:0]u8, self.argv.len + 1);
394 for (self.argv) |arg, i| {
395 const arg_buf = try arena.alloc(u8, arg.len + 1);
396 @memcpy(arg_buf.ptr, arg.ptr, arg.len);
397 arg_buf[arg.len] = 0;
398 argv_buf[i] = arg_buf[0..arg.len :0].ptr;
399 }
400 argv_buf[self.argv.len] = null;
401 const argv_ptr = argv_buf[0..self.argv.len :null].ptr;
402
403 const envp = m: {
404 if (self.env_map) |env_map| {
405 const envp_buf = try createNullDelimitedEnvMap(arena, env_map);
406 break :m envp_buf.ptr;
407 } else if (std.builtin.link_libc) {
408 break :m std.c.environ;
409 } else if (std.builtin.output_mode == .Exe) {
410 // Then we have Zig start code and this works.
411 // TODO type-safety for null-termination of `os.environ`.
412 break :m @ptrCast([*:null]?[*:0]u8, os.environ.ptr);
413 } else {
414 // TODO come up with a solution for this.
415 @compileError("missing std lib enhancement: ChildProcess implementation has no way to collect the environment variables to forward to the child process");
416 }
388417 };
389 defer {
390 if (we_own_env_map) env_map_owned.deinit();
391 }
392418
393419 // This pipe is used to communicate errors between the time of fork
394420 // and execve from the child process to the parent process.
......@@ -438,7 +464,10 @@ pub const ChildProcess = struct {
438464 os.setreuid(uid, uid) catch |err| forkChildErrReport(err_pipe[1], err);
439465 }
440466
441 const err = os.execvpe_expandArg0(self.allocator, self.expand_arg0, self.argv, env_map);
467 const err = switch (self.expand_arg0) {
468 .expand => os.execvpeZ_expandArg0(.expand, argv_buf.ptr[0].?, argv_ptr, envp),
469 .no_expand => os.execvpeZ_expandArg0(.no_expand, argv_buf.ptr[0].?, argv_ptr, envp),
470 };
442471 forkChildErrReport(err_pipe[1], err);
443472 }
444473
......@@ -881,3 +910,24 @@ pub fn createWindowsEnvBlock(allocator: *mem.Allocator, env_map: *const BufMap)
881910 i += 1;
882911 return allocator.shrink(result, i);
883912}
913
914pub fn createNullDelimitedEnvMap(arena: *mem.Allocator, env_map: *const std.BufMap) ![:null]?[*:0]u8 {
915 const envp_count = env_map.count();
916 const envp_buf = try arena.alloc(?[*:0]u8, envp_count + 1);
917 mem.set(?[*:0]u8, envp_buf, null);
918 {
919 var it = env_map.iterator();
920 var i: usize = 0;
921 while (it.next()) |pair| : (i += 1) {
922 const env_buf = try arena.alloc(u8, pair.key.len + pair.value.len + 2);
923 @memcpy(env_buf.ptr, pair.key.ptr, pair.key.len);
924 env_buf[pair.key.len] = '=';
925 @memcpy(env_buf.ptr + pair.key.len + 1, pair.value.ptr, pair.value.len);
926 const len = env_buf.len - 1;
927 env_buf[len] = 0;
928 envp_buf[i] = env_buf[0..len :0].ptr;
929 }
930 assert(i == envp_count);
931 }
932 return envp_buf[0..envp_count :null];
933}
lib/std/os.zig+2-81
......@@ -1348,89 +1348,10 @@ pub fn execvpeZ_expandArg0(
13481348/// If `file` is an absolute path, this is the same as `execveZ`.
13491349pub fn execvpeZ(
13501350 file: [*:0]const u8,
1351 argv: [*:null]const ?[*:0]const u8,
1351 argv_ptr: [*:null]const ?[*:0]const u8,
13521352 envp: [*:null]const ?[*:0]const u8,
13531353) ExecveError {
1354 return execvpeZ_expandArg0(.no_expand, file, argv, envp);
1355}
1356
1357/// This is the same as `execvpe` except if the `arg0_expand` parameter is set to `.expand`,
1358/// then argv[0] will be replaced with the expanded version of it, after resolving in accordance
1359/// with the PATH environment variable.
1360pub fn execvpe_expandArg0(
1361 allocator: *mem.Allocator,
1362 arg0_expand: Arg0Expand,
1363 argv_slice: []const []const u8,
1364 env_map: *const std.BufMap,
1365) (ExecveError || error{OutOfMemory}) {
1366 const argv_buf = try allocator.alloc(?[*:0]u8, argv_slice.len + 1);
1367 mem.set(?[*:0]u8, argv_buf, null);
1368 defer {
1369 for (argv_buf) |arg| {
1370 const arg_buf = mem.spanZ(arg) orelse break;
1371 allocator.free(arg_buf);
1372 }
1373 allocator.free(argv_buf);
1374 }
1375 for (argv_slice) |arg, i| {
1376 const arg_buf = try allocator.alloc(u8, arg.len + 1);
1377 @memcpy(arg_buf.ptr, arg.ptr, arg.len);
1378 arg_buf[arg.len] = 0;
1379 argv_buf[i] = arg_buf[0..arg.len :0].ptr;
1380 }
1381 argv_buf[argv_slice.len] = null;
1382 const argv_ptr = argv_buf[0..argv_slice.len :null].ptr;
1383
1384 const envp_buf = try createNullDelimitedEnvMap(allocator, env_map);
1385 defer freeNullDelimitedEnvMap(allocator, envp_buf);
1386
1387 switch (arg0_expand) {
1388 .expand => return execvpeZ_expandArg0(.expand, argv_buf.ptr[0].?, argv_ptr, envp_buf.ptr),
1389 .no_expand => return execvpeZ_expandArg0(.no_expand, argv_buf.ptr[0].?, argv_ptr, envp_buf.ptr),
1390 }
1391}
1392
1393/// This function must allocate memory to add a null terminating bytes on path and each arg.
1394/// It must also convert to KEY=VALUE\0 format for environment variables, and include null
1395/// pointers after the args and after the environment variables.
1396/// `argv_slice[0]` is the executable path.
1397/// This function also uses the PATH environment variable to get the full path to the executable.
1398pub fn execvpe(
1399 allocator: *mem.Allocator,
1400 argv_slice: []const []const u8,
1401 env_map: *const std.BufMap,
1402) (ExecveError || error{OutOfMemory}) {
1403 return execvpe_expandArg0(allocator, .no_expand, argv_slice, env_map);
1404}
1405
1406pub fn createNullDelimitedEnvMap(allocator: *mem.Allocator, env_map: *const std.BufMap) ![:null]?[*:0]u8 {
1407 const envp_count = env_map.count();
1408 const envp_buf = try allocator.alloc(?[*:0]u8, envp_count + 1);
1409 mem.set(?[*:0]u8, envp_buf, null);
1410 errdefer freeNullDelimitedEnvMap(allocator, envp_buf);
1411 {
1412 var it = env_map.iterator();
1413 var i: usize = 0;
1414 while (it.next()) |pair| : (i += 1) {
1415 const env_buf = try allocator.alloc(u8, pair.key.len + pair.value.len + 2);
1416 @memcpy(env_buf.ptr, pair.key.ptr, pair.key.len);
1417 env_buf[pair.key.len] = '=';
1418 @memcpy(env_buf.ptr + pair.key.len + 1, pair.value.ptr, pair.value.len);
1419 const len = env_buf.len - 1;
1420 env_buf[len] = 0;
1421 envp_buf[i] = env_buf[0..len :0].ptr;
1422 }
1423 assert(i == envp_count);
1424 }
1425 return envp_buf[0..envp_count :null];
1426}
1427
1428pub fn freeNullDelimitedEnvMap(allocator: *mem.Allocator, envp_buf: []?[*:0]u8) void {
1429 for (envp_buf) |env| {
1430 const env_buf = if (env) |ptr| ptr[0 .. mem.len(ptr) + 1] else break;
1431 allocator.free(env_buf);
1432 }
1433 allocator.free(envp_buf);
1354 return execvpeZ_expandArg0(.no_expand, file, argv_ptr, envp);
14341355}
14351356
14361357/// Get an environment variable.
lib/std/process.zig+66
......@@ -13,6 +13,7 @@ const math = std.math;
1313const Allocator = mem.Allocator;
1414const assert = std.debug.assert;
1515const testing = std.testing;
16const child_process = @import("child_process.zig");
1617
1718pub const abort = os.abort;
1819pub const exit = os.exit;
......@@ -778,3 +779,68 @@ pub fn getSelfExeSharedLibPaths(allocator: *Allocator) error{OutOfMemory}![][:0]
778779 else => @compileError("getSelfExeSharedLibPaths unimplemented for this target"),
779780 }
780781}
782
783/// Tells whether calling the `execv` or `execve` functions will be a compile error.
784pub const can_execv = std.builtin.os.tag != .windows;
785
786pub const ExecvError = std.os.ExecveError || error{OutOfMemory};
787
788/// Replaces the current process image with the executed process.
789/// This function must allocate memory to add a null terminating bytes on path and each arg.
790/// It must also convert to KEY=VALUE\0 format for environment variables, and include null
791/// pointers after the args and after the environment variables.
792/// `argv[0]` is the executable path.
793/// This function also uses the PATH environment variable to get the full path to the executable.
794/// Due to the heap-allocation, it is illegal to call this function in a fork() child.
795/// For that use case, use the `std.os` functions directly.
796pub fn execv(allocator: *mem.Allocator, argv: []const []const u8) ExecvError {
797 return execve(allocator, argv, null);
798}
799
800/// Replaces the current process image with the executed process.
801/// This function must allocate memory to add a null terminating bytes on path and each arg.
802/// It must also convert to KEY=VALUE\0 format for environment variables, and include null
803/// pointers after the args and after the environment variables.
804/// `argv[0]` is the executable path.
805/// This function also uses the PATH environment variable to get the full path to the executable.
806/// Due to the heap-allocation, it is illegal to call this function in a fork() child.
807/// For that use case, use the `std.os` functions directly.
808pub fn execve(
809 allocator: *mem.Allocator,
810 argv: []const []const u8,
811 env_map: ?*const std.BufMap,
812) ExecvError {
813 if (!can_execv) @compileError("The target OS does not support execv");
814
815 var arena_allocator = std.heap.ArenaAllocator.init(allocator);
816 defer arena_allocator.deinit();
817 const arena = &arena_allocator.allocator;
818
819 const argv_buf = try arena.alloc(?[*:0]u8, argv.len + 1);
820 for (argv) |arg, i| {
821 const arg_buf = try arena.alloc(u8, arg.len + 1);
822 @memcpy(arg_buf.ptr, arg.ptr, arg.len);
823 arg_buf[arg.len] = 0;
824 argv_buf[i] = arg_buf[0..arg.len :0].ptr;
825 }
826 argv_buf[argv.len] = null;
827 const argv_ptr = argv_buf[0..argv.len :null].ptr;
828
829 const envp = m: {
830 if (env_map) |m| {
831 const envp_buf = try child_process.createNullDelimitedEnvMap(arena, m);
832 break :m envp_buf.ptr;
833 } else if (std.builtin.link_libc) {
834 break :m std.c.environ;
835 } else if (std.builtin.output_mode == .Exe) {
836 // Then we have Zig start code and this works.
837 // TODO type-safety for null-termination of `os.environ`.
838 break :m @ptrCast([*:null]?[*:0]u8, os.environ.ptr);
839 } else {
840 // TODO come up with a solution for this.
841 @compileError("missing std lib enhancement: std.process.execv implementation has no way to collect the environment variables to forward to the child process");
842 }
843 };
844
845 return os.execvpeZ_expandArg0(.no_expand, argv_buf.ptr[0].?, argv_ptr, envp);
846}
src/Cache.zig+64-2
......@@ -12,6 +12,14 @@ const mem = std.mem;
1212const fmt = std.fmt;
1313const Allocator = std.mem.Allocator;
1414
15/// Process-scoped map keeping track of all locked Cache hashes, to detect deadlocks.
16/// This protection is conditionally compiled depending on `want_debug_deadlock`.
17var all_cache_digest_set: std.AutoHashMapUnmanaged(BinDigest, void) = .{};
18var all_cache_digest_lock: std.Mutex = .{};
19const want_debug_deadlock = std.debug.runtime_safety;
20const DebugBinDigest = if (want_debug_deadlock) BinDigest else void;
21const null_debug_bin_digest = if (want_debug_deadlock) ([1]u8{0} ** bin_digest_len) else {};
22
1523/// Be sure to call `Manifest.deinit` after successful initialization.
1624pub fn obtain(cache: *const Cache) Manifest {
1725 return Manifest{
......@@ -160,8 +168,15 @@ pub const HashHelper = struct {
160168
161169pub const Lock = struct {
162170 manifest_file: fs.File,
171 debug_bin_digest: DebugBinDigest,
163172
164173 pub fn release(lock: *Lock) void {
174 if (want_debug_deadlock) {
175 const held = all_cache_digest_lock.acquire();
176 defer held.release();
177
178 all_cache_digest_set.removeAssertDiscard(lock.debug_bin_digest);
179 }
165180 lock.manifest_file.close();
166181 lock.* = undefined;
167182 }
......@@ -178,6 +193,7 @@ pub const Manifest = struct {
178193 manifest_dirty: bool,
179194 files: std.ArrayListUnmanaged(File) = .{},
180195 hex_digest: [hex_digest_len]u8,
196 debug_bin_digest: DebugBinDigest = null_debug_bin_digest,
181197
182198 /// Add a file as a dependency of process being cached. When `hit` is
183199 /// called, the file's contents will be checked to ensure that it matches
......@@ -245,6 +261,23 @@ pub const Manifest = struct {
245261 var bin_digest: BinDigest = undefined;
246262 self.hash.hasher.final(&bin_digest);
247263
264 if (want_debug_deadlock) {
265 self.debug_bin_digest = bin_digest;
266
267 const held = all_cache_digest_lock.acquire();
268 defer held.release();
269
270 const gop = try all_cache_digest_set.getOrPut(self.cache.gpa, bin_digest);
271 if (gop.found_existing) {
272 std.debug.print("Cache deadlock detected in Cache.hit. Manifest has {d} files:\n", .{self.files.items.len});
273 for (self.files.items) |file| {
274 const p: []const u8 = file.path orelse "(null)";
275 std.debug.print(" file: {s}\n", .{p});
276 }
277 @panic("Cache deadlock detected");
278 }
279 }
280
248281 _ = std.fmt.bufPrint(&self.hex_digest, "{x}", .{bin_digest}) catch unreachable;
249282
250283 self.hash.hasher = hasher_init;
......@@ -570,15 +603,27 @@ pub const Manifest = struct {
570603 /// The `Manifest` remains safe to deinit.
571604 /// Don't forget to call `writeManifest` before this!
572605 pub fn toOwnedLock(self: *Manifest) Lock {
573 const manifest_file = self.manifest_file.?;
606 const lock: Lock = .{
607 .manifest_file = self.manifest_file.?,
608 .debug_bin_digest = self.debug_bin_digest,
609 };
574610 self.manifest_file = null;
575 return Lock{ .manifest_file = manifest_file };
611 self.debug_bin_digest = null_debug_bin_digest;
612 return lock;
576613 }
577614
578615 /// Releases the manifest file and frees any memory the Manifest was using.
579616 /// `Manifest.hit` must be called first.
580617 /// Don't forget to call `writeManifest` before this!
581618 pub fn deinit(self: *Manifest) void {
619 if (want_debug_deadlock) {
620 if (!mem.eql(u8, &self.debug_bin_digest, &null_debug_bin_digest)) {
621 const held = all_cache_digest_lock.acquire();
622 defer held.release();
623
624 all_cache_digest_set.removeAssertDiscard(self.debug_bin_digest);
625 }
626 }
582627 if (self.manifest_file) |file| {
583628 file.close();
584629 }
......@@ -662,6 +707,11 @@ test "cache file and then recall it" {
662707 // https://github.com/ziglang/zig/issues/5437
663708 return error.SkipZigTest;
664709 }
710 defer if (want_debug_deadlock) {
711 testing.expect(all_cache_digest_set.count() == 0);
712 all_cache_digest_set.clearAndFree(testing.allocator);
713 };
714
665715 const cwd = fs.cwd();
666716
667717 const temp_file = "test.txt";
......@@ -739,6 +789,10 @@ test "check that changing a file makes cache fail" {
739789 // https://github.com/ziglang/zig/issues/5437
740790 return error.SkipZigTest;
741791 }
792 defer if (want_debug_deadlock) {
793 testing.expect(all_cache_digest_set.count() == 0);
794 all_cache_digest_set.clearAndFree(testing.allocator);
795 };
742796 const cwd = fs.cwd();
743797
744798 const temp_file = "cache_hash_change_file_test.txt";
......@@ -815,6 +869,10 @@ test "no file inputs" {
815869 // https://github.com/ziglang/zig/issues/5437
816870 return error.SkipZigTest;
817871 }
872 defer if (want_debug_deadlock) {
873 testing.expect(all_cache_digest_set.count() == 0);
874 all_cache_digest_set.clearAndFree(testing.allocator);
875 };
818876 const cwd = fs.cwd();
819877 const temp_manifest_dir = "no_file_inputs_manifest_dir";
820878 defer cwd.deleteTree(temp_manifest_dir) catch {};
......@@ -860,6 +918,10 @@ test "Manifest with files added after initial hash work" {
860918 // https://github.com/ziglang/zig/issues/5437
861919 return error.SkipZigTest;
862920 }
921 defer if (want_debug_deadlock) {
922 testing.expect(all_cache_digest_set.count() == 0);
923 all_cache_digest_set.clearAndFree(testing.allocator);
924 };
863925 const cwd = fs.cwd();
864926
865927 const temp_file1 = "cache_hash_post_file_test1.txt";
src/main.zig+5-9
......@@ -116,15 +116,13 @@ pub fn main() anyerror!void {
116116 return mainArgs(gpa, arena, args);
117117}
118118
119const os_can_execve = std.builtin.os.tag != .windows;
120
121119pub fn mainArgs(gpa: *Allocator, arena: *Allocator, args: []const []const u8) !void {
122120 if (args.len <= 1) {
123121 std.log.info("{}", .{usage});
124122 fatal("expected command argument", .{});
125123 }
126124
127 if (os_can_execve and std.os.getenvZ("ZIG_IS_DETECTING_LIBC_PATHS") != null) {
125 if (std.process.can_execv and std.os.getenvZ("ZIG_IS_DETECTING_LIBC_PATHS") != null) {
128126 // In this case we have accidentally invoked ourselves as "the system C compiler"
129127 // to figure out where libc is installed. This is essentially infinite recursion
130128 // via child process execution due to the CC environment variable pointing to Zig.
......@@ -147,11 +145,11 @@ pub fn mainArgs(gpa: *Allocator, arena: *Allocator, args: []const []const u8) !v
147145 // CC environment variable. We detect and support this scenario here because of
148146 // the ZIG_IS_DETECTING_LIBC_PATHS environment variable.
149147 if (mem.eql(u8, args[1], "cc")) {
150 return std.os.execvpe(arena, args[1..], &env_map);
148 return std.process.execve(arena, args[1..], &env_map);
151149 } else {
152150 const modified_args = try arena.dupe([]const u8, args);
153151 modified_args[0] = "cc";
154 return std.os.execvpe(arena, modified_args, &env_map);
152 return std.process.execve(arena, modified_args, &env_map);
155153 }
156154 }
157155
......@@ -1841,10 +1839,8 @@ fn buildOutputType(
18411839 }
18421840 // We do not execve for tests because if the test fails we want to print the error message and
18431841 // invocation below.
1844 if (os_can_execve and arg_mode == .run and !watch) {
1845 // TODO improve the std lib so that we don't need a call to getEnvMap here.
1846 var env_vars = try process.getEnvMap(arena);
1847 const err = std.os.execvpe(gpa, argv.items, &env_vars);
1842 if (std.process.can_execv and arg_mode == .run and !watch) {
1843 const err = std.process.execv(gpa, argv.items);
18481844 const cmd = try argvCmd(arena, argv.items);
18491845 fatal("the following command failed to execve with '{s}':\n{s}", .{ @errorName(err), cmd });
18501846 } else {