Skip to content

tls.connect() emitting twice an error event #1119

Closed
Closed
tls.connect() emitting twice an error event#1119
Labels
confirmed-bugIssues and PRs for confirmed bugs.tlsIssues and PRs related to the tls subsystem.

Description

@user447463
user447463
opened on Mar 10, 2015

With certain hosts (and the particular cipher settings used below) tls.connect() appears to emit an error event twice.

var host='85.158.224.100';

var options = { rejectUnauthorized: false,
    servername: host,
    ciphers: 'EXPORT' };

require('tls').connect(443, host, options, function() {
    console.log('success');
}).on('error', function(error) {
    console.log(error);
    console.log();
});

Actual Result

{ [Error: socket hang up] code: 'ECONNRESET' }

[Error: 101057795:error:14094410:SSL routines:SSL3_READ_BYTES:sslv3 alert handsh
ake failure:openssl\ssl\s3_pkt.c:1293:SSL alert number 40
101057795:error:140780E5:SSL routines:SSL23_READ:ssl handshake failure:openssl\s
sl\s23_lib.c:138:
]

Expected Result

[Error: 101057795:error:14094410:SSL routines:SSL3_READ_BYTES:sslv3 alert handsh
ake failure:openssl\ssl\s3_pkt.c:1293:SSL alert number 40
101057795:error:140780E5:SSL routines:SSL23_READ:ssl handshake failure:openssl\s
sl\s23_lib.c:138:
]

Quick assumption would be it has something to do with SNI, as it does not occur if servername is not set.

Activity

  1. Fishrock123
    added
    tlsIssues and PRs related to the tls subsystem.
    on Mar 10, 2015
  2. mscdex
    added
    confirmed-bugIssues and PRs for confirmed bugs.
    on Mar 10, 2015

  3. user447463 commented on Mar 11, 2015

    @user447463
    user447463
    on Mar 11, 2015
    Author

    Update: It is not necessarily SNI related, as it also happens without servername being given.

    var host='204.11.109.195';

var options = { rejectUnauthorized: false,
    ciphers: 'EXPORT' };

require('tls').connect(443, host, options, function() {
    console.log('success');
}).on('error', function(error) {
    console.log(error);
    console.log();
});

  4. skenqbx commented on Mar 11, 2015

    @skenqbx
    skenqbx
    on Mar 11, 2015
    Contributor

    The socket hangup is only emitted when socket._hadError === false and the ssl error is only emitted when socket._writableState.errorEmitted === false. Therefore both get emitted.

    The following patch lets onHangUp() check socket._writableState.errorEmitted in the next tick to ensure the ssl error is emitted, since the socket hangup does have less useful information.

    diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index fcc216b..5f93270 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -947,10 +947,17 @@ exports.connect = function(/* [port, host], options, cb */) {
     // NOTE: This logic is shared with _http_client.js
     if (!socket._hadError) {
       socket._hadError = true;
-      var error = new Error('socket hang up');
-      error.code = 'ECONNRESET';
-      socket.destroy();
-      socket.emit('error', error);
+
+      setImmediate(function() {
+        if (socket._writableState.errorEmitted)
+          return;
+        socket._writableState.errorEmitted = true;
+
+        var error = new Error('socket hang up');
+        error.code = 'ECONNRESET';
+        socket.destroy();
+        socket.emit('error', error);
+      });
     }
   }
   socket.once('end', onHangUp);

    Another issue arose while checking on('close', function(hadError) {}); hadError is always false. Not sure why yet.

  5. Fishrock123 commented on May 13, 2015

    @Fishrock123
    Fishrock123
    on May 13, 2015
    Contributor

    cc @indutny?

  6. indutny commented on May 15, 2015

    @indutny
    indutny
    on May 15, 2015
    Member

    Looking.

  7. indutny commented on May 15, 2015

    @indutny
    indutny
    on May 15, 2015
    Member

    @skenqbx this change looks good to me. May I ask you to send a PR for it? I'll send a PR for fixing the 'close' event's argument in TLS.

    Many thanks!

  8. indutny
    added a commit that references this issue on May 15, 2015

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

    4badb4e

  9. indutny commented on May 15, 2015

    @indutny
    indutny
    on May 15, 2015
    Member

    @skenqbx here it is #1711

  10. skenqbx commented on May 18, 2015

    @skenqbx
    skenqbx
    on May 18, 2015
    Contributor

    Will do, give me until end of the week.

  11. indutny
    added a commit that references this issue on May 22, 2015

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

    80342f6

  12. indutny commented on May 22, 2015

    @indutny
    indutny
    on May 22, 2015
    Member

    Errors are now properly reported as of 80342f6

  13. silverwind
    closed this as completedon May 22, 2015
  14. silverwind
    reopened this on May 22, 2015

  15. silverwind commented on May 22, 2015

    @silverwind
    silverwind
    on May 22, 2015
    Contributor

    sorry, didn't see there's still a task pending here at first.

  16. skenqbx commented on May 22, 2015

    @skenqbx
    skenqbx
    on May 22, 2015
    Contributor

    The double emission of errors seems to be handled by #1711.

  17. indutny
    added a commit that references this issue on May 22, 2015

    tls: emit errors happening before handshake finish

    2a71f02

  18. skenqbx commented on May 23, 2015

    @skenqbx
    skenqbx
    on May 23, 2015
    Contributor

    @silverwind This can be closed now, thank you.

  19. Fishrock123
    closed this as completedon May 23, 2015
  20. andrewdeandrade
    added 2 commits that reference this issue on Jun 3, 2015

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

    b469ba7

    tls: emit errors happening before handshake finish

    43a77d4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    confirmed-bugIssues and PRs for confirmed bugs.tlsIssues and PRs related to the tls subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions