From 5486ed462008b29c07599f8ce2207c8de48bf213 Mon Sep 17 00:00:00 2001 From: Ouyang Yadong Date: Sun, 14 Oct 2018 20:13:03 +0800 Subject: [PATCH] tls: close StreamWrap and its stream correctly When sockets of the "net" module destroyed, they will call `this._handle.close()` which will also emit EOF if not emitted before. This feature makes sockets on the other side emit "end" and "close" even though we haven't called `end()`. As `stream` of `StreamWrap` are likely to be instances of `net.Socket`, calling `destroy()` manually will avoid issues that don't properly close wrapped connections. Fixes: https://github.com/nodejs/node/issues/14605 PR-URL: https://github.com/nodejs/node/pull/23654 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- lib/internal/wrap_js_stream.js | 14 +++ test/parallel/test-tls-destroy-stream.js | 69 +++++++++++ test/parallel/test-wrap-js-stream-destroy.js | 118 +++++++++++++++++++ 3 files changed, 201 insertions(+) create mode 100644 test/parallel/test-tls-destroy-stream.js create mode 100644 test/parallel/test-wrap-js-stream-destroy.js diff --git a/lib/internal/wrap_js_stream.js b/lib/internal/wrap_js_stream.js index e6de433676f7d3..7ca7ff8bf49d25 100644 --- a/lib/internal/wrap_js_stream.js +++ b/lib/internal/wrap_js_stream.js @@ -68,6 +68,12 @@ class JSStreamWrap extends Socket { if (this._handle) this._handle.emitEOF(); }); + // Some `Stream` don't pass `hasError` parameters when closed. + stream.once('close', () => { + // Errors emitted from `stream` have also been emitted to this instance + // so that we don't pass errors to `destroy()` again. + this.destroy(); + }); super({ handle, manualStart: true }); this.stream = stream; @@ -188,6 +194,14 @@ class JSStreamWrap extends Socket { doClose(cb) { const handle = this._handle; + // When sockets of the "net" module destroyed, they will call + // `this._handle.close()` which will also emit EOF if not emitted before. + // This feature makes sockets on the other side emit "end" and "close" + // even though we haven't called `end()`. As `stream` are likely to be + // instances of `net.Socket`, calling `stream.destroy()` manually will + // avoid issues that don't properly close wrapped connections. + this.stream.destroy(); + setImmediate(() => { // Should be already set by net.js assert.strictEqual(this._handle, null); diff --git a/test/parallel/test-tls-destroy-stream.js b/test/parallel/test-tls-destroy-stream.js new file mode 100644 index 00000000000000..eb7a2ca338bd40 --- /dev/null +++ b/test/parallel/test-tls-destroy-stream.js @@ -0,0 +1,69 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) common.skip('missing crypto'); + +const fixtures = require('../common/fixtures'); +const makeDuplexPair = require('../common/duplexpair'); +const net = require('net'); +const assert = require('assert'); +const tls = require('tls'); + +// This test ensures that an instance of StreamWrap should emit "end" and +// "close" when the socket on the other side call `destroy()` instead of +// `end()`. +// Refs: https://github.com/nodejs/node/issues/14605 +const CONTENT = 'Hello World'; +const tlsServer = tls.createServer( + { + key: fixtures.readSync('test_key.pem'), + cert: fixtures.readSync('test_cert.pem'), + ca: [fixtures.readSync('test_ca.pem')], + }, + (socket) => { + socket.on('error', common.mustNotCall()); + socket.on('close', common.mustCall()); + socket.write(CONTENT); + socket.destroy(); + }, +); + +const server = net.createServer((conn) => { + conn.on('error', common.mustNotCall()); + // Assume that we want to use data to determine what to do with connections. + conn.once('data', common.mustCall((chunk) => { + const { clientSide, serverSide } = makeDuplexPair(); + serverSide.on('close', common.mustCall(() => { + conn.destroy(); + })); + clientSide.pipe(conn); + conn.pipe(clientSide); + + conn.on('close', common.mustCall(() => { + clientSide.destroy(); + })); + clientSide.on('close', common.mustCall(() => { + conn.destroy(); + })); + + process.nextTick(() => { + conn.unshift(chunk); + }); + + tlsServer.emit('connection', serverSide); + })); +}); + +server.listen(0, () => { + const port = server.address().port; + const conn = tls.connect({ port, rejectUnauthorized: false }, () => { + conn.on('data', common.mustCall((data) => { + assert.strictEqual(data.toString('utf8'), CONTENT); + })); + conn.on('error', common.mustNotCall()); + conn.on( + 'close', + common.mustCall(() => server.close()), + ); + }); +}); diff --git a/test/parallel/test-wrap-js-stream-destroy.js b/test/parallel/test-wrap-js-stream-destroy.js new file mode 100644 index 00000000000000..16d3e75e2ca7e3 --- /dev/null +++ b/test/parallel/test-wrap-js-stream-destroy.js @@ -0,0 +1,118 @@ +'use strict'; + +const common = require('../common'); +const StreamWrap = require('_stream_wrap'); +const net = require('net'); + +// This test ensures that when we directly call `socket.destroy()` without +// having called `socket.end()` on an instance of streamWrap, it will +// still emit EOF which makes the socket on the other side emit "end" and +// "close" events, and vice versa. +{ + let port; + const server = net.createServer((socket) => { + socket.on('error', common.mustNotCall()); + socket.on('end', common.mustNotCall()); + socket.on('close', common.mustCall()); + socket.destroy(); + }); + + server.listen(() => { + port = server.address().port; + createSocket(); + }); + + function createSocket() { + let streamWrap; + const socket = new net.connect({ + port, + }, () => { + socket.on('error', common.mustNotCall()); + socket.on('end', common.mustCall()); + socket.on('close', common.mustCall()); + + streamWrap.on('error', common.mustNotCall()); + // The "end" events will be emitted which is as same as + // the same situation for an instance of `net.Socket` without + // `StreamWrap`. + streamWrap.on('end', common.mustCall()); + // Destroying a socket in the server side should emit EOF and cause + // the corresponding client-side socket closed. + streamWrap.on('close', common.mustCall(() => { + server.close(); + })); + }); + streamWrap = new StreamWrap(socket); + } +} + +// Destroy the streamWrap and test again. +{ + let port; + const server = net.createServer((socket) => { + socket.on('error', common.mustNotCall()); + socket.on('end', common.mustCall()); + socket.on('close', common.mustCall(() => { + server.close(); + })); + // Do not `socket.end()` and directly `socket.destroy()`. + }); + + server.listen(() => { + port = server.address().port; + createSocket(); + }); + + function createSocket() { + let streamWrap; + const socket = new net.connect({ + port, + }, () => { + socket.on('error', common.mustNotCall()); + socket.on('end', common.mustNotCall()); + socket.on('close', common.mustCall()); + + streamWrap.on('error', common.mustNotCall()); + streamWrap.on('end', common.mustNotCall()); + // Destroying a socket in the server side should emit EOF and cause + // the corresponding client-side socket closed. + streamWrap.on('close', common.mustCall()); + streamWrap.destroy(); + }); + streamWrap = new StreamWrap(socket); + } +} + +// Destroy the client socket and test again. +{ + let port; + const server = net.createServer((socket) => { + socket.on('error', common.mustNotCall()); + socket.on('end', common.mustCall()); + socket.on('close', common.mustCall(() => { + server.close(); + })); + }); + + server.listen(() => { + port = server.address().port; + createSocket(); + }); + + function createSocket() { + let streamWrap; + const socket = new net.connect({ + port, + }, () => { + socket.on('error', common.mustNotCall()); + socket.on('end', common.mustNotCall()); + socket.on('close', common.mustCall()); + + streamWrap.on('error', common.mustNotCall()); + streamWrap.on('end', common.mustNotCall()); + streamWrap.on('close', common.mustCall()); + socket.destroy(); + }); + streamWrap = new StreamWrap(socket); + } +}