StreamWrap doesn't properly close the wrapped connection

[tigerbot]

• Version: v8.2.1
• Platform: Linux 4.4.0-87-generic Update binnary alternatives #110-Ubuntu SMP x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: TLS

When emitting anything other than a fully opened net.Socket object to a TLS server that stream/connection gets wrapped in a way that prevents it from being properly closed when the decrypted side of the connection is destroyed. This makes it difficult to wrap encrypted sockets to get around issue #8752 when I need to peek at the data on the connection before giving it the the TLS server.

You can see that the client connection never receives the close event in the following test case

'use strict';
function log() {
  var args = Array.prototype.slice.call(arguments);
  args.unshift(process.uptime().toFixed(3));
  console.log.apply(console, args);
}

var net = require('net');
var tls = require('tls');
var socketPair = require('socket-pair');
var tlsOpts = require('localhost.daplie.me-certificates').merge({});

var tlsServer = tls.createServer(tlsOpts, function (socket) {
  socket.on('close', function () {
    log('decrypted connection closed');
  });
  socket.write('Hello World');
  socket.destroy();
});

function wrapConn(conn, firstChunk) {
  var reader = socketPair.create(function (err, writer) {
    if (err) {
      reader.emit('error', err);
      return;
    }

    conn.pipe(writer);
    writer.pipe(conn);
    process.nextTick(function () {
      conn.unshift(firstChunk);
    });

    conn.on('close', writer.destroy.bind(writer));
    writer.on('close', conn.destroy.bind(conn));
  });

  return reader;
}

var server = net.createServer(function (conn) {
  // tlsServer.emit('connection', conn);
  conn.once('data', function (chunk) {
    // use data to determine what to do with connection.
    tlsServer.emit('connection', wrapConn(conn, chunk));
  });
});

server.listen(0, function () {
  var port = server.address().port;
  var conn = tls.connect({port: port, rejectUnauthorized: false}, function () {
    conn.on('data', function (chunk) {
      log('client received data:', chunk.toString());
    });
    conn.on('close', function () {
      log('client connection closed');
      server.close();
      socketPair.closeAll();
    });
  });
});

[coolaj86]

following

[mcollina]

Is this a bug just in Node 8, or in the previous Node.js versions as well?

[tigerbot]

I just ran the script I included earlier using node 7.10.0 and encountered the same problem.

[mcollina]

@tigerbot there is no link to the source code of socket-pair. Can you please rewrite the example to not use that module?
The problem it is supposed to fix is fixed in:
b23d414

See #12716 (comment).

[coolaj86]

Here is the source of socket-pair:

https://git.daplie.com/Daplie/socket-pair/blob/master/lib/socket-pair.js

(also fixed the package.json to use an https link rather than a git link)

[coolaj86]

@mcollina This is a workaround that's about 4 levels deep. The topmost error that needs to be tested is #8752.

However, if that solution happens to work now, it only means that the workaround for this current issue is to use that prior solution... as a workaround - meaning that there is still a bug.

@tigerbot Have you tried using stream-pair? Or nothing at all?

[tigerbot]

I recall briefly trying stream-pair without success, but I think that was before I had narrowed down the problem so it's possible I had encountered some other issue. I do however know that our other method for wrapping a socket has the same problem.

var sockFuncs = [
  'address',
  'destroy',
  'ref',
  'unref',
  'setEncoding',
  'setKeepAlive',
  'setNoDelay',
  'setTimeout',
];
function wrapConnDuplex(conn, firstChunk) {
  var myDuplex = new require('stream').Duplex();

  // Handle everything needed for the write part of the Duplex. We need to overwrite the
  // `end` function because there is no other way to know when the other side calls it.
  myDuplex._write = conn.write.bind(conn);
  myDuplex.end = conn.end.bind(conn);

  // Handle everything needed for the read part of the Duplex. See the example under
  // https://nodejs.org/api/stream.html#stream_readable_push_chunk_encoding.
  myDuplex._read = function () {
    conn.resume();
  };
  if (!myDuplex.push(firstChunk)) {
    conn.pause();
  }
  conn.on('data', function (chunk) {
    if (!myDuplex.push(chunk)) {
      conn.pause();
    }
  });
  conn.on('end', function () {
    myDuplex.push(null);
  });

  // Handle the the things not directly related to reading or writing
  conn.on('error', function (err) {
    console.error('[error] wrapped socket errored - ' + err.toString());
    myDuplex.emit('error', err);
  });
  conn.on('close', function () {
    myDuplex.emit('close');
  });
  conn.on('timeout', function () {
    myDuplex.emit('timeout');
  });
  sockFuncs.forEach(function (name) {
    if (typeof conn[name] !== 'function') {
      console.warn('expected `'+name+'` to be a function on wrapped socket');
    } else {
      myDuplex[name] = conn[name].bind(conn);
    }
  });

  return myDuplex;
}

If you use the same code I included above and replace the wrapConn with this function you can see the same problem.

[tigerbot]

I just tested again with stream-pair and it's still hanging for me. Just like before you can replace wrapConn with the function below to reproduce the problem.

function wrapConnStreamPair(conn, firstChunk) {
  var reader = require('stream-pair').create();
  var writer = reader.other;

  writer.write(firstChunk);
  conn.pipe(writer);
  writer.pipe(conn);

  writer.on('close', conn.destroy.bind(conn));
  if (writer.destroy) {
    conn.on('close', writer.destroy.bind(writer));
  }

  return reader;
}

[tigerbot]

Also on the note of not trying to wrap it, if you use the following function instead of wrapConn you can not only get it to hang forever, but you don't even get the events for the decrypted side being closed or the data being received.

function noWrap(conn, firstChunk) {
  process.nextTick(function () {
    conn.unshift(firstChunk);
  });
  return conn;
}

[coolaj86]

This is still an issues in node v9.11.1

[jasnell]

@addaleax ... should this have been resolved with the refactoring you did here?