authorgravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2020-12-26 13:50:26-07:00
committergravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2020-12-26 13:50:26-07:00
log3f9588ca29c721f11eb9dae8f6de8ebd1155c7bf
treecef7fd69c3dc20dabdda4f90d3a7fc5b2c46d13f
parented39ff202baf5bb73e54f7ecc20df63d9c190dc9

std: do not call malloc() between fork() and execv()

We were violating the POSIX standard which resulted in a deadlock on musl v1.1.24 on aarch64 alpine linux, uncovered with the new ThreadPool usage in the stage2 compiler. std.os execv functions that accept an Allocator parameter are removed because they are footguns. The POSIX standard does not allow calls to malloc() between fork() and execv() and since it is common to both (1) call execv() after fork() and (2) use std.heap.c_allocator, Programmers are encouraged to go through the `std.process` API instead, causing some dissonance when combined with `std.os` APIs. I also slapped a big warning message on all the relevant doc comments.

4 files changed, 136 insertions(+), 103 deletions(-)

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/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 {