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. Backport-PR-URL: #25258 PR-URL: #24861 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
nodejs · Dec 6, 2018 · d7186252df043bdb693b47f34898e39a32b2c70a · d718625
1 parent 95353c7
commit d7186252df043bdb693b47f34898e39a32b2c70a
diff --git a/src/env-inl.h b/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_;
 }

diff --git a/src/env.cc b/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,

diff --git a/src/env.h b/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;
@@ -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();
@@ -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_;

diff --git a/src/node_binding.cc b/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
@@ -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() {
@@ -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]

diff --git a/src/node_binding.h b/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_
diff --git a/test/abort/test-addon-uv-handle-leak.js b/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)});

diff --git a/test/addons/at-exit/binding.cc b/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)