From 2503259f2a2e285aeaa699041c0e5231d240823c Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 21 Feb 2018 15:24:18 +0100 Subject: [PATCH] src: prevent persistent handle resource leaks Replace v8::Persistent with node::Persistent, a specialization that resets the persistent handle on destruction. Prevents accidental resource leaks when forgetting to call .Reset() manually. I'm fairly confident this commit fixes a number of resource leaks that have gone undiagnosed so far. Backport-PR-URL: https://github.com/nodejs/node/pull/19185 PR-URL: https://github.com/nodejs/node/pull/18656 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Joyee Cheung --- node.gyp | 3 ++- src/async_wrap.cc | 4 ++-- src/base_object-inl.h | 2 +- src/base_object.h | 10 +++------- src/env-inl.h | 4 ++-- src/env.h | 5 ++--- src/inspector_agent.cc | 1 - src/inspector_agent.h | 6 +++--- src/inspector_js_api.cc | 1 - src/module_wrap.h | 8 ++++---- src/node_api.cc | 26 +++++++++++++------------- src/node_buffer.cc | 1 - src/node_contextify.cc | 1 - src/node_contextify.h | 2 +- src/node_crypto.cc | 4 ++-- src/node_crypto.h | 12 ++++++------ src/node_file.cc | 6 ++++-- src/node_internals.h | 3 ++- src/node_persistent.h | 30 ++++++++++++++++++++++++++++++ src/node_zlib.cc | 1 - src/stream_base-inl.h | 26 ++++++++------------------ src/stream_base.h | 4 ++-- src/stream_wrap.cc | 4 ++-- src/util-inl.h | 8 ++++---- src/util.h | 7 ++++--- 25 files changed, 97 insertions(+), 82 deletions(-) create mode 100644 src/node_persistent.h diff --git a/node.gyp b/node.gyp index 7da486ff6d8c7b..1f47e88a3132fe 100644 --- a/node.gyp +++ b/node.gyp @@ -359,9 +359,10 @@ 'src/node_internals.h', 'src/node_javascript.h', 'src/node_mutex.h', - 'src/node_platform.h', 'src/node_perf.h', 'src/node_perf_common.h', + 'src/node_persistent.h', + 'src/node_platform.h', 'src/node_root_certs.h', 'src/node_version.h', 'src/node_watchdog.h', diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 93bd3d4864fd5d..f8e0992ee511b4 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -410,8 +410,8 @@ static void DisablePromiseHook(const FunctionCallbackInfo& args) { class DestroyParam { public: double asyncId; - v8::Persistent target; - v8::Persistent propBag; + Persistent target; + Persistent propBag; }; diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 900fc2b3edb9ca..6720bd6d886e1b 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -47,7 +47,7 @@ inline BaseObject::~BaseObject() { } -inline v8::Persistent& BaseObject::persistent() { +inline Persistent& BaseObject::persistent() { return persistent_handle_; } diff --git a/src/base_object.h b/src/base_object.h index 965683d029e43e..0640f91cbf5b88 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -24,6 +24,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "node_persistent.h" #include "v8.h" namespace node { @@ -39,12 +40,7 @@ class BaseObject { // persistent.IsEmpty() is true. inline v8::Local object(); - // The parent class is responsible for calling .Reset() on destruction - // when the persistent handle is strong because there is no way for - // BaseObject to know when the handle goes out of scope. - // Weak handles have been reset by the time the destructor runs but - // calling .Reset() again is harmless. - inline v8::Persistent& persistent(); + inline Persistent& persistent(); inline Environment* env() const; @@ -71,7 +67,7 @@ class BaseObject { // position of members in memory are predictable. For more information please // refer to `doc/guides/node-postmortem-support.md` friend int GenDebugSymbols(); - v8::Persistent persistent_handle_; + Persistent persistent_handle_; Environment* env_; }; diff --git a/src/env-inl.h b/src/env-inl.h index 0a2adae2bb0ade..5c0ff282f52829 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -541,8 +541,8 @@ void Environment::CreateImmediate(native_immediate_callback cb, native_immediate_callbacks_.push_back({ cb, data, - std::unique_ptr>(obj.IsEmpty() ? - nullptr : new v8::Persistent(isolate_, obj)), + std::unique_ptr>(obj.IsEmpty() ? + nullptr : new Persistent(isolate_, obj)), ref }); immediate_info()->count_inc(1); diff --git a/src/env.h b/src/env.h index 86612bf354d2b3..b08b767cb7cba1 100644 --- a/src/env.h +++ b/src/env.h @@ -800,7 +800,7 @@ class Environment { struct NativeImmediateCallback { native_immediate_callback cb_; void* data_; - std::unique_ptr> keep_alive_; + std::unique_ptr> keep_alive_; bool refed_; }; std::vector native_immediate_callbacks_; @@ -811,8 +811,7 @@ class Environment { v8::Local promise, v8::Local parent); -#define V(PropertyName, TypeName) \ - v8::Persistent PropertyName ## _; +#define V(PropertyName, TypeName) Persistent PropertyName ## _; ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) #undef V diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 7ba1a144711524..e143d316d2e6fa 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -30,7 +30,6 @@ using v8::HandleScope; using v8::Isolate; using v8::Local; using v8::Object; -using v8::Persistent; using v8::String; using v8::Value; diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 0555f5e18c2129..56fb407930fac5 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -97,7 +97,7 @@ class Agent { private: void ToggleAsyncHook(v8::Isolate* isolate, - const v8::Persistent& fn); + const Persistent& fn); node::Environment* parent_env_; std::unique_ptr client_; @@ -109,8 +109,8 @@ class Agent { bool pending_enable_async_hook_; bool pending_disable_async_hook_; - v8::Persistent enable_async_hook_function_; - v8::Persistent disable_async_hook_function_; + Persistent enable_async_hook_function_; + Persistent disable_async_hook_function_; }; } // namespace inspector diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 428d8391f2581a..9a4857f1a10cab 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -19,7 +19,6 @@ using v8::Local; using v8::MaybeLocal; using v8::NewStringType; using v8::Object; -using v8::Persistent; using v8::String; using v8::Value; diff --git a/src/module_wrap.h b/src/module_wrap.h index bedf665165c8f6..04f27decc2aeb4 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -49,11 +49,11 @@ class ModuleWrap : public BaseObject { v8::Local specifier, v8::Local referrer); - v8::Persistent module_; - v8::Persistent url_; + Persistent module_; + Persistent url_; bool linked_ = false; - std::unordered_map> resolve_cache_; - v8::Persistent context_; + std::unordered_map> resolve_cache_; + Persistent context_; }; } // namespace loader diff --git a/src/node_api.cc b/src/node_api.cc index 2c5f3066f728b1..7b0e110579b65b 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -38,10 +38,10 @@ struct napi_env__ { accessor_data_template.Reset(); } v8::Isolate* isolate; - v8::Persistent last_exception; - v8::Persistent wrap_template; - v8::Persistent function_data_template; - v8::Persistent accessor_data_template; + node::Persistent last_exception; + node::Persistent wrap_template; + node::Persistent function_data_template; + node::Persistent accessor_data_template; napi_extended_error_info last_error; int open_handle_scopes = 0; int open_callback_scopes = 0; @@ -274,13 +274,13 @@ static_assert(sizeof(v8::Local) == sizeof(napi_value), "Cannot convert between v8::Local and napi_value"); static -napi_deferred JsDeferredFromV8Persistent(v8::Persistent* local) { +napi_deferred JsDeferredFromNodePersistent(node::Persistent* local) { return reinterpret_cast(local); } static -v8::Persistent* V8PersistentFromJsDeferred(napi_deferred local) { - return reinterpret_cast*>(local); +node::Persistent* NodePersistentFromJsDeferred(napi_deferred local) { + return reinterpret_cast*>(local); } static @@ -360,7 +360,7 @@ class Finalizer { void* _finalize_hint; }; -// Wrapper around v8::Persistent that implements reference counting. +// Wrapper around node::Persistent that implements reference counting. class Reference : private Finalizer { private: Reference(napi_env env, @@ -470,7 +470,7 @@ class Reference : private Finalizer { } } - v8::Persistent _persistent; + node::Persistent _persistent; uint32_t _refcount; bool _delete_self; }; @@ -846,8 +846,8 @@ napi_status ConcludeDeferred(napi_env env, CHECK_ARG(env, result); v8::Local context = env->isolate->GetCurrentContext(); - v8::Persistent* deferred_ref = - V8PersistentFromJsDeferred(deferred); + node::Persistent* deferred_ref = + NodePersistentFromJsDeferred(deferred); v8::Local v8_deferred = v8::Local::New(env->isolate, *deferred_ref); @@ -3493,10 +3493,10 @@ napi_status napi_create_promise(napi_env env, CHECK_MAYBE_EMPTY(env, maybe, napi_generic_failure); auto v8_resolver = maybe.ToLocalChecked(); - auto v8_deferred = new v8::Persistent(); + auto v8_deferred = new node::Persistent(); v8_deferred->Reset(env->isolate, v8_resolver); - *deferred = v8impl::JsDeferredFromV8Persistent(v8_deferred); + *deferred = v8impl::JsDeferredFromNodePersistent(v8_deferred); *promise = v8impl::JsValueFromV8LocalValue(v8_resolver->GetPromise()); return GET_RETURN_STATUS(env); } diff --git a/src/node_buffer.cc b/src/node_buffer.cc index dff9d3c0995e02..d1983bde5ee354 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -78,7 +78,6 @@ using v8::Local; using v8::Maybe; using v8::MaybeLocal; using v8::Object; -using v8::Persistent; using v8::String; using v8::Uint32Array; using v8::Uint8Array; diff --git a/src/node_contextify.cc b/src/node_contextify.cc index aac72514575695..d6b77c6145b2cb 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -50,7 +50,6 @@ using v8::NamedPropertyHandlerConfiguration; using v8::Nothing; using v8::Object; using v8::ObjectTemplate; -using v8::Persistent; using v8::PropertyAttribute; using v8::PropertyCallbackInfo; using v8::PropertyDescriptor; diff --git a/src/node_contextify.h b/src/node_contextify.h index c2b5b4dd9c93aa..33252e39abc378 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -15,7 +15,7 @@ class ContextifyContext { enum { kSandboxObjectIndex = 1 }; Environment* const env_; - v8::Persistent context_; + Persistent context_; public: ContextifyContext(Environment* env, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 595097e92a4424..eb7d3fdf7aa545 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -109,7 +109,6 @@ using v8::MaybeLocal; using v8::Null; using v8::Object; using v8::ObjectTemplate; -using v8::Persistent; using v8::PropertyAttribute; using v8::ReadOnly; using v8::Signature; @@ -3619,7 +3618,8 @@ void Connection::GetServername(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&conn, args.Holder()); if (conn->is_server() && !conn->servername_.IsEmpty()) { - args.GetReturnValue().Set(conn->servername_); + args.GetReturnValue().Set( + PersistentToLocal(args.GetIsolate(), conn->servername_)); } else { args.GetReturnValue().Set(false); } diff --git a/src/node_crypto.h b/src/node_crypto.h index b866117f844358..2f831b6812c282 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -354,11 +354,11 @@ class SSLWrap { ClientHelloParser hello_parser_; #ifdef NODE__HAVE_TLSEXT_STATUS_CB - v8::Persistent ocsp_response_; + Persistent ocsp_response_; #endif // NODE__HAVE_TLSEXT_STATUS_CB #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB - v8::Persistent sni_context_; + Persistent sni_context_; #endif friend class SecureContext; @@ -380,13 +380,13 @@ class Connection : public AsyncWrap, public SSLWrap { void NewSessionDoneCb(); #ifndef OPENSSL_NO_NEXTPROTONEG - v8::Persistent npnProtos_; - v8::Persistent selectedNPNProto_; + Persistent npnProtos_; + Persistent selectedNPNProto_; #endif #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB - v8::Persistent sniObject_; - v8::Persistent servername_; + Persistent sniObject_; + Persistent servername_; #endif size_t self_size() const override { return sizeof(*this); } diff --git a/src/node_file.cc b/src/node_file.cc index 9df13be5bd2dff..10655e54e54e90 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -366,7 +366,8 @@ class fs_req_wrap { After(uv_req); \ req_wrap = nullptr; \ } else { \ - args.GetReturnValue().Set(req_wrap->persistent()); \ + args.GetReturnValue().Set( \ + PersistentToLocal(env->isolate(), req_wrap->persistent())); \ } #define ASYNC_CALL(func, req, encoding, ...) \ @@ -1140,7 +1141,8 @@ static void WriteString(const FunctionCallbackInfo& args) { return; } - return args.GetReturnValue().Set(req_wrap->persistent()); + return args.GetReturnValue().Set( + PersistentToLocal(env->isolate(), req_wrap->persistent())); } diff --git a/src/node_internals.h b/src/node_internals.h index ced92da3216a28..af716c83997c78 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -25,6 +25,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "node.h" +#include "node_persistent.h" #include "util-inl.h" #include "env-inl.h" #include "uv.h" @@ -214,7 +215,7 @@ class Environment; template inline v8::Local PersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent); + const Persistent& persistent); // Creates a new context with Node.js-specific tweaks. Currently, it removes // the `v8BreakIterator` property from the global `Intl` object if present. diff --git a/src/node_persistent.h b/src/node_persistent.h new file mode 100644 index 00000000000000..762842dd4bd373 --- /dev/null +++ b/src/node_persistent.h @@ -0,0 +1,30 @@ +#ifndef SRC_NODE_PERSISTENT_H_ +#define SRC_NODE_PERSISTENT_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "v8.h" + +namespace node { + +template +struct ResetInDestructorPersistentTraits { + static const bool kResetInDestructor = true; + template + // Disallow copy semantics by leaving this unimplemented. + inline static void Copy( + const v8::Persistent&, + v8::Persistent>*); +}; + +// v8::Persistent does not reset the object slot in its destructor. That is +// acknowledged as a flaw in the V8 API and expected to change in the future +// but for now node::Persistent is the easier and safer alternative. +template +using Persistent = v8::Persistent>; + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_NODE_PERSISTENT_H_ diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 8ef4383e0355de..388630d507a433 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -46,7 +46,6 @@ using v8::HandleScope; using v8::Local; using v8::Number; using v8::Object; -using v8::Persistent; using v8::String; using v8::Uint32Array; using v8::Value; diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 008f9c01513bc0..81adf7a866b927 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -223,8 +223,8 @@ inline StreamWriteResult StreamBase::Write( return StreamWriteResult { async, err, req_wrap }; } -template -SimpleShutdownWrap::SimpleShutdownWrap( +template +SimpleShutdownWrap::SimpleShutdownWrap( StreamBase* stream, v8::Local req_wrap_obj) : ShutdownWrap(stream, req_wrap_obj), @@ -234,14 +234,9 @@ SimpleShutdownWrap::SimpleShutdownWrap( Wrap(req_wrap_obj, static_cast(this)); } -template -SimpleShutdownWrap::~SimpleShutdownWrap() { +template +SimpleShutdownWrap::~SimpleShutdownWrap() { ClearWrap(static_cast(this)->object()); - if (kResetPersistent) { - auto& persistent = static_cast(this)->persistent(); - CHECK_EQ(persistent.IsEmpty(), false); - persistent.Reset(); - } } inline ShutdownWrap* StreamBase::CreateShutdownWrap( @@ -249,8 +244,8 @@ inline ShutdownWrap* StreamBase::CreateShutdownWrap( return new SimpleShutdownWrap(this, object); } -template -SimpleWriteWrap::SimpleWriteWrap( +template +SimpleWriteWrap::SimpleWriteWrap( StreamBase* stream, v8::Local req_wrap_obj) : WriteWrap(stream, req_wrap_obj), @@ -260,14 +255,9 @@ SimpleWriteWrap::SimpleWriteWrap( Wrap(req_wrap_obj, static_cast(this)); } -template -SimpleWriteWrap::~SimpleWriteWrap() { +template +SimpleWriteWrap::~SimpleWriteWrap() { ClearWrap(static_cast(this)->object()); - if (kResetPersistent) { - auto& persistent = static_cast(this)->persistent(); - CHECK_EQ(persistent.IsEmpty(), false); - persistent.Reset(); - } } inline WriteWrap* StreamBase::CreateWriteWrap( diff --git a/src/stream_base.h b/src/stream_base.h index 59b8ee7b7221f0..8af05059f49e47 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -322,7 +322,7 @@ class StreamBase : public StreamResource { // `OtherBase` must have a constructor that matches the `AsyncWrap` // constructors’s (Environment*, Local, AsyncWrap::Provider) signature // and be a subclass of `AsyncWrap`. -template +template class SimpleShutdownWrap : public ShutdownWrap, public OtherBase { public: SimpleShutdownWrap(StreamBase* stream, @@ -333,7 +333,7 @@ class SimpleShutdownWrap : public ShutdownWrap, public OtherBase { size_t self_size() const override { return sizeof(*this); } }; -template +template class SimpleWriteWrap : public WriteWrap, public OtherBase { public: SimpleWriteWrap(StreamBase* stream, diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index e1df9edd39e151..27fe48d1165c75 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -264,8 +264,8 @@ void LibuvStreamWrap::SetBlocking(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(uv_stream_set_blocking(wrap->stream(), enable)); } -typedef SimpleShutdownWrap, false> LibuvShutdownWrap; -typedef SimpleWriteWrap, false> LibuvWriteWrap; +typedef SimpleShutdownWrap> LibuvShutdownWrap; +typedef SimpleWriteWrap> LibuvWriteWrap; ShutdownWrap* LibuvStreamWrap::CreateShutdownWrap(Local object) { return new LibuvShutdownWrap(this, object); diff --git a/src/util-inl.h b/src/util-inl.h index c5a25c91ffb088..d07cfea9227fbe 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -168,7 +168,7 @@ inline ContainerOfHelper ContainerOf(Inner Outer::*field, template inline v8::Local PersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent) { + const Persistent& persistent) { if (persistent.IsWeak()) { return WeakPersistentToLocal(isolate, persistent); } else { @@ -178,15 +178,15 @@ inline v8::Local PersistentToLocal( template inline v8::Local StrongPersistentToLocal( - const v8::Persistent& persistent) { + const Persistent& persistent) { return *reinterpret_cast*>( - const_cast*>(&persistent)); + const_cast*>(&persistent)); } template inline v8::Local WeakPersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent) { + const Persistent& persistent) { return v8::Local::New(isolate, persistent); } diff --git a/src/util.h b/src/util.h index 21c566a4ca6cd6..7c679952d5fb1f 100644 --- a/src/util.h +++ b/src/util.h @@ -24,6 +24,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "node_persistent.h" #include "v8.h" #include @@ -218,7 +219,7 @@ inline ContainerOfHelper ContainerOf(Inner Outer::*field, template inline v8::Local PersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent); + const Persistent& persistent); // Unchecked conversion from a non-weak Persistent to Local, // use with care! @@ -227,12 +228,12 @@ inline v8::Local PersistentToLocal( // scope, it will destroy the reference to the object. template inline v8::Local StrongPersistentToLocal( - const v8::Persistent& persistent); + const Persistent& persistent); template inline v8::Local WeakPersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent); + const Persistent& persistent); // Convenience wrapper around v8::String::NewFromOneByte(). inline v8::Local OneByteString(v8::Isolate* isolate,