From 4d37472ea74c919aa86b84b86b59de6fa6d78306 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sat, 2 Jan 2016 23:05:40 -0500 Subject: [PATCH] tls_wrap: clear errors on return Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions. See: https://github.com/nodejs/node/issues/4485 PR-URL: https://github.com/nodejs/node/pull/4515 Reviewed-By: Ben Noordhuis v4.x-staging Commit Metadata: PR-URL: https://github.com/nodejs/node/pull/4709 Reviewed-By: James M Snell --- src/node_crypto.cc | 8 -------- src/node_crypto.h | 15 +++++++++++++++ src/tls_wrap.cc | 10 +++++++++- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 708f15f6961409..7911ce9e612816 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -116,14 +116,6 @@ static X509_NAME *cnnic_ev_name = d2i_X509_NAME(nullptr, &cnnic_ev_p, sizeof(CNNIC_EV_ROOT_CA_SUBJECT_DATA)-1); -// Forcibly clear OpenSSL's error stack on return. This stops stale errors -// from popping up later in the lifecycle of crypto operations where they -// would cause spurious failures. It's a rather blunt method, though. -// ERR_clear_error() isn't necessarily cheap either. -struct ClearErrorOnReturn { - ~ClearErrorOnReturn() { ERR_clear_error(); } -}; - static uv_mutex_t* locks; const char* const root_certs[] = { diff --git a/src/node_crypto.h b/src/node_crypto.h index 3bec02c38ebdec..e009fc1da63b01 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -41,6 +41,21 @@ namespace node { namespace crypto { +// Forcibly clear OpenSSL's error stack on return. This stops stale errors +// from popping up later in the lifecycle of crypto operations where they +// would cause spurious failures. It's a rather blunt method, though. +// ERR_clear_error() isn't necessarily cheap either. +struct ClearErrorOnReturn { + ~ClearErrorOnReturn() { ERR_clear_error(); } +}; + +// Pop errors from OpenSSL's error stack that were added +// between when this was constructed and destructed. +struct MarkPopErrorOnReturn { + MarkPopErrorOnReturn() { ERR_set_mark(); } + ~MarkPopErrorOnReturn() { ERR_pop_to_mark(); } +}; + enum CheckResult { CHECK_CERT_REVOKED = 0, CHECK_OK = 1 diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 1dca69f07a341c..1bdd4b7df947fe 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -31,7 +31,6 @@ using v8::Object; using v8::String; using v8::Value; - TLSWrap::TLSWrap(Environment* env, Kind kind, StreamBase* stream, @@ -401,6 +400,8 @@ void TLSWrap::ClearOut() { if (ssl_ == nullptr) return; + crypto::MarkPopErrorOnReturn mark_pop_error_on_return; + char out[kClearOutChunkSize]; int read; for (;;) { @@ -462,6 +463,8 @@ bool TLSWrap::ClearIn() { if (ssl_ == nullptr) return false; + crypto::MarkPopErrorOnReturn mark_pop_error_on_return; + int written = 0; while (clear_in_->Length() > 0) { size_t avail = 0; @@ -589,6 +592,8 @@ int TLSWrap::DoWrite(WriteWrap* w, if (ssl_ == nullptr) return UV_EPROTO; + crypto::MarkPopErrorOnReturn mark_pop_error_on_return; + int written = 0; for (i = 0; i < count; i++) { written = SSL_write(ssl_, bufs[i].base, bufs[i].len); @@ -704,8 +709,11 @@ void TLSWrap::DoRead(ssize_t nread, int TLSWrap::DoShutdown(ShutdownWrap* req_wrap) { + crypto::MarkPopErrorOnReturn mark_pop_error_on_return; + if (ssl_ != nullptr && SSL_shutdown(ssl_) == 0) SSL_shutdown(ssl_); + shutdown_ = true; EncOut(); return stream_->DoShutdown(req_wrap);