From 7e9a2b00b1e987f74046fc02c4330777a976f475 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Thu, 25 Jun 2020 02:08:46 +0200 Subject: [PATCH] [#1282] Checkpoint: hooks library done - correct use to do --- src/lib/hooks/hooks_manager.cc | 53 ++++++- src/lib/hooks/hooks_manager.h | 65 ++++++-- src/lib/hooks/hooks_messages.cc | 6 +- src/lib/hooks/hooks_messages.h | 4 +- src/lib/hooks/hooks_messages.mes | 8 + src/lib/hooks/library_manager.cc | 82 +++++++---- src/lib/hooks/library_manager.h | 53 +++---- src/lib/hooks/library_manager_collection.cc | 22 ++- src/lib/hooks/library_manager_collection.h | 19 ++- src/lib/hooks/tests/hooks_manager_unittest.cc | 139 +++++++++++++++--- .../hooks/tests/library_manager_unittest.cc | 39 ++--- 11 files changed, 371 insertions(+), 119 deletions(-) diff --git a/src/lib/hooks/hooks_manager.cc b/src/lib/hooks/hooks_manager.cc index 2083637131..584a4fdf6f 100644 --- a/src/lib/hooks/hooks_manager.cc +++ b/src/lib/hooks/hooks_manager.cc @@ -15,6 +15,7 @@ #include #include +#include #include #include @@ -96,8 +97,19 @@ HooksManager::loadLibrariesInternal(const HookLibsCollection& libraries) { ServerHooks::getServerHooks().getParkingLotsPtr()->clear(); - // Create the library manager and load the libraries. + // Keep a weak pointer on the existing library manager collection. + boost::weak_ptr weak_lmc(lm_collection_); + + // Create the library manager collection. lm_collection_.reset(new LibraryManagerCollection(libraries)); + + // If there was another owner the previous library manager collection + // was not destroyed and libraries not closed. + if (!weak_lmc.expired()) { + isc_throw(LibrariesStillOpened, "some libraries are still opened"); + } + + // Load the libraries. bool status = lm_collection_->loadLibraries(); if (status) { @@ -121,14 +133,45 @@ HooksManager::loadLibraries(const HookLibsCollection& libraries) { // cause the libraries to be unloaded) and initializes them with empty list if // requested. -void +bool HooksManager::unloadLibrariesInternal() { ServerHooks::getServerHooks().getParkingLotsPtr()->clear(); - init(); + + // Keep a weak pointer on the existing library manager collection. + boost::weak_ptr weak_lmc(lm_collection_); + + // Create the collection with any empty set of libraries. + HookLibsCollection libraries; + lm_collection_.reset(new LibraryManagerCollection(libraries)); + + // If there was another owner the previous library manager collection + // was not destroyed and libraries not closed. + if (!weak_lmc.expired()) { + // Restore the library manager collection. + lm_collection_ = weak_lmc.lock(); + return (false); + } + + // Load the empty set of libraries. + lm_collection_->loadLibraries(); + + // Get the CalloutManager. + callout_manager_ = lm_collection_->getCalloutManager(); + + return (true); +} + +bool HooksManager::unloadLibraries() { + return (getHooksManager().unloadLibrariesInternal()); +} + +void +HooksManager::prepareUnloadLibrariesInternal() { + static_cast(lm_collection_->prepareUnloadLibraries()); } -void HooksManager::unloadLibraries() { - getHooksManager().unloadLibrariesInternal(); +void HooksManager::prepareUnloadLibraries() { + getHooksManager().prepareUnloadLibrariesInternal(); } // Create a callout handle diff --git a/src/lib/hooks/hooks_manager.h b/src/lib/hooks/hooks_manager.h index e99c8c9077..85b6dc755a 100644 --- a/src/lib/hooks/hooks_manager.h +++ b/src/lib/hooks/hooks_manager.h @@ -19,6 +19,18 @@ namespace isc { namespace hooks { +/// @brief Libraries still opened. +/// +/// Thrown if an attempt is made to load libraries when some are still +/// in memory likely because they were not unloaded (logic error in Kea) +/// or they have some visible dangling pointers (logic error in a hook +/// library). +class LibrariesStillOpened : public Exception { +public: + LibrariesStillOpened(const char* file, size_t line, const char* what) : + isc::Exception(file, line, what) {} +}; + // Forward declarations class CalloutHandle; class CalloutManager; @@ -45,8 +57,8 @@ public: /// hook points) are configured and the libraries' "load" function /// called. /// - /// If libraries are already loaded, they are unloaded and the new - /// libraries loaded. + /// @note this method now requires the libraries are unloaded before + /// being called. /// /// If any library fails to load, an error message will be logged. The /// remaining libraries will be loaded if possible. @@ -58,6 +70,7 @@ public: /// @return true if all libraries loaded without a problem, false if one or /// more libraries failed to load. In the latter case, message will /// be logged that give the reason. + /// @throw LibrariesStillOpened when some libraries are already loaded. static bool loadLibraries(const HookLibsCollection& libraries); /// @brief Unload libraries @@ -65,15 +78,37 @@ public: /// Unloads the loaded libraries and leaves the hooks subsystem in the /// state it was after construction but before loadLibraries() is called. /// - /// @note: This method should be used with caution - see the notes for - /// the class LibraryManager for pitfalls. In general, a server - /// should not call this method: library unloading will automatically - /// take place when new libraries are loaded, and when appropriate - /// objects are destroyed. - /// - /// @return true if all libraries unloaded successfully, false on an error. - /// In the latter case, an error message will have been output. - static void unloadLibraries(); + /// @note: This method should be called after @ref prepareUnloadLibraries + /// in order to destroy appropriate objects. See notes for + /// the class LibraryManager for pitfalls. + /// @note: if even after @ref prepareUnloadLibraries there are still + /// visible pointers (i.e. callout handles owning the + /// library manager collection) the method will fail to close + /// libraries and returns false. It is a fatal error as there + /// is no possible recovery. It is a logic error in the hook + /// code too so the solution is to fix the it and to restart + /// the server with a correct hook library binary. + /// + /// @return true if all libraries unloaded successfully, false if they + /// are still in memory. + static bool unloadLibraries(); + + /// @brief Prepare the unloading of libraries + /// + /// Calls the unload functions when they exists and removes callouts. + /// + /// @note: after the call to this method there should be no visible + /// dangling pointers (i.e. callout handles owning the library + /// manager collection) nor invisible dangling pointers. + /// In the first case it will be impossible to close libraries + /// so they will remain in memory, in the second case a crash + /// is possible in particular at exit time during global + /// object finalization. In both cases the hook library code + /// causing the problem is incorrect and must be fixed. + /// @note: it is a logic error to not call this method before + /// @ref unloadLibraries even it hurts only with particular + /// hooks libraries. + static void prepareUnloadLibraries(); /// @brief Are callouts present? /// @@ -367,7 +402,13 @@ private: bool loadLibrariesInternal(const HookLibsCollection& libraries); /// @brief Unload libraries - void unloadLibrariesInternal(); + /// + /// @return true if all libraries unloaded successfully, false on an error. + /// In the latter case, an error message will have been output. + bool unloadLibrariesInternal(); + + /// @brief Prepare the unloading of libraries + void prepareUnloadLibrariesInternal(); /// @brief Are callouts present? /// diff --git a/src/lib/hooks/hooks_messages.cc b/src/lib/hooks/hooks_messages.cc index a78c3642c1..7d2d0ee878 100644 --- a/src/lib/hooks/hooks_messages.cc +++ b/src/lib/hooks/hooks_messages.cc @@ -1,4 +1,4 @@ -// File created from ../../../src/lib/hooks/hooks_messages.mes on Mon Jun 22 2020 17:18 +// File created from ../../../src/lib/hooks/hooks_messages.mes on Wed Jun 24 2020 22:17 #include #include @@ -19,12 +19,14 @@ extern const isc::log::MessageID HOOKS_CALLOUT_REGISTRATION = "HOOKS_CALLOUT_REG extern const isc::log::MessageID HOOKS_CLOSE_ERROR = "HOOKS_CLOSE_ERROR"; extern const isc::log::MessageID HOOKS_HOOK_LIST_RESET = "HOOKS_HOOK_LIST_RESET"; extern const isc::log::MessageID HOOKS_INCORRECT_VERSION = "HOOKS_INCORRECT_VERSION"; +extern const isc::log::MessageID HOOKS_LIBRARY_CLOSED = "HOOKS_LIBRARY_CLOSED"; extern const isc::log::MessageID HOOKS_LIBRARY_LOADED = "HOOKS_LIBRARY_LOADED"; extern const isc::log::MessageID HOOKS_LIBRARY_LOADING = "HOOKS_LIBRARY_LOADING"; extern const isc::log::MessageID HOOKS_LIBRARY_MULTI_THREADING_COMPATIBLE = "HOOKS_LIBRARY_MULTI_THREADING_COMPATIBLE"; extern const isc::log::MessageID HOOKS_LIBRARY_MULTI_THREADING_NOT_COMPATIBLE = "HOOKS_LIBRARY_MULTI_THREADING_NOT_COMPATIBLE"; extern const isc::log::MessageID HOOKS_LIBRARY_UNLOADED = "HOOKS_LIBRARY_UNLOADED"; extern const isc::log::MessageID HOOKS_LIBRARY_UNLOADING = "HOOKS_LIBRARY_UNLOADING"; +extern const isc::log::MessageID HOOKS_LIBRARY_UNLOAD_ALREADY_CALLED = "HOOKS_LIBRARY_UNLOAD_ALREADY_CALLED"; extern const isc::log::MessageID HOOKS_LIBRARY_VERSION = "HOOKS_LIBRARY_VERSION"; extern const isc::log::MessageID HOOKS_LOAD_ERROR = "HOOKS_LOAD_ERROR"; extern const isc::log::MessageID HOOKS_LOAD_EXCEPTION = "HOOKS_LOAD_EXCEPTION"; @@ -60,12 +62,14 @@ const char* values[] = { "HOOKS_CLOSE_ERROR", "failed to close hook library %1: %2", "HOOKS_HOOK_LIST_RESET", "the list of hooks has been reset", "HOOKS_INCORRECT_VERSION", "hook library %1 is at version %2, require version %3", + "HOOKS_LIBRARY_CLOSED", "hooks library %1 successfully closed", "HOOKS_LIBRARY_LOADED", "hooks library %1 successfully loaded", "HOOKS_LIBRARY_LOADING", "loading hooks library %1", "HOOKS_LIBRARY_MULTI_THREADING_COMPATIBLE", "hooks library %1 reports its multi-threading compatibility as %2", "HOOKS_LIBRARY_MULTI_THREADING_NOT_COMPATIBLE", "hooks library %1 is not compatible with multi-threading", "HOOKS_LIBRARY_UNLOADED", "hooks library %1 successfully unloaded", "HOOKS_LIBRARY_UNLOADING", "unloading library %1", + "HOOKS_LIBRARY_UNLOAD_ALREADY_CALLED", "hooks library %1 unload() was already called", "HOOKS_LIBRARY_VERSION", "hooks library %1 reports its version as %2", "HOOKS_LOAD_ERROR", "'load' function in hook library %1 returned error %2", "HOOKS_LOAD_EXCEPTION", "'load' function in hook library %1 threw an exception", diff --git a/src/lib/hooks/hooks_messages.h b/src/lib/hooks/hooks_messages.h index 77024f41ab..a1b0ac3215 100644 --- a/src/lib/hooks/hooks_messages.h +++ b/src/lib/hooks/hooks_messages.h @@ -1,4 +1,4 @@ -// File created from ../../../src/lib/hooks/hooks_messages.mes on Mon Jun 22 2020 17:18 +// File created from ../../../src/lib/hooks/hooks_messages.mes on Wed Jun 24 2020 22:17 #ifndef HOOKS_MESSAGES_H #define HOOKS_MESSAGES_H @@ -20,12 +20,14 @@ extern const isc::log::MessageID HOOKS_CALLOUT_REGISTRATION; extern const isc::log::MessageID HOOKS_CLOSE_ERROR; extern const isc::log::MessageID HOOKS_HOOK_LIST_RESET; extern const isc::log::MessageID HOOKS_INCORRECT_VERSION; +extern const isc::log::MessageID HOOKS_LIBRARY_CLOSED; extern const isc::log::MessageID HOOKS_LIBRARY_LOADED; extern const isc::log::MessageID HOOKS_LIBRARY_LOADING; extern const isc::log::MessageID HOOKS_LIBRARY_MULTI_THREADING_COMPATIBLE; extern const isc::log::MessageID HOOKS_LIBRARY_MULTI_THREADING_NOT_COMPATIBLE; extern const isc::log::MessageID HOOKS_LIBRARY_UNLOADED; extern const isc::log::MessageID HOOKS_LIBRARY_UNLOADING; +extern const isc::log::MessageID HOOKS_LIBRARY_UNLOAD_ALREADY_CALLED; extern const isc::log::MessageID HOOKS_LIBRARY_VERSION; extern const isc::log::MessageID HOOKS_LOAD_ERROR; extern const isc::log::MessageID HOOKS_LOAD_EXCEPTION; diff --git a/src/lib/hooks/hooks_messages.mes b/src/lib/hooks/hooks_messages.mes index 36bf6d2f87..061d457ade 100644 --- a/src/lib/hooks/hooks_messages.mes +++ b/src/lib/hooks/hooks_messages.mes @@ -79,6 +79,10 @@ This is most likely due to the installation of a new version of Kea without rebuilding the hook library. A rebuild and re-install of the library should fix the problem in most cases. +% HOOKS_LIBRARY_CLOSED hooks library %1 successfully closed +This information message is issued when a user-supplied hooks library +has been successfully closed. + % HOOKS_LIBRARY_LOADED hooks library %1 successfully loaded This information message is issued when a user-supplied hooks library has been successfully loaded. @@ -103,6 +107,10 @@ from the configuration or the multi-threading disabled. This information message is issued when a user-supplied hooks library has been successfully unloaded. +% HOOKS_LIBRARY_UNLOAD_ALREADY_CALLED hooks library %1 unload() was already called +This warning message is issued when the unload() function of a +user-supplied hooks library was already called. + % HOOKS_LIBRARY_UNLOADING unloading library %1 This is a debug message called when the specified library is being unloaded. If all is successful, it will be followed by the diff --git a/src/lib/hooks/library_manager.cc b/src/lib/hooks/library_manager.cc index b18dcbf92e..cd5bc677df 100644 --- a/src/lib/hooks/library_manager.cc +++ b/src/lib/hooks/library_manager.cc @@ -55,15 +55,15 @@ LibraryManager::LibraryManager(const std::string& name) // Destructor. LibraryManager::~LibraryManager() { - if (manager_) { + if (index_ >= 0) { // LibraryManager instantiated to load a library, so ensure that // it is unloaded before exiting. - static_cast(unloadLibrary()); - } else { - // LibraryManager instantiated to validate a library, so just ensure - // that it is closed before exiting. - static_cast(closeLibrary()); + static_cast(prepareUnloadLibrary()); } + + // LibraryManager instantiated to validate a library, so just ensure + // that it is closed before exiting. + static_cast(closeLibrary()); } // Open the library @@ -95,6 +95,8 @@ LibraryManager::closeLibrary() { if (status != 0) { LOG_ERROR(hooks_logger, HOOKS_CLOSE_ERROR).arg(library_name_) .arg(dlerror()); + } else { + LOG_INFO(hooks_logger, HOOKS_LIBRARY_CLOSED).arg(library_name_); } } @@ -242,9 +244,22 @@ LibraryManager::runLoad() { // Run the "unload" function if present. bool -LibraryManager::runUnload() { +LibraryManager::prepareUnloadLibrary() { + + // Nothing to do. + if (dl_handle_ == NULL) { + return (true); + } + + // Call once. + if (index_ < 0) { + LOG_WARN(hooks_logger, HOOKS_LIBRARY_UNLOAD_ALREADY_CALLED) + .arg(library_name_); + return (false); + } // Get the pointer to the "load" function. + bool result = false; PointerConverter pc(dlsym(dl_handle_, UNLOAD_FUNCTION_NAME)); if (pc.unloadPtr() != NULL) { @@ -254,31 +269,48 @@ LibraryManager::runUnload() { int status = -1; try { status = (*pc.unloadPtr())(); + result = true; } catch (const isc::Exception& ex) { LOG_ERROR(hooks_logger, HOOKS_UNLOAD_FRAMEWORK_EXCEPTION) .arg(library_name_).arg(ex.what()); - return (false); } catch (...) { // Exception generated. Note a warning as the unload will occur // anyway. LOG_WARN(hooks_logger, HOOKS_UNLOAD_EXCEPTION).arg(library_name_); - return (false); } - if (status != 0) { - LOG_ERROR(hooks_logger, HOOKS_UNLOAD_ERROR).arg(library_name_) - .arg(status); - return (false); - } else { - LOG_DEBUG(hooks_logger, HOOKS_DBG_TRACE, HOOKS_UNLOAD_SUCCESS) - .arg(library_name_); + if (result) { + if (status != 0) { + LOG_ERROR(hooks_logger, HOOKS_UNLOAD_ERROR).arg(library_name_) + .arg(status); + result = false; + } else { + LOG_DEBUG(hooks_logger, HOOKS_DBG_TRACE, HOOKS_UNLOAD_SUCCESS) + .arg(library_name_); + } } } else { LOG_DEBUG(hooks_logger, HOOKS_DBG_TRACE, HOOKS_NO_UNLOAD) .arg(library_name_); + result = true; } - return (true); + // Regardless of status, remove all callouts associated with this + // library on all hooks. + vector hooks = ServerHooks::getServerHooks().getHookNames(); + manager_->setLibraryIndex(index_); + for (size_t i = 0; i < hooks.size(); ++i) { + bool removed = manager_->deregisterAllCallouts(hooks[i], index_); + if (removed) { + LOG_DEBUG(hooks_logger, HOOKS_DBG_CALLS, HOOKS_CALLOUTS_REMOVED) + .arg(hooks[i]).arg(library_name_); + } + } + + // Mark as unload() ran. + index_ = -1; + + return (result); } // The main library loading function. @@ -327,7 +359,7 @@ LibraryManager::loadLibrary() { // function (if present) in case "load" allocated resources that // need to be freed and (b) we need to remove any callouts that // have been installed. - static_cast(unloadLibrary()); + static_cast(prepareUnloadLibrary()); } } @@ -355,18 +387,8 @@ LibraryManager::unloadLibrary() { // Call the unload() function if present. Note that this is done first // - operations take place in the reverse order to which they were done // when the library was loaded. - result = runUnload(); - - // Regardless of status, remove all callouts associated with this - // library on all hooks. - vector hooks = ServerHooks::getServerHooks().getHookNames(); - manager_->setLibraryIndex(index_); - for (size_t i = 0; i < hooks.size(); ++i) { - bool removed = manager_->deregisterAllCallouts(hooks[i], index_); - if (removed) { - LOG_DEBUG(hooks_logger, HOOKS_DBG_CALLS, HOOKS_CALLOUTS_REMOVED) - .arg(hooks[i]).arg(library_name_); - } + if (index_ >= 0) { + result = prepareUnloadLibrary(); } // ... and close the library. diff --git a/src/lib/hooks/library_manager.h b/src/lib/hooks/library_manager.h index ef31333589..f3a376fa62 100644 --- a/src/lib/hooks/library_manager.h +++ b/src/lib/hooks/library_manager.h @@ -41,15 +41,16 @@ class LibraryManager; /// known hooks and locates their symbols, registering each callout as it does /// so. Finally it locates the "load" function (if present) and calls it. /// -/// On unload, it calls the "unload" method if present, clears the callouts on -/// all hooks, and closes the library. +/// On unload, it clears the callouts on all hooks, calls the "unload" +/// method if present, clears the callouts on all hooks, and +/// closes the library. /// -/// @note Caution needs to be exercised when using the unload method. During +/// @note Caution needs to be exercised when using the close method. During /// normal use, data will pass between the server and the library. In /// this process, the library may allocate memory and pass it back to the /// server. This could happen by the server setting arguments or context /// in the CalloutHandle object, or by the library modifying the content -/// of pointed-to data. If the library is unloaded, this memory may lie +/// of pointed-to data. If the library is closed, this memory may lie /// in the virtual address space deleted in that process. (The word "may" /// is used, as this could be operating-system specific.) Should this /// happen, any reference to the memory will cause a segmentation fault. @@ -57,7 +58,7 @@ class LibraryManager; /// a destructor of an STL class when it is deleting memory allocated /// when the data structure was extended by a function in the library. /// -/// @note The only safe way to run the "unload" function is to ensure that all +/// @note The only safe way to run the "close" function is to ensure that all /// possible references to it are removed first. This means that all /// CalloutHandles must be destroyed, as must any data items that were /// passed to the callouts. In practice, it could mean that a server @@ -92,7 +93,7 @@ public: /// If the library is open, closes it. This is principally a safety /// feature to ensure closure in the case of an exception destroying this /// object. However, see the caveat in the class header about when it is - /// safe to unload libraries. + /// safe to close libraries. ~LibraryManager(); /// @brief Validate library @@ -122,6 +123,27 @@ public: /// In the latter case, the library will be unloaded if possible. bool loadLibrary(); + /// @brief Prepares library unloading + /// + /// Searches for the "unload" framework function and, if present, runs it. + /// Regardless of status, remove all callouts associated with this + /// library on all hooks. + /// + /// @return bool true if not found or found and run successfully, + /// false on an error. In this case, an error message will + /// have been output. + bool prepareUnloadLibrary(); + + /// @brief Return library name + /// + /// @return Name of this library + std::string getName() const { + return (library_name_); + } + +protected: + // The following methods are protected as they are accessed in testing. + /// @brief Unloads a library /// /// Calls the libraries "unload" function if present, the closes the @@ -136,16 +158,6 @@ public: /// the library is closed if possible. bool unloadLibrary(); - /// @brief Return library name - /// - /// @return Name of this library - std::string getName() const { - return (library_name_); - } - -protected: - // The following methods are protected as they are accessed in testing. - /// @brief Open library /// /// Opens the library associated with this LibraryManager. A message is @@ -204,15 +216,6 @@ protected: /// have been output. bool runLoad(); - /// @brief Run the unload function if present - /// - /// Searches for the "unload" framework function and, if present, runs it. - /// - /// @return bool true if not found or found and run successfully, - /// false on an error. In this case, an error message will - /// have been output. - bool runUnload(); - private: /// @brief Validating constructor /// diff --git a/src/lib/hooks/library_manager_collection.cc b/src/lib/hooks/library_manager_collection.cc index edb401160e..5856b83cd0 100644 --- a/src/lib/hooks/library_manager_collection.cc +++ b/src/lib/hooks/library_manager_collection.cc @@ -52,8 +52,10 @@ LibraryManagerCollection::LibraryManagerCollection(const HookLibsCollection& lib bool LibraryManagerCollection::loadLibraries() { - // Unload libraries if any are loaded. - static_cast(unloadLibraries()); + // There must be not libraries still in memory. + if (!lib_managers_.empty()) { + isc_throw(LibrariesStillOpened, "some libraries are still opened"); + } // Access the callout manager, (re)creating it if required. // @@ -106,16 +108,26 @@ LibraryManagerCollection::unloadLibraries() { // Delete the library managers in the reverse order to which they were // created, then clear the library manager vector. - for (int i = lib_managers_.size() - 1; i >= 0; --i) { - lib_managers_[i].reset(); + while (!lib_managers_.empty()) { + lib_managers_.pop_back(); } - lib_managers_.clear(); // Get rid of the callout manager. (The other member, the list of library // names, was cleared when the libraries were loaded.) callout_manager_.reset(); } +// Prepare the unloading of libraries. +bool +LibraryManagerCollection::prepareUnloadLibraries() { + bool result = true; + // Iterate on library managers in reverse order. + for (auto lm = lib_managers_.rbegin(); lm != lib_managers_.rend(); ++lm) { + result = (*lm)->prepareUnloadLibrary() && result; + } + return (result); +} + // Return number of loaded libraries. int LibraryManagerCollection::getLoadedLibraryCount() const { diff --git a/src/lib/hooks/library_manager_collection.h b/src/lib/hooks/library_manager_collection.h index 0f6756d095..53a9669a49 100644 --- a/src/lib/hooks/library_manager_collection.h +++ b/src/lib/hooks/library_manager_collection.h @@ -42,14 +42,14 @@ class LibraryManager; /// /// As described in the LibraryManager documentation, a CalloutHandle may end /// up with pointers to memory within the address space of a loaded library. -/// If the library is unloaded before this address space is deleted, the +/// If the library is closed before this address space is deleted, the /// deletion of the CalloutHandle may attempt to free memory into the newly- /// unmapped address space and cause a segmentation fault. /// /// To prevent this, each CalloutHandle maintains a shared pointer to the /// LibraryManagerCollection current when it was created. In addition, the -/// containing HooksManager object also maintains a shared pointer to it. A -/// a LibraryManagerCollection is never explicitly deleted: when a new set +/// containing HooksManager object also maintains a shared pointer to it. +/// A LibraryManagerCollection is never explicitly deleted: when a new set /// of libraries is loaded, the HooksManager clears its pointer to the /// collection. The LibraryManagerCollection is only destroyed when all /// CallHandle objects referencing it are destroyed. @@ -144,6 +144,19 @@ public: static std::vector validateLibraries(const std::vector& libraries); + /// @brief Prepare libaries unloading + /// + /// Utility function to call before closing libraries. It runs the + /// unload() function when it exists and removes associated callout. + /// When this function returns either there is only one owner + /// (the hook manager) or some visible dangling pointers so + /// libraries are not closed to lower the probability of a crash. + /// See @ref LibraryManager::prepareUnloadLibrary. + /// + /// @return true if all libraries unload were not found or run + /// successfully, false on an error. + bool prepareUnloadLibraries(); + protected: /// @brief Unload libraries /// diff --git a/src/lib/hooks/tests/hooks_manager_unittest.cc b/src/lib/hooks/tests/hooks_manager_unittest.cc index 2890e067be..282f78916d 100644 --- a/src/lib/hooks/tests/hooks_manager_unittest.cc +++ b/src/lib/hooks/tests/hooks_manager_unittest.cc @@ -19,8 +19,11 @@ #include #include +#include #include +#include + using namespace isc; using namespace isc::hooks; using namespace isc::data; @@ -39,17 +42,48 @@ public: /// to be reset for each test. HooksManagerTest() { HooksManager::setTestMode(false); - HooksManager::unloadLibraries(); + HooksManager::prepareUnloadLibraries(); + bool status = HooksManager::unloadLibraries(); + if (!status) { + cerr << "(fixture ctor) unloadLibraries failed" << endl; + } + // Ensure the marker file is not present at the start of a test. + static_cast(remove(MARKER_FILE)); } /// @brief Destructor /// /// Unload all libraries. ~HooksManagerTest() { + static_cast(remove(MARKER_FILE)); HooksManager::setTestMode(false); - HooksManager::unloadLibraries(); + HooksManager::prepareUnloadLibraries(); + bool status = HooksManager::unloadLibraries(); + if (!status) { + cerr << "(fixture dtor) unloadLibraries failed" << endl; + } } + /// @brief Marker file present + /// + /// Convenience function to check whether a marker file is present. It + /// does this by opening the file. + /// + /// @return true if the marker file is present. + bool markerFilePresent() const { + + // Try to open it. + std::fstream marker; + marker.open(MARKER_FILE, std::fstream::in); + + // Check if it is open and close it if so. + bool exists = marker.is_open(); + if (exists) { + marker.close(); + } + + return (exists); + } /// @brief Call callouts test /// @@ -137,8 +171,6 @@ private: /// To avoid unused variable errors std::string dummy(int i) { if (i == 0) { - return (MARKER_FILE); - } else if (i > 0) { return (LOAD_CALLOUT_LIBRARY); } else { return (LOAD_ERROR_CALLOUT_LIBRARY); @@ -185,7 +217,10 @@ TEST_F(HooksManagerTest, LoadLibraries) { } // Try unloading the libraries. - EXPECT_NO_THROW(HooksManager::unloadLibraries()); + EXPECT_NO_THROW(HooksManager::prepareUnloadLibraries()); + bool status = false; + EXPECT_NO_THROW(status = HooksManager::unloadLibraries()); + EXPECT_TRUE(status); // Re-execute the calculation - callouts can be called but as nothing // happens, the result should always be -1. @@ -244,10 +279,14 @@ TEST_F(HooksManagerTest, CalloutHandleUnloadLibrary) { HooksManager::callCallouts(ServerHooks::CONTEXT_CREATE, *handle); // Unload the libraries. - HooksManager::unloadLibraries(); + HooksManager::prepareUnloadLibraries(); + EXPECT_FALSE(HooksManager::unloadLibraries()); // Deleting the callout handle should not cause a segmentation fault. handle.reset(); + + // And allows unload. + EXPECT_TRUE(HooksManager::unloadLibraries()); } // Test that we can load a new set of libraries while we have a CalloutHandle @@ -285,17 +324,20 @@ TEST_F(HooksManagerTest, CalloutHandleLoadLibrary) { data::ConstElementPtr())); // Load the libraries. + EXPECT_THROW(HooksManager::loadLibraries(new_library_names), + LibrariesStillOpened); + + // Deleting the old callout handle should not cause a segmentation fault. + handle.reset(); + + // But it allows the load of the new library. EXPECT_TRUE(HooksManager::loadLibraries(new_library_names)); - // Execute the calculation. Note that we still have the CalloutHandle - // for the old library: however, this should not affect the new calculation. + // Execute the calculation. { SCOPED_TRACE("Calculation with basic callout library loaded"); executeCallCallouts(10, 7, 17, 3, 51, 16, 35); } - - // Deleting the old callout handle should not cause a segmentation fault. - handle.reset(); } // This is effectively the same test as the LoadLibraries test. @@ -423,6 +465,9 @@ TEST_F(HooksManagerTest, PrePostCalloutTest) { handle->getArgument("result", result); EXPECT_EQ(2024, result); + // Reset the handle to allow a reload. + handle.reset(); + // ... and check that the pre- and post- callout functions don't survive a // reload. EXPECT_TRUE(HooksManager::loadLibraries(library_names)); @@ -469,6 +514,9 @@ TEST_F(HooksManagerTest, TestModeEnabledPrePostSurviveLoad) { handle->getArgument("result", result); EXPECT_EQ(2054, result); + // Reset the handle to allow a reload. + handle.reset(); + // ... and check that the pre- and post- callout functions survive a // reload. EXPECT_TRUE(HooksManager::loadLibraries(library_names)); @@ -516,6 +564,9 @@ TEST_F(HooksManagerTest, TestModeDisabledPrePostDoNotSurviveLoad) { handle->getArgument("result", result); EXPECT_EQ(2054, result); + // Reset the handle to allow a reload. + handle.reset(); + // ... and check that the pre- and post- callout functions don't survive a // reload. EXPECT_TRUE(HooksManager::loadLibraries(library_names)); @@ -560,6 +611,9 @@ TEST_F(HooksManagerTest, TestModeEnabledTooLatePrePostDoNotSurvive) { handle->getArgument("result", result); EXPECT_EQ(2054, result); + // Reset the handle to allow a reload. + handle.reset(); + // ... and check that the pre- and post- callout functions don't survive a // reload. EXPECT_TRUE(HooksManager::loadLibraries(library_names)); @@ -654,7 +708,10 @@ TEST_F(HooksManagerTest, LibraryNames) { EXPECT_TRUE(extractNames(library_names) == loaded_names); // Unload the libraries and check again. - EXPECT_NO_THROW(HooksManager::unloadLibraries()); + EXPECT_NO_THROW(HooksManager::prepareUnloadLibraries()); + bool status = false; + EXPECT_NO_THROW(status = HooksManager::unloadLibraries()); + EXPECT_TRUE(status); loaded_names = HooksManager::getLibraryNames(); EXPECT_TRUE(loaded_names.empty()); } @@ -732,6 +789,25 @@ TEST_F(HooksManagerTest, validateLibraries) { EXPECT_TRUE(failed == expected_failures); } +// This test verifies that unload is called by the prepare method. +TEST_F(HooksManagerTest, prepareUnload) { + + // Set up the list of libraries to be loaded and load them. + HookLibsCollection library_names; + library_names.push_back(make_pair(std::string(UNLOAD_CALLOUT_LIBRARY), + data::ConstElementPtr())); + EXPECT_TRUE(HooksManager::loadLibraries(library_names)); + + // Check that the marker file is not present. + EXPECT_FALSE(markerFilePresent()); + + // Prepare unload libraries runs unload functions. + HooksManager::prepareUnloadLibraries(); + + // Now the marker file is present. + EXPECT_TRUE(markerFilePresent()); +} + // This test verifies that the specified parameters are accessed properly. TEST_F(HooksManagerTest, LibraryParameters) { @@ -755,7 +831,10 @@ TEST_F(HooksManagerTest, LibraryParameters) { EXPECT_TRUE(HooksManager::loadLibraries(library_names)); // Try unloading the libraries. - EXPECT_NO_THROW(HooksManager::unloadLibraries()); + EXPECT_NO_THROW(HooksManager::prepareUnloadLibraries()); + bool status = false; + EXPECT_NO_THROW(status = HooksManager::unloadLibraries()); + EXPECT_TRUE(status); } // This test verifies that an object can be parked in two different @@ -798,7 +877,7 @@ TEST_F(HooksManagerTest, Parking) { ); // We have two callouts which should have returned pointers to the - // functions which we can call to siumulate completion of asynchronous + // functions which we can call to simulate completion of asynchronous // tasks. std::function unpark_trigger_func1; handle->getArgument("unpark_trigger1", unpark_trigger_func1); @@ -816,8 +895,14 @@ TEST_F(HooksManagerTest, Parking) { unpark_trigger_func2(); EXPECT_TRUE(unparked); + // Resetting the handle makes return from test body to crash. + // Try unloading the libraries. - EXPECT_NO_THROW(HooksManager::unloadLibraries()); + EXPECT_NO_THROW(HooksManager::prepareUnloadLibraries()); + bool status = false; + EXPECT_NO_THROW(status = HooksManager::unloadLibraries()); + // Handle is still active. + EXPECT_FALSE(status); } // This test verifies that the server can also unpark the packet. @@ -862,8 +947,14 @@ TEST_F(HooksManagerTest, ServerUnpark) { EXPECT_TRUE(unparked); + // Reset the handle to allow unload. + handle.reset(); + // Try unloading the libraries. - EXPECT_NO_THROW(HooksManager::unloadLibraries()); + EXPECT_NO_THROW(HooksManager::prepareUnloadLibraries()); + bool status = false; + EXPECT_NO_THROW(status = HooksManager::unloadLibraries()); + EXPECT_TRUE(status); } // This test verifies that the server can drop parked packet. @@ -908,8 +999,14 @@ TEST_F(HooksManagerTest, ServerDropParked) { // is not parked anymore. EXPECT_FALSE(HooksManager::unpark("hookpt_one", "foo")); + // Reset the handle to allow unload. + handle.reset(); + // Try unloading the libraries. - EXPECT_NO_THROW(HooksManager::unloadLibraries()); + EXPECT_NO_THROW(HooksManager::prepareUnloadLibraries()); + bool status = false; + EXPECT_NO_THROW(status = HooksManager::unloadLibraries()); + EXPECT_TRUE(status); } // This test verifies that parked objects are removed when libraries are @@ -947,8 +1044,14 @@ TEST_F(HooksManagerTest, UnloadBeforeUnpark) { unparked = true; }); + // Reset the handle to allow a reload. + handle.reset(); + // Try reloading the libraries. - EXPECT_NO_THROW(HooksManager::unloadLibraries()); + EXPECT_NO_THROW(HooksManager::prepareUnloadLibraries()); + bool status = false; + EXPECT_NO_THROW(status = HooksManager::unloadLibraries()); + EXPECT_TRUE(status); EXPECT_TRUE(HooksManager::loadLibraries(library_names)); // Parked object should have been removed. diff --git a/src/lib/hooks/tests/library_manager_unittest.cc b/src/lib/hooks/tests/library_manager_unittest.cc index 3d8a7b8a55..63eda0a0c1 100644 --- a/src/lib/hooks/tests/library_manager_unittest.cc +++ b/src/lib/hooks/tests/library_manager_unittest.cc @@ -141,13 +141,14 @@ public: {} /// Public methods that call protected methods on the superclass. + using LibraryManager::unloadLibrary; using LibraryManager::openLibrary; using LibraryManager::closeLibrary; using LibraryManager::checkVersion; using LibraryManager::checkMultiThreadingCompatible; using LibraryManager::registerStandardCallouts; using LibraryManager::runLoad; - using LibraryManager::runUnload; + using LibraryManager::prepareUnloadLibrary; }; @@ -337,7 +338,7 @@ TEST_F(LibraryManagerTest, RegisterStandardCallouts) { // Load the only library, specifying the index of 0 as it's the only // library. This should load all callouts. PublicLibraryManager lib_manager(std::string(BASIC_CALLOUT_LIBRARY), - 0, callout_manager_); + 0, callout_manager_); EXPECT_TRUE(lib_manager.openLibrary()); // Check the version of the library. @@ -455,7 +456,7 @@ TEST_F(LibraryManagerTest, CheckNoUnload) { EXPECT_TRUE(lib_manager.openLibrary()); // Check that no unload function returns true. - EXPECT_TRUE(lib_manager.runUnload()); + EXPECT_TRUE(lib_manager.prepareUnloadLibrary()); // Tidy up EXPECT_TRUE(lib_manager.closeLibrary()); @@ -472,7 +473,7 @@ TEST_F(LibraryManagerTest, CheckUnloadError) { EXPECT_TRUE(lib_manager.openLibrary()); // Check that unload function returning an error returns false. - EXPECT_FALSE(lib_manager.runUnload()); + EXPECT_FALSE(lib_manager.prepareUnloadLibrary()); // Tidy up EXPECT_TRUE(lib_manager.closeLibrary()); @@ -489,7 +490,7 @@ TEST_F(LibraryManagerTest, CheckUnloadException) { EXPECT_TRUE(lib_manager.openLibrary()); // Check that we detect that the unload function throws an exception. - EXPECT_FALSE(lib_manager.runUnload()); + EXPECT_FALSE(lib_manager.prepareUnloadLibrary()); // Tidy up EXPECT_TRUE(lib_manager.closeLibrary()); @@ -512,7 +513,7 @@ TEST_F(LibraryManagerTest, CheckUnload) { EXPECT_FALSE(markerFilePresent()); // Check that unload function runs and returns a success - EXPECT_TRUE(lib_manager.runUnload()); + EXPECT_TRUE(lib_manager.prepareUnloadLibrary()); // Check that the marker file was created. EXPECT_TRUE(markerFilePresent()); @@ -531,7 +532,7 @@ TEST_F(LibraryManagerTest, LibUnload) { // Load the only library, specifying the index of 0 as it's the only // library. This should load all callouts. PublicLibraryManager lib_manager(std::string(LOAD_CALLOUT_LIBRARY), - 0, callout_manager_); + 0, callout_manager_); EXPECT_TRUE(lib_manager.openLibrary()); // Check the version of the library. @@ -603,8 +604,8 @@ TEST_F(LibraryManagerTest, LoadLibraryWrongVersion) { // Check that the full loadLibrary call works. TEST_F(LibraryManagerTest, LoadLibrary) { - LibraryManager lib_manager(std::string(FULL_CALLOUT_LIBRARY), 0, - callout_manager_); + PublicLibraryManager lib_manager(std::string(FULL_CALLOUT_LIBRARY), 0, + callout_manager_); EXPECT_TRUE(lib_manager.loadLibrary()); // Now execute the callouts in the order expected. The library performs @@ -631,28 +632,28 @@ TEST_F(LibraryManagerTest, LoadLibrary) { TEST_F(LibraryManagerTest, LoadMultipleLibraries) { // Load a library with all framework functions. - LibraryManager lib_manager_1(std::string(FULL_CALLOUT_LIBRARY), 0, - callout_manager_); + PublicLibraryManager lib_manager_1(std::string(FULL_CALLOUT_LIBRARY), + 0, callout_manager_); EXPECT_TRUE(lib_manager_1.loadLibrary()); // Attempt to load a library with no version() function. We should detect // this and not end up calling the function from the already loaded // library. - LibraryManager lib_manager_2(std::string(NO_VERSION_LIBRARY), 1, - callout_manager_); + PublicLibraryManager lib_manager_2(std::string(NO_VERSION_LIBRARY), + 1, callout_manager_); EXPECT_FALSE(lib_manager_2.loadLibrary()); // Attempt to load the library with an incorrect version. This should // be detected. - LibraryManager lib_manager_3(std::string(INCORRECT_VERSION_LIBRARY), 1, - callout_manager_); + PublicLibraryManager lib_manager_3(std::string(INCORRECT_VERSION_LIBRARY), + 1, callout_manager_); EXPECT_FALSE(lib_manager_3.loadLibrary()); // Load the basic callout library. This only has standard callouts so, // if the first library's load() function gets called, some callouts // will be registered twice and lead to incorrect results. - LibraryManager lib_manager_4(std::string(BASIC_CALLOUT_LIBRARY), 1, - callout_manager_); + PublicLibraryManager lib_manager_4(std::string(BASIC_CALLOUT_LIBRARY), + 1, callout_manager_); EXPECT_TRUE(lib_manager_4.loadLibrary()); // Execute the callouts. The first library implements the calculation. @@ -712,8 +713,8 @@ TEST_F(LibraryManagerTest, validateLibraries) { TEST_F(LibraryManagerTest, libraryLoggerSetup) { // Load a library with all framework functions. - LibraryManager lib_manager(std::string(BASIC_CALLOUT_LIBRARY), 0, - callout_manager_); + PublicLibraryManager lib_manager(std::string(BASIC_CALLOUT_LIBRARY), + 0, callout_manager_); EXPECT_TRUE(lib_manager.loadLibrary()); // After loading the library, the global logging dictionary should -- 2.47.2