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: #14605

PR-URL: #23654
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

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);

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()),
+    );
+  });
+});

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);
+  }
+}