diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc index 6a35e5c5e4c..22527586cf8 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -516,19 +516,73 @@ static void ReallyExit(const FunctionCallbackInfo& args) { // 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); + int flags; + do { + flags = fcntl(fd, F_GETFD, 0); + } while (flags < 0 && errno == EINTR); if (flags < 0) { return flags; } - if (fcntl(fd, F_SETFD, flags & ~FD_CLOEXEC) < 0) { + int err; + do { + err = fcntl(fd, F_SETFD, flags & ~FD_CLOEXEC); + } while (err < 0 && errno == EINTR); + + if (err < 0) { return -1; } return flags; } +// Clears O_NONBLOCK on `fd` so the descriptor is blocking in the replacement +// process, matching child_process.spawn() and normal exit behavior. +// On success returns the previous F_GETFL flags (>= 0) so callers can +// restore them if execve(2) subsequently fails. On failure returns -1 with +// errno set. +inline int restore_standard_stream_blocking(int fd) { + int flags; + do { + flags = fcntl(fd, F_GETFL, 0); + } while (flags < 0 && errno == EINTR); + + if (flags < 0) { + return flags; + } + + if (flags & O_NONBLOCK) { + int err; + do { + err = fcntl(fd, F_SETFL, flags & ~O_NONBLOCK); + } while (err < 0 && errno == EINTR); + + if (err < 0) { + return -1; + } + } + + return flags; +} + +inline void restore_standard_stream_flags(int fd, + int saved_fd_flags, + int saved_fl_flags) { + if (saved_fd_flags >= 0) { + int err; + do { + err = fcntl(fd, F_SETFD, saved_fd_flags); + } while (err < 0 && errno == EINTR); + } + if (saved_fl_flags >= 0 && (saved_fl_flags & O_NONBLOCK)) { + int err; + do { + err = fcntl(fd, F_SETFL, saved_fl_flags); + } while (err < 0 && errno == EINTR); + } +} + static void Execve(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -580,21 +634,40 @@ 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 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}; + // process will inherit them, and restore blocking mode so that the new + // process does not inherit O_NONBLOCK set by libuv. 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_fd_flags[3] = {-1, -1, -1}; + int saved_stdio_fl_flags[3] = {-1, -1, -1}; for (int fd = 0; fd < 3; fd++) { - int prev = persist_standard_stream(fd); - if (prev < 0) { + int prev_fd = persist_standard_stream(fd); + if (prev_fd < 0) { + int fcntl_errno = errno; + // Undo changes already applied to earlier fds before throwing. + for (int j = 0; j < fd; j++) { + restore_standard_stream_flags( + j, saved_stdio_fd_flags[j], saved_stdio_fl_flags[j]); + } + env->ThrowErrnoException(fcntl_errno, "fcntl"); + return; + } + saved_stdio_fd_flags[fd] = prev_fd; + + int prev_fl = restore_standard_stream_blocking(fd); + if (prev_fl < 0) { int fcntl_errno = errno; + // Undo F_SETFD on current fd. + restore_standard_stream_flags(fd, saved_stdio_fd_flags[fd], -1); // Undo changes already applied to earlier fds before throwing. for (int j = 0; j < fd; j++) { - fcntl(j, F_SETFD, saved_stdio_flags[j]); + restore_standard_stream_flags( + j, saved_stdio_fd_flags[j], saved_stdio_fl_flags[j]); } env->ThrowErrnoException(fcntl_errno, "fcntl"); return; } - saved_stdio_flags[fd] = prev; + saved_stdio_fl_flags[fd] = prev_fl; } // Perform the execve operation. @@ -606,12 +679,13 @@ static void Execve(const FunctionCallbackInfo& args) { // can be observed and handled by JS code. execve(*executable, argv.data(), envp.data()); - // 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. + // If execve returned, it failed. Restore the FD_CLOEXEC and O_NONBLOCK flags + // we modified 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]); + restore_standard_stream_flags( + fd, saved_stdio_fd_flags[fd], saved_stdio_fl_flags[fd]); } env->ThrowErrnoException(execve_errno, "execve", nullptr, *executable); } @@ -878,3 +952,4 @@ NODE_BINDING_PER_ISOLATE_INIT(process_methods, node::process::CreatePerIsolateProperties) NODE_BINDING_EXTERNAL_REFERENCE(process_methods, node::process::RegisterExternalReferences) + diff --git a/test/parallel/test-process-execve-stdio-blocking.js b/test/parallel/test-process-execve-stdio-blocking.js new file mode 100644 index 00000000000..b549db32f16 --- /dev/null +++ b/test/parallel/test-process-execve-stdio-blocking.js @@ -0,0 +1,120 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { execFileSync, spawn, spawnSync } = require('child_process'); +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'); +} + +let pythonPath; +try { + pythonPath = execFileSync('which', ['python3'], { encoding: 'utf8' }).trim(); +} catch { + if (process.env.PYTHON) { + pythonPath = process.env.PYTHON; + } +} + +if (!pythonPath) { + common.skip('python3 is required for this test'); +} + +if (process.argv[2] === 'check-flags') { + // Touch stdio handles to initialize libuv and set O_NONBLOCK. + assert.ok(process.stdin); + assert.ok(process.stdout); + assert.ok(process.stderr); + + const pythonScript = [ + 'import fcntl, os, sys', + 'for fd in (0, 1, 2):', + ' try:', + ' flags = fcntl.fcntl(fd, fcntl.F_GETFL)', + ' except OSError:', + ' continue', + ' if flags & os.O_NONBLOCK:', + ' sys.stderr.write(f"fd {fd} has O_NONBLOCK\\n")', + ' sys.exit(1)', + 'print("SUCCESS_BLOCKING")', + ].join('\n'); + + process.execve(pythonPath, ['python3', '-c', pythonScript], process.env); + assert.fail('process.execve should not return on success'); +} else if (process.argv[2] === 'check-pipe-buffer') { + assert.ok(process.stdout); + + const pythonScript = [ + 'import sys', + 'data = b"x" * 262144', + 'sys.stdout.buffer.write(data)', + 'sys.stdout.buffer.flush()', + ].join('\n'); + + process.execve(pythonPath, ['python3', '-c', pythonScript], process.env); + assert.fail('process.execve should not return on success'); +} + +// 1. Verify that fcntl F_GETFL does not contain O_NONBLOCK after process.execve() +{ + const result = spawnSync(process.execPath, [__filename, 'check-flags'], { + stdio: ['pipe', 'pipe', 'pipe'], + encoding: 'utf8', + }); + + assert.strictEqual( + result.status, + 0, + `Child exited with code ${result.status}, stderr: ${result.stderr}` + ); + assert.match(result.stdout, /SUCCESS_BLOCKING/); +} + +// 2. Verify that writing more than pipe buffer capacity succeeds without EAGAIN +{ + const child = spawn(process.execPath, [__filename, 'check-pipe-buffer'], { + stdio: ['inherit', 'pipe', 'pipe'], + }); + + let receivedBytes = 0; + child.stdout.on('data', (chunk) => { + receivedBytes += chunk.length; + }); + + let stderr = ''; + child.stderr.on('data', (chunk) => { + stderr += chunk; + }); + + child.on('close', common.mustCall((code) => { + assert.strictEqual(code, 0, `Child exited with code ${code}, stderr: ${stderr}`); + assert.strictEqual(receivedBytes, 262144); + })); +} + +// 3. Verify that if process.execve() fails, stdio streams remain writable +{ + assert.ok(process.stdout); + assert.ok(process.stderr); + + assert.throws( + () => { + process.execve( + `${process.execPath}_non_existing_executable`, + [process.execPath, 'arg'], + process.env + ); + }, + { + code: 'ENOENT', + syscall: 'execve', + } + ); + + assert.strictEqual(process.stdout.writable, true); + assert.strictEqual(process.stderr.writable, true); +}