Skip to content

Commit

Permalink
src: unload addons when environment quits
Browse files Browse the repository at this point in the history 
This is an alternative to #23319
which attaches the loaded addons to the environment and closes them
when the environment is destroyed.

Backport-PR-URL: #25258
PR-URL: #24861
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
  • Loading branch information
@targos
Gabriel Schulhof authored and targos committed Jan 1, 2019
1 parent 95353c7 commit d718625
Show file tree
Hide file tree
Showing 8 changed files with 163 additions and 109 deletions.
10 changes: 10 additions & 0 deletions src/env-inl.h
View file
Expand Up @@ -396,6 +396,16 @@ inline uv_loop_t* Environment::event_loop() const {
return isolate_data()->event_loop();
}

inline void Environment::TryLoadAddon(
const char* filename,
int flags,
std::function<bool(binding::DLib*)> was_loaded) {
loaded_addons_.emplace_back(filename, flags);
if (!was_loaded(&loaded_addons_.back())) {
loaded_addons_.pop_back();
}
}

inline Environment::AsyncHooks* Environment::async_hooks() {
return &async_hooks_;
}
Expand Down
5 changes: 5 additions & 0 deletions src/env.cc
View file
Expand Up @@ -276,6 +276,11 @@ Environment::~Environment() {

TRACE_EVENT_NESTABLE_ASYNC_END0(
TRACING_CATEGORY_NODE1(environment), "Environment", this);

// Dereference all addons that were loaded into this environment.
for (binding::DLib& addon : loaded_addons_) {
addon.Close();
}
}

void Environment::Start(const std::vector<std::string>& args,
Expand Down
10 changes: 8 additions & 2 deletions src/env.h
View file
Expand Up @@ -30,18 +30,20 @@
#endif
#include "handle_wrap.h"
#include "node.h"
#include "node_binding.h"
#include "node_http2_state.h"
#include "node_options.h"
#include "req_wrap.h"
#include "util.h"
#include "uv.h"
#include "v8.h"

#include <list>
#include <stdint.h>
#include <vector>
#include <functional>
#include <list>
#include <unordered_map>
#include <unordered_set>
#include <vector>

struct nghttp2_rcbuf;

Expand Down Expand Up @@ -640,6 +642,9 @@ class Environment {
inline v8::Isolate* isolate() const;
inline uv_loop_t* event_loop() const;
inline uint32_t watched_providers() const;
inline void TryLoadAddon(const char* filename,
int flags,
std::function<bool(binding::DLib*)> was_loaded);

static inline Environment* from_timer_handle(uv_timer_t* handle);
inline uv_timer_t* timer_handle();
Expand Down Expand Up @@ -926,6 +931,7 @@ class Environment {
inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
const char* errmsg);

std::list<binding::DLib> loaded_addons_;
v8::Isolate* const isolate_;
IsolateData* const isolate_data_;
uv_timer_t timer_handle_;
Expand Down
182 changes: 80 additions & 102 deletions src/node_binding.cc
View file
Expand Up @@ -2,10 +2,6 @@
#include "node_internals.h"
#include "node_native_module.h"

#if defined(__POSIX__)
#include <dlfcn.h>
#endif

#if HAVE_OPENSSL
#define NODE_BUILTIN_OPENSSL_MODULES(V) V(crypto) V(tls_wrap)
#else
Expand Down Expand Up @@ -126,31 +122,8 @@ extern "C" void node_module_register(void* m) {

namespace binding {

class DLib {
public:
#ifdef __POSIX__
static const int kDefaultFlags = RTLD_LAZY;
#else
static const int kDefaultFlags = 0;
#endif

inline DLib(const char* filename, int flags)
: filename_(filename), flags_(flags), handle_(nullptr) {}

inline bool Open();
inline void Close();
inline void* GetSymbolAddress(const char* name);

const std::string filename_;
const int flags_;
std::string errmsg_;
void* handle_;
#ifndef __POSIX__
uv_lib_t lib_;
#endif
private:
DISALLOW_COPY_AND_ASSIGN(DLib);
};
DLib::DLib(const char* filename, int flags)
: filename_(filename), flags_(flags), handle_(nullptr) {}

#ifdef __POSIX__
bool DLib::Open() {
Expand Down Expand Up @@ -247,87 +220,92 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
}

node::Utf8Value filename(env->isolate(), args[1]); // Cast
DLib dlib(*filename, flags);
bool is_opened = dlib.Open();

// Objects containing v14 or later modules will have registered themselves
// on the pending list. Activate all of them now. At present, only one
// module per object is supported.
node_module* const mp =
static_cast<node_module*>(uv_key_get(&thread_local_modpending));
uv_key_set(&thread_local_modpending, nullptr);

if (!is_opened) {
Local<String> errmsg = OneByteString(env->isolate(), dlib.errmsg_.c_str());
dlib.Close();
env->TryLoadAddon(*filename, flags, [&](DLib* dlib) {
const bool is_opened = dlib->Open();

// Objects containing v14 or later modules will have registered themselves
// on the pending list. Activate all of them now. At present, only one
// module per object is supported.
node_module* const mp =
static_cast<node_module*>(uv_key_get(&thread_local_modpending));
uv_key_set(&thread_local_modpending, nullptr);

if (!is_opened) {
Local<String> errmsg =
OneByteString(env->isolate(), dlib->errmsg_.c_str());
dlib->Close();
#ifdef _WIN32
// Windows needs to add the filename into the error message
errmsg = String::Concat(
env->isolate(), errmsg, args[1]->ToString(context).ToLocalChecked());
// Windows needs to add the filename into the error message
errmsg = String::Concat(
env->isolate(), errmsg, args[1]->ToString(context).ToLocalChecked());
#endif // _WIN32
env->isolate()->ThrowException(Exception::Error(errmsg));
return;
}
env->isolate()->ThrowException(Exception::Error(errmsg));
return false;
}

if (mp == nullptr) {
if (auto callback = GetInitializerCallback(&dlib)) {
callback(exports, module, context);
} else if (auto napi_callback = GetNapiInitializerCallback(&dlib)) {
napi_module_register_by_symbol(exports, module, context, napi_callback);
} else {
dlib.Close();
env->ThrowError("Module did not self-register.");
if (mp == nullptr) {
if (auto callback = GetInitializerCallback(dlib)) {
callback(exports, module, context);
} else if (auto napi_callback = GetNapiInitializerCallback(dlib)) {
napi_module_register_by_symbol(exports, module, context, napi_callback);
} else {
dlib->Close();
env->ThrowError("Module did not self-register.");
return false;
}
return true;
}
return;
}

// -1 is used for N-API modules
if ((mp->nm_version != -1) && (mp->nm_version != NODE_MODULE_VERSION)) {
// Even if the module did self-register, it may have done so with the wrong
// version. We must only give up after having checked to see if it has an
// appropriate initializer callback.
if (auto callback = GetInitializerCallback(&dlib)) {
callback(exports, module, context);
return;
// -1 is used for N-API modules
if ((mp->nm_version != -1) && (mp->nm_version != NODE_MODULE_VERSION)) {
// Even if the module did self-register, it may have done so with the
// wrong version. We must only give up after having checked to see if it
// has an appropriate initializer callback.
if (auto callback = GetInitializerCallback(dlib)) {
callback(exports, module, context);
return true;
}
char errmsg[1024];
snprintf(errmsg,
sizeof(errmsg),
"The module '%s'"
"\nwas compiled against a different Node.js version using"
"\nNODE_MODULE_VERSION %d. This version of Node.js requires"
"\nNODE_MODULE_VERSION %d. Please try re-compiling or "
"re-installing\nthe module (for instance, using `npm rebuild` "
"or `npm install`).",
*filename,
mp->nm_version,
NODE_MODULE_VERSION);

// NOTE: `mp` is allocated inside of the shared library's memory, calling
// `dlclose` will deallocate it
dlib->Close();
env->ThrowError(errmsg);
return false;
}
if (mp->nm_flags & NM_F_BUILTIN) {
dlib->Close();
env->ThrowError("Built-in module self-registered.");
return false;
}
char errmsg[1024];
snprintf(errmsg,
sizeof(errmsg),
"The module '%s'"
"\nwas compiled against a different Node.js version using"
"\nNODE_MODULE_VERSION %d. This version of Node.js requires"
"\nNODE_MODULE_VERSION %d. Please try re-compiling or "
"re-installing\nthe module (for instance, using `npm rebuild` "
"or `npm install`).",
*filename,
mp->nm_version,
NODE_MODULE_VERSION);

// NOTE: `mp` is allocated inside of the shared library's memory, calling
// `dlclose` will deallocate it
dlib.Close();
env->ThrowError(errmsg);
return;
}
if (mp->nm_flags & NM_F_BUILTIN) {
dlib.Close();
env->ThrowError("Built-in module self-registered.");
return;
}

mp->nm_dso_handle = dlib.handle_;
mp->nm_link = modlist_addon;
modlist_addon = mp;
mp->nm_dso_handle = dlib->handle_;
mp->nm_link = modlist_addon;
modlist_addon = mp;

if (mp->nm_context_register_func != nullptr) {
mp->nm_context_register_func(exports, module, context, mp->nm_priv);
} else if (mp->nm_register_func != nullptr) {
mp->nm_register_func(exports, module, mp->nm_priv);
} else {
dlib.Close();
env->ThrowError("Module has no declared entry point.");
return;
}
if (mp->nm_context_register_func != nullptr) {
mp->nm_context_register_func(exports, module, context, mp->nm_priv);
} else if (mp->nm_register_func != nullptr) {
mp->nm_register_func(exports, module, mp->nm_priv);
} else {
dlib->Close();
env->ThrowError("Module has no declared entry point.");
return false;
}

return true;
});

// Tell coverity that 'handle' should not be freed when we return.
// coverity[leaked_storage]
Expand Down
34 changes: 34 additions & 0 deletions src/node_binding.h
View file
Expand Up @@ -3,8 +3,14 @@

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#if defined(__POSIX__)
#include <dlfcn.h>
#endif

#include "node.h"
#define NAPI_EXPERIMENTAL
#include "node_api.h"
#include "util.h"
#include "uv.h"
#include "v8.h"

Expand Down Expand Up @@ -46,6 +52,32 @@ extern bool node_is_initialized;

namespace binding {

class DLib {
public:
#ifdef __POSIX__
static const int kDefaultFlags = RTLD_LAZY;
#else
static const int kDefaultFlags = 0;
#endif

DLib(const char* filename, int flags);

bool Open();
void Close();
void* GetSymbolAddress(const char* name);

const std::string filename_;
const int flags_;
std::string errmsg_;
void* handle_;
#ifndef __POSIX__
uv_lib_t lib_;
#endif

private:
DISALLOW_COPY_AND_ASSIGN(DLib);
};

// Call _register<module_name> functions for all of
// the built-in modules. Because built-in modules don't
// use the __attribute__((constructor)). Need to
Expand All @@ -60,5 +92,7 @@ void DLOpen(const v8::FunctionCallbackInfo<v8::Value>& args);

} // namespace node

#include "node_binding.h"

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
#endif // SRC_NODE_BINDING_H_
10 changes: 10 additions & 0 deletions test/abort/test-addon-uv-handle-leak.js
View file
Expand Up @@ -18,6 +18,16 @@ if (!fs.existsSync(bindingPath))
common.skip('binding not built yet');

if (process.argv[2] === 'child') {

// The worker thread loads and then unloads `bindingPath`. Because of this the
// symbols in `bindingPath` are lost when the worker thread quits, but the
// number of open handles in the worker thread's event loop is assessed in the
// main thread afterwards, and the names of the callbacks associated with the
// open handles is retrieved at that time as well. Thus, we require
// `bindingPath` here so that the symbols and their names survive the life
// cycle of the worker thread.
require(bindingPath);

new Worker(`
const binding = require(${JSON.stringify(bindingPath)});
Expand Down
16 changes: 14 additions & 2 deletions test/addons/at-exit/binding.cc
View file
Expand Up @@ -9,6 +9,19 @@ using v8::Isolate;
using v8::Local;
using v8::Object;

#if defined(_MSC_VER)
#pragma section(".CRT$XPU", read)
#define NODE_C_DTOR(fn) \
NODE_CTOR_PREFIX void __cdecl fn(void); \
__declspec(dllexport, allocate(".CRT$XPU")) \
void (__cdecl*fn ## _)(void) = fn; \
NODE_CTOR_PREFIX void __cdecl fn(void)
#else
#define NODE_C_DTOR(fn) \
NODE_CTOR_PREFIX void fn(void) __attribute__((destructor)); \
NODE_CTOR_PREFIX void fn(void)
#endif

static char cookie[] = "yum yum";
static int at_exit_cb1_called = 0;
static int at_exit_cb2_called = 0;
Expand All @@ -27,7 +40,7 @@ static void at_exit_cb2(void* arg) {
at_exit_cb2_called++;
}

static void sanity_check(void) {
NODE_C_DTOR(sanity_check) {
assert(at_exit_cb1_called == 1);
assert(at_exit_cb2_called == 2);
}
Expand All @@ -36,7 +49,6 @@ void init(Local<Object> exports) {
AtExit(at_exit_cb1, exports->GetIsolate());
AtExit(at_exit_cb2, cookie);
AtExit(at_exit_cb2, cookie);
atexit(sanity_check);
}

NODE_MODULE(NODE_GYP_MODULE_NAME, init)

0 comments on commit d718625

Please sign in to comment.