make it possible to deinit createNullDelimitedEnvMap

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 .. argv.items.len - 1 :null].ptr),
+ @ptrCast(env_array.items[0 .. env_array.items.len - 1 :null].ptr),
  ) catch |err| {
  process_allocator.destroy(subprocess);
  spawn_options.deinit();

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),

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,

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);

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.. list.items.len - 1 :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);
  }

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);
  }
 };
 

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,
  });

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);