From 42c8064636bdd33c5dd497e4f1f49536df34f97b Mon Sep 17 00:00:00 2001
From: Thiago Padilha <thiago@padilha.cc>
Date: Mon, 2 Sep 2019 15:51:29 -0300
Subject: [PATCH] child_process: add 'overlapped' stdio flag

The 'overlapped' value sets the UV_OVERLAPPED_PIPE libuv flag in the
child process stdio.

Fixes: https://github.com/nodejs/node/issues/29238

PR-URL: https://github.com/nodejs/node/pull/29412
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 doc/api/child_process.md                      | 22 +++--
 lib/internal/child_process.js                 |  6 +-
 node.gyp                                      | 18 ++++
 src/env.h                                     |  1 +
 src/process_wrap.cc                           |  5 ++
 test/overlapped-checker/main_unix.c           | 51 +++++++++++
 test/overlapped-checker/main_win.c            | 85 +++++++++++++++++++
 .../test-child-process-stdio-overlapped.js    | 75 ++++++++++++++++
 8 files changed, 255 insertions(+), 8 deletions(-)
 create mode 100644 test/overlapped-checker/main_unix.c
 create mode 100644 test/overlapped-checker/main_win.c
 create mode 100644 test/parallel/test-child-process-stdio-overlapped.js

diff --git a/doc/api/child_process.md b/doc/api/child_process.md
index 68931d0a860e68..e3151b78e91957 100644
--- a/doc/api/child_process.md
+++ b/doc/api/child_process.md
@@ -660,6 +660,9 @@ subprocess.unref();
 <!-- YAML
 added: v0.7.10
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/29412
+    description: Added the `overlapped` stdio flag.
   - version: v3.3.1
     pr-url: https://github.com/nodejs/node/pull/2727
     description: The value `0` is now accepted as a file descriptor.
@@ -675,6 +678,7 @@ equal to `['pipe', 'pipe', 'pipe']`.
 For convenience, `options.stdio` may be one of the following strings:
 
 * `'pipe'`: equivalent to `['pipe', 'pipe', 'pipe']` (the default)
+* `'overlapped'`: equivalent to `['overlapped', 'overlapped', 'overlapped']`
 * `'ignore'`: equivalent to `['ignore', 'ignore', 'ignore']`
 * `'inherit'`: equivalent to `['inherit', 'inherit', 'inherit']` or `[0, 1, 2]`
 
@@ -688,7 +692,13 @@ pipes between the parent and child. The value is one of the following:
    `child_process` object as [`subprocess.stdio[fd]`][`subprocess.stdio`]. Pipes
    created for fds 0, 1, and 2 are also available as [`subprocess.stdin`][],
    [`subprocess.stdout`][] and [`subprocess.stderr`][], respectively.
-2. `'ipc'`: Create an IPC channel for passing messages/file descriptors
+1. `'overlapped'`: Same as `'pipe'` except that the `FILE_FLAG_OVERLAPPED` flag
+   is set on the handle. This is necessary for overlapped I/O on the child
+   process's stdio handles. See the
+   [docs](https://docs.microsoft.com/en-us/windows/win32/fileio/synchronous-and-asynchronous-i-o)
+   for more details. This is exactly the same as `'pipe'` on non-Windows
+   systems.
+1. `'ipc'`: Create an IPC channel for passing messages/file descriptors
    between parent and child. A [`ChildProcess`][] may have at most one IPC
    stdio file descriptor. Setting this option enables the
    [`subprocess.send()`][] method. If the child is a Node.js process, the
@@ -699,25 +709,25 @@ pipes between the parent and child. The value is one of the following:
    Accessing the IPC channel fd in any way other than [`process.send()`][]
    or using the IPC channel with a child process that is not a Node.js instance
    is not supported.
-3. `'ignore'`: Instructs Node.js to ignore the fd in the child. While Node.js
+1. `'ignore'`: Instructs Node.js to ignore the fd in the child. While Node.js
    will always open fds 0, 1, and 2 for the processes it spawns, setting the fd
    to `'ignore'` will cause Node.js to open `/dev/null` and attach it to the
    child's fd.
-4. `'inherit'`: Pass through the corresponding stdio stream to/from the
+1. `'inherit'`: Pass through the corresponding stdio stream to/from the
    parent process. In the first three positions, this is equivalent to
    `process.stdin`, `process.stdout`, and `process.stderr`, respectively. In
    any other position, equivalent to `'ignore'`.
-5. {Stream} object: Share a readable or writable stream that refers to a tty,
+1. {Stream} object: Share a readable or writable stream that refers to a tty,
    file, socket, or a pipe with the child process. The stream's underlying
    file descriptor is duplicated in the child process to the fd that
    corresponds to the index in the `stdio` array. The stream must have an
    underlying descriptor (file streams do not until the `'open'` event has
    occurred).
-6. Positive integer: The integer value is interpreted as a file descriptor
+1. Positive integer: The integer value is interpreted as a file descriptor
    that is currently open in the parent process. It is shared with the child
    process, similar to how {Stream} objects can be shared. Passing sockets
    is not supported on Windows.
-7. `null`, `undefined`: Use default value. For stdio fds 0, 1, and 2 (in other
+1. `null`, `undefined`: Use default value. For stdio fds 0, 1, and 2 (in other
    words, stdin, stdout, and stderr) a pipe is created. For fd 3 and up, the
    default is `'ignore'`.
 
diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
index 0ec5bed4dbb0d1..68b4b447f68ad7 100644
--- a/lib/internal/child_process.js
+++ b/lib/internal/child_process.js
@@ -224,6 +224,7 @@ function stdioStringToArray(stdio, channel) {
 
   switch (stdio) {
     case 'ignore':
+    case 'overlapped':
     case 'pipe': options.push(stdio, stdio, stdio); break;
     case 'inherit': options.push(0, 1, 2); break;
     default:
@@ -968,9 +969,10 @@ function getValidStdio(stdio, sync) {
 
     if (stdio === 'ignore') {
       acc.push({ type: 'ignore' });
-    } else if (stdio === 'pipe' || (typeof stdio === 'number' && stdio < 0)) {
+    } else if (stdio === 'pipe' || stdio === 'overlapped' ||
+               (typeof stdio === 'number' && stdio < 0)) {
       const a = {
-        type: 'pipe',
+        type: stdio === 'overlapped' ? 'overlapped' : 'pipe',
         readable: i === 0,
         writable: i !== 0
       };
diff --git a/node.gyp b/node.gyp
index 8ba0dfebe890e8..3daf1bc10705d3 100644
--- a/node.gyp
+++ b/node.gyp
@@ -1367,6 +1367,24 @@
       ],
     }, # embedtest
 
+    {
+      'target_name': 'overlapped-checker',
+      'type': 'executable',
+
+      'conditions': [
+        ['OS=="win"', {
+          'sources': [
+            'test/overlapped-checker/main_win.c'
+          ],
+        }],
+        ['OS!="win"', {
+          'sources': [
+            'test/overlapped-checker/main_unix.c'
+          ],
+        }],
+      ]
+    }, # overlapped-checker
+
     # TODO(joyeecheung): do not depend on node_lib,
     # instead create a smaller static library node_lib_base that does
     # just enough for node_native_module.cc and the cache builder to
diff --git a/src/env.h b/src/env.h
index 52046a69be2f7c..813d785c397864 100644
--- a/src/env.h
+++ b/src/env.h
@@ -319,6 +319,7 @@ constexpr size_t kFsStatsBufferLength =
   V(options_string, "options")                                                 \
   V(order_string, "order")                                                     \
   V(output_string, "output")                                                   \
+  V(overlapped_string, "overlapped")                                           \
   V(parse_error_string, "Parse Error")                                         \
   V(password_string, "password")                                               \
   V(path_string, "path")                                                       \
diff --git a/src/process_wrap.cc b/src/process_wrap.cc
index 3d1065c9922183..bbb00153f7a8a8 100644
--- a/src/process_wrap.cc
+++ b/src/process_wrap.cc
@@ -125,6 +125,11 @@ class ProcessWrap : public HandleWrap {
         options->stdio[i].flags = static_cast<uv_stdio_flags>(
             UV_CREATE_PIPE | UV_READABLE_PIPE | UV_WRITABLE_PIPE);
         options->stdio[i].data.stream = StreamForWrap(env, stdio);
+      } else if (type->StrictEquals(env->overlapped_string())) {
+        options->stdio[i].flags = static_cast<uv_stdio_flags>(
+            UV_CREATE_PIPE | UV_READABLE_PIPE | UV_WRITABLE_PIPE |
+            UV_OVERLAPPED_PIPE);
+        options->stdio[i].data.stream = StreamForWrap(env, stdio);
       } else if (type->StrictEquals(env->wrap_string())) {
         options->stdio[i].flags = UV_INHERIT_STREAM;
         options->stdio[i].data.stream = StreamForWrap(env, stdio);
diff --git a/test/overlapped-checker/main_unix.c b/test/overlapped-checker/main_unix.c
new file mode 100644
index 00000000000000..9d3d83af0f1068
--- /dev/null
+++ b/test/overlapped-checker/main_unix.c
@@ -0,0 +1,51 @@
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+
+#include <errno.h>
+#include <unistd.h>
+
+static size_t r(char* buf, size_t buf_size) {
+  ssize_t read_count;
+  do
+    read_count = read(0, buf, buf_size);
+  while (read_count < 0 && errno == EINTR);
+  if (read_count <= 0)
+    abort();
+  return (size_t)read_count;
+}
+
+static void w(const char* buf, size_t count) {
+  const char* end = buf + count;
+
+  while (buf < end) {
+    ssize_t write_count;
+    do
+      write_count = write(1, buf, count);
+    while (write_count < 0 && errno == EINTR);
+    if (write_count <= 0)
+      abort();
+    buf += write_count;
+  }
+
+  fprintf(stderr, "%zu", count);
+  fflush(stderr);
+}
+
+int main(void) {
+  w("0", 1);
+
+  while (1) {
+    char buf[256];
+    size_t read_count = r(buf, sizeof(buf));
+    // The JS part (test-child-process-stdio-overlapped.js) only writes the
+    // "exit" string when the buffer is empty, so the read is guaranteed to be
+    // atomic due to it being less than PIPE_BUF.
+    if (!strncmp(buf, "exit", read_count)) {
+      break;
+    }
+    w(buf, read_count);
+  }
+
+  return 0;
+}
diff --git a/test/overlapped-checker/main_win.c b/test/overlapped-checker/main_win.c
new file mode 100644
index 00000000000000..c38b7febe6ddc5
--- /dev/null
+++ b/test/overlapped-checker/main_win.c
@@ -0,0 +1,85 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <windows.h>
+
+static char buf[256];
+static DWORD read_count;
+static DWORD write_count;
+static HANDLE stdin_h;
+static OVERLAPPED stdin_o;
+
+static void die(const char* buf) {
+  fprintf(stderr, "%s\n", buf);
+  fflush(stderr);
+  exit(100);
+}
+
+static void overlapped_read(void) {
+  if (ReadFile(stdin_h, buf, sizeof(buf), NULL, &stdin_o)) {
+    // Since we start the read operation immediately before requesting a write,
+    // it should never complete synchronously since no data would be available
+    die("read completed synchronously");
+  }
+  if (GetLastError() != ERROR_IO_PENDING) {
+    die("overlapped read failed");
+  }
+}
+
+static void write(const char* buf, size_t buf_size) {
+  overlapped_read();
+  DWORD write_count;
+  HANDLE stdout_h = GetStdHandle(STD_OUTPUT_HANDLE);
+  if (!WriteFile(stdout_h, buf, buf_size, &write_count, NULL)) {
+    die("overlapped write failed");
+  }
+  fprintf(stderr, "%d", write_count);
+  fflush(stderr);
+}
+
+int main(void) {
+  HANDLE event = CreateEvent(NULL, FALSE, FALSE, NULL);
+  if (event == NULL) {
+    die("failed to create event handle");
+  }
+
+  stdin_h = GetStdHandle(STD_INPUT_HANDLE);
+  stdin_o.hEvent = event;
+
+  write("0", 1);
+
+  while (1) {
+    DWORD result = WaitForSingleObject(event, INFINITE);
+    if (result == WAIT_OBJECT_0) {
+      if (!GetOverlappedResult(stdin_h, &stdin_o, &read_count, FALSE)) {
+        die("failed to get overlapped read result");
+      }
+      if (strncmp(buf, "exit", read_count) == 0) {
+        break;
+      }
+      write(buf, read_count);
+    } else {
+      char emsg[0xfff];
+      int ecode = GetLastError();
+      DWORD rv = FormatMessage(
+          FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
+          NULL,
+          ecode,
+          MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+          (LPSTR)emsg,
+          sizeof(emsg),
+          NULL);
+      if (rv > 0) {
+        snprintf(emsg, sizeof(emsg),
+            "WaitForSingleObject failed. Error %d (%s)", ecode, emsg);
+      } else {
+        snprintf(emsg, sizeof(emsg),
+            "WaitForSingleObject failed. Error %d", ecode);
+      }
+      die(emsg);
+    }
+  }
+
+  return 0;
+}
diff --git a/test/parallel/test-child-process-stdio-overlapped.js b/test/parallel/test-child-process-stdio-overlapped.js
new file mode 100644
index 00000000000000..70a6454ff8fc9a
--- /dev/null
+++ b/test/parallel/test-child-process-stdio-overlapped.js
@@ -0,0 +1,75 @@
+// Test for "overlapped" stdio option. This test uses the "overlapped-checker"
+// helper program which basically a specialized echo program.
+//
+// The test has two goals:
+//
+// - Verify that overlapped I/O works on windows. The test program will deadlock
+//   if stdin doesn't have the FILE_FLAG_OVERLAPPED flag set on startup (see
+//   test/overlapped-checker/main_win.c for more details).
+// - Verify that "overlapped" stdio option works transparently as a pipe (on
+//   unix/windows)
+//
+// This is how the test works:
+//
+// - This script assumes only numeric strings are written to the test program
+//   stdout.
+// - The test program will be spawned with "overlapped" set on stdin and "pipe"
+//   set on stdout/stderr and at startup writes a number to its stdout
+// - When this script receives some data, it will parse the number, add 50 and
+//   write to the test program's stdin.
+// - The test program will then echo the number back to us which will repeat the
+//   cycle until the number reaches 200, at which point we send the "exit"
+//   string, which causes the test program to exit.
+// - Extra assertion: Every time the test program writes a string to its stdout,
+//   it will write the number of bytes written to stderr.
+// - If overlapped I/O is not setup correctly, this test is going to hang.
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const path = require('path');
+const child_process = require('child_process');
+
+const exeExtension = process.platform === 'win32' ? '.exe' : '';
+const exe = 'overlapped-checker' + exeExtension;
+const exePath = path.join(path.dirname(process.execPath), exe);
+
+const child = child_process.spawn(exePath, [], {
+  stdio: ['overlapped', 'pipe', 'pipe']
+});
+
+child.stdin.setEncoding('utf8');
+child.stdout.setEncoding('utf8');
+child.stderr.setEncoding('utf8');
+
+function writeNext(n) {
+  child.stdin.write((n + 50).toString());
+}
+
+child.stdout.on('data', (s) => {
+  const n = Number(s);
+  if (n >= 200) {
+    child.stdin.write('exit');
+    return;
+  }
+  writeNext(n);
+});
+
+let stderr = '';
+child.stderr.on('data', (s) => {
+  stderr += s;
+});
+
+child.stderr.on('end', common.mustCall(() => {
+  // This is the sequence of numbers sent to us:
+  // - 0 (1 byte written)
+  // - 50 (2 bytes written)
+  // - 100 (3 bytes written)
+  // - 150 (3 bytes written)
+  // - 200 (3 bytes written)
+  assert.strictEqual(stderr, '12333');
+}));
+
+child.on('exit', common.mustCall((status) => {
+  // The test program will return the number of writes as status code.
+  assert.strictEqual(status, 0);
+}));