src: unload addons when environment quits

This is an alternative to #23319 which attaches the loaded addons to the environment and closes them when the environment is destroyed. PR-URL: #24861 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Author: Gabriel Schulhof

src/env-inl.h

@@ -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_;
 }

src/env.cc

@@ -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,

src/env.h

@@ -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;
 
@@ -637,6 +639,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();
@@ -923,6 +928,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_;

src/node_binding.cc

@@ -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
@@ -127,31 +123,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() {
@@ -248,87 +221,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]

src/node_binding.h

@@ -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"
 
@@ -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
@@ -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_

test/abort/test-addon-uv-handle-leak.js

@@ -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)});
 

test/addons/at-exit/binding.cc

@@ -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;
@@ -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);
 }
@@ -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)