From 4cb37ae1f8263f836a8b07cfd97997bc68698ce6 Mon Sep 17 00:00:00 2001
From: Santiago Gimeno <santiago.gimeno@gmail.com>
Date: Thu, 16 Jun 2016 23:00:16 +0200
Subject: [PATCH] child_process: workaround fd passing issue on OS X

There's an issue on some `OS X` versions when passing fd's between processes.
When the handle associated to a specific file descriptor is closed by the sender
process before it's received in the destination, the handle is indeed closed
while it should remain opened. In order to fix this behaviour, don't close the
handle until the `NODE_HANDLE_ACK` is received by the sender.
Added `test-child-process-pass-fd` that is basically `test-cluster-net-send` but
creating lots of workers, so the issue reproduces on `OS X` consistently.

Fixes: https://github.com/nodejs/node/issues/7512
Ref: https://github.com/nodejs/node/pull/8904
PR-URL: https://github.com/nodejs/node/pull/7572
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 lib/internal/child_process.js                 | 41 +++++++++++---
 test/sequential/test-child-process-pass-fd.js | 53 +++++++++++++++++++
 2 files changed, 86 insertions(+), 8 deletions(-)
 create mode 100644 test/sequential/test-child-process-pass-fd.js

diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
index 3613bfd0efeec8..6bd3585b32e855 100644
--- a/lib/internal/child_process.js
+++ b/lib/internal/child_process.js
@@ -90,10 +90,21 @@ const handleConversion = {
       return handle;
     },
 
-    postSend: function(handle) {
-      // Close the Socket handle after sending it
-      if (handle)
-        handle.close();
+    postSend: function(handle, target) {
+      // Store the handle after successfully sending it, so it can be closed
+      // when the NODE_HANDLE_ACK is received. If the handle could not be sent,
+      // just close it.
+      if (handle) {
+        if (target) {
+          // There can only be one _pendingHandle as passing handles are
+          // processed one at a time: handles are stored in _handleQueue while
+          // waiting for the NODE_HANDLE_ACK of the current passing handle.
+          assert(!target._pendingHandle);
+          target._pendingHandle = handle;
+        } else {
+          handle.close();
+        }
+      }
     },
 
     got: function(message, handle, emit) {
@@ -396,6 +407,7 @@ ChildProcess.prototype.unref = function() {
 function setupChannel(target, channel) {
   target._channel = channel;
   target._handleQueue = null;
+  target._pendingHandle = null;
 
   const control = new class extends EventEmitter {
     constructor() {
@@ -461,6 +473,11 @@ function setupChannel(target, channel) {
   target.on('internalMessage', function(message, handle) {
     // Once acknowledged - continue sending handles.
     if (message.cmd === 'NODE_HANDLE_ACK') {
+      if (target._pendingHandle) {
+        target._pendingHandle.close();
+        target._pendingHandle = null;
+      }
+
       assert(Array.isArray(target._handleQueue));
       var queue = target._handleQueue;
       target._handleQueue = null;
@@ -587,13 +604,16 @@ function setupChannel(target, channel) {
     var err = channel.writeUtf8String(req, string, handle);
 
     if (err === 0) {
-      if (handle && !this._handleQueue)
-        this._handleQueue = [];
+      if (handle) {
+        if (!this._handleQueue)
+          this._handleQueue = [];
+        if (obj && obj.postSend)
+          obj.postSend(handle, target);
+      }
+
       req.oncomplete = function() {
         if (this.async === true)
           control.unref();
-        if (obj && obj.postSend)
-          obj.postSend(handle);
         if (typeof callback === 'function')
           callback(null);
       };
@@ -654,6 +674,11 @@ function setupChannel(target, channel) {
     // This marks the fact that the channel is actually disconnected.
     this._channel = null;
 
+    if (this._pendingHandle) {
+      this._pendingHandle.close();
+      this._pendingHandle = null;
+    }
+
     var fired = false;
     function finish() {
       if (fired) return;
diff --git a/test/sequential/test-child-process-pass-fd.js b/test/sequential/test-child-process-pass-fd.js
new file mode 100644
index 00000000000000..a49baa31a9b9aa
--- /dev/null
+++ b/test/sequential/test-child-process-pass-fd.js
@@ -0,0 +1,53 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const fork = require('child_process').fork;
+const net = require('net');
+
+if ((process.config.variables.arm_version === '6') ||
+    (process.config.variables.arm_version === '7')) {
+  common.skip('Too slow for armv6 and armv7 bots');
+  return;
+}
+
+const N = 80;
+
+if (process.argv[2] !== 'child') {
+  for (let i = 0; i < N; ++i) {
+    const worker = fork(__filename, ['child', common.PORT + i]);
+    worker.once('message', common.mustCall((msg, handle) => {
+      assert.strictEqual(msg, 'handle');
+      assert.ok(handle);
+      worker.send('got');
+
+      let recvData = '';
+      handle.on('data', common.mustCall((data) => {
+        recvData += data;
+      }));
+
+      handle.on('end', () => {
+        assert.strictEqual(recvData, 'hello');
+        worker.kill();
+      });
+    }));
+  }
+} else {
+  let socket;
+  const port = process.argv[3];
+  let cbcalls = 0;
+  function socketConnected() {
+    if (++cbcalls === 2)
+      process.send('handle', socket);
+  }
+
+  const server = net.createServer((c) => {
+    process.once('message', function(msg) {
+      assert.strictEqual(msg, 'got');
+      c.end('hello');
+    });
+    socketConnected();
+  });
+  server.listen(port, common.localhostIPv4, () => {
+    socket = net.connect(port, common.localhostIPv4, socketConnected);
+  });
+}