src: prevent persistent handle resource leaks

bnoordhuis · MylesBorins · commit 2503259f2a2e · 2018-03-06T14:57:04.000-08:00
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: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
diff --git 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
@@ -410,8 +410,8 @@ static void DisablePromiseHook(const FunctionCallbackInfo<Value>& args) {
 class DestroyParam {
  public:
   double asyncId;
-  v8::Persistent<Object> target;
-  v8::Persistent<Object> propBag;
+  Persistent<Object> target;
+  Persistent<Object> propBag;
 };
 
 
diff --git a/src/base_object-inl.h b/src/base_object-inl.h
@@ -47,7 +47,7 @@ inline BaseObject::~BaseObject() {
 }
 
 
-inline v8::Persistent<v8::Object>& BaseObject::persistent() {
+inline Persistent<v8::Object>& BaseObject::persistent() {
   return persistent_handle_;
 }
 
diff --git 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<v8::Object> 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<v8::Object>& persistent();
+  inline Persistent<v8::Object>& 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<v8::Object> persistent_handle_;
+  Persistent<v8::Object> persistent_handle_;
   Environment* env_;
 };
 
diff --git 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<v8::Persistent<v8::Object>>(obj.IsEmpty() ?
-        nullptr : new v8::Persistent<v8::Object>(isolate_, obj)),
+    std::unique_ptr<Persistent<v8::Object>>(obj.IsEmpty() ?
+        nullptr : new Persistent<v8::Object>(isolate_, obj)),
     ref
   });
   immediate_info()->count_inc(1);
diff --git 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<v8::Persistent<v8::Object>> keep_alive_;
+    std::unique_ptr<Persistent<v8::Object>> keep_alive_;
     bool refed_;
   };
   std::vector<NativeImmediateCallback> native_immediate_callbacks_;
@@ -811,8 +811,7 @@ class Environment {
                              v8::Local<v8::Promise> promise,
                              v8::Local<v8::Value> parent);
 
-#define V(PropertyName, TypeName)                                             \
-  v8::Persistent<TypeName> PropertyName ## _;
+#define V(PropertyName, TypeName) Persistent<TypeName> PropertyName ## _;
   ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
 #undef V
 
diff --git 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
@@ -97,7 +97,7 @@ class Agent {
 
  private:
   void ToggleAsyncHook(v8::Isolate* isolate,
-                       const v8::Persistent<v8::Function>& fn);
+                       const Persistent<v8::Function>& fn);
 
   node::Environment* parent_env_;
   std::unique_ptr<NodeInspectorClient> client_;
@@ -109,8 +109,8 @@ class Agent {
 
   bool pending_enable_async_hook_;
   bool pending_disable_async_hook_;
-  v8::Persistent<v8::Function> enable_async_hook_function_;
-  v8::Persistent<v8::Function> disable_async_hook_function_;
+  Persistent<v8::Function> enable_async_hook_function_;
+  Persistent<v8::Function> disable_async_hook_function_;
 };
 
 }  // namespace inspector
diff --git 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
@@ -49,11 +49,11 @@ class ModuleWrap : public BaseObject {
       v8::Local<v8::String> specifier,
       v8::Local<v8::Module> referrer);
 
-  v8::Persistent<v8::Module> module_;
-  v8::Persistent<v8::String> url_;
+  Persistent<v8::Module> module_;
+  Persistent<v8::String> url_;
   bool linked_ = false;
-  std::unordered_map<std::string, v8::Persistent<v8::Promise>> resolve_cache_;
-  v8::Persistent<v8::Context> context_;
+  std::unordered_map<std::string, Persistent<v8::Promise>> resolve_cache_;
+  Persistent<v8::Context> context_;
 };
 
 }  // namespace loader
diff --git 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<v8::Value> last_exception;
-  v8::Persistent<v8::ObjectTemplate> wrap_template;
-  v8::Persistent<v8::ObjectTemplate> function_data_template;
-  v8::Persistent<v8::ObjectTemplate> accessor_data_template;
+  node::Persistent<v8::Value> last_exception;
+  node::Persistent<v8::ObjectTemplate> wrap_template;
+  node::Persistent<v8::ObjectTemplate> function_data_template;
+  node::Persistent<v8::ObjectTemplate> 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<v8::Value>) == sizeof(napi_value),
   "Cannot convert between v8::Local<v8::Value> and napi_value");
 
 static
-napi_deferred JsDeferredFromV8Persistent(v8::Persistent<v8::Value>* local) {
+napi_deferred JsDeferredFromNodePersistent(node::Persistent<v8::Value>* local) {
   return reinterpret_cast<napi_deferred>(local);
 }
 
 static
-v8::Persistent<v8::Value>* V8PersistentFromJsDeferred(napi_deferred local) {
-  return reinterpret_cast<v8::Persistent<v8::Value>*>(local);
+node::Persistent<v8::Value>* NodePersistentFromJsDeferred(napi_deferred local) {
+  return reinterpret_cast<node::Persistent<v8::Value>*>(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<v8::Value> _persistent;
+  node::Persistent<v8::Value> _persistent;
   uint32_t _refcount;
   bool _delete_self;
 };
@@ -846,8 +846,8 @@ napi_status ConcludeDeferred(napi_env env,
   CHECK_ARG(env, result);
 
   v8::Local<v8::Context> context = env->isolate->GetCurrentContext();
-  v8::Persistent<v8::Value>* deferred_ref =
-      V8PersistentFromJsDeferred(deferred);
+  node::Persistent<v8::Value>* deferred_ref =
+      NodePersistentFromJsDeferred(deferred);
   v8::Local<v8::Value> v8_deferred =
       v8::Local<v8::Value>::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<v8::Value>();
+  auto v8_deferred = new node::Persistent<v8::Value>();
   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
@@ -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
@@ -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
@@ -15,7 +15,7 @@ class ContextifyContext {
   enum { kSandboxObjectIndex = 1 };
 
   Environment* const env_;
-  v8::Persistent<v8::Context> context_;
+  Persistent<v8::Context> context_;
 
  public:
   ContextifyContext(Environment* env,
diff --git 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<Value>& 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
@@ -354,11 +354,11 @@ class SSLWrap {
   ClientHelloParser hello_parser_;
 
 #ifdef NODE__HAVE_TLSEXT_STATUS_CB
-  v8::Persistent<v8::Object> ocsp_response_;
+  Persistent<v8::Object> ocsp_response_;
 #endif  // NODE__HAVE_TLSEXT_STATUS_CB
 
 #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
-  v8::Persistent<v8::Value> sni_context_;
+  Persistent<v8::Value> sni_context_;
 #endif
 
   friend class SecureContext;
@@ -380,13 +380,13 @@ class Connection : public AsyncWrap, public SSLWrap<Connection> {
   void NewSessionDoneCb();
 
 #ifndef OPENSSL_NO_NEXTPROTONEG
-  v8::Persistent<v8::Object> npnProtos_;
-  v8::Persistent<v8::Value> selectedNPNProto_;
+  Persistent<v8::Object> npnProtos_;
+  Persistent<v8::Value> selectedNPNProto_;
 #endif
 
 #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
-  v8::Persistent<v8::Object> sniObject_;
-  v8::Persistent<v8::String> servername_;
+  Persistent<v8::Object> sniObject_;
+  Persistent<v8::String> servername_;
 #endif
 
   size_t self_size() const override { return sizeof(*this); }
diff --git 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<Value>& 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
@@ -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 <class TypeName>
 inline v8::Local<TypeName> PersistentToLocal(
     v8::Isolate* isolate,
-    const v8::Persistent<TypeName>& persistent);
+    const Persistent<TypeName>& 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
@@ -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 <typename T>
+struct ResetInDestructorPersistentTraits {
+  static const bool kResetInDestructor = true;
+  template <typename S, typename M>
+  // Disallow copy semantics by leaving this unimplemented.
+  inline static void Copy(
+      const v8::Persistent<S, M>&,
+      v8::Persistent<T, ResetInDestructorPersistentTraits<T>>*);
+};
+
+// 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 <typename T>
+using Persistent = v8::Persistent<T, ResetInDestructorPersistentTraits<T>>;
+
+}  // 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
@@ -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
diff --git a/src/stream_base.h b/src/stream_base.h
diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc
diff --git a/src/util-inl.h b/src/util-inl.h
diff --git a/src/util.h b/src/util.h

Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ inline BaseObject::~BaseObject() {`
`47`	`47`	`}`
`48`	`48`
`49`	`49`
`50`		`-inline v8::Persistent<v8::Object>& BaseObject::persistent() {`
	`50`	`+inline Persistent<v8::Object>& BaseObject::persistent() {`
`51`	`51`	`return persistent_handle_;`
`52`	`52`	`}`
`53`	`53`