tls: emit errors on close whilst async action #1702

indutny · 2015-05-14T10:46:01Z

When loading session, OCSP response, SNI, always check that the
self._handle is present. If it is not - the socket was closed - and we
should emit the error instead of throwing an uncaught exception.

Fix: nodejs/node-v0.x-archive#8780
Fix: #1696

indutny · 2015-05-14T10:47:06Z

cc @iojs/crypto

migounette · 2015-05-14T12:36:07Z

@indutny You fixed at the JS part by checking the _handle, but what I found during an intensive play with TLS is that the _encout was NULL (a C++ level). It is a kind of SSL Close issue race condition (for me), but I need to change the test case because it's very difficult for me to reproduce.

Thanks for the fix

indutny · 2015-05-18T18:34:50Z

@migounette @EricTheOne: I have updated the PR, please take a look

EricTheOne · 2015-05-19T10:05:09Z

@indutny thanks, applied both patches in order on v2.0.2, getting a segfault elsewhere (maybe it's progress):

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000000000d18d00 in node::StreamBase::EmitData(long, v8::Local<v8::Object>, v8::Local<v8::Object>) ()
(gdb) bt
#0  0x0000000000d18d00 in node::StreamBase::EmitData(long, v8::Local<v8::Object>, v8::Local<v8::Object>) ()
#1  0x0000000000d3cc8a in node::TLSWrap::OnReadSelf(long, uv_buf_t const*, uv_handle_type, void*) ()
#2  0x0000000000d40d19 in node::TLSWrap::OnReadImpl(long, uv_buf_t const*, uv_handle_type, void*) ()
#3  0x000000000084fa02 in v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) ()
#4  0x0000000000876c1b in v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) ()

indutny · 2015-05-19T11:19:08Z

@EricTheOne pushed one more commit, PTAL

EricTheOne · 2015-05-19T11:44:51Z

@indutny I've applied all three commits in order, on v2.0.2. Getting a different segfault now:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00000000030f1490 in ?? ()
(gdb) bt
#0  0x00000000030f1490 in ?? ()
#1  0x0000000000d40d29 in node::TLSWrap::OnReadImpl(long, uv_buf_t const*, uv_handle_type, void*) ()
#2  0x000000000084fa02 in v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) ()
#3  0x0000000000876c1b in v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) ()

indutny · 2015-05-22T19:35:58Z

@EricTheOne looks like I don't fully understand problem yet. Any way I can reproduce it locally?

indutny · 2015-05-29T15:23:22Z

Just a heads up, @evanlucas discovered a crash in a test: https://gist.github.com/evanlucas/6ab0a62ebe7ffa5d0ddf

It is reproducible after patching the test suite:

diff --git a/test/parallel/test-tls-js-stream.js b/test/parallel/test-tls-js-stream.js
index e156f44..292bd4f 100644
--- a/test/parallel/test-tls-js-stream.js
+++ b/test/parallel/test-tls-js-stream.js
@@ -61,6 +61,7 @@ var server = tls.createServer({

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

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

Going to investigate it in detail later.

indutny · 2015-05-29T18:28:36Z

@evanlucas and it is actually fixed by this commit ad3a799

indutny · 2015-05-29T18:28:49Z

Erm, not by commit, rather by this PR.

evanlucas · 2015-05-29T18:29:31Z

yay!

indutny · 2015-05-29T18:31:11Z

Which is kind of sad for the @EricTheOne's thing...

indutny · 2015-05-29T18:32:32Z

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/730/

When loading session, OCSP response, SNI, always check that the `self._handle` is present. If it is not - the socket was closed - and we should emit the error instead of throwing an uncaught exception. Fix: nodejs/node-v0.x-archive#8780 Fix: nodejs#1696

* Destroy `SSL*` and friends on a next tick to make sure that we are not doing it in one of the OpenSSL callbacks * Add more checks to the C++ methods that might be invoked during destructor's pending queue cleanup Fix: nodejs/node-v0.x-archive#8780 Fix: nodejs#1696

indutny · 2015-06-01T16:39:15Z

@migounette @EricTheOne any news? ;)

indutny · 2015-06-01T16:39:42Z

CI (with style fixes): https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/747/

indutny · 2015-06-01T19:32:07Z

CI is green.

trevnorris · 2015-06-04T18:32:43Z

@indutny going to merge this?

indutny · 2015-06-04T18:43:40Z

Yeah, let's land it. @trevnorris LGTY?

trevnorris · 2015-06-04T18:47:08Z

LGTM

When loading session, OCSP response, SNI, always check that the `self._handle` is present. If it is not - the socket was closed - and we should emit the error instead of throwing an uncaught exception. Fix: nodejs/node-v0.x-archive#8780 Fix: #1696 PR-URL: #1702 Reviewed-By: Trevor Norris <[email protected]>

* Destroy `SSL*` and friends on a next tick to make sure that we are not doing it in one of the OpenSSL callbacks * Add more checks to the C++ methods that might be invoked during destructor's pending queue cleanup Fix: nodejs/node-v0.x-archive#8780 Fix: #1696 PR-URL: #1702 Reviewed-By: Trevor Norris <[email protected]>

PR-URL: #1702 Reviewed-By: Trevor Norris <[email protected]>

indutny · 2015-06-04T21:41:18Z

Landed in 5795e83, 75930bb, and 59d9734. Thank you!

thefourtheye · 2015-06-21T03:39:37Z

src/tls_wrap.cc

@@ -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");


Not sure if the error message sounds like a TypeError

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

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

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

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.

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.

This was referenced May 14, 2015

segfault in TLS #1696

Closed

Segfault node v0.11.14 related to TLS nodejs/node-v0.x-archive#8780

Closed

mscdex added the tls Issues and PRs related to the tls subsystem. label May 14, 2015

indutny force-pushed the fix/gh-node-8780 branch from a9e9b9a to ad3a799 Compare May 29, 2015 18:28

indutny added 3 commits June 1, 2015 18:34

tls_wrap: invoke queued callbacks in DestroySSL

3533378

indutny force-pushed the fix/gh-node-8780 branch from ad3a799 to 3533378 Compare June 1, 2015 16:38

bnoordhuis force-pushed the master branch from 311c3e4 to b926718 Compare June 2, 2015 17:25

indutny added a commit that referenced this pull request Jun 4, 2015

tls_wrap: invoke queued callbacks in DestroySSL

59d9734

PR-URL: #1702 Reviewed-By: Trevor Norris <[email protected]>

rvagg mentioned this pull request Jun 11, 2015

Release proposal: 2.3.0 #1939

Closed

brendanashworth closed this Jun 21, 2015

indutny deleted the fix/gh-node-8780 branch June 21, 2015 01:49

thefourtheye reviewed Jun 21, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: emit errors on close whilst async action #1702

tls: emit errors on close whilst async action #1702

indutny commented May 14, 2015

indutny commented May 14, 2015

migounette commented May 14, 2015

indutny commented May 18, 2015

EricTheOne commented May 19, 2015

indutny commented May 19, 2015

EricTheOne commented May 19, 2015

indutny commented May 22, 2015

indutny commented May 29, 2015

indutny commented May 29, 2015

indutny commented May 29, 2015

evanlucas commented May 29, 2015

indutny commented May 29, 2015

indutny commented May 29, 2015

indutny commented Jun 1, 2015

indutny commented Jun 1, 2015

indutny commented Jun 1, 2015

trevnorris commented Jun 4, 2015

indutny commented Jun 4, 2015

trevnorris commented Jun 4, 2015

indutny commented Jun 4, 2015

thefourtheye Jun 21, 2015

indutny Jun 28, 2015

thefourtheye Jun 28, 2015

indutny Jun 28, 2015

thefourtheye Jun 28, 2015

indutny Jun 28, 2015

tls: emit errors on close whilst async action #1702

tls: emit errors on close whilst async action #1702

Conversation

indutny commented May 14, 2015

indutny commented May 14, 2015

migounette commented May 14, 2015

indutny commented May 18, 2015

EricTheOne commented May 19, 2015

indutny commented May 19, 2015

EricTheOne commented May 19, 2015

indutny commented May 22, 2015

indutny commented May 29, 2015

indutny commented May 29, 2015

indutny commented May 29, 2015

evanlucas commented May 29, 2015

indutny commented May 29, 2015

indutny commented May 29, 2015

indutny commented Jun 1, 2015

indutny commented Jun 1, 2015

indutny commented Jun 1, 2015

trevnorris commented Jun 4, 2015

indutny commented Jun 4, 2015

trevnorris commented Jun 4, 2015

indutny commented Jun 4, 2015

thefourtheye Jun 21, 2015

Choose a reason for hiding this comment

indutny Jun 28, 2015

Choose a reason for hiding this comment

thefourtheye Jun 28, 2015

Choose a reason for hiding this comment

indutny Jun 28, 2015

Choose a reason for hiding this comment

thefourtheye Jun 28, 2015

Choose a reason for hiding this comment

indutny Jun 28, 2015

Choose a reason for hiding this comment