Skip to content

Commit 16b6b4b

Browse files
robobunrobobun
committed
child_process: match node's option validation for uid, gid, timeout, maxBuffer and windowsHide
normalizeSpawnArguments, validateTimeout and validateMaxBuffer carried the checks that node removed in nodejs/node#59416 (v25.0.0). Each check built one error for every bad value, so the error did not match node 26: - uid and gid: NaN, a fraction, Infinity or a value outside int32 threw ERR_INVALID_ARG_TYPE. A BigInt or a Symbol threw a TypeError with no code. - timeout and maxBuffer: a value that is not a number threw ERR_OUT_OF_RANGE. The three checks now call validateInt32, validateInteger and validateNumber from internal/validators, as node does. The error class and code now match node. The message matches too, except for the "Received ..." text that the shared error formatter prints for some unusual values. Two more differences on the same path: - execFile() now passes `!!options.windowsHide` and `!!options.windowsVerbatimArguments` to spawn(), as node does. exec() and execFile() threw for a value that is not a boolean. Node accepts it. - A null byte in `cwd` or in the module path of fork() now gives node's text: "must be a string, Uint8Array, or URL without null bytes". The four node tests that pin the uid, gid, timeout and maxBuffer errors are replaced with their v26.3.0 versions.
1 parent 6c1eb06 commit 16b6b4b

6 files changed

Lines changed: 244 additions & 58 deletions

‎src/js/node/child_process.ts‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ const { kHandle } = require("internal/shared");
44
const {
55
validateBoolean,
66
validateFunction,
7+
validateInteger,
8+
validateInt32,
9+
validateNumber,
710
validateString,
811
validateAbortSignal,
912
validateArray,
@@ -37,7 +40,6 @@ const ArrayPrototypeSplice = Array.prototype.splice;
3740

3841
var ArrayBufferIsView = ArrayBuffer.isView;
3942

40-
var NumberIsInteger = Number.isInteger;
4143
var ObjectHasOwn = Object.hasOwn;
4244
var StringPrototypeIncludes = String.prototype.includes;
4345
var StringPrototypeStartsWith = String.prototype.startsWith;
@@ -267,8 +269,8 @@ function execFile(file, args, options?, callback?) {
267269
uid: options.uid,
268270
gid: options.gid,
269271
cgroup: options.cgroup,
270-
windowsHide: options.windowsHide,
271-
windowsVerbatimArguments: options.windowsVerbatimArguments,
272+
windowsHide: !!options.windowsHide,
273+
windowsVerbatimArguments: !!options.windowsVerbatimArguments,
272274
shell: options.shell,
273275
signal: options.signal,
274276
});
@@ -986,14 +988,14 @@ function normalizeSpawnArguments(file, args, options?) {
986988

987989
// Validate the uid, if present.
988990
const uid = options.uid;
989-
if (uid != null && !isInt32(uid)) {
990-
throw $ERR_INVALID_ARG_TYPE("options.uid", "int32", uid);
991+
if (uid != null) {
992+
validateInt32(uid, "options.uid");
991993
}
992994

993995
// Validate the gid, if present.
994996
const gid = options.gid;
995-
if (gid != null && !isInt32(gid)) {
996-
throw $ERR_INVALID_ARG_TYPE("options.gid", "int32", gid);
997+
if (gid != null) {
998+
validateInt32(gid, "options.gid");
997999
}
9981000

9991001
// Validate the shell, if present.
@@ -1932,8 +1934,8 @@ class Control extends EventEmitter {
19321934
//------------------------------------------------------------------------------
19331935

19341936
function validateMaxBuffer(maxBuffer) {
1935-
if (maxBuffer != null && !(typeof maxBuffer === "number" && maxBuffer >= 0)) {
1936-
throw $ERR_OUT_OF_RANGE("options.maxBuffer", "a positive number", maxBuffer);
1937+
if (maxBuffer != null) {
1938+
validateNumber(maxBuffer, "options.maxBuffer", 0);
19371939
}
19381940
}
19391941

@@ -1950,15 +1952,11 @@ function validateArgumentsNullCheck(args, propName) {
19501952
}
19511953

19521954
function validateTimeout(timeout) {
1953-
if (timeout != null && !(NumberIsInteger(timeout) && timeout >= 0)) {
1954-
throw $ERR_OUT_OF_RANGE("timeout", "an unsigned integer", timeout);
1955+
if (timeout != null) {
1956+
validateInteger(timeout, "timeout", 0);
19551957
}
19561958
}
19571959

1958-
function isInt32(value) {
1959-
return value === (value | 0);
1960-
}
1961-
19621960
function nullCheck(path, propName, throwError = true) {
19631961
const pathIsString = typeof path === "string";
19641962
const pathIsUint8Array = isUint8Array(path);
@@ -1972,7 +1970,7 @@ function nullCheck(path, propName, throwError = true) {
19721970
return;
19731971
}
19741972

1975-
const err = $ERR_INVALID_ARG_VALUE(propName, path, "must be a string or Uint8Array without null bytes");
1973+
const err = $ERR_INVALID_ARG_VALUE(propName, path, "must be a string, Uint8Array, or URL without null bytes");
19761974
if (throwError) {
19771975
throw err;
19781976
}

‎test/js/node/child_process/child_process.test.ts‎

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1320,6 +1320,73 @@ done
13201320
expect(asyncOut.trim().split("\n")).toEqual(expected);
13211321
});
13221322

1323+
// node:child_process validates its options as node 26.3.0 does. The errors below are node's.
1324+
type ValidationError = { name: string; code: string; message: string };
1325+
function invalidArgType(name: string, type: string, received: string): ValidationError {
1326+
return {
1327+
name: "TypeError",
1328+
code: "ERR_INVALID_ARG_TYPE",
1329+
message: `The "${name}" ${name.includes(".") ? "property" : "argument"} must be of type ${type}. Received ${received}`,
1330+
};
1331+
}
1332+
function outOfRange(name: string, range: string, received: string): ValidationError {
1333+
return {
1334+
name: "RangeError",
1335+
code: "ERR_OUT_OF_RANGE",
1336+
message: `The value of "${name}" is out of range. It must be ${range}. Received ${received}`,
1337+
};
1338+
}
1339+
const notNumbers: [value: unknown, received: string][] = [
1340+
["0", "type string ('0')"],
1341+
[true, "type boolean (true)"],
1342+
[{}, "an instance of Object"],
1343+
[[], "an instance of Array"],
1344+
[0n, "type bigint (0n)"],
1345+
[Symbol("s"), "type symbol (Symbol(s))"],
1346+
[function fn() {}, "function fn"],
1347+
];
1348+
1349+
// Every function export validates its options before it starts a process.
1350+
const functionExports: Record<string, (options: any) => unknown> = {
1351+
spawn: options => spawn(bunExe(), ["-e", "0"], options),
1352+
spawnSync: options => spawnSync(bunExe(), ["-e", "0"], options),
1353+
execFile: options => execFile(bunExe(), ["-e", "0"], options, () => {}),
1354+
execFileSync: options => execFileSync(bunExe(), ["-e", "0"], options),
1355+
exec: options => exec("exit 0", options, () => {}),
1356+
execSync: options => execSync("exit 0", options),
1357+
fork: options => fork(path.join(import.meta.dir, "fixtures", "child-process-echo-argv.js"), options),
1358+
};
1359+
1360+
/** The error that `call()` throws. A child that got past the validation is killed. */
1361+
function thrown(call: () => unknown): ValidationError | undefined {
1362+
try {
1363+
const child = call();
1364+
if (child instanceof ChildProcess) {
1365+
child.on("error", () => {});
1366+
child.kill();
1367+
}
1368+
} catch (e: any) {
1369+
return { name: e.name, code: e.code, message: e.message };
1370+
}
1371+
}
1372+
function thrownBy(fn: string, options: object) {
1373+
return thrown(() => functionExports[fn](options));
1374+
}
1375+
1376+
/** Expects `fn` to throw `error` for each `[value, error]` row, with `value` as the option. */
1377+
function expectErrors(fn: string, option: string, rows: [value: unknown, error: ValidationError][]) {
1378+
expect(rows.map(([value]) => thrownBy(fn, { [option]: value }))).toEqual(rows.map(([, error]) => error));
1379+
}
1380+
1381+
/** Expects each value to pass the validation: spawnSync() then gets as far as the missing command. */
1382+
function expectAccepted(option: string, values: unknown[]) {
1383+
const codes = values.map(value => {
1384+
const { error } = spawnSync("bun-test-no-such-command", { [option]: value });
1385+
return (error as NodeJS.ErrnoException | undefined)?.code;
1386+
});
1387+
expect(codes).toEqual(values.map(() => "ENOENT"));
1388+
}
1389+
13231390
describe("uid/gid options", () => {
13241391
const isRoot = process.getuid?.() === 0;
13251392
// 65534 is "nobody" on every Linux distro and on macOS.
@@ -1418,6 +1485,125 @@ console.log(JSON.stringify({ uid: process.getuid(), threwCode: thrown?.code, thr
14181485
const r = spawnSync("cmd.exe", ["/c", "exit 0"], { gid: 0 });
14191486
expect((r.error as NodeJS.ErrnoException | undefined)?.code).toBe("ENOTSUP");
14201487
});
1488+
1489+
// The check runs before a process exists, so it needs no privilege and is the same on every platform.
1490+
describe.each(["uid", "gid"])("options.%s", key => {
1491+
const name = `options.${key}`;
1492+
const int32 = ">= -2147483648 && <= 2147483647";
1493+
const rejected: [value: unknown, error: ValidationError][] = [
1494+
[NaN, outOfRange(name, "an integer", "NaN")],
1495+
[1.5, outOfRange(name, "an integer", "1.5")],
1496+
[Infinity, outOfRange(name, "an integer", "Infinity")],
1497+
[-Infinity, outOfRange(name, "an integer", "-Infinity")],
1498+
[2 ** 31, outOfRange(name, int32, "2147483648")],
1499+
[2 ** 32, outOfRange(name, int32, "4294967296")],
1500+
[-(2 ** 31) - 1, outOfRange(name, int32, "-2147483649")],
1501+
[2 ** 63, outOfRange(name, int32, "9_223_372_036_854_776_000")],
1502+
...notNumbers.map(([value, received]): [unknown, ValidationError] => [
1503+
value,
1504+
invalidArgType(name, "number", received),
1505+
]),
1506+
];
1507+
1508+
it.each(Object.keys(functionExports))("%s() throws node's error for a value that is not an int32", fn => {
1509+
expectErrors(fn, key, rejected);
1510+
});
1511+
1512+
it("is not converted to a number, and every int32, null and undefined pass", () => {
1513+
let conversions = 0;
1514+
const object = { valueOf: () => (conversions++, 0) };
1515+
expect(thrownBy("spawnSync", { [key]: object })).toEqual(invalidArgType(name, "number", "an instance of Object"));
1516+
expect(conversions).toBe(0);
1517+
1518+
expectAccepted(key, [-(2 ** 31), -1, -0, 0, 2 ** 31 - 1, null, undefined]);
1519+
1520+
// node's validateInt32 accepts -0 too. node then rejects it in C++ (spawnSync throws, spawn aborts).
1521+
// Bun uses id 0.
1522+
const run = (value: number) => {
1523+
const command = isWindows ? "cmd.exe" : "id";
1524+
const args = isWindows ? ["/c", "exit 0"] : [key === "uid" ? "-u" : "-g"];
1525+
const { status, stdout, error } = spawnSync(command, args, { [key]: value, encoding: "utf8" });
1526+
return { status, stdout, error: (error as NodeJS.ErrnoException | undefined)?.code };
1527+
};
1528+
expect(run(-0)).toEqual(run(0));
1529+
});
1530+
});
1531+
});
1532+
1533+
describe("timeout, maxBuffer, windowsHide and cwd options", () => {
1534+
const safeInteger = ">= 0 && <= 9007199254740991";
1535+
const timeout: [value: unknown, error: ValidationError][] = [
1536+
[NaN, outOfRange("timeout", "an integer", "NaN")],
1537+
[1.5, outOfRange("timeout", "an integer", "1.5")],
1538+
[Infinity, outOfRange("timeout", "an integer", "Infinity")],
1539+
[-Infinity, outOfRange("timeout", "an integer", "-Infinity")],
1540+
[-1, outOfRange("timeout", safeInteger, "-1")],
1541+
[2 ** 53, outOfRange("timeout", safeInteger, "9_007_199_254_740_992")],
1542+
...notNumbers.map(([value, received]): [unknown, ValidationError] => [
1543+
value,
1544+
invalidArgType("timeout", "number", received),
1545+
]),
1546+
];
1547+
const maxBuffer: [value: unknown, error: ValidationError][] = [
1548+
[NaN, outOfRange("options.maxBuffer", ">= 0", "NaN")],
1549+
[-1, outOfRange("options.maxBuffer", ">= 0", "-1")],
1550+
[-Infinity, outOfRange("options.maxBuffer", ">= 0", "-Infinity")],
1551+
...notNumbers.map(([value, received]): [unknown, ValidationError] => [
1552+
value,
1553+
invalidArgType("options.maxBuffer", "number", received),
1554+
]),
1555+
];
1556+
1557+
it.each(Object.keys(functionExports))("%s() throws node's error for an invalid timeout", fn => {
1558+
expectErrors(fn, "timeout", timeout);
1559+
});
1560+
1561+
// spawn() and fork() have no maxBuffer.
1562+
it.each(["spawnSync", "execFile", "execFileSync", "exec", "execSync"])(
1563+
"%s() throws node's error for an invalid maxBuffer",
1564+
fn => {
1565+
expectErrors(fn, "maxBuffer", maxBuffer);
1566+
},
1567+
);
1568+
1569+
it("accepts every integer timeout from 0 to 2 ** 53 - 1, and spawnSync() throws above that", () => {
1570+
expectAccepted("timeout", [0, -0, 1, 2 ** 31 - 1, 2 ** 31, 2 ** 53 - 1, null, undefined]);
1571+
// Bun.spawnSync rejected this value, and spawnSync() returned that as `result.error`. node throws.
1572+
expect(thrownBy("spawnSync", { timeout: 2 ** 53 })).toEqual(
1573+
outOfRange("timeout", safeInteger, "9_007_199_254_740_992"),
1574+
);
1575+
});
1576+
1577+
it("reports the same option as node when two options are invalid", () => {
1578+
const options = { uid: NaN, timeout: "x" };
1579+
// execFile() validates the timeout before spawn() validates the uid. spawnSync() validates the uid first.
1580+
expect(thrownBy("execFile", options)).toEqual(invalidArgType("timeout", "number", "type string ('x')"));
1581+
expect(thrownBy("spawnSync", options)).toEqual(outOfRange("options.uid", "an integer", "NaN"));
1582+
});
1583+
1584+
it("exec() and execFile() coerce windowsHide and windowsVerbatimArguments, the other functions validate them", async () => {
1585+
const options: any = { windowsHide: "yes", windowsVerbatimArguments: 1 };
1586+
// node's execFile() passes `!!options.windowsHide` to spawn(), so these two run the child.
1587+
expect(await promisify(execFile)(bunExe(), ["-e", "0"], options)).toEqual({ stdout: "", stderr: "" });
1588+
expect(await promisify(exec)("exit 0", options)).toEqual({ stdout: "", stderr: "" });
1589+
1590+
for (const fn of ["spawn", "spawnSync", "execFileSync", "execSync", "fork"]) {
1591+
expect(thrownBy(fn, options)).toEqual(invalidArgType("options.windowsHide", "boolean", "type string ('yes')"));
1592+
}
1593+
});
1594+
1595+
it("rejects a null byte in cwd and in the module path of fork() with node's text", () => {
1596+
const nullByte = (subject: string, received: string) => ({
1597+
name: "TypeError",
1598+
code: "ERR_INVALID_ARG_VALUE",
1599+
message: `The ${subject} must be a string, Uint8Array, or URL without null bytes. Received ${received}`,
1600+
});
1601+
expect(thrownBy("spawn", { cwd: "a\0b" })).toEqual(nullByte("property 'options.cwd'", "'a\\x00b'"));
1602+
expect(thrownBy("spawnSync", { cwd: Buffer.from("a\0b") })).toEqual(
1603+
nullByte("property 'options.cwd'", "<Buffer 61 00 62>"),
1604+
);
1605+
expect(thrown(() => fork("a\0b"))).toEqual(nullByte("argument 'modulePath'", "'a\\x00b'"));
1606+
});
14211607
});
14221608

14231609
// Regression: Bun registered the stdout/stderr poll immediately, so the native

‎test/js/node/test/parallel/test-child-process-fork-timeout-kill-signal.js‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,17 @@
11
'use strict';
22

33
const { mustCall } = require('../common');
4-
const { strictEqual, throws } = require('assert');
4+
const assert = require('assert');
55
const fixtures = require('../common/fixtures');
66
const { fork } = require('child_process');
7-
const { getEventListeners } = require('events');
7+
const { listenerCount } = require('events');
88

99
{
1010
// Verify default signal
1111
const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
1212
timeout: 5,
1313
});
14-
cp.on('exit', mustCall((code, ks) => strictEqual(ks, 'SIGTERM')));
14+
cp.on('exit', mustCall((code, ks) => assert.strictEqual(ks, 'SIGTERM')));
1515
}
1616

1717
{
@@ -20,18 +20,18 @@ const { getEventListeners } = require('events');
2020
timeout: 5,
2121
killSignal: 'SIGKILL',
2222
});
23-
cp.on('exit', mustCall((code, ks) => strictEqual(ks, 'SIGKILL')));
23+
cp.on('exit', mustCall((code, ks) => assert.strictEqual(ks, 'SIGKILL')));
2424
}
2525

2626
{
2727
// Verify timeout verification
28-
throws(() => fork(fixtures.path('child-process-stay-alive-forever.js'), {
28+
assert.throws(() => fork(fixtures.path('child-process-stay-alive-forever.js'), {
2929
timeout: 'badValue',
30-
}), /ERR_OUT_OF_RANGE/);
30+
}), /ERR_INVALID_ARG_TYPE/);
3131

32-
throws(() => fork(fixtures.path('child-process-stay-alive-forever.js'), {
32+
assert.throws(() => fork(fixtures.path('child-process-stay-alive-forever.js'), {
3333
timeout: {},
34-
}), /ERR_OUT_OF_RANGE/);
34+
}), /ERR_INVALID_ARG_TYPE/);
3535
}
3636

3737
{
@@ -43,8 +43,8 @@ const { getEventListeners } = require('events');
4343
timeout: 6,
4444
signal,
4545
});
46-
strictEqual(getEventListeners(signal, 'abort').length, 1);
46+
assert.strictEqual(listenerCount(signal, 'abort'), 1);
4747
cp.on('exit', mustCall(() => {
48-
strictEqual(getEventListeners(signal, 'abort').length, 0);
48+
assert.strictEqual(listenerCount(signal, 'abort'), 0);
4949
}));
5050
}

‎test/js/node/test/parallel/test-child-process-spawn-timeout-kill-signal.js‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,18 @@
11
'use strict';
22

33
const { mustCall } = require('../common');
4-
const { strictEqual, throws } = require('assert');
4+
const assert = require('assert');
55
const fixtures = require('../common/fixtures');
66
const { spawn } = require('child_process');
7-
const { getEventListeners } = require('events');
7+
const { listenerCount } = require('events');
88

99
const aliveForeverFile = 'child-process-stay-alive-forever.js';
1010
{
1111
// Verify default signal + closes
1212
const cp = spawn(process.execPath, [fixtures.path(aliveForeverFile)], {
1313
timeout: 5,
1414
});
15-
cp.on('exit', mustCall((code, ks) => strictEqual(ks, 'SIGTERM')));
15+
cp.on('exit', mustCall((code, ks) => assert.strictEqual(ks, 'SIGTERM')));
1616
}
1717

1818
{
@@ -21,18 +21,18 @@ const aliveForeverFile = 'child-process-stay-alive-forever.js';
2121
timeout: 6,
2222
killSignal: 'SIGKILL',
2323
});
24-
cp.on('exit', mustCall((code, ks) => strictEqual(ks, 'SIGKILL')));
24+
cp.on('exit', mustCall((code, ks) => assert.strictEqual(ks, 'SIGKILL')));
2525
}
2626

2727
{
2828
// Verify timeout verification
29-
throws(() => spawn(process.execPath, [fixtures.path(aliveForeverFile)], {
29+
assert.throws(() => spawn(process.execPath, [fixtures.path(aliveForeverFile)], {
3030
timeout: 'badValue',
31-
}), /ERR_OUT_OF_RANGE/);
31+
}), /ERR_INVALID_ARG_TYPE/);
3232

33-
throws(() => spawn(process.execPath, [fixtures.path(aliveForeverFile)], {
33+
assert.throws(() => spawn(process.execPath, [fixtures.path(aliveForeverFile)], {
3434
timeout: {},
35-
}), /ERR_OUT_OF_RANGE/);
35+
}), /ERR_INVALID_ARG_TYPE/);
3636
}
3737

3838
{
@@ -43,8 +43,8 @@ const aliveForeverFile = 'child-process-stay-alive-forever.js';
4343
timeout: 6,
4444
signal,
4545
});
46-
strictEqual(getEventListeners(signal, 'abort').length, 1);
46+
assert.strictEqual(listenerCount(signal, 'abort'), 1);
4747
cp.on('exit', mustCall(() => {
48-
strictEqual(getEventListeners(signal, 'abort').length, 0);
48+
assert.strictEqual(listenerCount(signal, 'abort'), 0);
4949
}));
5050
}

0 commit comments

Comments
 (0)