From b85f945bd3630ec7129ede871f714a970525f906 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 5 Mar 2015 18:26:01 -0500 Subject: [PATCH 1/4] stream_wrap: add HandleScope's in uv callbacks Ensure that no handles will leak into global HandleScope by adding HandleScope's in all JS-calling libuv callbacks in `stream_wrap.cc`. Fix: https://github.com/iojs/io.js/issues/1075 --- src/stream_wrap.cc | 7 +++++++ src/tls_wrap.cc | 14 -------------- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 8f6fcac85d76b1..69d721cbe69cf7 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -145,6 +145,9 @@ void StreamWrap::OnAlloc(uv_handle_t* handle, size_t suggested_size, uv_buf_t* buf) { StreamWrap* wrap = static_cast(handle->data); + HandleScope scope(wrap->env()->isolate()); + Context::Scope context_scope(wrap->env()->context()); + CHECK_EQ(wrap->stream(), reinterpret_cast(handle)); return static_cast(wrap)->OnAlloc(suggested_size, buf); @@ -229,6 +232,7 @@ void StreamWrap::OnReadCommon(uv_stream_t* handle, const uv_buf_t* buf, uv_handle_type pending) { StreamWrap* wrap = static_cast(handle->data); + HandleScope scope(wrap->env()->isolate()); // We should not be getting this callback if someone as already called // uv_close() on the handle. @@ -280,6 +284,7 @@ int StreamWrap::DoShutdown(ShutdownWrap* req_wrap) { void StreamWrap::AfterShutdown(uv_shutdown_t* req, int status) { ShutdownWrap* req_wrap = ContainerOf(&ShutdownWrap::req_, req); + HandleScope scope(req_wrap->env()->isolate()); req_wrap->Done(status); } @@ -354,6 +359,8 @@ int StreamWrap::DoWrite(WriteWrap* w, void StreamWrap::AfterWrite(uv_write_t* req, int status) { WriteWrap* req_wrap = ContainerOf(&WriteWrap::req_, req); + HandleScope scope(req_wrap->env()->isolate()); + Context::Scope context_scope(req_wrap->env()->context()); req_wrap->Done(status); } diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 1647ab669b29a3..b05b8891d0fdd5 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -26,7 +26,6 @@ using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::Handle; -using v8::HandleScope; using v8::Integer; using v8::Local; using v8::Null; @@ -251,8 +250,6 @@ void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) { SSL* ssl = const_cast(ssl_); TLSWrap* c = static_cast(SSL_get_app_data(ssl)); Environment* env = c->env(); - HandleScope handle_scope(env->isolate()); - Context::Scope context_scope(env->context()); Local object = c->object(); if (where & SSL_CB_HANDSHAKE_START) { @@ -395,9 +392,6 @@ void TLSWrap::ClearOut() { if (eof_) return; - HandleScope handle_scope(env()->isolate()); - Context::Scope context_scope(env()->context()); - CHECK_NE(ssl_, nullptr); char out[kClearOutChunkSize]; @@ -470,9 +464,6 @@ bool TLSWrap::ClearIn() { return true; } - HandleScope handle_scope(env()->isolate()); - Context::Scope context_scope(env()->context()); - // Error or partial write int err; Local arg = GetSSLError(written, &err, &error_); @@ -588,8 +579,6 @@ int TLSWrap::DoWrite(WriteWrap* w, if (i != count) { int err; - HandleScope handle_scope(env()->isolate()); - Context::Scope context_scope(env()->context()); Local arg = GetSSLError(written, &err, &error_); if (!arg.IsEmpty()) return UV_EPROTO; @@ -662,8 +651,6 @@ void TLSWrap::DoRead(ssize_t nread, eof_ = true; } - HandleScope handle_scope(env()->isolate()); - Context::Scope context_scope(env()->context()); OnRead(nread, nullptr); return; } @@ -796,7 +783,6 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) { if (servername == nullptr) return SSL_TLSEXT_ERR_OK; - HandleScope scope(env->isolate()); // Call the SNI callback and use its return value as context Local object = p->object(); Local ctx = object->Get(env->sni_context_string()); From 6866719a4e9000dd31cd561ffc323675ad345fa0 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 5 Mar 2015 20:26:12 -0500 Subject: [PATCH 2/4] js_stream: fix leak of instances Don't forget to call `MakeWeak` to ensure that instance objects are garbage collectable. --- src/js_stream.cc | 1 + src/js_stream.h | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/js_stream.cc b/src/js_stream.cc index 21f5ce07ac08b3..7fcdfd9a94d9fd 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -26,6 +26,7 @@ JSStream::JSStream(Environment* env, Handle obj, AsyncWrap* parent) : StreamBase(env), AsyncWrap(env, obj, AsyncWrap::PROVIDER_JSSTREAM, parent) { node::Wrap(obj, this); + MakeWeak(this); } diff --git a/src/js_stream.h b/src/js_stream.h index 8e2ff13258b219..6bc763b36e2bd1 100644 --- a/src/js_stream.h +++ b/src/js_stream.h @@ -14,6 +14,8 @@ class JSStream : public StreamBase, public AsyncWrap { v8::Handle unused, v8::Handle context); + ~JSStream(); + void* Cast() override; bool IsAlive() override; bool IsClosing() override; @@ -28,7 +30,6 @@ class JSStream : public StreamBase, public AsyncWrap { protected: JSStream(Environment* env, v8::Handle obj, AsyncWrap* parent); - ~JSStream(); AsyncWrap* GetAsyncWrap() override; From 6879b0580656abb58816de50af29346afdb954bc Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 5 Mar 2015 20:27:58 -0500 Subject: [PATCH 3/4] tls_wrap: do not hold persistent ref to parent Hold non-persistent reference in JS, rather than in C++ to avoid cycles. --- src/tls_wrap.cc | 8 ++------ src/tls_wrap.h | 2 -- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index b05b8891d0fdd5..af2b1d50f84e48 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -37,7 +37,6 @@ using v8::Value; TLSWrap::TLSWrap(Environment* env, Kind kind, StreamBase* stream, - Handle stream_obj, Handle sc) : SSLWrap(env, Unwrap(sc), kind), StreamBase(env), @@ -47,7 +46,6 @@ TLSWrap::TLSWrap(Environment* env, sc_(Unwrap(sc)), sc_handle_(env->isolate(), sc), stream_(stream), - stream_handle_(env->isolate(), stream_obj), enc_in_(nullptr), enc_out_(nullptr), clear_in_(nullptr), @@ -85,8 +83,6 @@ TLSWrap::~TLSWrap() { sc_ = nullptr; sc_handle_.Reset(); - stream_handle_.Reset(); - persistent().Reset(); #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB sni_context_.Reset(); @@ -196,9 +192,9 @@ void TLSWrap::Wrap(const FunctionCallbackInfo& args) { }); CHECK_NE(stream, nullptr); - TLSWrap* res = new TLSWrap(env, kind, stream, stream_obj, sc); + TLSWrap* res = new TLSWrap(env, kind, stream, sc); - args.GetReturnValue().Set(res->persistent()); + args.GetReturnValue().Set(res->object()); } diff --git a/src/tls_wrap.h b/src/tls_wrap.h index 73a9f84ec0d1b7..6e9b3de88f9b3c 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -78,7 +78,6 @@ class TLSWrap : public crypto::SSLWrap, TLSWrap(Environment* env, Kind kind, StreamBase* stream, - v8::Handle stream_obj, v8::Handle sc); static void SSLInfoCallback(const SSL* ssl_, int where, int ret); @@ -143,7 +142,6 @@ class TLSWrap : public crypto::SSLWrap, crypto::SecureContext* sc_; v8::Persistent sc_handle_; StreamBase* stream_; - v8::Persistent stream_handle_; BIO* enc_in_; BIO* enc_out_; NodeBIO* clear_in_; From 2879b03afc23532a8d6420f5e16ee8205a2a141b Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 5 Mar 2015 20:31:07 -0500 Subject: [PATCH 4/4] tls_wrap: hold `secureContext` ref in JS-land Ensure no persistent-induced loops in C++-land by storing `SecureContext` reference in JS-land. --- lib/_tls_wrap.js | 1 + src/tls_wrap.cc | 10 ++++------ src/tls_wrap.h | 3 +-- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 0d1a2457283624..2fd13ef83db1c9 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -286,6 +286,7 @@ TLSSocket.prototype._wrapHandle = function(handle) { tls.createSecureContext(); res = tls_wrap.wrap(handle, context.context, options.isServer); res._parent = handle; + res._secureContext = context; res.reading = handle.reading; Object.defineProperty(handle, 'reading', { get: function readingGetter() { diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index af2b1d50f84e48..a42e5faa70d8ae 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -37,14 +37,13 @@ using v8::Value; TLSWrap::TLSWrap(Environment* env, Kind kind, StreamBase* stream, - Handle sc) - : SSLWrap(env, Unwrap(sc), kind), + SecureContext* sc) + : SSLWrap(env, sc, kind), StreamBase(env), AsyncWrap(env, env->tls_wrap_constructor_function()->NewInstance(), AsyncWrap::PROVIDER_TLSWRAP), - sc_(Unwrap(sc)), - sc_handle_(env->isolate(), sc), + sc_(sc), stream_(stream), enc_in_(nullptr), enc_out_(nullptr), @@ -82,7 +81,6 @@ TLSWrap::~TLSWrap() { clear_in_ = nullptr; sc_ = nullptr; - sc_handle_.Reset(); #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB sni_context_.Reset(); @@ -192,7 +190,7 @@ void TLSWrap::Wrap(const FunctionCallbackInfo& args) { }); CHECK_NE(stream, nullptr); - TLSWrap* res = new TLSWrap(env, kind, stream, sc); + TLSWrap* res = new TLSWrap(env, kind, stream, Unwrap(sc)); args.GetReturnValue().Set(res->object()); } diff --git a/src/tls_wrap.h b/src/tls_wrap.h index 6e9b3de88f9b3c..9f095355bb58bd 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -78,7 +78,7 @@ class TLSWrap : public crypto::SSLWrap, TLSWrap(Environment* env, Kind kind, StreamBase* stream, - v8::Handle sc); + crypto::SecureContext* sc); static void SSLInfoCallback(const SSL* ssl_, int where, int ret); void InitSSL(); @@ -140,7 +140,6 @@ class TLSWrap : public crypto::SSLWrap, #endif // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB crypto::SecureContext* sc_; - v8::Persistent sc_handle_; StreamBase* stream_; BIO* enc_in_; BIO* enc_out_;