tls: use `.destroy(err)` instead of destroy+emit

Emit errors using `.destroy(err)` instead of `.destroy()` and
`.emit('error', err)`. Otherwise `close` event is emitted with the
`error` argument set to `false`, even if the connection was torn down
because of the error.

See: https://github.com/nodejs/io.js/issues/1119
PR-URL: https://github.com/nodejs/io.js/pull/1711
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 5a35c3b..544fe95 100644 (file)
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -38,7 +38,7 @@ function onhandshakestart() {
     // callback to destroy the connection right now, it would crash and burn.
     setImmediate(function() {
       var err = new Error('TLS session renegotiation attack detected.');
-      self._tlsError(err);
+      self._emitTLSError(err);
     });
   }
 }
@@ -233,7 +233,7 @@ function TLSSocket(socket, options) {
   // Proxy for API compatibility
   this.ssl = this._handle;
 
-  this.on('error', this._tlsError);
+  this.on('error', this._emitTLSError);
 
   this._init(socket, wrap);
 
@@ -363,8 +363,7 @@ TLSSocket.prototype._init = function(socket, wrap) {
 
     // Destroy socket if error happened before handshake's finish
     if (!self._secureEstablished) {
-      self._tlsError(err);
-      self.destroy();
+      self.destroy(self._tlsError(err));
     } else if (options.isServer &&
                rejectUnauthorized &&
                /peer did not return a certificate/.test(err.message)) {
@@ -372,7 +371,7 @@ TLSSocket.prototype._init = function(socket, wrap) {
       self.destroy();
     } else {
       // Throw error
-      self._tlsError(err);
+      self._emitTLSError(err);
     }
   };
 
@@ -416,7 +415,7 @@ TLSSocket.prototype._init = function(socket, wrap) {
   // Assume `tls.connect()`
   if (wrap) {
     wrap.on('error', function(err) {
-      self._tlsError(err);
+      self._emitTLSError(err);
     });
   } else {
     assert(!socket);
@@ -472,20 +471,27 @@ TLSSocket.prototype.getTLSTicket = function getTLSTicket() {
 };
 
 TLSSocket.prototype._handleTimeout = function() {
-  this._tlsError(new Error('TLS handshake timeout'));
+  this._emitTLSError(new Error('TLS handshake timeout'));
+};
+
+TLSSocket.prototype._emitTLSError = function(err) {
+  var e = this._tlsError(err);
+  if (e)
+    this.emit('error', e);
 };
 
 TLSSocket.prototype._tlsError = function(err) {
   this.emit('_tlsError', err);
   if (this._controlReleased)
-    this.emit('error', err);
+    return err;
+  return null;
 };
 
 TLSSocket.prototype._releaseControl = function() {
   if (this._controlReleased)
     return false;
   this._controlReleased = true;
-  this.removeListener('error', this._tlsError);
+  this.removeListener('error', this._emitTLSError);
   return true;
 };
 
@@ -717,7 +723,11 @@ function Server(/* [options], listener */) {
     });
 
     var errorEmitted = false;
-    socket.on('close', function() {
+    socket.on('close', function(err) {
+      // Closed because of error - no need to emit it twice
+      if (err)
+        return;
+
       // Emit ECONNRESET
       if (!socket._controlReleased && !errorEmitted) {
         errorEmitted = true;
@@ -936,8 +946,7 @@ exports.connect = function(/* [port, host], options, cb */) {
       socket.authorizationError = verifyError.code || verifyError.message;
 
       if (options.rejectUnauthorized) {
-        socket.emit('error', verifyError);
-        socket.destroy();
+        socket.destroy(verifyError);
         return;
       } else {
         socket.emit('secureConnect');
@@ -957,8 +966,7 @@ exports.connect = function(/* [port, host], options, cb */) {
       socket._hadError = true;
       var error = new Error('socket hang up');
       error.code = 'ECONNRESET';
-      socket.destroy();
-      socket.emit('error', error);
+      socket.destroy(error);
     }
   }
   socket.once('end', onHangUp);

diff --git a/test/parallel/test-tls-close-error.js b/test/parallel/test-tls-close-error.js
new file mode 100644 (file)
index 0000000..556fb1d
--- /dev/null
+++ b/test/parallel/test-tls-close-error.js
@@ -0,0 +1,42 @@
+'use strict';
+
+var assert = require('assert');
+var common = require('../common');
+
+if (!common.hasCrypto) {
+  console.log('1..0 # Skipped: missing crypto');
+  process.exit();
+}
+var tls = require('tls');
+
+var fs = require('fs');
+var net = require('net');
+
+var errorCount = 0;
+var closeCount = 0;
+
+var server = tls.createServer({
+  key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'),
+  cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem')
+}, function(c) {
+}).listen(common.PORT, function() {
+  var c = tls.connect(common.PORT, function() {
+    assert(false, 'should not be called');
+  });
+
+  c.on('error', function(err) {
+    errorCount++;
+  });
+
+  c.on('close', function(err) {
+    if (err)
+      closeCount++;
+
+    server.close();
+  });
+});
+
+process.on('exit', function() {
+  assert.equal(errorCount, 1);
+  assert.equal(closeCount, 1);
+});