diff --git a/doc/api/process.md b/doc/api/process.md index 61ea3c673e5f19..634ae87496a41f 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -1735,6 +1735,13 @@ that started the Node.js process. Symbolic links, if any, are resolved. added: - v23.11.0 - v22.15.0 +changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/62878 + description: A failed `execve(2)` system call now throws an exception + instead of aborting the process. Native `AtExit` + callbacks registered via the embedder API are no longer + invoked before the `execve(2)` call. --> > Stability: 1 - Experimental @@ -1751,10 +1758,19 @@ This is achieved by using the `execve` POSIX function and therefore no memory or resources from the current process are preserved, except for the standard input, standard output and standard error file descriptor. -All other resources are discarded by the system when the processes are swapped, without triggering -any exit or close events and without running any cleanup handler. - -This function will never return, unless an error occurred. +On success, all other resources are discarded by the system when the +processes are swapped, without triggering any exit or close events, without +running any JavaScript cleanup handler (for example `process.on('exit')`), +and without invoking native `AtExit` callbacks registered through the +embedder API. Callers that need to run cleanup logic should do so before +calling `process.execve()`. + +This function does not return on success. If the underlying `execve(2)` +system call fails, an `Error` is thrown whose `code` property is set to the +corresponding `errno` string (for example, `'ENOENT'` when `file` does not +exist), with `syscall` set to `'execve'` and `path` set to `file`. When +`execve(2)` fails the current process continues to run with its state +unchanged, so a caller may handle the error and take another action. This function is not available on Windows or IBM i. diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc index 6488fca90d68a8..1c695be2d5fa36 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -510,6 +510,10 @@ static void ReallyExit(const FunctionCallbackInfo& args) { } #if defined __POSIX__ && !defined(__PASE__) +// Clears FD_CLOEXEC on `fd` so the descriptor is inherited across execve(2). +// On success returns the previous F_GETFD flags (>= 0) so callers can +// restore them if execve(2) subsequently fails. On failure returns -1 with +// errno set. inline int persist_standard_stream(int fd) { int flags = fcntl(fd, F_GETFD, 0); @@ -517,8 +521,11 @@ inline int persist_standard_stream(int fd) { return flags; } - flags &= ~FD_CLOEXEC; - return fcntl(fd, F_SETFD, flags); + if (fcntl(fd, F_SETFD, flags & ~FD_CLOEXEC) < 0) { + return -1; + } + + return flags; } static void Execve(const FunctionCallbackInfo& args) { @@ -571,32 +578,41 @@ static void Execve(const FunctionCallbackInfo& args) { envp[envp_array->Length()] = nullptr; - // Set stdin, stdout and stderr to be non-close-on-exec - // so that the new process will inherit it. - if (persist_standard_stream(0) < 0 || persist_standard_stream(1) < 0 || - persist_standard_stream(2) < 0) { - env->ThrowErrnoException(errno, "fcntl"); - return; + // Set stdin, stdout and stderr to be non-close-on-exec so that the new + // process will inherit them. Save the previous flags on each fd so we can + // restore them if execve(2) fails and we throw back to JS. + int saved_stdio_flags[3] = {-1, -1, -1}; + for (int fd = 0; fd < 3; fd++) { + int prev = persist_standard_stream(fd); + if (prev < 0) { + int fcntl_errno = errno; + // Undo changes already applied to earlier fds before throwing. + for (int j = 0; j < fd; j++) { + fcntl(j, F_SETFD, saved_stdio_flags[j]); + } + env->ThrowErrnoException(fcntl_errno, "fcntl"); + return; + } + saved_stdio_flags[fd] = prev; } // Perform the execve operation. - RunAtExit(env); + // + // Note: we intentionally do not invoke RunAtExit(env) here. On success the + // kernel discards the current address space when loading the new image, so + // any in-memory side effects of AtExit callbacks are lost anyway. On + // failure we want to leave the environment intact so the thrown exception + // can be observed and handled by JS code. execve(*executable, argv.data(), envp.data()); - // If it returns, it means that the execve operation failed. - // In that case we abort the process. - auto error_message = std::string("process.execve failed with error code ") + - errors::errno_string(errno); - - // Abort the process - Local exception = - ErrnoException(isolate, errno, "execve", *executable); - Local message = v8::Exception::CreateMessage(isolate, exception); - - std::string info = FormatErrorMessage( - isolate, context, error_message.c_str(), message, true); - FPrintF(stderr, "%s\n", info); - ABORT(); + // If execve returned, it failed. Restore the FD_CLOEXEC flags we cleared + // above so that a failed call leaves no observable side effects, then + // throw an ErrnoException so JS can catch it. + int execve_errno = errno; + for (int fd = 0; fd < 3; fd++) { + fcntl(fd, F_SETFD, saved_stdio_flags[fd]); + } + env->ThrowErrnoException(execve_errno, "execve", nullptr, *executable); } #endif diff --git a/test/parallel/test-process-execve-abort.js b/test/parallel/test-process-execve-abort.js deleted file mode 100644 index e7c90fd1301c41..00000000000000 --- a/test/parallel/test-process-execve-abort.js +++ /dev/null @@ -1,26 +0,0 @@ -'use strict'; - -const { skip, isWindows, isIBMi } = require('../common'); -const assert = require('assert'); -const { spawnSync } = require('child_process'); -const { isMainThread } = require('worker_threads'); - -if (!isMainThread) { - skip('process.execve is not available in Workers'); -} else if (isWindows || isIBMi) { - skip('process.execve is not available in Windows or IBM i'); -} - -if (process.argv[2] === 'child') { - process.execve( - process.execPath + '_non_existing', - [__filename, 'replaced'], - { ...process.env, EXECVE_A: 'FIRST', EXECVE_B: 'SECOND', CWD: process.cwd() } - ); -} else { - const child = spawnSync(`${process.execPath}`, [`${__filename}`, 'child']); - const stderr = child.stderr.toString(); - - assert.ok(stderr.includes('process.execve failed with error code ENOENT'), stderr); - assert.ok(stderr.includes('execve (node:internal/process/per_thread'), stderr); -} diff --git a/test/parallel/test-process-execve-throws.js b/test/parallel/test-process-execve-throws.js new file mode 100644 index 00000000000000..c9280aa2c914fc --- /dev/null +++ b/test/parallel/test-process-execve-throws.js @@ -0,0 +1,39 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { isMainThread } = require('worker_threads'); + +if (!isMainThread) { + common.skip('process.execve is not available in Workers'); +} else if (common.isWindows || common.isIBMi) { + common.skip('process.execve is not available in Windows or IBM i'); +} + +assert.throws( + () => { + process.execve( + `${process.execPath}_non_existing`, + [process.execPath, 'arg'], + ); + }, + (err) => { + assert.ok(err instanceof Error); + assert.strictEqual(err.code, 'ENOENT'); + assert.strictEqual(err.syscall, 'execve'); + assert.strictEqual(typeof err.errno, 'number'); + assert.ok(err.errno > 0, `expected positive errno, got ${err.errno}`); + assert.match(err.path, /_non_existing$/); + return true; + }, +); + +assert.strictEqual(process.stdout.writable, true); +assert.strictEqual(process.stderr.writable, true); +assert.strictEqual(typeof process.pid, 'number'); + +let tickFired = false; +process.nextTick(common.mustCall(() => { tickFired = true; })); +setImmediate(common.mustCall(() => { + assert.strictEqual(tickFired, true); +}));