From 4e066c6ce608691fbeb90f518c91d6d70522950c Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sun, 26 Apr 2015 14:26:57 +0200 Subject: [PATCH 1/4] tls: destroy SSL once it is out of use Do not keep SSL structure in memory once socket is closed. This should lower the memory usage in many cases. Fix: https://github.com/iojs/io.js/issues/1522 --- lib/_tls_wrap.js | 9 +++++++++ src/node_crypto.cc | 11 +++++++++++ src/node_crypto.h | 7 +++---- src/tls_wrap.cc | 39 ++++++++++++++++++++++++++++++++++----- src/tls_wrap.h | 1 + 5 files changed, 58 insertions(+), 9 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 3e091b0fc1be0d..169e4184fa22b1 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -295,9 +295,15 @@ TLSSocket.prototype._wrapHandle = function(handle) { } }); + this.on('close', this._destroySSL); + return res; }; +TLSSocket.prototype._destroySSL = function _destroySSL() { + return this.ssl.destroySSL(); +}; + TLSSocket.prototype._init = function(socket, wrap) { var self = this; var options = this._tlsOptions; @@ -416,6 +422,9 @@ TLSSocket.prototype.renegotiate = function(options, callback) { var requestCert = this._requestCert, rejectUnauthorized = this._rejectUnauthorized; + if (this.destroyed) + return; + if (typeof options.requestCert !== 'undefined') requestCert = !!options.requestCert; if (typeof options.rejectUnauthorized !== 'undefined') diff --git a/src/node_crypto.cc b/src/node_crypto.cc index b6cb4f10e5698f..af69cb73677584 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -131,6 +131,7 @@ template int SSLWrap::SelectNextProtoCallback( void* arg); #endif template int SSLWrap::TLSExtStatusCallback(SSL* s, void* arg); +template void SSLWrap::DestroySSL(); static void crypto_threadid_cb(CRYPTO_THREADID* tid) { @@ -1871,6 +1872,16 @@ void SSLWrap::SSLGetter(Local property, } +template +void SSLWrap::DestroySSL() { + if (ssl_ == nullptr) + return; + + SSL_free(ssl_); + ssl_ = nullptr; +} + + void Connection::OnClientHelloParseEnd(void* arg) { Connection* conn = static_cast(arg); diff --git a/src/node_crypto.h b/src/node_crypto.h index 75ffe4f31ddeea..8fec4bb6253c2e 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -144,10 +144,7 @@ class SSLWrap { } virtual ~SSLWrap() { - if (ssl_ != nullptr) { - SSL_free(ssl_); - ssl_ = nullptr; - } + DestroySSL(); if (next_sess_ != nullptr) { SSL_SESSION_free(next_sess_); next_sess_ = nullptr; @@ -221,6 +218,8 @@ class SSLWrap { static void SSLGetter(v8::Local property, const v8::PropertyCallbackInfo& info); + void DestroySSL(); + inline Environment* ssl_env() const { return env_; } diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index c774a8490b54f2..c013bf935eb9cb 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -208,7 +208,7 @@ void TLSWrap::Receive(const FunctionCallbackInfo& args) { uv_buf_t buf; // Copy given buffer entirely or partiall if handle becomes closed - while (len > 0 && !wrap->IsClosing()) { + while (len > 0 && wrap->IsAlive() && !wrap->IsClosing()) { wrap->stream_->OnAlloc(len, &buf); size_t copy = buf.len > len ? len : buf.len; memcpy(buf.base, data, copy); @@ -282,6 +282,9 @@ void TLSWrap::EncOut() { if (established_ && !write_item_queue_.IsEmpty()) MakePending(); + if (ssl_ == nullptr) + return; + // No data to write if (BIO_pending(enc_out_) == 0) { if (clear_in_->Length() == 0) @@ -396,7 +399,8 @@ void TLSWrap::ClearOut() { if (eof_) return; - CHECK_NE(ssl_, nullptr); + if (ssl_ == nullptr) + return; char out[kClearOutChunkSize]; int read; @@ -451,6 +455,9 @@ bool TLSWrap::ClearIn() { if (!hello_parser_.IsEnded()) return false; + if (ssl_ == nullptr) + return false; + int written = 0; while (clear_in_->Length() > 0) { size_t avail = 0; @@ -503,7 +510,7 @@ int TLSWrap::GetFD() { bool TLSWrap::IsAlive() { - return stream_->IsAlive(); + return ssl_ != nullptr && stream_->IsAlive(); } @@ -573,6 +580,9 @@ int TLSWrap::DoWrite(WriteWrap* w, return 0; } + if (ssl_ == nullptr) + return UV_EPROTO; + int written = 0; for (i = 0; i < count; i++) { written = SSL_write(ssl_, bufs[i].base, bufs[i].len); @@ -660,7 +670,10 @@ void TLSWrap::DoRead(ssize_t nread, } // Only client connections can receive data - CHECK_NE(ssl_, nullptr); + if (ssl_ == nullptr) { + OnRead(UV_EPROTO, nullptr); + return; + } // Commit read data NodeBIO* enc_in = NodeBIO::FromBIO(enc_in_); @@ -680,7 +693,7 @@ void TLSWrap::DoRead(ssize_t nread, int TLSWrap::DoShutdown(ShutdownWrap* req_wrap) { - if (SSL_shutdown(ssl_) == 0) + if (ssl_ != nullptr && SSL_shutdown(ssl_) == 0) SSL_shutdown(ssl_); shutdown_ = true; EncOut(); @@ -696,6 +709,9 @@ void TLSWrap::SetVerifyMode(const FunctionCallbackInfo& args) { if (args.Length() < 2 || !args[0]->IsBoolean() || !args[1]->IsBoolean()) return env->ThrowTypeError("Bad arguments, expected two booleans"); + if (wrap->ssl_ == nullptr) + return env->ThrowTypeError("SetVerifyMode after destroySSL"); + int verify_mode; if (wrap->is_server()) { bool request_cert = args[0]->IsTrue(); @@ -735,6 +751,14 @@ void TLSWrap::EnableHelloParser(const FunctionCallbackInfo& args) { } +void TLSWrap::DestroySSL(const FunctionCallbackInfo& args) { + TLSWrap* wrap = Unwrap(args.Holder()); + wrap->SSLWrap::DestroySSL(); + delete wrap->clear_in_; + wrap->clear_in_ = nullptr; +} + + void TLSWrap::OnClientHelloParseEnd(void* arg) { TLSWrap* c = static_cast(arg); c->Cycle(); @@ -747,6 +771,8 @@ void TLSWrap::GetServername(const FunctionCallbackInfo& args) { TLSWrap* wrap = Unwrap(args.Holder()); + CHECK_NE(wrap->ssl_, nullptr); + const char* servername = SSL_get_servername(wrap->ssl_, TLSEXT_NAMETYPE_host_name); if (servername != nullptr) { @@ -771,6 +797,8 @@ void TLSWrap::SetServername(const FunctionCallbackInfo& args) { if (!wrap->is_client()) return; + CHECK_NE(wrap->ssl_, nullptr); + #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB node::Utf8Value servername(env->isolate(), args[0].As()); SSL_set_tlsext_host_name(wrap->ssl_, *servername); @@ -830,6 +858,7 @@ void TLSWrap::Initialize(Handle target, env->SetProtoMethod(t, "setVerifyMode", SetVerifyMode); env->SetProtoMethod(t, "enableSessionCallbacks", EnableSessionCallbacks); env->SetProtoMethod(t, "enableHelloParser", EnableHelloParser); + env->SetProtoMethod(t, "destroySSL", DestroySSL); StreamBase::AddMethods(env, t, StreamBase::kFlagHasWritev); SSLWrap::AddMethods(env, t); diff --git a/src/tls_wrap.h b/src/tls_wrap.h index 9f095355bb58bd..25088d30261189 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -132,6 +132,7 @@ class TLSWrap : public crypto::SSLWrap, const v8::FunctionCallbackInfo& args); static void EnableHelloParser( const v8::FunctionCallbackInfo& args); + static void DestroySSL(const v8::FunctionCallbackInfo& args); #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB static void GetServername(const v8::FunctionCallbackInfo& args); From 6c9620ad978072fdae43fda61bb3dcdc61b2b75f Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sun, 26 Apr 2015 14:27:46 +0200 Subject: [PATCH 2/4] crypto: track external memory for SSL structures Ensure that GC kicks in at the right times and the RSS does not blow up. Fix: https://github.com/iojs/io.js/issues/1522 --- src/node_crypto.cc | 1 + src/node_crypto.h | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index af69cb73677584..9e62e1e61fbdac 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1878,6 +1878,7 @@ void SSLWrap::DestroySSL() { return; SSL_free(ssl_); + env_->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize); ssl_ = nullptr; } diff --git a/src/node_crypto.h b/src/node_crypto.h index 8fec4bb6253c2e..a623ccbf2637a5 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -64,6 +64,7 @@ class SecureContext : public BaseObject { static const int kMaxSessionSize = 10 * 1024; protected: + static const int64_t kExternalSize = sizeof(SSL_CTX); static void New(const v8::FunctionCallbackInfo& args); static void Init(const v8::FunctionCallbackInfo& args); @@ -97,10 +98,12 @@ class SecureContext : public BaseObject { cert_(nullptr), issuer_(nullptr) { MakeWeak(this); + env->isolate()->AdjustAmountOfExternalAllocatedMemory(kExternalSize); } void FreeCTXMem() { if (ctx_) { + env()->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize); if (ctx_->cert_store == root_cert_store) { // SSL_CTX_free() will attempt to free the cert_store as well. // Since we want our root_cert_store to stay around forever @@ -140,6 +143,7 @@ class SSLWrap { session_callbacks_(false), new_session_wait_(false) { ssl_ = SSL_new(sc->ctx_); + env_->isolate()->AdjustAmountOfExternalAllocatedMemory(kExternalSize); CHECK_NE(ssl_, nullptr); } @@ -166,6 +170,12 @@ class SSLWrap { inline bool is_waiting_new_session() const { return new_session_wait_; } protected: + // Size allocated by OpenSSL: one for SSL structure, one for SSL3_STATE and + // some for buffers. + // NOTE: Actually it is much more than this + static const int64_t kExternalSize = + sizeof(SSL) + sizeof(SSL3_STATE) + 42 * 1024; + static void InitNPN(SecureContext* sc); static void AddMethods(Environment* env, v8::Handle t); From dbac0612d34f224cffc66e0cbf092d53503cea66 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sun, 26 Apr 2015 14:19:38 +0200 Subject: [PATCH 3/4] tls: zero SSL_CTX freelist for a singleUse socket When connecting to server with `keepAlive` turned off - make sure that the read/write buffers won't be kept in a single use SSL_CTX instance after the socket will be destroyed. Fix: https://github.com/iojs/io.js/issues/1522 --- lib/_tls_common.js | 4 ++++ lib/_tls_wrap.js | 2 ++ src/node_crypto.cc | 8 ++++++++ src/node_crypto.h | 2 ++ 4 files changed, 16 insertions(+) diff --git a/lib/_tls_common.js b/lib/_tls_common.js index 2c15d91df8d4c3..3040b3a5b40c9f 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -133,6 +133,10 @@ exports.createSecureContext = function createSecureContext(options, context) { } } + // Do not keep read/write buffers in free list + if (options.singleUse) + c.context.setFreeListLength(0); + return c; }; diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 169e4184fa22b1..d4774312dcad6a 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -862,6 +862,8 @@ exports.connect = function(/* [port, host], options, cb */) { }; options = util._extend(defaults, options || {}); + if (!options.keepAlive) + options.singleUse = true; assert(typeof options.checkServerIdentity === 'function'); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 9e62e1e61fbdac..1d112001e88a0b 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -265,6 +265,7 @@ void SecureContext::Initialize(Environment* env, Handle target) { env->SetProtoMethod(t, "loadPKCS12", SecureContext::LoadPKCS12); env->SetProtoMethod(t, "getTicketKeys", SecureContext::GetTicketKeys); env->SetProtoMethod(t, "setTicketKeys", SecureContext::SetTicketKeys); + env->SetProtoMethod(t, "setFreeListLength", SecureContext::SetFreeListLength); env->SetProtoMethod(t, "getCertificate", SecureContext::GetCertificate); env->SetProtoMethod(t, "getIssuer", SecureContext::GetCertificate); @@ -933,6 +934,13 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo& args) { } +void SecureContext::SetFreeListLength(const FunctionCallbackInfo& args) { + SecureContext* wrap = Unwrap(args.Holder()); + + wrap->ctx_->freelist_max_len = args[0]->Int32Value(); +} + + void SecureContext::CtxGetter(Local property, const PropertyCallbackInfo& info) { HandleScope scope(info.GetIsolate()); diff --git a/src/node_crypto.h b/src/node_crypto.h index a623ccbf2637a5..f6069f88410c0b 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -85,6 +85,8 @@ class SecureContext : public BaseObject { static void LoadPKCS12(const v8::FunctionCallbackInfo& args); static void GetTicketKeys(const v8::FunctionCallbackInfo& args); static void SetTicketKeys(const v8::FunctionCallbackInfo& args); + static void SetFreeListLength( + const v8::FunctionCallbackInfo& args); static void CtxGetter(v8::Local property, const v8::PropertyCallbackInfo& info); From 1357735aca71f8caa3f39e848cf9f79e77212423 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Mon, 27 Apr 2015 09:39:48 +0200 Subject: [PATCH 4/4] tls: destroy singleUse context immediately Destroy singleUse context right after it is going out of use. Fix: https://github.com/iojs/io.js/issues/1522 --- lib/_tls_common.js | 4 +++- lib/_tls_wrap.js | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/_tls_common.js b/lib/_tls_common.js index 3040b3a5b40c9f..d857717dabae15 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -134,8 +134,10 @@ exports.createSecureContext = function createSecureContext(options, context) { } // Do not keep read/write buffers in free list - if (options.singleUse) + if (options.singleUse) { + c.singleUse = true; c.context.setFreeListLength(0); + } return c; }; diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index d4774312dcad6a..deb9b5ae6bfa8e 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -301,7 +301,9 @@ TLSSocket.prototype._wrapHandle = function(handle) { }; TLSSocket.prototype._destroySSL = function _destroySSL() { - return this.ssl.destroySSL(); + this.ssl.destroySSL(); + if (this.ssl._secureContext.singleUse) + this.ssl._secureContext.context.close(); }; TLSSocket.prototype._init = function(socket, wrap) {