Skip to content

Commit

Permalink
tls_wrap: clear errors on return
Browse files Browse the repository at this point in the history
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: nodejs#4485
PR-URL: nodejs#4515
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
indutny committed Jan 13, 2016
1 parent bd8b2f0 commit 5f58cff
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 9 deletions.
8 changes: 0 additions & 8 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = {
Expand Down
15 changes: 15 additions & 0 deletions src/node_crypto.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 9 additions & 1 deletion src/tls_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ using v8::Object;
using v8::String;
using v8::Value;


TLSWrap::TLSWrap(Environment* env,
Kind kind,
StreamBase* stream,
Expand Down Expand Up @@ -401,6 +400,8 @@ void TLSWrap::ClearOut() {
if (ssl_ == nullptr)
return;

crypto::MarkPopErrorOnReturn mark_pop_error_on_return;

char out[kClearOutChunkSize];
int read;
for (;;) {
Expand Down Expand Up @@ -460,6 +461,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;
Expand Down Expand Up @@ -587,6 +590,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);
Expand Down Expand Up @@ -702,8 +707,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);
Expand Down

0 comments on commit 5f58cff

Please sign in to comment.