Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: emit errors on close whilst async action #1702

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ function loadSession(self, hello, cb) {
if (err)
return cb(err);

if (!self._handle)
return cb(new Error('Socket is closed'));

// NOTE: That we have disabled OpenSSL's internal session storage in
// `node_crypto.cc` and hence its safe to rely on getting servername only
// from clienthello or this place.
Expand Down Expand Up @@ -91,6 +94,9 @@ function loadSNI(self, servername, cb) {
if (err)
return cb(err);

if (!self._handle)
return cb(new Error('Socket is closed'));

// TODO(indutny): eventually disallow raw `SecureContext`
if (context)
self._handle.sni_context = context.context || context;
Expand Down Expand Up @@ -127,6 +133,9 @@ function requestOCSP(self, hello, ctx, cb) {
if (err)
return cb(err);

if (!self._handle)
return cb(new Error('Socket is closed'));

if (response)
self._handle.setOCSPResponse(response);
cb(null);
Expand Down Expand Up @@ -157,6 +166,9 @@ function oncertcb(info) {
if (err)
return self.destroy(err);

if (!self._handle)
return cb(new Error('Socket is closed'));

self._handle.certCbDone();
});
});
Expand All @@ -179,6 +191,9 @@ function onnewsession(key, session) {
return;
once = true;

if (!self._handle)
return cb(new Error('Socket is closed'));

self._handle.newSessionDone();

self._newSessionPending = false;
Expand Down Expand Up @@ -290,13 +305,18 @@ TLSSocket.prototype._wrapHandle = function(handle) {
});

this.on('close', function() {
this._destroySSL();
// Make sure we are not doing it on OpenSSL's stack
setImmediate(destroySSL, this);
res = null;
});

return res;
};

function destroySSL(self) {
self._destroySSL();
}

TLSSocket.prototype._destroySSL = function _destroySSL() {
if (!this.ssl) return;
this.ssl.destroySSL();
Expand Down
28 changes: 22 additions & 6 deletions src/tls_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,6 @@ TLSWrap::~TLSWrap() {
sni_context_.Reset();
#endif // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB

// Move all writes to pending
MakePending();

// And destroy
InvokeQueued(UV_ECANCELED);

ClearError();
}

Expand Down Expand Up @@ -337,6 +331,9 @@ void TLSWrap::EncOutCb(WriteWrap* req_wrap, int status) {
return;
}

if (wrap->ssl_ == nullptr)
return;

// Commit
NodeBIO::FromBIO(wrap->enc_out_)->Read(nullptr, wrap->write_size_);

Expand Down Expand Up @@ -554,6 +551,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
size_t count,
uv_stream_t* send_handle) {
CHECK_EQ(send_handle, nullptr);
CHECK_NE(ssl_, nullptr);

bool empty = true;

Expand Down Expand Up @@ -627,6 +625,11 @@ void TLSWrap::OnAfterWriteImpl(WriteWrap* w, void* ctx) {
void TLSWrap::OnAllocImpl(size_t suggested_size, uv_buf_t* buf, void* ctx) {
TLSWrap* wrap = static_cast<TLSWrap*>(ctx);

if (wrap->ssl_ == nullptr) {
*buf = uv_buf_init(nullptr, 0);
return;
}

size_t size = 0;
buf->base = NodeBIO::FromBIO(wrap->enc_in_)->PeekWritable(&size);
buf->len = size;
Expand Down Expand Up @@ -747,6 +750,10 @@ void TLSWrap::SetVerifyMode(const FunctionCallbackInfo<Value>& args) {
void TLSWrap::EnableSessionCallbacks(
const FunctionCallbackInfo<Value>& args) {
TLSWrap* wrap = Unwrap<TLSWrap>(args.Holder());
if (wrap->ssl_ == nullptr) {
return wrap->env()->ThrowTypeError(
"EnableSessionCallbacks after destroySSL");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the error message sounds like a TypeError

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it is worth submitting a follow-up fix PR. ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny I feel that the default types of Errors is not enough. We should at least have something like ValueError. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say it might be too expensive to create them in C++, unless I'm missing something obvious.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I locally created a patch which fixes all the inconsistent error messages like this, in the js files. And I had an internal/errors.js file which defined ValueError. Looks like that solves most of the wrong data error messages. But I am not sure if it is worthy introducing a new Error type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think anyone really checks the type of error. It kind of serves only an informational purpose when it pops up in the stderr.

}
wrap->enable_session_callbacks();
NodeBIO::FromBIO(wrap->enc_in_)->set_initial(kMaxHelloLength);
wrap->hello_parser_.Start(SSLWrap<TLSWrap>::OnClientHello,
Expand All @@ -757,7 +764,16 @@ void TLSWrap::EnableSessionCallbacks(

void TLSWrap::DestroySSL(const FunctionCallbackInfo<Value>& args) {
TLSWrap* wrap = Unwrap<TLSWrap>(args.Holder());

// Move all writes to pending
wrap->MakePending();

// And destroy
wrap->InvokeQueued(UV_ECANCELED);

// Destroy the SSL structure and friends
wrap->SSLWrap<TLSWrap>::DestroySSL();

delete wrap->clear_in_;
wrap->clear_in_ = nullptr;
}
Expand Down
73 changes: 73 additions & 0 deletions test/parallel/test-tls-async-cb-after-socket-end.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
'use strict';

var common = require('../common');

var assert = require('assert');
var path = require('path');
var fs = require('fs');
var constants = require('constants');

if (!common.hasCrypto) {
console.log('1..0 # Skipped: missing crypto');
process.exit();
}

var tls = require('tls');

var options = {
secureOptions: constants.SSL_OP_NO_TICKET,
key: fs.readFileSync(path.join(common.fixturesDir, 'test_key.pem')),
cert: fs.readFileSync(path.join(common.fixturesDir, 'test_cert.pem'))
};

var server = tls.createServer(options, function(c) {
});

var sessionCb = null;
var client = null;

server.on('newSession', function(key, session, done) {
done();
});

server.on('resumeSession', function(id, cb) {
sessionCb = cb;

next();
});

server.listen(1443, function() {
var clientOpts = {
port: 1443,
rejectUnauthorized: false,
session: false
};

var s1 = tls.connect(clientOpts, function() {
clientOpts.session = s1.getSession();
console.log('1st secure');

s1.destroy();
var s2 = tls.connect(clientOpts, function(s) {
console.log('2nd secure');

s2.destroy();
}).on('connect', function() {
console.log('2nd connected');
client = s2;

next();
});
});
});

function next() {
if (!client || !sessionCb)
return;

client.destroy();
setTimeout(function() {
sessionCb();
server.close();
}, 100);
}
1 change: 1 addition & 0 deletions test/parallel/test-tls-js-stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ var server = tls.createServer({

socket.end('hello');
socket.resume();
socket.destroy();
});

socket.once('close', function() {
Expand Down