Skip to content
Browse files

Drain OpenSSL error queue? Addresses #1719

  • Loading branch information...
1 parent e06ce75 commit 6312e889b1cd76fb5089056fd80f335319ad103e @ry ry committed
Showing with 12 additions and 1 deletion.
  1. +12 −1 src/node_crypto.cc
View
13 src/node_crypto.cc
@@ -504,11 +504,22 @@ int Connection::HandleSSLError(const char* func, int rv) {
static char ssl_error_buf[512];
ERR_error_string_n(err, ssl_error_buf, sizeof(ssl_error_buf));
@koichik
koichik added a note

Because err is obtained from SSL_get_error(), it should not be passed to ERR_error_string_n().
(see also #1516 (comment))

@koichik
koichik added a note

This is the reason why we have seen such a message: error:00000001:lib(0):func(0):reason(1).

@bnoordhuis Node.js Foundation member

Sounds plausible. We need to use ERR_print_errors(bio) here and verify that err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL (any other error here probably means that something is very wrong).

@bnoordhuis Node.js Foundation member

@koichik: https://github.com/bnoordhuis/node/compare/ERR_print_errors

The one drawback is that if there's multiple errors queued, they all get written to the same string (separated by newlines).

@koichik
koichik added a note

@bnoordhuis - Nice! +1.
Can you add the check of SSL_ERROR_NONE?

   int err = SSL_get_error(ssl_, rv);

+  if (err == SSL_ERROR_NONE) {
+    return 0;

-  if (err == SSL_ERROR_WANT_WRITE) {
+  } else if (err == SSL_ERROR_WANT_WRITE) {

they all get written to the same string (separated by newlines).

I feel that it is okay.

@bnoordhuis Node.js Foundation member

We don't really have to, SSL_ERROR_NONE is what SSL_get_error() returns if rv > 0 and it's less than zero at that point. But I'll add the check anyway, it won't hurt.

@bnoordhuis Node.js Foundation member

Committed in 44bebc0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ // XXX We need to drain the error queue for this thread or else OpenSSL
+ // has the possibility of blocking connections? This problem is not well
+ // understood. And we should be somehow propigating these errors up
+ // into JavaScript. There is no test which demonstrates this problem.
+ // https://github.com/joyent/node/issues/1719
+ while ((err = ERR_get_error()) != 0) {
+ ERR_error_string_n(err, ssl_error_buf, sizeof(ssl_error_buf));
+ fprintf(stderr, "(node SSL) %s\n", ssl_error_buf);
+ }
+
HandleScope scope;
Local<Value> e = Exception::Error(String::New(ssl_error_buf));
handle_->Set(String::New("error"), e);
- DEBUG_PRINT("[%p] SSL: %s failed: (%d:%d) %s\n", ssl_, func, err, rv, ssl_error_buf);
+ DEBUG_PRINT("[%p] SSL: %s failed: (%d:%d) %s\n", ssl_, func, err, rv,
+ ssl_error_buf);
return rv;
}

0 comments on commit 6312e88

Please sign in to comment.
Something went wrong with that request. Please try again.