Skip to content

Commit

Permalink
node-api: force env shutdown deferring behavior
Browse files Browse the repository at this point in the history
The finalizer normally never gets called while a reference is strong.
However, during environment shutdown all finalizers must get called. In
order to unify the deferring behavior with that of a regular
finalization, we must force the reference to be weak when we call its
finalizer during environment shutdown.

Fixes: #37236
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
PR-URL: #37303
Backport-PR-URL: #42512
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
  • Loading branch information
2 people authored and richardlau committed Mar 30, 2022
1 parent 1193290 commit 171bb66
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 0 deletions.
9 changes: 9 additions & 0 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,17 @@ class RefBase : protected Finalizer, RefTracker {

protected:
inline void Finalize(bool is_env_teardown = false) override {
// During environment teardown we have to convert a strong reference to
// a weak reference to force the deferring behavior if the user's finalizer
// happens to delete this reference so that the code in this function that
// follows the call to the user's finalizer may safely access variables from
// this instance.
if (is_env_teardown && RefCount() > 0) _refcount = 0;

if (_finalize_callback != nullptr) {
_env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint);
// This ensures that we never call the finalizer twice.
_finalize_callback = nullptr;
}

// this is safe because if a request to delete the reference
Expand Down
11 changes: 11 additions & 0 deletions test/js-native-api/test_reference_double_free/binding.gyp
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"targets": [
{
"target_name": "test_reference_double_free",
"sources": [
"../entry_point.c",
"test_reference_double_free.c"
]
}
]
}
11 changes: 11 additions & 0 deletions test/js-native-api/test_reference_double_free/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
'use strict';

// This test makes no assertions. It tests a fix without which it will crash
// with a double free.

const { buildType } = require('../../common');

const addon = require(`./build/${buildType}/test_reference_double_free`);

{ new addon.MyObject(true); }
{ new addon.MyObject(false); }
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
#include <stdlib.h>
#include <js_native_api.h>
#include "../common.h"

static size_t g_call_count = 0;

static void Destructor(napi_env env, void* data, void* nothing) {
napi_ref* ref = data;
NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref));
free(ref);
}

static void NoDeleteDestructor(napi_env env, void* data, void* hint) {
napi_ref* ref = data;
size_t* call_count = hint;

// This destructor must be called exactly once.
if ((*call_count) > 0) abort();
*call_count = ((*call_count) + 1);
free(ref);
}

static napi_value New(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value js_this, js_delete;
bool delete;
napi_ref* ref = malloc(sizeof(*ref));

NAPI_CALL(env,
napi_get_cb_info(env, info, &argc, &js_delete, &js_this, NULL));
NAPI_CALL(env, napi_get_value_bool(env, js_delete, &delete));

if (delete) {
NAPI_CALL(env,
napi_wrap(env, js_this, ref, Destructor, NULL, ref));
} else {
NAPI_CALL(env,
napi_wrap(env, js_this, ref, NoDeleteDestructor, &g_call_count, ref));
}
NAPI_CALL(env, napi_reference_ref(env, *ref, NULL));

return js_this;
}

EXTERN_C_START
napi_value Init(napi_env env, napi_value exports) {
napi_value myobj_ctor;
NAPI_CALL(env,
napi_define_class(
env, "MyObject", NAPI_AUTO_LENGTH, New, NULL, 0, NULL, &myobj_ctor));
NAPI_CALL(env,
napi_set_named_property(env, exports, "MyObject", myobj_ctor));
return exports;
}
EXTERN_C_END

0 comments on commit 171bb66

Please sign in to comment.