process: make process.execve's args argument optional

[mureinik]

Align the code with the documentation and similar methods used to execute os commands - the args argument should be optional, and if omitted, treated as an empty array ([]).

Fixes: #58411

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.21%. Comparing base (74acfdf) to head (9694800).
Report is 133 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58412      +/-   ##
==========================================
- Coverage   90.21%   90.21%   -0.01%     
==========================================
  Files         635      635              
  Lines      187174   187174              
  Branches    36747    36750       +3     
==========================================
- Hits       168861   168857       -4     
+ Misses      11103    11073      -30     
- Partials     7210     7244      +34     

Files with missing lines	Coverage Δ
lib/internal/process/per_thread.js	99.45% <100.00%> (ø)

... and 33 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.

[lpinca]

Can you please add a test?

[mureinik]

Can you please add a test?

@lpinca I'm actually a bit lost as how we'd test this, and would appreciate a pointer.
The other tests under (in test-process-execve*.js) call process.execve with the same node.js executable, the first argument as the same test, and some other args/env, and then assert that the replaced process gets the correct args/env passed to it.

For a test without any args, the best I could think of is something down the lines of replacing the process with something known and just asserting the process was replaced:

process.execve(
    SOMETHING_GOES_HERE
);

// If process.execve succeed, this should never be executed.
fail('process.execve failed');

but I don't have a good idea for an executable that is :

1. Known to be available on any platform the CI runs on
2. Won't block once executed

The only two things I can think of that are known to be available is the node executable itself (process.execPath), but it opens an interactive repl and never returns and maybe the current shell (process.env.SHELL), but that would also open an interactive shell and never return.

If you can help point me at the right direction, I'd be happy to add a test.

FWIW, on my local env I tested this with process.execve('/bin/ls'), but I don't think that in the CI we can make assumptions on the location of ls.

[lpinca]

FWIW, on my local env I tested this with process.execve('/bin/ls'), but I don't think that in the CI we can make assumptions on the location of ls.

What do think about finding the location first? Something like this

const path = child_process.spawnSync('which', ['ls'], { encoding: 'utf8' }).stdout.trim();

[mureinik]

@lpinca I was trying to avoid building a test that relies on child_process, but I guess these are more like integration tests than unit tests - the existing tests rely on worker_threads and (indirectly) other process properties/functions anyway, so this is probably a moot point.

I've added such a test, and rebased on top of the latest main.

For the record - I'm using ps instead of ls. On some platforms (e.g., my laptop), calling ls without arguments spews a usage message to stderr, which causes test.py to fail the test.

[mureinik]

This is embarrassing - my laptop is a mac, and the new test passes locally on it. On the CI env, macOS is the only thing that fails :-(
I'm looking in to it.

[mureinik]

Using ps failed the macOS CI. I'm trying with date to help triage what's going on here - i.e., is this a problem with ps, or something fundamentally different in the CI compared to my local env.

[mureinik]

@lpinca I'm not sure what exactly went wrong with using ps there, but TBH, I'd rather not dig too deeply into it.
Using date did the trick, and it seems common enough in *nix-like systems that it can be used for these tests.

[github-actions]

Failed to start CI

   ⚠  Commits were pushed since the last approving review:
   ⚠  - process: make execve's args argument optional
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/15193843206

[nodejs-github-bot]

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

[mureinik]

The external CI run for the previous iteration of the MR failed (@richardlau - thanks for triggering it!).
Some of the failures are noise where the Jenkins instance wasn't able to start the CI (regardless of these changes), but some of the failures were real, e.g. (https://ci.nodejs.org/job/node-test-commit-plinux/59119/nodes=rhel8-ppc64le/testReport/junit/(root)/parallel/test_process_execve_no_args/):

duration_ms: 582.606
exitcode: -6
severity: crashed
stack: A NULL argv[0] was passed through an exec system call.

When using execve or similar mechanisms, if argc is 0, argv[0] could be NULL (see, e.g., the good answers in https://stackoverflow.com/q/36673765/2422776).
This in itself isn't a bug, but some GNU coreutils explicitly check this and error out with the above error when argv[0] == NULL.

Instead of making assumptions about what binaries are available on every platform Node.js is built on and whether or not they allow argv[0] to be NULL, a more robust approach is to supply our own, do-nothing, binary for this test (see nop.c in the updated PR).

Side note for clarification:
The fact that calling processs.execve('/bin/date') fails with this error on some platforms is not a bug in this proposed fix - it's a user error. Even without this change, a user could always call processs.execve('/bin/date', []) explicitly and get the exact same error.

[mureinik]

@richardlau, apologies for nagging, but it looks like this PR lost its steam. I updated it to handle the (relevant) errors seen in the Jenkins CI, but IIUC, I can't re-trigger that run.
Can it please be retriggered so we can move forward with this?

[nodejs-github-bot]

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

[richardlau]

@richardlau, apologies for nagging, but it looks like this PR lost its steam. I updated it to handle the (relevant) errors seen in the Jenkins CI, but IIUC, I can't re-trigger that run. Can it please be retriggered so we can move forward with this?

I've retriggered the Jenkins CI.

[mureinik]

@richardlau Thanks!

Looking at the failures, node-test-linux-alpine-last-latest-x64 fails on the startup time of node (see https://ci.nodejs.org/job/node-test-commit-linux/nodes=alpine-last-latest-x64/65075/testReport/junit/(root)/sequential/test_perf_hooks/) that probably isn't related to this MR (i.e., starting up node shouldn't call execve).

In the other two failed jobs, it shows that there are no tests. IIUC, these are just aggregate jobs, and the "real" failure is the aforementioned startup time failure.

I'm not quite sure how to proceed from here, so any guidence would be appreciated.

[nodejs-github-bot]

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

[mureinik]

Following up on my prev comment - for the record, now the CI passes. I'm assuming this means the tests that failed aren't 100% stable, and as explained above, are probably unrelated to the PR's content.