From 80bd3254cc4feb6be716cd89d1fa48a004338388 Mon Sep 17 00:00:00 2001 From: Georgijs <48869301+gvilums@users.noreply.github.com> Date: Wed, 24 Jan 2024 20:54:01 -0800 Subject: [PATCH] fix argv parsing on windows (#8458) * fix argv parsing on windows * directly use zig stdlib * remove debug comments, fix double deinit * change bun.argv() to return slices, not null-terminated pointers * fix test on windows to escape file paths correctly --------- Co-authored-by: Jarred Sumner --- src/bun.js/node/types.zig | 5 ++- src/bun.zig | 33 ++++++++----------- src/cli.zig | 22 +++++-------- src/cli/bunx_command.zig | 6 ++-- src/cli/init_command.zig | 5 ++- src/cli/install_completions_command.zig | 4 +-- src/cli/run_command.zig | 4 +-- src/cli/upgrade_command.zig | 6 ++-- src/deps/zig-clap/clap/args.zig | 4 +-- src/main.zig | 4 +++ src/standalone_bun.zig | 13 ++++---- .../js/bun/resolve/non-english-import.test.js | 2 +- 12 files changed, 49 insertions(+), 59 deletions(-) diff --git a/src/bun.js/node/types.zig b/src/bun.js/node/types.zig index 60c9653257..0ff9706e3d 100644 --- a/src/bun.js/node/types.zig +++ b/src/bun.js/node/types.zig @@ -2414,7 +2414,7 @@ pub const Path = struct { pub const Process = struct { pub fn getArgv0(globalObject: *JSC.JSGlobalObject) callconv(.C) JSC.JSValue { - return JSC.ZigString.fromUTF8(bun.span(bun.argv()[0])).toValueGC(globalObject); + return JSC.ZigString.fromUTF8(bun.argv()[0]).toValueGC(globalObject); } pub fn getExecPath(globalObject: *JSC.JSGlobalObject) callconv(.C) JSC.JSValue { @@ -2452,8 +2452,7 @@ pub const Process = struct { var used: usize = 0; const offset: usize = 1; - for (bun.argv()[@min(bun.argv().len, offset)..]) |arg_| { - const arg = bun.span(arg_); + for (bun.argv()[@min(bun.argv().len, offset)..]) |arg| { if (arg.len == 0) continue; diff --git a/src/bun.zig b/src/bun.zig index 2548e3c7c6..b67ca73bb8 100644 --- a/src/bun.zig +++ b/src/bun.zig @@ -1374,7 +1374,7 @@ pub fn reloadProcess( const bun = @This(); const dupe_argv = allocator.allocSentinel(?[*:0]const u8, bun.argv().len, null) catch unreachable; for (bun.argv(), dupe_argv) |src, *dest| { - dest.* = (allocator.dupeZ(u8, sliceTo(src, 0)) catch unreachable).ptr; + dest.* = (allocator.dupeZ(u8, src) catch unreachable).ptr; } const environ_slice = std.mem.span(std.c.environ); @@ -1391,7 +1391,7 @@ pub fn reloadProcess( const exec_path = (allocator.dupeZ(u8, std.fs.selfExePathAlloc(allocator) catch unreachable) catch unreachable).ptr; // we clone argv so that the memory address isn't the same as the libc one - const argv = @as([*:null]?[*:0]const u8, @ptrCast(dupe_argv.ptr)); + const newargv = @as([*:null]?[*:0]const u8, @ptrCast(dupe_argv.ptr)); // we clone envp so that the memory address of environment variables isn't the same as the libc one const envp = @as([*:null]?[*:0]const u8, @ptrCast(environ.ptr)); @@ -1419,7 +1419,7 @@ pub fn reloadProcess( C.POSIX_SPAWN_SETEXEC | C.POSIX_SPAWN_SETSIGDEF | C.POSIX_SPAWN_SETSIGMASK, ) catch unreachable; - switch (PosixSpawn.spawnZ(exec_path, actions, attrs, @as([*:null]?[*:0]const u8, @ptrCast(argv)), @as([*:null]?[*:0]const u8, @ptrCast(envp)))) { + switch (PosixSpawn.spawnZ(exec_path, actions, attrs, @as([*:null]?[*:0]const u8, @ptrCast(newargv)), @as([*:null]?[*:0]const u8, @ptrCast(envp)))) { .err => |err| { Output.panic("Unexpected error while reloading: {d} {s}", .{ err.errno, @tagName(err.getErrno()) }); }, @@ -1428,7 +1428,7 @@ pub fn reloadProcess( } else { const err = std.os.execveZ( exec_path, - argv, + newargv, envp, ); Output.panic("Unexpected error while reloading: {s}", .{@errorName(err)}); @@ -1811,18 +1811,21 @@ const WindowsStat = extern struct { pub const Stat = if (Environment.isWindows) windows.libuv.uv_stat_t else std.os.Stat; +var _argv: [][:0]u8 = &[_][:0]u8{}; + +pub inline fn argv() [][:0]u8 { + return _argv; +} + +pub fn initArgv(allocator: std.mem.Allocator) !void { + _argv = try std.process.argsAlloc(allocator); +} + pub const posix = struct { pub const STDIN_FD = toFD(0); pub const STDOUT_FD = toFD(1); pub const STDERR_FD = toFD(2); - pub inline fn argv() [][*:0]u8 { - return std.os.argv; - } - pub inline fn setArgv(new_ptr: [][*:0]u8) void { - std.os.argv = new_ptr; - } - pub fn stdio(i: anytype) FileDescriptor { return switch (i) { 1 => STDOUT_FD, @@ -1840,14 +1843,6 @@ pub const win32 = struct { pub var STDERR_FD: FileDescriptor = undefined; pub var STDIN_FD: FileDescriptor = undefined; - pub inline fn argv() [][*:0]u8 { - return std.os.argv; - } - - pub inline fn setArgv(new_ptr: [][*:0]u8) void { - std.os.argv = new_ptr; - } - pub fn stdio(i: anytype) FileDescriptor { return switch (i) { 0 => STDIN_FD, diff --git a/src/cli.zig b/src/cli.zig index 72f7174d65..ecb1cbed3a 100644 --- a/src/cli.zig +++ b/src/cli.zig @@ -1131,7 +1131,7 @@ pub const Command = struct { // std.process.args allocates! const ArgsIterator = struct { - buf: [][*:0]u8 = undefined, + buf: [][:0]u8 = undefined, i: u32 = 0, pub fn next(this: *ArgsIterator) ?[]const u8 { @@ -1140,7 +1140,7 @@ pub const Command = struct { } const i = this.i; this.i += 1; - return std.mem.span(this.buf[i]); + return this.buf[i]; } pub fn skip(this: *ArgsIterator) bool { @@ -1197,8 +1197,7 @@ pub const Command = struct { RootCommandMatcher.case("install"), => brk: { for (args_iter.buf) |arg| { - const span = std.mem.span(arg); - if (span.len > 0 and (strings.eqlComptime(span, "-g") or strings.eqlComptime(span, "--global"))) { + if (arg.len > 0 and (strings.eqlComptime(arg, "-g") or strings.eqlComptime(arg, "--global"))) { break :brk .AddCommand; } } @@ -1292,13 +1291,11 @@ pub const Command = struct { }; ctx.args.target = Api.Target.bun; - const argv = try bun.default_allocator.alloc(string, bun.argv().len -| 1); if (bun.argv().len > 1) { - for (argv, bun.argv()[1..]) |*dest, src| { - dest.* = bun.span(src); - } + ctx.passthrough = bun.argv()[1..]; + } else { + ctx.passthrough = &[_]string{}; } - ctx.passthrough = argv; try @import("./bun_js.zig").Run.bootStandalone( ctx, @@ -1510,8 +1507,7 @@ pub const Command = struct { // iterate over args // if --help, print help and exit const print_help = brk: { - for (bun.argv()) |arg_| { - const arg = bun.span(arg_); + for (bun.argv()) |arg| { if (strings.eqlComptime(arg, "--help")) { break :brk true; } @@ -1595,7 +1591,7 @@ pub const Command = struct { example_tag != CreateCommandExample.Tag.local_folder; if (use_bunx) { - const bunx_args = try allocator.alloc([*:0]const u8, args.len - template_name_start); + const bunx_args = try allocator.alloc([:0]const u8, args.len - template_name_start); bunx_args[0] = try BunxCommand.addCreatePrefix(allocator, template_name); for (bunx_args[1..], args[template_name_start + 1 ..]) |*dest, src| { dest.* = src; @@ -1663,7 +1659,7 @@ pub const Command = struct { if (ctx.args.entry_points.len == 1) { if (strings.eqlComptime(extension, ".lockb")) { for (bun.argv()) |arg| { - if (strings.eqlComptime(std.mem.span(arg), "--hash")) { + if (strings.eqlComptime(arg, "--hash")) { try PackageManagerCommand.printHash(ctx, ctx.args.entry_points[0]); return; } diff --git a/src/cli/bunx_command.zig b/src/cli/bunx_command.zig index 73e788dac0..242265d964 100644 --- a/src/cli/bunx_command.zig +++ b/src/cli/bunx_command.zig @@ -162,7 +162,7 @@ pub const BunxCommand = struct { Global.exit(1); } - pub fn exec(ctx_: bun.CLI.Command.Context, argv: [][*:0]const u8) !void { + pub fn exec(ctx_: bun.CLI.Command.Context, argv: [][:0]const u8) !void { var ctx = ctx_; var requests_buf = bun.PackageManager.UpdateRequest.Array.init(0) catch unreachable; var run_in_bun = ctx.debug.run_in_bun; @@ -172,9 +172,7 @@ pub const BunxCommand = struct { { var found_subcommand_name = false; - for (argv) |positional_| { - const positional = bun.span(positional_); - + for (argv) |positional| { if (positional.len == 0) continue; if (positional[0] != '-') { diff --git a/src/cli/init_command.zig b/src/cli/init_command.zig index f66017ab65..a74044f986 100644 --- a/src/cli/init_command.zig +++ b/src/cli/init_command.zig @@ -89,10 +89,9 @@ pub const InitCommand = struct { entry_point: string = "", }; - pub fn exec(alloc: std.mem.Allocator, argv: [][*:0]u8) !void { + pub fn exec(alloc: std.mem.Allocator, argv: [][:0]u8) !void { const print_help = brk: { - for (argv) |arg_| { - const arg = bun.span(arg_); + for (argv) |arg| { if (strings.eqlComptime(arg, "--help")) { break :brk true; } diff --git a/src/cli/install_completions_command.zig b/src/cli/install_completions_command.zig index 5da411693b..b2cf708249 100644 --- a/src/cli/install_completions_command.zig +++ b/src/cli/install_completions_command.zig @@ -138,9 +138,9 @@ pub const InstallCompletionsCommand = struct { var completions_dir: string = ""; var output_dir: std.fs.Dir = found: { for (bun.argv(), 0..) |arg, i| { - if (strings.eqlComptime(std.mem.span(arg), "completions")) { + if (strings.eqlComptime(arg, "completions")) { if (bun.argv().len > i + 1) { - const input = std.mem.span(bun.argv()[i + 1]); + const input = bun.argv()[i + 1]; if (!std.fs.path.isAbsolute(input)) { completions_dir = resolve_path.joinAbs( diff --git a/src/cli/run_command.zig b/src/cli/run_command.zig index 2a933cc318..b757409252 100644 --- a/src/cli/run_command.zig +++ b/src/cli/run_command.zig @@ -510,7 +510,7 @@ pub const RunCommand = struct { // if we are already an absolute path, use that // if the user started the application via a shebang, it's likely that the path is absolute already if (bun.argv()[0][0] == '/') { - optional_bun_path.* = bun.span(bun.argv()[0]); + optional_bun_path.* = bun.argv()[0]; argv0 = bun.argv()[0]; } else if (optional_bun_path.len == 0) { // otherwise, ask the OS for the absolute path @@ -1176,7 +1176,7 @@ pub const RunCommand = struct { shebang = std.mem.trim(u8, shebang, " \r\n\t"); if (strings.hasPrefixComptime(shebang, "#!")) { - const first_arg: string = if (bun.argv().len > 0) bun.span(bun.argv()[0]) else ""; + const first_arg: string = if (bun.argv().len > 0) bun.argv()[0] else ""; const filename = std.fs.path.basename(first_arg); // are we attempting to run the script with bun? if (!strings.contains(shebang, filename)) { diff --git a/src/cli/upgrade_command.zig b/src/cli/upgrade_command.zig index 58e695d056..05c7de16df 100644 --- a/src/cli/upgrade_command.zig +++ b/src/cli/upgrade_command.zig @@ -438,14 +438,14 @@ pub const UpgradeCommand = struct { const use_canary = brk: { const default_use_canary = Environment.is_canary; - if (default_use_canary and strings.containsAny(bun.span(bun.argv()), "--stable")) + if (default_use_canary and strings.containsAny(bun.argv(), "--stable")) break :brk false; break :brk strings.eqlComptime(env_loader.map.get("BUN_CANARY") orelse "0", "1") or - strings.containsAny(bun.span(bun.argv()), "--canary") or default_use_canary; + strings.containsAny(bun.argv(), "--canary") or default_use_canary; }; - const use_profile = strings.containsAny(bun.span(bun.argv()), "--profile"); + const use_profile = strings.containsAny(bun.argv(), "--profile"); if (!use_canary) { var refresher = std.Progress{}; diff --git a/src/deps/zig-clap/clap/args.zig b/src/deps/zig-clap/clap/args.zig index 045ae95408..422264e5e3 100644 --- a/src/deps/zig-clap/clap/args.zig +++ b/src/deps/zig-clap/clap/args.zig @@ -50,7 +50,7 @@ pub const OsIterator = struct { const Error = process.ArgIterator.InitError; arena: @import("root").bun.ArenaAllocator, - remain: [][*:0]u8, + remain: [][:0]u8, /// The executable path (this is the first argument passed to the program) /// TODO: Is it the right choice for this to be null? Maybe `init` should @@ -73,7 +73,7 @@ pub const OsIterator = struct { pub fn next(iter: *OsIterator) ?[:0]const u8 { if (iter.remain.len > 0) { - const res = bun.sliceTo(iter.remain[0], 0); + const res = iter.remain[0]; iter.remain = iter.remain[1..]; return res; } diff --git a/src/main.zig b/src/main.zig index 48b3a90237..a032a45bc0 100644 --- a/src/main.zig +++ b/src/main.zig @@ -26,6 +26,10 @@ pub fn main() void { const Output = bun.Output; const Environment = bun.Environment; + bun.initArgv(bun.default_allocator) catch |err| { + Output.panic("Failed to initialize argv: {s}\n", .{@errorName(err)}); + }; + if (Environment.isRelease and Environment.isPosix) CrashReporter.start() catch unreachable; if (Environment.isWindows) { diff --git a/src/standalone_bun.zig b/src/standalone_bun.zig index 3a6db42b7c..c18772e0de 100644 --- a/src/standalone_bun.zig +++ b/src/standalone_bun.zig @@ -576,11 +576,10 @@ pub const StandaloneModuleGraph = struct { // and also just makes sense. const argv = bun.argv(); if (argv.len > 0) { - const argv0_len = bun.len(argv[0]); - if (argv0_len > 0) { - const argv0 = argv[0][0..argv0_len]; - - if (argv0_len == 3) { + // const argv0_len = bun.len(argv[0]); + const argv0 = argv[0]; + if (argv0.len > 0) { + if (argv0.len == 3) { if (bun.strings.eqlComptimeIgnoreLen(argv0, "bun")) { return null; } @@ -592,7 +591,7 @@ pub const StandaloneModuleGraph = struct { } } - if (argv0_len == 4) { + if (argv0.len == 4) { if (bun.strings.eqlComptimeIgnoreLen(argv0, "bunx")) { return null; } @@ -617,7 +616,7 @@ pub const StandaloneModuleGraph = struct { &whichbuf, bun.getenvZ("PATH") orelse return error.FileNotFound, "", - bun.span(bun.argv()[0]), + bun.argv()[0], )) |path| { return bun.toFD((try std.fs.cwd().openFileZ(path, flags)).handle); } diff --git a/test/js/bun/resolve/non-english-import.test.js b/test/js/bun/resolve/non-english-import.test.js index 944218b36a..8b588eff3c 100644 --- a/test/js/bun/resolve/non-english-import.test.js +++ b/test/js/bun/resolve/non-english-import.test.js @@ -76,7 +76,7 @@ test("latin1 & utf16 imports", async () => { mkdirSync(prefix, { recursive: true }); } catch (e) {} - const entryCode = imports.map(i => `import "${i}";`).join("\n"); + let entryCode = imports.map(i => `import ${JSON.stringify(i)};`).join("\n"); await Bun.write(inputPath, entryCode); for (let importPath of imports) {