child_process: validate strings in exec and spawn #56148

puskin · 2024-12-05T16:53:46Z

I went through the exec, execFile, spawn, execSync, execFileSync and spawnSync functions in child_process and edited all the functions to properly validate their string parameters

codecov · 2024-12-05T18:23:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.16%. Comparing base (3c2da4b) to head (e67638f).
Report is 1084 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56148      +/-   ##
==========================================
+ Coverage   87.99%   90.16%   +2.17%     
==========================================
  Files         656      630      -26     
  Lines      188999   186467    -2532     
  Branches    35981    36616     +635     
==========================================
+ Hits       166301   168131    +1830     
+ Misses      15865    11125    -4740     
- Partials     6833     7211     +378

Files with missing lines	Coverage Δ
lib/child_process.js	`97.74% <100.00%> (+0.30%)`	⬆️

... and 372 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lib/child_process.js

aduh95

Can you please remove all the unrelated changes? It makes the PR hard to review. Please only include changes that are necessary to make the added test pass, and all the other changes should be made in a separate PR.

test/parallel/test-child-process-exec-error.js

test/parallel/test-child-process-spawn-error.js

nodejs-github-bot · 2024-12-10T00:49:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63969/

nodejs-github-bot · 2024-12-10T23:42:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63992/

aduh95

It looks like the added test pass on main, meaning either the change in lib/ is only a refactor, or we are not adding sufficient coverage to avoid regression.

puskin · 2024-12-11T10:25:08Z

It looks like the added test pass on main, meaning either the change in lib/ is only a refactor, or we are not adding sufficient coverage to avoid regression.

it was mainly refactoring because, for example, when calling execFile, the validation was not done immediately but down the road when, at the very end, the method itself was calling the spawn function.
Tests are passing in main because the validation in spawn is already present; my change is mainly about doing it as soon as possible to reject as soon as possible in order to save some cycles and having a better stack trace

aduh95 · 2024-12-11T10:45:48Z

in order to save some cycles and having a better stack trace

I don't think it's the right approach, we should optimize for the happy path, where there are no error thrown. IIUC, with this change we would be checking twice if the arguments are valid, so in order to save some cycles we should not land this.

puskin · 2024-12-11T11:22:20Z

in order to save some cycles and having a better stack trace

I don't think it's the right approach, we should optimize for the happy path, where there are no error thrown. IIUC, with this change we would be checking twice if the arguments are valid, so in order to save some cycles we should not land this.

gotcha. I went in that direction because I noticed that was the case already. With the latest push all the validation is done down the line and only once

aduh95 · 2025-01-08T10:50:38Z

The added tests are passing on latest main. If we want to move forward with this, we should split this into two PRs/commits, one that increases the coverage, one that refactors the implementation

aduh95 · 2025-01-08T23:17:53Z

The commit message of 80fcad1 should say it's a refactor, e.g. child_process: refactor string validation in exec and spawn.

The commit message of 2f43432 should be using test: subsystem, not child_process:, e.g. test: increase coverage of argument validation of cp methods.

IMO the order of commits should be reversed, tests should land first.

Addressed

puskin · 2025-01-17T08:45:22Z

Is anything else needed here?

nodejs-github-bot · 2025-01-22T19:12:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64617/

nodejs-github-bot · 2025-02-05T06:39:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64998/

puskin · 2025-02-21T14:02:12Z

can I please request a rerun of the pipeline?

puskin · 2025-03-21T15:32:47Z

anything else needed here?

BridgeAR

LGTM with the only the two obsolete validations removed.

BridgeAR · 2025-05-01T11:16:04Z

lib/child_process.js

+function validateStringParam(param, paramName) {
+  validateString(param, paramName);
+  validateArgumentNullCheck(param, paramName);
+
+  if (param.length === 0) {
+    throw new ERR_INVALID_ARG_VALUE(paramName, param, 'cannot be empty');
+  }
+}

-  if (file.length === 0)
-    throw new ERR_INVALID_ARG_VALUE('file', file, 'cannot be empty');
+function normalizeSpawnArguments(file, args, options) {
+  validateStringParam(file, 'file');


If we keep the changes as before, the compiler has to do less inlining.

Suggested change

function validateStringParam(param, paramName) {

validateString(param, paramName);

validateArgumentNullCheck(param, paramName);

if (param.length === 0) {

throw new ERR_INVALID_ARG_VALUE(paramName, param, 'cannot be empty');

}

}

if (file.length === 0)

throw new ERR_INVALID_ARG_VALUE('file', file, 'cannot be empty');

function normalizeSpawnArguments(file, args, options) {

validateStringParam(file, 'file');

function normalizeSpawnArguments(file, args, options) {

validateString(file, 'file');

validateArgumentNullCheck(file, 'file');

if (file.length === 0) {

throw new ERR_INVALID_ARG_VALUE(file, 'file', 'cannot be empty');

}

BridgeAR · 2025-05-02T17:02:10Z

lib/child_process.js

-  if (file.length === 0)
-    throw new ERR_INVALID_ARG_VALUE('file', file, 'cannot be empty');
+  if (file.length === 0) {
+    throw new ERR_INVALID_ARG_VALUE(file, 'file', 'cannot be empty');


Suggested change

throw new ERR_INVALID_ARG_VALUE(file, 'file', 'cannot be empty');

throw new ERR_INVALID_ARG_VALUE('file', file, 'cannot be empty');

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Dec 5, 2024

puskin force-pushed the child-process-docs-and-validation branch from 358bd79 to 40a0a9f Compare December 5, 2024 17:12

aduh95 reviewed Dec 8, 2024

View reviewed changes

lib/child_process.js Outdated Show resolved Hide resolved

aduh95 reviewed Dec 8, 2024

View reviewed changes

puskin force-pushed the child-process-docs-and-validation branch from 40a0a9f to 18ddc47 Compare December 9, 2024 07:55

aduh95 reviewed Dec 9, 2024

View reviewed changes

test/parallel/test-child-process-exec-error.js Outdated Show resolved Hide resolved

puskin force-pushed the child-process-docs-and-validation branch from 18ddc47 to 6d02bc8 Compare December 9, 2024 11:21

aduh95 reviewed Dec 9, 2024

View reviewed changes

test/parallel/test-child-process-spawn-error.js Outdated Show resolved Hide resolved

puskin force-pushed the child-process-docs-and-validation branch from 6d02bc8 to 0a84382 Compare December 9, 2024 13:43

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 10, 2024

aduh95 approved these changes Dec 10, 2024

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2024

puskin changed the title ~~child_process: improve docs and validate strings in exec and spawn~~ child_process: validate strings in exec and spawn Dec 10, 2024

ZYSzys approved these changes Dec 11, 2024

View reviewed changes

aduh95 previously requested changes Dec 11, 2024

View reviewed changes

puskin force-pushed the child-process-docs-and-validation branch from 0a84382 to a3b5b1a Compare December 11, 2024 11:19

pmarchini requested a review from aduh95 January 8, 2025 09:51

puskin force-pushed the child-process-docs-and-validation branch from a3b5b1a to 80fcad1 Compare January 8, 2025 19:35

puskin force-pushed the child-process-docs-and-validation branch from 2f43432 to c07b9d8 Compare January 9, 2025 08:38

test: increase coverage of argument validation of cp methods

52c7bb1

puskin force-pushed the child-process-docs-and-validation branch from c07b9d8 to 52c7bb1 Compare January 9, 2025 08:41

jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 19, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 21, 2025

This comment was marked as outdated.

Sign in to view

puskin requested a review from ZYSzys January 26, 2025 20:11

ZYSzys approved these changes Feb 5, 2025

View reviewed changes

BridgeAR reviewed May 1, 2025

View reviewed changes

puskin force-pushed the child-process-docs-and-validation branch from 446cf6a to b6fcf31 Compare May 2, 2025 07:48

BridgeAR reviewed May 2, 2025

View reviewed changes

child_process: refactor string validation in exec and spawn

e67638f

puskin force-pushed the child-process-docs-and-validation branch from b6fcf31 to e67638f Compare May 2, 2025 20:21

	throw new ERR_INVALID_ARG_VALUE(file, 'file', 'cannot be empty');
	throw new ERR_INVALID_ARG_VALUE('file', file, 'cannot be empty');

Uh oh!

child_process: validate strings in exec and spawn #56148

Are you sure you want to change the base?

child_process: validate strings in exec and spawn #56148

Uh oh!

Conversation

puskin commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 10, 2024

Uh oh!

nodejs-github-bot commented Dec 10, 2024

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

puskin commented Dec 11, 2024

Uh oh!

aduh95 commented Dec 11, 2024

Uh oh!

puskin commented Dec 11, 2024

Uh oh!

aduh95 commented Jan 8, 2025

Uh oh!

aduh95 commented Jan 8, 2025

Uh oh!

puskin commented Jan 17, 2025

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Jan 22, 2025

Uh oh!

nodejs-github-bot commented Feb 5, 2025

Uh oh!

puskin commented Feb 21, 2025

Uh oh!

puskin commented Mar 21, 2025

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR May 1, 2025

Choose a reason for hiding this comment

Uh oh!

BridgeAR May 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

puskin commented Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading