Skip to content

Commit

Permalink
src: prevent persistent handle resource leaks
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
bnoordhuis authored and MylesBorins committed Mar 7, 2018
1 parent 08bcdde commit 67a9742
Show file tree
Hide file tree
Showing 25 changed files with 97 additions and 82 deletions.
3 changes: 2 additions & 1 deletion node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
4 changes: 2 additions & 2 deletions src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};


Expand Down
2 changes: 1 addition & 1 deletion src/base_object-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ inline BaseObject::~BaseObject() {
}


inline v8::Persistent<v8::Object>& BaseObject::persistent() {
inline Persistent<v8::Object>& BaseObject::persistent() {
return persistent_handle_;
}

Expand Down
10 changes: 3 additions & 7 deletions src/base_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include "node_persistent.h"
#include "v8.h"

namespace node {
Expand All @@ -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;

Expand All @@ -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_;
};

Expand Down
4 changes: 2 additions & 2 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 2 additions & 3 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand All @@ -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

Expand Down
1 change: 0 additions & 1 deletion src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
6 changes: 3 additions & 3 deletions src/inspector_agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand All @@ -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
Expand Down
1 change: 0 additions & 1 deletion src/inspector_js_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
8 changes: 4 additions & 4 deletions src/module_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 13 additions & 13 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -470,7 +470,7 @@ class Reference : private Finalizer {
}
}

v8::Persistent<v8::Value> _persistent;
node::Persistent<v8::Value> _persistent;
uint32_t _refcount;
bool _delete_self;
};
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
}
Expand Down
1 change: 0 additions & 1 deletion src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/node_contextify.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
12 changes: 6 additions & 6 deletions src/node_crypto.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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); }
Expand Down
6 changes: 4 additions & 2 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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, ...) \
Expand Down Expand Up @@ -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()));
}


Expand Down
3 changes: 2 additions & 1 deletion src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down
30 changes: 30 additions & 0 deletions src/node_persistent.h
Original file line number Diff line number Diff line change
@@ -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_
1 change: 0 additions & 1 deletion src/node_zlib.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading

0 comments on commit 67a9742

Please sign in to comment.