Skip to content

Commit

Permalink
objectwrap: avoid double-free on old Node.js
Browse files Browse the repository at this point in the history
On Node.js versions earlier than 10.15.3 references were being cleaned
up differently. This has resulted in a segfault because of a double
free when the `ObjectWrap<T>` subclass constructor throws. We need a
fix that only runs on Node.js < 10.15.3. We use
`Napi::VersionManagement::GetNodeVersion()` at addon load time to
determine whether we need the workaround. We store the result in an
atomic boolean and consult its value whenever we destroy an
`ObjectWrap<T>` instance.
  • Loading branch information
Gabriel Schulhof committed May 16, 2020
1 parent e58b770 commit 31be779
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 5 deletions.
34 changes: 29 additions & 5 deletions napi-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
// Note: Do not include this file directly! Include "napi.h" instead.

#include <algorithm>
#include <atomic>
#include <cstring>
#include <mutex>
#include <type_traits>
Expand All @@ -19,6 +20,8 @@ namespace Napi {
// Helpers to handle functions exposed from C++.
namespace details {

extern std::atomic_bool needs_objectwrap_destructor_fix;

// Attach a data item to an object and delete it when the object gets
// garbage-collected.
// TODO: Replace this code with `napi_add_finalizer()` whenever it becomes
Expand Down Expand Up @@ -251,11 +254,16 @@ struct AccessorCallbackData {
// Module registration
////////////////////////////////////////////////////////////////////////////////

#define NODE_API_MODULE(modname, regfunc) \
napi_value __napi_ ## regfunc(napi_env env, \
napi_value exports) { \
return Napi::RegisterModule(env, exports, regfunc); \
} \
#define NODE_API_MODULE(modname, regfunc) \
namespace Napi { \
namespace details { \
std::atomic_bool needs_objectwrap_destructor_fix(false); \
} \
} \
napi_value __napi_ ## regfunc(napi_env env, \
napi_value exports) { \
return Napi::RegisterModule(env, exports, regfunc); \
} \
NAPI_MODULE(modname, __napi_ ## regfunc)

// Adapt the NAPI_MODULE registration function:
Expand All @@ -264,6 +272,12 @@ struct AccessorCallbackData {
inline napi_value RegisterModule(napi_env env,
napi_value exports,
ModuleRegisterCallback registerCallback) {
const napi_node_version* nver = Napi::VersionManagement::GetNodeVersion(env);
Napi::details::needs_objectwrap_destructor_fix =
(nver->major < 10 ||
(nver->major == 10 && nver->minor < 15) ||
(nver->major == 10 && nver->minor == 15 && nver->patch < 3));

return details::WrapCallback([&] {
return napi_value(registerCallback(Napi::Env(env),
Napi::Object(env, exports)));
Expand Down Expand Up @@ -2985,6 +2999,16 @@ inline ObjectWrap<T>::~ObjectWrap() {
// This happens e.g. during garbage collection.
if (!object.IsEmpty() && _construction_failed) {
napi_remove_wrap(Env(), object, nullptr);

if (Napi::details::needs_objectwrap_destructor_fix) {
// If construction failed we delete the reference via
// `napi_remove_wrap()`, not via `napi_delete_reference()` in the
// `Reference<Object>` destructor. This will prevent the
// `Reference<Object>` destructor from doing a double delete of this
// reference.
_ref = nullptr;
_env = nullptr;
}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions test/objectwrap_constructor_exception.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ class ConstructorExceptionTest :
public:
ConstructorExceptionTest(const Napi::CallbackInfo& info) :
Napi::ObjectWrap<ConstructorExceptionTest>(info) {
fprintf(stderr, "That was the interesting reference\n");
Napi::Error error = Napi::Error::New(info.Env(), "an exception");
#ifdef NAPI_DISABLE_CPP_EXCEPTIONS
error.ThrowAsJavaScriptException();
Expand Down

0 comments on commit 31be779

Please sign in to comment.