Skip to content

Commit 7172a0a

Browse files
committed
src: call napi_remove_wrap() in ObjectWrap dtor
Currently, when the `ObjectWrap` constructor runs, it calls `napi_wrap()`, adding a finalize callback to the freshly created JS object. However, if the `ObjectWrap` instance is prematurely deleted, for example because a subclass constructor throws – which seems like a reasonable scenario – that finalize callback was not removed, possibly leading to a use-after-free crash. This commit adds a call `napi_remove_wrap()` from the `ObjectWrap` destructor, and a test for that scenario. Fixes: node-ffi-napi/weak-napi#16
1 parent e8935bd commit 7172a0a

File tree

7 files changed

+74
-2
lines changed

7 files changed

+74
-2
lines changed

napi-inl.h

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3145,8 +3145,11 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
31453145
*instanceRef = Reference<Object>(env, ref);
31463146
}
31473147

3148-
template<typename T>
3149-
inline ObjectWrap<T>::~ObjectWrap() {}
3148+
template <typename T>
3149+
inline ObjectWrap<T>::~ObjectWrap() {
3150+
if (!IsEmpty())
3151+
napi_remove_wrap(Env(), Value(), nullptr);
3152+
}
31503153

31513154
template<typename T>
31523155
inline T* ObjectWrap<T>::Unwrap(Object wrapper) {

test/binding.cc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ Object InitThreadSafeFunction(Env env);
5151
Object InitTypedArray(Env env);
5252
Object InitObjectWrap(Env env);
5353
Object InitObjectWrapConstructorException(Env env);
54+
Object InitObjectWrapRemoveWrap(Env env);
5455
Object InitObjectReference(Env env);
5556
Object InitVersionManagement(Env env);
5657
Object InitThunkingManual(Env env);
@@ -107,6 +108,7 @@ Object Init(Env env, Object exports) {
107108
exports.Set("objectwrap", InitObjectWrap(env));
108109
exports.Set("objectwrapConstructorException",
109110
InitObjectWrapConstructorException(env));
111+
exports.Set("objectwrap_removewrap", InitObjectWrapRemoveWrap(env));
110112
exports.Set("objectreference", InitObjectReference(env));
111113
exports.Set("version_management", InitVersionManagement(env));
112114
exports.Set("thunking_manual", InitThunkingManual(env));

test/binding.gyp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
'typedarray.cc',
4343
'objectwrap.cc',
4444
'objectwrap_constructor_exception.cc',
45+
'objectwrap-removewrap.cc',
4546
'objectreference.cc',
4647
'version_management.cc',
4748
'thunking_manual.cc',

test/index.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ let testModules = [
5050
'typedarray-bigint',
5151
'objectwrap',
5252
'objectwrap_constructor_exception',
53+
'objectwrap-removewrap',
5354
'objectreference',
5455
'version_management'
5556
];

test/objectwrap-removewrap.cc

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
#include <napi.h>
2+
#include <assert.h>
3+
4+
#ifdef NAPI_CPP_EXCEPTIONS
5+
namespace {
6+
7+
static int dtor_called = 0;
8+
9+
class DtorCounter {
10+
public:
11+
~DtorCounter() {
12+
assert(dtor_called == 0);
13+
dtor_called++;
14+
}
15+
};
16+
17+
Napi::Value GetDtorCalled(const Napi::CallbackInfo& info) {
18+
return Napi::Number::New(info.Env(), dtor_called);
19+
}
20+
21+
class Test : public Napi::ObjectWrap<Test> {
22+
public:
23+
Test(const Napi::CallbackInfo& info) : Napi::ObjectWrap<Test>(info) {
24+
throw Napi::Error::New(Env(), "Some error");
25+
}
26+
27+
static void Initialize(Napi::Env env, Napi::Object exports) {
28+
exports.Set("Test", DefineClass(env, "Test", {}));
29+
exports.Set("getDtorCalled", Napi::Function::New(env, GetDtorCalled));
30+
}
31+
32+
private:
33+
DtorCounter dtor_ounter_;
34+
};
35+
36+
} // anonymous namespace
37+
#endif // NAPI_CPP_EXCEPTIONS
38+
39+
Napi::Object InitObjectWrapRemoveWrap(Napi::Env env) {
40+
Napi::Object exports = Napi::Object::New(env);
41+
#ifdef NAPI_CPP_EXCEPTIONS
42+
Test::Initialize(env, exports);
43+
#endif
44+
return exports;
45+
}

test/objectwrap-removewrap.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
'use strict';
2+
const buildType = process.config.target_defaults.default_configuration;
3+
const assert = require('assert');
4+
5+
const test = (binding) => {
6+
const Test = binding.objectwrap_removewrap.Test;
7+
const getDtorCalled = binding.objectwrap_removewrap.getDtorCalled;
8+
9+
assert.strictEqual(getDtorCalled(), 0);
10+
assert.throws(() => {
11+
new Test();
12+
});
13+
assert.strictEqual(getDtorCalled(), 1);
14+
global.gc(); // Does not crash.
15+
}
16+
17+
test(require(`./build/${buildType}/binding.node`));

test/objectwrap.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,9 @@ const test = (binding) => {
273273
// `Test` is needed for accessing exposed symbols
274274
testObj(new Test(), Test);
275275
testClass(Test);
276+
277+
// Make sure the C++ object can be garbage collected without issues.
278+
setImmediate(global.gc);
276279
}
277280

278281
test(require(`./build/${buildType}/binding.node`));

0 commit comments

Comments
 (0)