From 375af79b23ed02c3a46a3b28f12a4d6eb3721990 Mon Sep 17 00:00:00 2001 From: Taylor Woll Date: Tue, 28 Mar 2017 16:09:19 -0700 Subject: [PATCH] Refcount for v8impl::Reference should be unsigned (#207) * Refcount for v8impl::Reference should be unsigned Add ```Reference::RefCount()``` and check the refcount value before trying to call ```Reference::Unref()``` to avoid underflowing the refcount. This allows us to continue returning an error from ```napi_reference_unref``` if it is called with a reference that already has refcount set to zero. * Add check for _refcount == 0 in Reference::Unref to avoid an underflow --- src/node_api.cc | 32 ++++++++++++++++++++------------ src/node_api.h | 6 +++--- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index ce202fc67c825d..63d755e61464e8 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -144,7 +144,7 @@ class Reference : private Finalizer { private: Reference(v8::Isolate* isolate, v8::Local value, - int initial_refcount, + uint32_t initial_refcount, bool delete_self, napi_finalize finalize_callback, void* finalize_data, @@ -173,7 +173,7 @@ class Reference : private Finalizer { public: static Reference* New(v8::Isolate* isolate, v8::Local value, - int initial_refcount, + uint32_t initial_refcount, bool delete_self, napi_finalize finalize_callback = nullptr, void* finalize_data = nullptr, @@ -191,7 +191,7 @@ class Reference : private Finalizer { delete reference; } - int Ref() { + uint32_t Ref() { if (++_refcount == 1) { _persistent.ClearWeak(); } @@ -199,7 +199,10 @@ class Reference : private Finalizer { return _refcount; } - int Unref() { + uint32_t Unref() { + if (_refcount == 0) { + return 0; + } if (--_refcount == 0) { _persistent.SetWeak( this, FinalizeCallback, v8::WeakCallbackType::kParameter); @@ -209,6 +212,10 @@ class Reference : private Finalizer { return _refcount; } + uint32_t RefCount() { + return _refcount; + } + v8::Local Get() { if (_persistent.IsEmpty()) { return v8::Local(); @@ -239,7 +246,7 @@ class Reference : private Finalizer { } v8::Persistent _persistent; - int _refcount; + uint32_t _refcount; bool _delete_self; }; @@ -1901,11 +1908,10 @@ napi_status napi_get_value_external(napi_env env, // Set initial_refcount to 0 for a weak reference, >0 for a strong reference. napi_status napi_create_reference(napi_env env, napi_value value, - int initial_refcount, + uint32_t initial_refcount, napi_ref* result) { NAPI_PREAMBLE(env); CHECK_ARG(result); - RETURN_STATUS_IF_FALSE(initial_refcount >= 0, napi_invalid_arg); v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(env); @@ -1933,12 +1939,12 @@ napi_status napi_delete_reference(napi_env env, napi_ref ref) { // refcount is >0, and the referenced object is effectively "pinned". // Calling this when the refcount is 0 and the object is unavailable // results in an error. -napi_status napi_reference_ref(napi_env env, napi_ref ref, int* result) { +napi_status napi_reference_ref(napi_env env, napi_ref ref, uint32_t* result) { NAPI_PREAMBLE(env); CHECK_ARG(ref); v8impl::Reference* reference = reinterpret_cast(ref); - int count = reference->Ref(); + uint32_t count = reference->Ref(); if (result != nullptr) { *result = count; @@ -1951,16 +1957,18 @@ napi_status napi_reference_ref(napi_env env, napi_ref ref, int* result) { // the result is 0 the reference is now weak and the object may be GC'd at any // time if there are no other references. Calling this when the refcount is // already 0 results in an error. -napi_status napi_reference_unref(napi_env env, napi_ref ref, int* result) { +napi_status napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) { NAPI_PREAMBLE(env); CHECK_ARG(ref); v8impl::Reference* reference = reinterpret_cast(ref); - int count = reference->Unref(); - if (count < 0) { + + if (reference->RefCount() == 0) { return napi_set_last_error(napi_generic_failure); } + uint32_t count = reference->Unref(); + if (result != nullptr) { *result = count; } diff --git a/src/node_api.h b/src/node_api.h index b0fff07ec123e4..61372322a97906 100644 --- a/src/node_api.h +++ b/src/node_api.h @@ -352,7 +352,7 @@ NAPI_EXTERN napi_status napi_get_value_external(napi_env env, // Set initial_refcount to 0 for a weak reference, >0 for a strong reference. NAPI_EXTERN napi_status napi_create_reference(napi_env env, napi_value value, - int initial_refcount, + uint32_t initial_refcount, napi_ref* result); // Deletes a reference. The referenced value is released, and may @@ -366,7 +366,7 @@ NAPI_EXTERN napi_status napi_delete_reference(napi_env env, napi_ref ref); // results in an error. NAPI_EXTERN napi_status napi_reference_ref(napi_env env, napi_ref ref, - int* result); + uint32_t* result); // Decrements the reference count, optionally returning the resulting count. // If the result is 0 the reference is now weak and the object may be GC'd @@ -374,7 +374,7 @@ NAPI_EXTERN napi_status napi_reference_ref(napi_env env, // refcount is already 0 results in an error. NAPI_EXTERN napi_status napi_reference_unref(napi_env env, napi_ref ref, - int* result); + uint32_t* result); // Attempts to get a referenced value. If the reference is weak, // the value might no longer be available, in that case the call