process: restore blocking stdio before execve

src/node_process_methods.cc

@@ -516,19 +516,73 @@ static void ReallyExit(const FunctionCallbackInfo<Value>& 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<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   Isolate* isolate = env->isolate();
@@ -580,21 +634,40 @@ static void Execve(const FunctionCallbackInfo<Value>& 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<Value>& 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)
+

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);
+}