Already buffered data when creating a TLSSocket doesn't trigger/throw 'error'

[jameshartig]

Brought over from nodejs/node-v0.x-archive#9355

If you pass a socket with already buffered data to TLSSocket and there's an error like:

[Error: 139745410942944:error:1408A0C1:SSL routines:SSL3_GET_CLIENT_HELLO:no shared cipher:s3_srvr.c:1352]

It will fire as a _tlsError immediately while in the constructor instead of either throwing an error (if there were no error handlers) or waiting till the next tick to fire the _tlsError.

This can be solved by wrapping this._init(socket) in a process.nextTick or delaying:

  // Socket already has some buffered data - emulate receiving it
  if (socket && socket._readableState.length) {
    var buf;
    while ((buf = socket.read()) !== null)
      this.ssl.receive(buf);
  }

It seems like the second option (delaying the initial buffer read) is better but let me know what you guys think.

[brendanashworth]

Could you, by chance, provide us some example code illustrating the issue? That might speed up a fix.

[jameshartig]

Here's some pseudo code based a project that is hindered by this. The project basically opens a port and allows websocket/HTTP/HTTPS/raw connections on the port. It's sniffing the first packet to see what type it is and then forwarding it onto the correct callback.
https://gist.github.com/fastest963/f1b6b8b6e642aa427240

Once again that's pseudo code but its pretty close to the actual code. Basically when calling the httpsConnectionListener, if there's a handshake error (which there would be since new Server() was called without a cert), it will fire _tlsError after processing the initial data in the buffer (see: https://github.com/iojs/io.js/blob/v1.x/lib/_tls_wrap.js#L396). However we still haven't even added the listener for _tlsError since were still stuck at https://github.com/iojs/io.js/blob/v1.x/lib/_tls_wrap.js#L686 and the listener is added on line 725 (https://github.com/iojs/io.js/blob/v1.x/lib/_tls_wrap.js#L725).

There is initial data because the server is inspecting the first packet before sending it to the tls wrapper to make sure its actually a tls header (indicating an HTTPS connection).

[jameshartig]

Any update on this? If someone could just point me in the right direction I'd love to submit a PR to fix this. I'm just not sure which option I should go about doing since I'm not very familiar with this code.

Personally I think putting sending the initial data behind a process.nextTick is maybe the quickest solution but might introduce weird bugs if we read data during the rest of the tick? Once again, not too familiar with this stuff to know if that's possible or not.

A possibly better option would be to not call _init from the constructor and instead make it a separate step we can do so we can set up listeners before calling it. This might be an API-change though so it might be more difficult to land...

[brendanashworth]

I don't really know what to say; from the links through the code that you've given, you probably know enough to try and do a PR (at least more than what I know, TLS-wise). I'd say you should throw together some code and PR it in, or at least chat with one of the crypto's in the io.js irc.

Personally what you're trying to do seems hackish to me and looks like it uses at least a few undocumented things, and it really doesn't look like there is an easy, supported way to do it. Good luck though.

[jameshartig]

@brendanashworth, I appreciate you taking the time to look at it. I've opened a PR above that fixes this issue. I decided to go with using process.nextTick to read data after we've added event listeners on _tlsError. I brought up the issue in IRC a few times with no reply.

[brendanashworth]

Fixed by #1496 / 061342a.