From 2ce5dfb1ec7f373c0c39a8f357555ffd43de3f13 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. 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 | 1 - src/node_crypto.h | 4 ++-- src/node_file.h | 1 - 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, 87 insertions(+), 76 deletions(-) create mode 100644 src/node_persistent.h diff --git a/node.gyp b/node.gyp index 44c46ad2bc8854..f0e51d7a84af36 100644 --- a/node.gyp +++ b/node.gyp @@ -371,9 +371,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 7fa5f0ade9ade4..c23535f094d336 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -409,8 +409,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 2b2b0cba58cd27..2aec83f50372d2 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -550,8 +550,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 a9118082b8adda..dab9aec9970790 100644 --- a/src/env.h +++ b/src/env.h @@ -847,7 +847,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_; @@ -858,8 +858,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 ee3740b56119f6..3969e2a37878fc 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -62,11 +62,11 @@ class ModuleWrap : public BaseObject { static ModuleWrap* GetFromModule(node::Environment*, v8::Local); - 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 72776304c8eb70..c511562b8027b6 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 35b39d62e1fc77..6966fac6f6326b 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -48,7 +48,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 b2f32850ac1c43..5c38c5d363d55f 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 a0b21e7c39d76c..1ae744d6afc5c8 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -100,7 +100,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; diff --git a/src/node_crypto.h b/src/node_crypto.h index 7ce40697d402cd..1c17dc108198f8 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -353,11 +353,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; diff --git a/src/node_file.h b/src/node_file.h index 094963c53ac586..a65b9f0e5fcca9 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -14,7 +14,6 @@ using v8::HandleScope; using v8::Local; using v8::MaybeLocal; using v8::Object; -using v8::Persistent; using v8::Promise; using v8::Undefined; using v8::Value; diff --git a/src/node_internals.h b/src/node_internals.h index 094fcc2d839d5f..140826bb7c5ebd 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" @@ -215,7 +216,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 b479e04bae4c8a..c864feb9270b52 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -225,8 +225,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), @@ -236,14 +236,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( @@ -251,8 +246,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), @@ -262,14 +257,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,