diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 87f40cc24bd69a..93aa99d4fd5097 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -668,11 +668,6 @@ void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) { EncOut(); } -int TLSWrap::GetSSLError(int status) const { - // ssl_ might already be destroyed for reading EOF from a close notify alert. - return ssl_ != nullptr ? SSL_get_error(ssl_.get(), status) : 0; -} - void TLSWrap::ClearOut() { Debug(this, "Trying to read cleartext output"); // Ignore cycling data if ClientHello wasn't yet parsed @@ -726,19 +721,25 @@ void TLSWrap::ClearOut() { } } - int flags = SSL_get_shutdown(ssl_.get()); - if (!eof_ && flags & SSL_RECEIVED_SHUTDOWN) { - eof_ = true; - EmitRead(UV_EOF); - } - // We need to check whether an error occurred or the connection was // shutdown cleanly (SSL_ERROR_ZERO_RETURN) even when read == 0. - // See node#1642 and SSL_read(3SSL) for details. + // See node#1642 and SSL_read(3SSL) for details. SSL_get_error must be + // called immediately after SSL_read, without calling into JS, which may + // change OpenSSL's error queue, modify ssl_, or even destroy ssl_ + // altogether. if (read <= 0) { + int err = SSL_get_error(ssl_.get(), read); + unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) + const std::string error_str = GetBIOError(); + + int flags = SSL_get_shutdown(ssl_.get()); + if (!eof_ && flags & SSL_RECEIVED_SHUTDOWN) { + eof_ = true; + EmitRead(UV_EOF); + } + HandleScope handle_scope(env()->isolate()); Local error; - int err = GetSSLError(read); switch (err) { case SSL_ERROR_ZERO_RETURN: // Ignore ZERO_RETURN after EOF, it is basically not an error. @@ -749,11 +750,8 @@ void TLSWrap::ClearOut() { case SSL_ERROR_SSL: case SSL_ERROR_SYSCALL: { - unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) - Local context = env()->isolate()->GetCurrentContext(); if (UNLIKELY(context.IsEmpty())) return; - const std::string error_str = GetBIOError(); Local message = OneByteString( env()->isolate(), error_str.c_str(), error_str.size()); if (UNLIKELY(message.IsEmpty())) return; @@ -829,7 +827,7 @@ void TLSWrap::ClearIn() { } // Error or partial write - int err = GetSSLError(written); + int err = SSL_get_error(ssl_.get(), written); if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { Debug(this, "Got SSL error (%d)", err); write_callback_scheduled_ = true; @@ -1005,7 +1003,7 @@ int TLSWrap::DoWrite(WriteWrap* w, if (written == -1) { // If we stopped writing because of an error, it's fatal, discard the data. - int err = GetSSLError(written); + int err = SSL_get_error(ssl_.get(), written); if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { // TODO(@jasnell): What are we doing with the error? Debug(this, "Got SSL error (%d), returning UV_EPROTO", err); diff --git a/src/crypto/crypto_tls.h b/src/crypto/crypto_tls.h index 6a7e2f62a20004..5d1b24ff772328 100644 --- a/src/crypto/crypto_tls.h +++ b/src/crypto/crypto_tls.h @@ -167,8 +167,6 @@ class TLSWrap : public AsyncWrap, int SetCACerts(SecureContext* sc); - int GetSSLError(int status) const; - static int SelectSNIContextCallback(SSL* s, int* ad, void* arg); static void CertCbDone(const v8::FunctionCallbackInfo& args);