Skip to content

Commit

Permalink
tls: close StreamWrap and its stream correctly
Browse files Browse the repository at this point in the history
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: #14605

PR-URL: #23654
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
oyyd authored and jasnell committed Oct 21, 2018
1 parent dd75624 commit 7db4281
Show file tree
Hide file tree
Showing 3 changed files with 201 additions and 0 deletions.
14 changes: 14 additions & 0 deletions lib/internal/wrap_js_stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
69 changes: 69 additions & 0 deletions test/parallel/test-tls-destroy-stream.js
Original file line number Diff line number Diff line change
@@ -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()),
);
});
});
118 changes: 118 additions & 0 deletions test/parallel/test-wrap-js-stream-destroy.js
Original file line number Diff line number Diff line change
@@ -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);
}
}

0 comments on commit 7db4281

Please sign in to comment.