Skip to content

Commit

Permalink
node:child_process: fix propagation of windowsHide and windowsVerbati…
Browse files Browse the repository at this point in the history
…mArguments option (#10193)
  • Loading branch information
nektro authored Apr 12, 2024
1 parent ca98138 commit 7d673dd
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 14 deletions.
5 changes: 5 additions & 0 deletions packages/bun-types/bun.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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]`.
*
Expand Down
8 changes: 8 additions & 0 deletions src/bun.js/api/bun/subprocess.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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();
}
}
}
}
}
Expand Down Expand Up @@ -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 {},
};
Expand Down
60 changes: 46 additions & 14 deletions src/js/node/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -437,6 +439,7 @@ function execFile(file, args, options, callback) {
* uid?: number;
* gid?: number;
* windowsHide?: boolean;
* windowsVerbatimArguments?: boolean;
* }} [options]
* @param {(
* error?: Error,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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.
Expand All @@ -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");
Expand All @@ -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];
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;

Expand Down
15 changes: 15 additions & 0 deletions test/js/node/child_process/child-process-exec.test.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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`]);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
for (const item of process.argv.slice(2)) {
console.log(item);
}

0 comments on commit 7d673dd

Please sign in to comment.