From 2bc7c3a8dd593aa502c9995633838d2b7470d8a4 Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Thu, 25 May 2017 12:29:05 +0200 Subject: [PATCH] child_process: fix handleless NODE_HANDLE handling It is possible that `recvmsg()` may return an error on ancillary data reception when receiving a `NODE_HANDLE` message (for example `MSG_CTRUNC`). This would end up, if the handle type was `net.Socket`, on a `message` event with a non null but invalid `sendHandle`. To improve the situation, send a `NODE_HANDLE_NACK` that'll cause the sending process to retransmit the message again. In case the same message is retransmitted 3 times without success, close the handle and print a warning. Fixes: https://github.com/nodejs/node/issues/9706 PR-URL: https://github.com/nodejs/node/pull/13235 Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig --- lib/internal/child_process.js | 61 ++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 16 deletions(-) diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index 6033e9705e4969..f11d81c6d90b6b 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -29,6 +29,9 @@ module.exports = { getSocketList }; +const MAX_HANDLE_RETRANSMISSIONS = 3; + + // this object contain function to convert TCP objects to native handle objects // and back again. const handleConversion = { @@ -94,17 +97,18 @@ const handleConversion = { return handle; }, - postSend: function(handle, options, target) { + postSend: function(message, handle, options, callback, 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 && !options.keepOpen) { if (target) { - // There can only be one _pendingHandle as passing handles are + // There can only be one _pendingMessage 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; + assert(!target._pendingMessage); + target._pendingMessage = + { callback, message, handle, options, retransmissions: 0 }; } else { handle.close(); } @@ -267,6 +271,11 @@ function getHandleWrapType(stream) { return false; } +function closePendingHandle(target) { + target._pendingMessage.handle.close(); + target._pendingMessage = null; +} + ChildProcess.prototype.spawn = function(options) { const self = this; @@ -437,7 +446,7 @@ class Control extends EventEmitter { function setupChannel(target, channel) { target._channel = channel; target._handleQueue = null; - target._pendingHandle = null; + target._pendingMessage = null; const control = new Control(channel); @@ -489,16 +498,31 @@ function setupChannel(target, channel) { // handlers will go through this 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; + if (message.cmd === 'NODE_HANDLE_ACK' || + message.cmd === 'NODE_HANDLE_NACK') { + + if (target._pendingMessage) { + if (message.cmd === 'NODE_HANDLE_ACK') { + closePendingHandle(target); + } else if (target._pendingMessage.retransmissions++ === + MAX_HANDLE_RETRANSMISSIONS) { + closePendingHandle(target); + process.emitWarning('Handle did not reach the receiving process ' + + 'correctly', 'SentHandleNotReceivedWarning'); + } } assert(Array.isArray(target._handleQueue)); var queue = target._handleQueue; target._handleQueue = null; + if (target._pendingMessage) { + target._send(target._pendingMessage.message, + target._pendingMessage.handle, + target._pendingMessage.options, + target._pendingMessage.callback); + } + for (var i = 0; i < queue.length; i++) { var args = queue[i]; target._send(args.message, args.handle, args.options, args.callback); @@ -513,6 +537,12 @@ function setupChannel(target, channel) { if (message.cmd !== 'NODE_HANDLE') return; + // It is possible that the handle is not received because of some error on + // ancillary data reception such as MSG_CTRUNC. In this case, report the + // sender about it by sending a NODE_HANDLE_NACK message. + if (!handle) + return target._send({ cmd: 'NODE_HANDLE_NACK' }, null, true); + // Acknowledge handle receival. Don't emit error events (for example if // the other side has disconnected) because this call to send() is not // initiated by the user and it shouldn't be fatal to be unable to ACK @@ -623,7 +653,8 @@ function setupChannel(target, channel) { net._setSimultaneousAccepts(handle); } } else if (this._handleQueue && - !(message && message.cmd === 'NODE_HANDLE_ACK')) { + !(message && (message.cmd === 'NODE_HANDLE_ACK' || + message.cmd === 'NODE_HANDLE_NACK'))) { // Queue request anyway to avoid out-of-order messages. this._handleQueue.push({ callback: callback, @@ -645,7 +676,7 @@ function setupChannel(target, channel) { if (!this._handleQueue) this._handleQueue = []; if (obj && obj.postSend) - obj.postSend(handle, options, target); + obj.postSend(message, handle, options, callback, target); } req.oncomplete = function() { @@ -662,7 +693,7 @@ function setupChannel(target, channel) { } else { // Cleanup handle on error if (obj && obj.postSend) - obj.postSend(handle, options); + obj.postSend(message, handle, options, callback); if (!options.swallowErrors) { const ex = errnoException(err, 'write'); @@ -711,10 +742,8 @@ 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; - } + if (this._pendingMessage) + closePendingHandle(this); var fired = false; function finish() {