From 7d673dd7d89d7894357ca19f1ef2bff2cab86bf2 Mon Sep 17 00:00:00 2001 From: Meghan Denny Date: Thu, 11 Apr 2024 20:24:47 -0700 Subject: [PATCH] node:child_process: fix propagation of windowsHide and windowsVerbatimArguments option (#10193) --- packages/bun-types/bun.d.ts | 5 ++ src/bun.js/api/bun/subprocess.zig | 8 +++ src/js/node/child_process.js | 60 ++++++++++++++----- .../child_process/child-process-exec.test.ts | 15 +++++ .../fixtures/child-process-echo-argv.js | 3 + 5 files changed, 77 insertions(+), 14 deletions(-) create mode 100644 test/js/node/child_process/fixtures/child-process-echo-argv.js diff --git a/packages/bun-types/bun.d.ts b/packages/bun-types/bun.d.ts index e2e4800788347..4a8758e58b9b1 100644 --- a/packages/bun-types/bun.d.ts +++ b/packages/bun-types/bun.d.ts @@ -4145,6 +4145,11 @@ declare module "bun" { */ windowsHide?: boolean; + /** + * If true, no quoting or escaping of arguments is done on Windows. + */ + windowsVerbatimArguments?: boolean; + /** * Path to the executable to run in the subprocess. This defaults to `cmds[0]`. * diff --git a/src/bun.js/api/bun/subprocess.zig b/src/bun.js/api/bun/subprocess.zig index 7aba7f26e796c..4a9ae15ad7806 100644 --- a/src/bun.js/api/bun/subprocess.zig +++ b/src/bun.js/api/bun/subprocess.zig @@ -1591,6 +1591,7 @@ pub const Subprocess = struct { var argv0: ?[*:0]const u8 = null; var windows_hide: bool = false; + var windows_verbatim_arguments: bool = false; { if (args.isEmptyOrUndefinedOrNull()) { @@ -1869,6 +1870,12 @@ pub const Subprocess = struct { windows_hide = val.asBoolean(); } } + + if (args.get(globalThis, "windowsVerbatimArguments")) |val| { + if (val.isBoolean()) { + windows_verbatim_arguments = val.asBoolean(); + } + } } } } @@ -1966,6 +1973,7 @@ pub const Subprocess = struct { .windows = if (Environment.isWindows) .{ .hide_window = windows_hide, + .verbatim_arguments = windows_verbatim_arguments, .loop = JSC.EventLoopHandle.init(jsc_vm), } else {}, }; diff --git a/src/js/node/child_process.js b/src/js/node/child_process.js index 96f4b255f1074..c3ddb32a1f46b 100644 --- a/src/js/node/child_process.js +++ b/src/js/node/child_process.js @@ -84,10 +84,6 @@ var ReadableFromWeb; // Section 1. Exported child_process functions //------------------------------------------------------------------------------ -// TODO: Implement these props when Windows is supported -// * windowsVerbatimArguments?: boolean; -// * windowsHide?: boolean; - // Copyright Joyent, Inc. and other Node contributors. // // Permission is hereby granted, free of charge, to any person obtaining a @@ -134,6 +130,8 @@ function spawnTimeoutFunction(child, timeoutHolder) { * gid?: number; * serialization?: string; * shell?: boolean | string; + * windowsHide?: boolean; + * windowsVerbatimArguments?: boolean; * signal?: AbortSignal; * timeout?: number; * killSignal?: string | number; @@ -243,10 +241,14 @@ function execFile(file, args, options, callback) { const child = spawn(file, args, { cwd: options.cwd, env: options.env, - // gid: options.gid, + timeout: options.timeout, + killSignal: options.killSignal, + uid: options.uid, + gid: options.gid, + windowsHide: options.windowsHide, + windowsVerbatimArguments: options.windowsVerbatimArguments, shell: options.shell, signal: options.signal, - // uid: options.uid, }); let encoding; @@ -437,6 +439,7 @@ function execFile(file, args, options, callback) { * uid?: number; * gid?: number; * windowsHide?: boolean; + * windowsVerbatimArguments?: boolean; * }} [options] * @param {( * error?: Error, @@ -496,6 +499,8 @@ Object.defineProperty(exec, Symbol.for("nodejs.util.promisify.custom"), { * maxBuffer?: number; * encoding?: string; * shell?: boolean | string; + * windowsHide?: boolean; + * windowsVerbatimArguments?: boolean; * }} [options] * @returns {{ * pid: number; @@ -551,6 +556,8 @@ function spawnSync(file, args, options) { env: options.env || undefined, cwd: options.cwd || undefined, stdio: bunStdio, + windowsVerbatimArguments: options.windowsVerbatimArguments, + windowsHide: options.windowsHide, }); const result = { @@ -866,12 +873,19 @@ function normalizeSpawnArguments(file, args, options) { cwd = getValidatedPath(cwd, "options.cwd"); } - // TODO: Gid check - // TODO: Uid check - var detached = false; - const { detached: detachedOption } = options; - if (detachedOption != null) { - detached = !!detachedOption; + // Validate detached, if present. + if (options.detached != null) { + validateBoolean(options.detached, "options.detached"); + } + + // Validate the uid, if present. + if (options.uid != null && !isInt32(options.uid)) { + throw new ERR_INVALID_ARG_TYPE("options.uid", "int32", options.uid); + } + + // Validate the gid, if present. + if (options.gid != null && !isInt32(options.gid)) { + throw new ERR_INVALID_ARG_TYPE("options.gid", "int32", options.gid); } // Validate the shell, if present. @@ -885,6 +899,11 @@ function normalizeSpawnArguments(file, args, options) { validateArgumentNullCheck(options.argv0, "options.argv0"); } + // Validate windowsHide, if present. + if (options.windowsHide != null) { + validateBoolean(options.windowsHide, "options.windowsHide"); + } + let { windowsVerbatimArguments } = options; if (windowsVerbatimArguments != null) { validateBoolean(windowsVerbatimArguments, "options.windowsVerbatimArguments"); @@ -900,7 +919,7 @@ function normalizeSpawnArguments(file, args, options) { else file = process.env.comspec || "cmd.exe"; // '/d /s /c' is used only for cmd.exe. if (/^(?:.*\\)?cmd(?:\.exe)?$/i.exec(file) !== null) { - args = ["/d", "/s", "/c", command]; + args = ["/d", "/s", "/c", `"${command}"`]; windowsVerbatimArguments = true; } else { args = ["-c", command]; @@ -929,7 +948,18 @@ function normalizeSpawnArguments(file, args, options) { // TODO: Windows env support here... - return { ...options, detached, file, args, cwd, envPairs }; + return { + // Make a shallow copy so we don't clobber the user's options object. + __proto__: null, + ...options, + args, + cwd, + detached: !!options.detached, + envPairs, + file, + windowsHide: !!options.windowsHide, + windowsVerbatimArguments: !!windowsVerbatimArguments, + }; } function checkExecSyncError(ret, args, cmd) { @@ -1205,6 +1235,8 @@ class ChildProcess extends EventEmitter { ipc: ipc ? this.#emitIpcMessage.bind(this) : undefined, serialization, argv0, + windowsHide: !!options.windowsHide, + windowsVerbatimArguments: !!options.windowsVerbatimArguments, }); this.pid = this.#handle.pid; diff --git a/test/js/node/child_process/child-process-exec.test.ts b/test/js/node/child_process/child-process-exec.test.ts index bc25dfdcb3aac..d06cd3f165f14 100644 --- a/test/js/node/child_process/child-process-exec.test.ts +++ b/test/js/node/child_process/child-process-exec.test.ts @@ -1,4 +1,5 @@ import { test, expect, describe } from "bun:test"; +import { bunExe } from "harness"; import { exec } from "node:child_process"; // https://github.com/oven-sh/bun/issues/5319 @@ -89,3 +90,17 @@ describe("child_process.exec", () => { }); }); }); + +test("exec with verbatim arguments", async () => { + const { resolve, reject, promise } = Promise.withResolvers(); + + const fixture = require.resolve("./fixtures/child-process-echo-argv.js"); + const child = exec(`${bunExe()} ${fixture} tasklist /FI "IMAGENAME eq chrome.exe"`, (err, stdout, stderr) => { + if (err) return reject(err); + return resolve({ stdout, stderr }); + }); + expect(!!child).toBe(true); + + const { stdout } = await promise; + expect(stdout.trim().split("\n")).toEqual([`tasklist`, `/FI`, `IMAGENAME eq chrome.exe`]); +}); diff --git a/test/js/node/child_process/fixtures/child-process-echo-argv.js b/test/js/node/child_process/fixtures/child-process-echo-argv.js new file mode 100644 index 0000000000000..fc1a53f2ea6a1 --- /dev/null +++ b/test/js/node/child_process/fixtures/child-process-echo-argv.js @@ -0,0 +1,3 @@ +for (const item of process.argv.slice(2)) { + console.log(item); +}