diff --git a/src/bun.js/api/bun/subprocess.zig b/src/bun.js/api/bun/subprocess.zig index f4aa21aae93639..5ab5150581ff9a 100644 --- a/src/bun.js/api/bun/subprocess.zig +++ b/src/bun.js/api/bun/subprocess.zig @@ -1688,7 +1688,6 @@ pub const Subprocess = struct { } if (args != .zero and args.isObject()) { - // This must run before the stdio parsing happens if (args.getTruthy(globalThis, "ipc")) |val| { if (val.isCell() and val.isCallable(globalThis.vm())) { @@ -1848,9 +1847,8 @@ pub const Subprocess = struct { } if (!override_env and env_array.items.len == 0) { - env_array.items = jsc_vm.bundler.env.map.createNullDelimitedEnvMap(allocator) catch |err| + env_array = jsc_vm.bundler.env.map.createEnvArrayList(allocator) catch |err| return globalThis.handleError(err, "in Bun.spawn"); - env_array.capacity = env_array.items.len; } inline for (0..stdio.len) |fd_index| { @@ -1949,8 +1947,8 @@ pub const Subprocess = struct { var spawned = switch (bun.spawn.spawnProcess( &spawn_options, - @ptrCast(argv.items.ptr), - @ptrCast(env_array.items.ptr), + @ptrCast(argv.items[0.. :null].ptr), + @ptrCast(env_array.items[0.. :null].ptr), ) catch |err| { process_allocator.destroy(subprocess); spawn_options.deinit(); diff --git a/src/bun.js/event_loop.zig b/src/bun.js/event_loop.zig index b6a164e13b4f56..17885bc19df538 100644 --- a/src/bun.js/event_loop.zig +++ b/src/bun.js/event_loop.zig @@ -2172,7 +2172,7 @@ pub const EventLoopHandle = union(enum) { this.loop().unref(); } - pub inline fn createNullDelimitedEnvMap(this: @This(), alloc: Allocator) ![:null]?[*:0]u8 { + pub inline fn createNullDelimitedEnvMap(this: @This(), alloc: Allocator) !bun.DotEnv.Map.NullDelimitedEnvMap { return switch (this) { .js => this.js.virtual_machine.bundler.env.map.createNullDelimitedEnvMap(alloc), .mini => this.mini.env.?.map.createNullDelimitedEnvMap(alloc), diff --git a/src/cli/bunx_command.zig b/src/cli/bunx_command.zig index 8f2794a6b78750..6664a3d04c0d2b 100644 --- a/src/cli/bunx_command.zig +++ b/src/cli/bunx_command.zig @@ -602,10 +602,13 @@ pub const BunxCommand = struct { debug("installing package: {s}", .{bun.fmt.fmtSlice(argv_to_use, " ")}); this_bundler.env.map.put("BUN_INTERNAL_BUNX_INSTALL", "true") catch bun.outOfMemory(); + const envp = try this_bundler.env.map.createNullDelimitedEnvMap(bun.default_allocator); + defer envp.deinit(bun.default_allocator); + const spawn_result = switch ((bun.spawnSync(&.{ .argv = argv_to_use, - .envp = try this_bundler.env.map.createNullDelimitedEnvMap(bun.default_allocator), + .envp = envp.envp, .cwd = bunx_cache_dir, .stderr = .inherit, diff --git a/src/cli/run_command.zig b/src/cli/run_command.zig index 99e534afbf13cc..18775cfe96dc3f 100644 --- a/src/cli/run_command.zig +++ b/src/cli/run_command.zig @@ -347,13 +347,14 @@ pub const RunCommand = struct { Output.flush(); } + const envp = try env.map.createNullDelimitedEnvMap(bun.default_allocator); + defer envp.deinit(bun.default_allocator); + const spawn_result = switch ((bun.spawnSync(&.{ .argv = &argv, .argv0 = shell_bin.ptr, - // TODO: remember to free this when we add --filter or --concurrent - // in the meantime we don't need to free it. - .envp = try env.map.createNullDelimitedEnvMap(bun.default_allocator), + .envp = envp.envp, .cwd = cwd, .stderr = .inherit, @@ -513,14 +514,15 @@ pub const RunCommand = struct { argv = try array_list.toOwnedSlice(); } + const envp = try env.map.createNullDelimitedEnvMap(bun.default_allocator); + defer envp.deinit(bun.default_allocator); + const silent = ctx.debug.silent; const spawn_result = bun.spawnSync(&.{ .argv = argv, .argv0 = executableZ, - // TODO: remember to free this when we add --filter or --concurrent - // in the meantime we don't need to free it. - .envp = try env.map.createNullDelimitedEnvMap(bun.default_allocator), + .envp = envp.envp, .cwd = cwd, .stderr = .inherit, @@ -880,16 +882,6 @@ pub const RunCommand = struct { this_bundler.options.production = true; this_bundler.runEnvLoader(true) catch {}; - - // sus - // if (root_dir_info.getEntries(0)) |dir| { - // // Run .env again if it exists in a parent dir - // if (this_bundler.options.production) { - // this_bundler.env.load(dir, this_bundler.options.env.files, .production, true) catch {}; - // } else { - // this_bundler.env.load(dir, this_bundler.options.env.files, .development, true) catch {}; - // } - // } } this_bundler.env.map.putDefault("npm_config_local_prefix", this_bundler.fs.top_level_dir) catch unreachable; @@ -1049,11 +1041,8 @@ pub const RunCommand = struct { { this_bundler.env.loadProcess(); - if (this_bundler.env.get("NODE_ENV")) |node_env| { - if (strings.eqlComptime(node_env, "production")) { - this_bundler.options.production = true; - } - } + if (this_bundler.env.isProduction()) + this_bundler.options.production = true; } const ResultList = bun.StringArrayHashMap(void); diff --git a/src/env_loader.zig b/src/env_loader.zig index b94df564fddc40..6c5ebb96d4bce6 100644 --- a/src/env_loader.zig +++ b/src/env_loader.zig @@ -1058,13 +1058,9 @@ pub const Map = struct { map: HashTable, /// Creates a environment block for use in Posix Spawn APIs. - /// As in, a sentinel slice of sentinel string pointers. - /// - /// TODO: the returned value of this is very hard to cleanup manually, and currently must be - /// backed by an arena allocator to avoid a memory leak. We use this in many places without - /// an area, so those usages are leaking here. Solution would be to make this return a struct - /// with a deinit that contained the allocation length. - pub fn createNullDelimitedEnvMap(this: *Map, alloc: std.mem.Allocator) ![:null]?[*:0]u8 { + /// The return value is a struct with a field '.envp' which can be passed to a posix api. + /// Call .deinit with the same allocator to free the memory. + pub fn createNullDelimitedEnvMap(this: *Map, alloc: std.mem.Allocator) !NullDelimitedEnvMap { var env_map = &this.map; var envp_count: usize = 0; @@ -1077,12 +1073,17 @@ pub const Map = struct { total_bytes += (pair.key_ptr.len + pair.value_ptr.len + "=\x00".len); } } - total_bytes += (envp_count + 1) * @sizeOf(?[*:0]u8); // +1 for the null ptr after the pointer list + total_bytes += (envp_count + 1) * @sizeOf(?[*:0]const u8); // +1 for the null ptr after the pointer list - const buf = try alloc.alignedAlloc(u8, @alignOf(?[*:0]u8), total_bytes); + // Instead of creating separate allocations for each string, let's allocate + // enough bytes for everything. The buffer contains the pointers, and then + // all the string bytes afterwards. For this to pass the ptrCast a few + // lines later, we have to make a pointer-aligned allocation. + const buf = try alloc.alignedAlloc(u8, @alignOf(?[*:0]const u8), total_bytes); + comptime std.debug.assert(@TypeOf(buf.ptr) == NullDelimitedEnvMap.BufPtrType); const envp = envp: { - const p: [*]?[*:0]u8 = @ptrCast(buf.ptr); + const p: [*]?[*:0]const u8 = @ptrCast(buf.ptr); p[envp_count] = null; break :envp p[0..envp_count :null]; }; @@ -1110,11 +1111,66 @@ pub const Map = struct { if (comptime Environment.allow_assert) { std.debug.assert(fba.end_index == fba.buffer.len); // incorrect counting above. every pointer should be byte-aligned + std.debug.assert(@intFromPtr(envp.ptr) == @intFromPtr(buf.ptr)); } - return envp; + return .{ + .envp = envp, + // In order to properly free this, the Zig allocator must be passed + // the original slice it was given. The pointer is technically not + // enough information. + .allocation_size = total_bytes, + }; } + /// Creates a environment block for use in Posix Spawn APIs. It is assumed that the caller + /// will want to add other strings to this block, **so there is no null terminator** in the + /// return value. + /// + /// If passing this into a Posix Spawn API, you will need to append a `null`, and then + /// use `list.items[0.. :null]` to get a null-terminated array. + /// + /// Unlike `createNullDelimitedEnvMap`, this does a separate allocation for each string, + /// meaning you likely want to use an arena allocator for the input so you can quickly + /// free everything. + pub fn createEnvArrayList(this: *Map, alloc: std.mem.Allocator) !std.ArrayListUnmanaged(?[*:0]const u8) { + const envp_count = this.map.count(); + // Allocate an extra entry so a caller that wants a null pointer does not have to resize the ArrayList. + var list = try std.ArrayListUnmanaged(?[*:0]const u8).initCapacity(alloc, envp_count + 1); + errdefer { + for (list.items) |item| { + alloc.free(bun.span( + item orelse unreachable, // no null values are added + )); + } + list.deinit(alloc); + } + + var it = this.map.iterator(); + while (it.next()) |pair| { + const variable_buf = try alloc.allocSentinel(u8, pair.key_ptr.len + pair.value_ptr.len + 1, 0); + @memcpy(variable_buf[0..pair.key_ptr.len], pair.key_ptr.*); + variable_buf[pair.key_ptr.len] = '='; + @memcpy(variable_buf[pair.key_ptr.len + 1 ..], pair.value_ptr.*); + list.appendAssumeCapacity(variable_buf.ptr); + } + + return list; + } + + pub const NullDelimitedEnvMap = struct { + envp: [:null]?[*:0]const u8, + allocation_size: usize, + + const BufPtrType = [*]align(@alignOf(?[*:0]const u8)) u8; + + pub inline fn deinit(this: NullDelimitedEnvMap, alloc: std.mem.Allocator) void { + alloc.free( + @as(BufPtrType, @ptrCast(this.envp.ptr))[0..this.allocation_size], + ); + } + }; + /// Returns a wrapper around the std.process.EnvMap that does not duplicate the memory of /// the keys and values, but instead points into the memory of the bun env map. /// @@ -1241,10 +1297,6 @@ pub const Map = struct { _ = try this.map.getOrPutValue(key, value); } - pub inline fn getOrPut(this: *Map, key: string, value: string) !void { - _ = try this.map.getOrPutValue(key, value); - } - pub fn remove(this: *Map, key: string) void { this.map.remove(key); } diff --git a/src/install/install.zig b/src/install/install.zig index 244144376424f4..f7e1262385feb6 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -10321,13 +10321,12 @@ pub const PackageManager = struct { try PATH.appendSlice(original_path); } - this_bundler.env.map.put("PATH", PATH.items) catch unreachable; - - const envp = try this_bundler.env.map.createNullDelimitedEnvMap(this.allocator); + this_bundler.env.map.put("PATH", PATH.items) catch bun.outOfMemory(); + const env = try this_bundler.env.map.createNullDelimitedEnvMap(this.allocator); try this_bundler.env.map.put("PATH", original_path); PATH.deinit(); - try LifecycleScriptSubprocess.spawnPackageScripts(this, list, envp, log_level); + try LifecycleScriptSubprocess.spawnPackageScripts(this, list, env, log_level); } }; diff --git a/src/install/lifecycle_script_runner.zig b/src/install/lifecycle_script_runner.zig index 6295d852eb4f3d..d79de1ab8a6305 100644 --- a/src/install/lifecycle_script_runner.zig +++ b/src/install/lifecycle_script_runner.zig @@ -25,7 +25,7 @@ pub const LifecycleScriptSubprocess = struct { stderr: OutputReader = OutputReader.init(@This()), has_called_process_exit: bool = false, manager: *PackageManager, - envp: [:null]?[*:0]u8, + env: bun.DotEnv.Map.NullDelimitedEnvMap, timer: ?Timer = null, @@ -174,7 +174,7 @@ pub const LifecycleScriptSubprocess = struct { }; this.remaining_fds = 0; - var spawned = try (try bun.spawn.spawnProcess(&spawn_options, @ptrCast(&argv), this.envp)).unwrap(); + var spawned = try (try bun.spawn.spawnProcess(&spawn_options, @ptrCast(&argv), this.env.envp)).unwrap(); if (comptime Environment.isPosix) { if (spawned.stdout) |stdout| { @@ -391,18 +391,19 @@ pub const LifecycleScriptSubprocess = struct { this.stderr.deinit(); } + this.env.deinit(this.manager.allocator); this.destroy(); } pub fn spawnPackageScripts( manager: *PackageManager, list: Lockfile.Package.Scripts.List, - envp: [:null]?[*:0]u8, + env: bun.DotEnv.Map.NullDelimitedEnvMap, comptime log_level: PackageManager.Options.LogLevel, ) !void { var lifecycle_subprocess = LifecycleScriptSubprocess.new(.{ .manager = manager, - .envp = envp, + .env = env, .scripts = list, .package_name = list.package_name, }); diff --git a/src/shell/interpreter.zig b/src/shell/interpreter.zig index 15642507b5b157..00b7f0b3dcb0e7 100644 --- a/src/shell/interpreter.zig +++ b/src/shell/interpreter.zig @@ -3423,8 +3423,6 @@ pub const Interpreter = struct { var arena = &this.spawn_arena; var arena_allocator = arena.allocator(); var spawn_args = Subprocess.SpawnArgs.default(arena, this.base.interpreter.event_loop, false); - - spawn_args.argv = std.ArrayListUnmanaged(?[*:0]const u8){}; spawn_args.cmd_parent = this; spawn_args.cwd = this.base.shell.cwdZ(); @@ -3608,7 +3606,7 @@ pub const Interpreter = struct { .child = undefined, .buffered_closed = buffered_closed, } }; - const subproc = switch (Subprocess.spawnAsync(this.base.eventLoop(), &shellio, spawn_args, &this.exec.subproc.child)) { + const subproc = switch (Subprocess.spawnAsync(this.base.eventLoop(), &shellio, &spawn_args, &this.exec.subproc.child)) { .result => this.exec.subproc.child, .err => |*e| { this.base.throw(e); diff --git a/src/shell/subproc.zig b/src/shell/subproc.zig index ef3941433496da..3b20a9c035af5f 100644 --- a/src/shell/subproc.zig +++ b/src/shell/subproc.zig @@ -610,10 +610,7 @@ pub const ShellSubprocess = struct { cmd_parent: ?*ShellCmd = null, override_env: bool = false, - env_array: std.ArrayListUnmanaged(?[*:0]const u8) = .{ - .items = &.{}, - .capacity = 0, - }, + env_array: std.ArrayListUnmanaged(?[*:0]const u8) = .{}, cwd: []const u8, stdio: [3]Stdio = .{ .ignore, @@ -686,10 +683,7 @@ pub const ShellSubprocess = struct { .arena = arena, .override_env = false, - .env_array = .{ - .items = &.{}, - .capacity = 0, - }, + .env_array = .{}, .cwd = event_loop.topLevelDir(), .stdio = .{ .{ .ignore = {} }, @@ -698,7 +692,7 @@ pub const ShellSubprocess = struct { }, .lazy = false, .PATH = event_loop.env().get("PATH") orelse "", - .argv = undefined, + .argv = .{}, .detached = false, // .ipc_mode = IPCMode.none, // .ipc_callback = .zero, @@ -708,6 +702,7 @@ pub const ShellSubprocess = struct { out.stdio[1] = .{ .pipe = {} }; out.stdio[2] = .{ .pipe = {} }; } + return out; } @@ -752,21 +747,19 @@ pub const ShellSubprocess = struct { pub fn spawnAsync( event_loop: JSC.EventLoopHandle, shellio: *ShellIO, - spawn_args_: SpawnArgs, + spawn_args: *SpawnArgs, out: **@This(), ) bun.shell.Result(void) { var arena = @import("root").bun.ArenaAllocator.init(bun.default_allocator); defer arena.deinit(); - var spawn_args = spawn_args_; - _ = switch (spawnMaybeSyncImpl( .{ .is_sync = false, }, event_loop, arena.allocator(), - &spawn_args, + spawn_args, shellio, out, )) { @@ -789,11 +782,36 @@ pub const ShellSubprocess = struct { ) bun.shell.Result(*@This()) { const is_sync = config.is_sync; - if (!spawn_args.override_env and spawn_args.env_array.items.len == 0) { - // spawn_args.env_array.items = jsc_vm.bundler.env.map.createNullDelimitedEnvMap(allocator) catch bun.outOfMemory(); - spawn_args.env_array.items = event_loop.createNullDelimitedEnvMap(allocator) catch bun.outOfMemory(); - spawn_args.env_array.capacity = spawn_args.env_array.items.len; - } + const override_env = !spawn_args.override_env and spawn_args.env_array.items.len == 0; + const env: bun.DotEnv.Map.NullDelimitedEnvMap = brk: { + if (override_env) { + break :brk event_loop.createNullDelimitedEnvMap(allocator) catch { + return .{ .err = .{ + .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory(), + } }; + }; + } else { + // Make sure this is a null-terminated + if (spawn_args.env_array.items.len == 0 or + spawn_args.env_array.getLast() != null) + { + spawn_args.argv.append(allocator, null) catch { + return .{ .err = .{ + .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory(), + } }; + }; + } + + break :brk .{ + .envp = spawn_args.env_array.items[0.. :null], + // undefined is fine because `env.deinit` is only called if the `override_env` path is taken. + .allocation_size = undefined, + }; + } + }; + + // var overridden_env: bun + // defer if (override_env) var should_close_memfd = Environment.isLinux; @@ -845,14 +863,14 @@ pub const ShellSubprocess = struct { return .{ .err = .{ .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory() } }; }; - spawn_args.env_array.append(allocator, null) catch { - return .{ .err = .{ .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory() } }; - }; + // spawn_args.env_array.append(allocator, null) catch { + // return .{ .err = .{ .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory() } }; + // }; var spawn_result = switch (bun.spawn.spawnProcess( &spawn_options, @ptrCast(spawn_args.argv.items.ptr), - @ptrCast(spawn_args.env_array.items.ptr), + env.envp, ) catch |err| { return .{ .err = .{ .custom = std.fmt.allocPrint(bun.default_allocator, "Failed to spawn process: {s}", .{@errorName(err)}) catch bun.outOfMemory() } }; }) {