Fix the damn deadlock

ci/drone/linux_script

@@ -17,8 +17,7 @@ git config core.abbrev 9
 
 mkdir build
 cd build
-# TODO figure out why Drone CI is deadlocking and stop passing -DZIG_SINGLE_THREADED=ON
-cmake .. -DCMAKE_BUILD_TYPE=Release "-DCMAKE_INSTALL_PREFIX=$DISTDIR" -DZIG_STATIC=ON -DCMAKE_PREFIX_PATH=/deps/local -GNinja -DZIG_SINGLE_THREADED=ON
+cmake .. -DCMAKE_BUILD_TYPE=Release "-DCMAKE_INSTALL_PREFIX=$DISTDIR" -DZIG_STATIC=ON -DCMAKE_PREFIX_PATH=/deps/local -GNinja
 
 samu install
 ./zig build test -Dskip-release -Dskip-non-native

lib/std/child_process.zig

@@ -19,6 +19,7 @@ const builtin = @import("builtin");
 const Os = builtin.Os;
 const TailQueue = std.TailQueue;
 const maxInt = std.math.maxInt;
+const assert = std.debug.assert;
 
 pub const ChildProcess = struct {
     pid: if (builtin.os.tag == .windows) void else i32,
@@ -376,19 +377,44 @@ pub const ChildProcess = struct {
             if (any_ignore) os.close(dev_null_fd);
         }
 
-        var env_map_owned: BufMap = undefined;
-        var we_own_env_map: bool = undefined;
-        const env_map = if (self.env_map) |env_map| x: {
-            we_own_env_map = false;
-            break :x env_map;
-        } else x: {
-            we_own_env_map = true;
-            env_map_owned = try process.getEnvMap(self.allocator);
-            break :x &env_map_owned;
+        var arena_allocator = std.heap.ArenaAllocator.init(self.allocator);
+        defer arena_allocator.deinit();
+        const arena = &arena_allocator.allocator;
+
+        // The POSIX standard does not allow malloc() between fork() and execve(),
+        // and `self.allocator` may be a libc allocator.
+        // I have personally observed the child process deadlocking when it tries
+        // to call malloc() due to a heap allocation between fork() and execve(),
+        // in musl v1.1.24.
+        // Additionally, we want to reduce the number of possible ways things
+        // can fail between fork() and execve().
+        // Therefore, we do all the allocation for the execve() before the fork().
+        // This means we must do the null-termination of argv and env vars here.
+        const argv_buf = try arena.alloc(?[*:0]u8, self.argv.len + 1);
+        for (self.argv) |arg, i| {
+            const arg_buf = try arena.alloc(u8, arg.len + 1);
+            @memcpy(arg_buf.ptr, arg.ptr, arg.len);
+            arg_buf[arg.len] = 0;
+            argv_buf[i] = arg_buf[0..arg.len :0].ptr;
+        }
+        argv_buf[self.argv.len] = null;
+        const argv_ptr = argv_buf[0..self.argv.len :null].ptr;
+
+        const envp = m: {
+            if (self.env_map) |env_map| {
+                const envp_buf = try createNullDelimitedEnvMap(arena, env_map);
+                break :m envp_buf.ptr;
+            } else if (std.builtin.link_libc) {
+                break :m std.c.environ;
+            } else if (std.builtin.output_mode == .Exe) {
+                // Then we have Zig start code and this works.
+                // TODO type-safety for null-termination of `os.environ`.
+                break :m @ptrCast([*:null]?[*:0]u8, os.environ.ptr);
+            } else {
+                // TODO come up with a solution for this.
+                @compileError("missing std lib enhancement: ChildProcess implementation has no way to collect the environment variables to forward to the child process");
+            }
         };
-        defer {
-            if (we_own_env_map) env_map_owned.deinit();
-        }
 
         // This pipe is used to communicate errors between the time of fork
         // and execve from the child process to the parent process.
@@ -438,7 +464,10 @@ pub const ChildProcess = struct {
                 os.setreuid(uid, uid) catch |err| forkChildErrReport(err_pipe[1], err);
             }
 
-            const err = os.execvpe_expandArg0(self.allocator, self.expand_arg0, self.argv, env_map);
+            const err = switch (self.expand_arg0) {
+                .expand => os.execvpeZ_expandArg0(.expand, argv_buf.ptr[0].?, argv_ptr, envp),
+                .no_expand => os.execvpeZ_expandArg0(.no_expand, argv_buf.ptr[0].?, argv_ptr, envp),
+            };
             forkChildErrReport(err_pipe[1], err);
         }
 
@@ -881,3 +910,24 @@ pub fn createWindowsEnvBlock(allocator: *mem.Allocator, env_map: *const BufMap)
     i += 1;
     return allocator.shrink(result, i);
 }
+
+pub fn createNullDelimitedEnvMap(arena: *mem.Allocator, env_map: *const std.BufMap) ![:null]?[*:0]u8 {
+    const envp_count = env_map.count();
+    const envp_buf = try arena.alloc(?[*:0]u8, envp_count + 1);
+    mem.set(?[*:0]u8, envp_buf, null);
+    {
+        var it = env_map.iterator();
+        var i: usize = 0;
+        while (it.next()) |pair| : (i += 1) {
+            const env_buf = try arena.alloc(u8, pair.key.len + pair.value.len + 2);
+            @memcpy(env_buf.ptr, pair.key.ptr, pair.key.len);
+            env_buf[pair.key.len] = '=';
+            @memcpy(env_buf.ptr + pair.key.len + 1, pair.value.ptr, pair.value.len);
+            const len = env_buf.len - 1;
+            env_buf[len] = 0;
+            envp_buf[i] = env_buf[0..len :0].ptr;
+        }
+        assert(i == envp_count);
+    }
+    return envp_buf[0..envp_count :null];
+}

lib/std/os.zig

@@ -1348,89 +1348,10 @@ pub fn execvpeZ_expandArg0(
 /// If `file` is an absolute path, this is the same as `execveZ`.
 pub fn execvpeZ(
     file: [*:0]const u8,
-    argv: [*:null]const ?[*:0]const u8,
+    argv_ptr: [*:null]const ?[*:0]const u8,
     envp: [*:null]const ?[*:0]const u8,
 ) ExecveError {
-    return execvpeZ_expandArg0(.no_expand, file, argv, envp);
-}
-
-/// This is the same as `execvpe` except if the `arg0_expand` parameter is set to `.expand`,
-/// then argv[0] will be replaced with the expanded version of it, after resolving in accordance
-/// with the PATH environment variable.
-pub fn execvpe_expandArg0(
-    allocator: *mem.Allocator,
-    arg0_expand: Arg0Expand,
-    argv_slice: []const []const u8,
-    env_map: *const std.BufMap,
-) (ExecveError || error{OutOfMemory}) {
-    const argv_buf = try allocator.alloc(?[*:0]u8, argv_slice.len + 1);
-    mem.set(?[*:0]u8, argv_buf, null);
-    defer {
-        for (argv_buf) |arg| {
-            const arg_buf = mem.spanZ(arg) orelse break;
-            allocator.free(arg_buf);
-        }
-        allocator.free(argv_buf);
-    }
-    for (argv_slice) |arg, i| {
-        const arg_buf = try allocator.alloc(u8, arg.len + 1);
-        @memcpy(arg_buf.ptr, arg.ptr, arg.len);
-        arg_buf[arg.len] = 0;
-        argv_buf[i] = arg_buf[0..arg.len :0].ptr;
-    }
-    argv_buf[argv_slice.len] = null;
-    const argv_ptr = argv_buf[0..argv_slice.len :null].ptr;
-
-    const envp_buf = try createNullDelimitedEnvMap(allocator, env_map);
-    defer freeNullDelimitedEnvMap(allocator, envp_buf);
-
-    switch (arg0_expand) {
-        .expand => return execvpeZ_expandArg0(.expand, argv_buf.ptr[0].?, argv_ptr, envp_buf.ptr),
-        .no_expand => return execvpeZ_expandArg0(.no_expand, argv_buf.ptr[0].?, argv_ptr, envp_buf.ptr),
-    }
-}
-
-/// This function must allocate memory to add a null terminating bytes on path and each arg.
-/// It must also convert to KEY=VALUE\0 format for environment variables, and include null
-/// pointers after the args and after the environment variables.
-/// `argv_slice[0]` is the executable path.
-/// This function also uses the PATH environment variable to get the full path to the executable.
-pub fn execvpe(
-    allocator: *mem.Allocator,
-    argv_slice: []const []const u8,
-    env_map: *const std.BufMap,
-) (ExecveError || error{OutOfMemory}) {
-    return execvpe_expandArg0(allocator, .no_expand, argv_slice, env_map);
-}
-
-pub fn createNullDelimitedEnvMap(allocator: *mem.Allocator, env_map: *const std.BufMap) ![:null]?[*:0]u8 {
-    const envp_count = env_map.count();
-    const envp_buf = try allocator.alloc(?[*:0]u8, envp_count + 1);
-    mem.set(?[*:0]u8, envp_buf, null);
-    errdefer freeNullDelimitedEnvMap(allocator, envp_buf);
-    {
-        var it = env_map.iterator();
-        var i: usize = 0;
-        while (it.next()) |pair| : (i += 1) {
-            const env_buf = try allocator.alloc(u8, pair.key.len + pair.value.len + 2);
-            @memcpy(env_buf.ptr, pair.key.ptr, pair.key.len);
-            env_buf[pair.key.len] = '=';
-            @memcpy(env_buf.ptr + pair.key.len + 1, pair.value.ptr, pair.value.len);
-            const len = env_buf.len - 1;
-            env_buf[len] = 0;
-            envp_buf[i] = env_buf[0..len :0].ptr;
-        }
-        assert(i == envp_count);
-    }
-    return envp_buf[0..envp_count :null];
-}
-
-pub fn freeNullDelimitedEnvMap(allocator: *mem.Allocator, envp_buf: []?[*:0]u8) void {
-    for (envp_buf) |env| {
-        const env_buf = if (env) |ptr| ptr[0 .. mem.len(ptr) + 1] else break;
-        allocator.free(env_buf);
-    }
-    allocator.free(envp_buf);
+    return execvpeZ_expandArg0(.no_expand, file, argv_ptr, envp);
 }
 
 /// Get an environment variable.

lib/std/process.zig

@@ -13,6 +13,7 @@ const math = std.math;
 const Allocator = mem.Allocator;
 const assert = std.debug.assert;
 const testing = std.testing;
+const child_process = @import("child_process.zig");
 
 pub const abort = os.abort;
 pub const exit = os.exit;
@@ -778,3 +779,68 @@ pub fn getSelfExeSharedLibPaths(allocator: *Allocator) error{OutOfMemory}![][:0]
         else => @compileError("getSelfExeSharedLibPaths unimplemented for this target"),
     }
 }
+
+/// Tells whether calling the `execv` or `execve` functions will be a compile error.
+pub const can_execv = std.builtin.os.tag != .windows;
+
+pub const ExecvError = std.os.ExecveError || error{OutOfMemory};
+
+/// Replaces the current process image with the executed process.
+/// This function must allocate memory to add a null terminating bytes on path and each arg.
+/// It must also convert to KEY=VALUE\0 format for environment variables, and include null
+/// pointers after the args and after the environment variables.
+/// `argv[0]` is the executable path.
+/// This function also uses the PATH environment variable to get the full path to the executable.
+/// Due to the heap-allocation, it is illegal to call this function in a fork() child.
+/// For that use case, use the `std.os` functions directly.
+pub fn execv(allocator: *mem.Allocator, argv: []const []const u8) ExecvError {
+    return execve(allocator, argv, null);
+}
+
+/// Replaces the current process image with the executed process.
+/// This function must allocate memory to add a null terminating bytes on path and each arg.
+/// It must also convert to KEY=VALUE\0 format for environment variables, and include null
+/// pointers after the args and after the environment variables.
+/// `argv[0]` is the executable path.
+/// This function also uses the PATH environment variable to get the full path to the executable.
+/// Due to the heap-allocation, it is illegal to call this function in a fork() child.
+/// For that use case, use the `std.os` functions directly.
+pub fn execve(
+    allocator: *mem.Allocator,
+    argv: []const []const u8,
+    env_map: ?*const std.BufMap,
+) ExecvError {
+    if (!can_execv) @compileError("The target OS does not support execv");
+
+    var arena_allocator = std.heap.ArenaAllocator.init(allocator);
+    defer arena_allocator.deinit();
+    const arena = &arena_allocator.allocator;
+
+    const argv_buf = try arena.alloc(?[*:0]u8, argv.len + 1);
+    for (argv) |arg, i| {
+        const arg_buf = try arena.alloc(u8, arg.len + 1);
+        @memcpy(arg_buf.ptr, arg.ptr, arg.len);
+        arg_buf[arg.len] = 0;
+        argv_buf[i] = arg_buf[0..arg.len :0].ptr;
+    }
+    argv_buf[argv.len] = null;
+    const argv_ptr = argv_buf[0..argv.len :null].ptr;
+
+    const envp = m: {
+        if (env_map) |m| {
+            const envp_buf = try child_process.createNullDelimitedEnvMap(arena, m);
+            break :m envp_buf.ptr;
+        } else if (std.builtin.link_libc) {
+            break :m std.c.environ;
+        } else if (std.builtin.output_mode == .Exe) {
+            // Then we have Zig start code and this works.
+            // TODO type-safety for null-termination of `os.environ`.
+            break :m @ptrCast([*:null]?[*:0]u8, os.environ.ptr);
+        } else {
+            // TODO come up with a solution for this.
+            @compileError("missing std lib enhancement: std.process.execv implementation has no way to collect the environment variables to forward to the child process");
+        }
+    };
+
+    return os.execvpeZ_expandArg0(.no_expand, argv_buf.ptr[0].?, argv_ptr, envp);
+}