FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

process: throw on execve(2) failure instead of aborting · nodejs/node@ab66de8 · GitHub

/ node Public

Commit ab66de8

Browse files
bengl
authored and
aduh95
committed
process: throw on execve(2) failure instead of aborting
When the underlying execve(2) system call fails, process.execve() previously printed an error to stderr and called ABORT(), preventing JS code from detecting or recovering from common failures such as a missing binary. Throw an ErrnoException instead, carrying the standard code, errno, syscall, and path properties. To leave the process in a clean state when execve(2) fails, no longer run native AtExit callbacks before the call (their in-memory effects are discarded on success anyway), and snapshot and restore the FD_CLOEXEC flags on stdio so a failed call has no observable side effects. Rename and update test-process-execve-abort.js accordingly and document the new behavior. Signed-off-by: Bryan English <bryan@bryanenglish.com> PR-URL: #62878 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 7d52f20 commit ab66de8

4 files changed

Lines changed: 98 additions & 53 deletions

File tree

‎doc/api/process.md‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1735,6 +1735,13 @@ that started the Node.js process. Symbolic links, if any, are resolved.
17351735
added:
17361736
- v23.11.0
17371737
- v22.15.0
1738+
changes:
1739+
- version: REPLACEME
1740+
pr-url: https://github.com/nodejs/node/pull/62878
1741+
description: A failed `execve(2)` system call now throws an exception
1742+
instead of aborting the process. Native `AtExit`
1743+
callbacks registered via the embedder API are no longer
1744+
invoked before the `execve(2)` call.
17381745
-->
17391746
17401747
> Stability: 1 - Experimental
@@ -1751,10 +1758,19 @@ This is achieved by using the `execve` POSIX function and therefore no memory or
17511758
resources from the current process are preserved, except for the standard input,
17521759
standard output and standard error file descriptor.
17531760
1754-
All other resources are discarded by the system when the processes are swapped, without triggering
1755-
any exit or close events and without running any cleanup handler.
1756-
1757-
This function will never return, unless an error occurred.
1761+
On success, all other resources are discarded by the system when the
1762+
processes are swapped, without triggering any exit or close events, without
1763+
running any JavaScript cleanup handler (for example `process.on('exit')`),
1764+
and without invoking native `AtExit` callbacks registered through the
1765+
embedder API. Callers that need to run cleanup logic should do so before
1766+
calling `process.execve()`.
1767+
1768+
This function does not return on success. If the underlying `execve(2)`
1769+
system call fails, an `Error` is thrown whose `code` property is set to the
1770+
corresponding `errno` string (for example, `'ENOENT'` when `file` does not
1771+
exist), with `syscall` set to `'execve'` and `path` set to `file`. When
1772+
`execve(2)` fails the current process continues to run with its state
1773+
unchanged, so a caller may handle the error and take another action.
17581774
17591775
This function is not available on Windows or IBM i.
17601776

‎src/node_process_methods.cc‎

Lines changed: 39 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -510,15 +510,22 @@ static void ReallyExit(const FunctionCallbackInfo<Value>& args) {
510510
}
511511

512512
#if defined __POSIX__ && !defined(__PASE__)
513+
// Clears FD_CLOEXEC on `fd` so the descriptor is inherited across execve(2).
514+
// On success returns the previous F_GETFD flags (>= 0) so callers can
515+
// restore them if execve(2) subsequently fails. On failure returns -1 with
516+
// errno set.
513517
inline int persist_standard_stream(int fd) {
514518
int flags = fcntl(fd, F_GETFD, 0);
515519

516520
if (flags < 0) {
517521
return flags;
518522
}
519523

520-
flags &= ~FD_CLOEXEC;
521-
return fcntl(fd, F_SETFD, flags);
524+
if (fcntl(fd, F_SETFD, flags & ~FD_CLOEXEC) < 0) {
525+
return -1;
526+
}
527+
528+
return flags;
522529
}
523530

524531
static void Execve(const FunctionCallbackInfo<Value>& args) {
@@ -571,32 +578,41 @@ static void Execve(const FunctionCallbackInfo<Value>& args) {
571578

572579
envp[envp_array->Length()] = nullptr;
573580

574-
// Set stdin, stdout and stderr to be non-close-on-exec
575-
// so that the new process will inherit it.
576-
if (persist_standard_stream(0) < 0 || persist_standard_stream(1) < 0 ||
577-
persist_standard_stream(2) < 0) {
578-
env->ThrowErrnoException(errno, "fcntl");
579-
return;
581+
// Set stdin, stdout and stderr to be non-close-on-exec so that the new
582+
// process will inherit them. Save the previous flags on each fd so we can
583+
// restore them if execve(2) fails and we throw back to JS.
584+
int saved_stdio_flags[3] = {-1, -1, -1};
585+
for (int fd = 0; fd < 3; fd++) {
586+
int prev = persist_standard_stream(fd);
587+
if (prev < 0) {
588+
int fcntl_errno = errno;
589+
// Undo changes already applied to earlier fds before throwing.
590+
for (int j = 0; j < fd; j++) {
591+
fcntl(j, F_SETFD, saved_stdio_flags[j]);
592+
}
593+
env->ThrowErrnoException(fcntl_errno, "fcntl");
594+
return;
595+
}
596+
saved_stdio_flags[fd] = prev;
580597
}
581598

582599
// Perform the execve operation.
583-
RunAtExit(env);
600+
//
601+
// Note: we intentionally do not invoke RunAtExit(env) here. On success the
602+
// kernel discards the current address space when loading the new image, so
603+
// any in-memory side effects of AtExit callbacks are lost anyway. On
604+
// failure we want to leave the environment intact so the thrown exception
605+
// can be observed and handled by JS code.
584606
execve(*executable, argv.data(), envp.data());
585607

586-
// If it returns, it means that the execve operation failed.
587-
// In that case we abort the process.
588-
auto error_message = std::string("process.execve failed with error code ") +
589-
errors::errno_string(errno);
590-
591-
// Abort the process
592-
Local<v8::Value> exception =
593-
ErrnoException(isolate, errno, "execve", *executable);
594-
Local<v8::Message> message = v8::Exception::CreateMessage(isolate, exception);
595-
596-
std::string info = FormatErrorMessage(
597-
isolate, context, error_message.c_str(), message, true);
598-
FPrintF(stderr, "%s\n", info);
599-
ABORT();
608+
// If execve returned, it failed. Restore the FD_CLOEXEC flags we cleared
609+
// above so that a failed call leaves no observable side effects, then
610+
// throw an ErrnoException so JS can catch it.
611+
int execve_errno = errno;
612+
for (int fd = 0; fd < 3; fd++) {
613+
fcntl(fd, F_SETFD, saved_stdio_flags[fd]);
614+
}
615+
env->ThrowErrnoException(execve_errno, "execve", nullptr, *executable);
600616
}
601617
#endif
602618

‎test/parallel/test-process-execve-abort.js‎

Lines changed: 0 additions & 26 deletions
This file was deleted.

‎test/parallel/test-process-execve-throws.js‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const { isMainThread } = require('worker_threads');
6+
7+
if (!isMainThread) {
8+
common.skip('process.execve is not available in Workers');
9+
} else if (common.isWindows || common.isIBMi) {
10+
common.skip('process.execve is not available in Windows or IBM i');
11+
}
12+
13+
assert.throws(
14+
() => {
15+
process.execve(
16+
`${process.execPath}_non_existing`,
17+
[process.execPath, 'arg'],
18+
);
19+
},
20+
(err) => {
21+
assert.ok(err instanceof Error);
22+
assert.strictEqual(err.code, 'ENOENT');
23+
assert.strictEqual(err.syscall, 'execve');
24+
assert.strictEqual(typeof err.errno, 'number');
25+
assert.ok(err.errno > 0, `expected positive errno, got ${err.errno}`);
26+
assert.match(err.path, /_non_existing$/);
27+
return true;
28+
},
29+
);
30+
31+
assert.strictEqual(process.stdout.writable, true);
32+
assert.strictEqual(process.stderr.writable, true);
33+
assert.strictEqual(typeof process.pid, 'number');
34+
35+
let tickFired = false;
36+
process.nextTick(common.mustCall(() => { tickFired = true; }));
37+
setImmediate(common.mustCall(() => {
38+
assert.strictEqual(tickFired, true);
39+
}));

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL