Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Commit

Permalink
Drain OpenSSL error queue? Addresses #1719
Browse files Browse the repository at this point in the history
  • Loading branch information
@ry
ry committed Sep 15, 2011
1 parent e06ce75 commit 6312e88
Showing 1 changed file with 12 additions and 1 deletion.
13 changes: 12 additions & 1 deletion src/node_crypto.cc
View file
Expand Up @@ -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));

This comment has been minimized.

Sign in to view

Copy link
@koichik

koichik Sep 16, 2011

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

This comment has been minimized.

Sign in to view

Copy link
@koichik

koichik Sep 16, 2011

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

This comment has been minimized.

Sign in to view

Copy link
@bnoordhuis

bnoordhuis Sep 19, 2011

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).

This comment has been minimized.

Sign in to view

Copy link
@bnoordhuis

bnoordhuis Sep 19, 2011

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).

This comment has been minimized.

Sign in to view

Copy link
@koichik

koichik Sep 20, 2011

@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.

This comment has been minimized.

Sign in to view

Copy link
@bnoordhuis

bnoordhuis Sep 20, 2011

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.

This comment has been minimized.

Sign in to view

Copy link
@bnoordhuis

bnoordhuis Sep 20, 2011

Member

Committed in 44bebc0.

// 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;
}
Expand Down

0 comments on commit 6312e88

Please sign in to comment.