From 3ebfcdf0aec581faf4c011f66c12d3ed2c55df23 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Fri, 6 Nov 2020 11:45:23 +0000 Subject: [PATCH 1/4] Add test creating multiple plugins with load dependencies. If we load plugins purely alphabetically, the 1st Zeek run in the test will success while the 2nd will fail. --- .../plugins.plugin-load-dependency/output | 18 +++++++++++ .../btest/plugins/plugin-load-dependency.zeek | 31 +++++++++++++++++++ .../plugin-load-dependency/.btest-ignore | 0 .../plugin-load-dependency/1/src/Plugin.cc | 23 ++++++++++++++ .../plugin-load-dependency/1/src/Plugin.h | 17 ++++++++++ .../plugin-load-dependency/2/src/Plugin.cc | 21 +++++++++++++ .../plugin-load-dependency/2/src/Plugin.h | 17 ++++++++++ .../plugin-load-dependency/3/src/Plugin.cc | 23 ++++++++++++++ .../plugin-load-dependency/3/src/Plugin.h | 17 ++++++++++ 9 files changed, 167 insertions(+) create mode 100644 testing/btest/Baseline/plugins.plugin-load-dependency/output create mode 100644 testing/btest/plugins/plugin-load-dependency.zeek create mode 100644 testing/btest/plugins/plugin-load-dependency/.btest-ignore create mode 100644 testing/btest/plugins/plugin-load-dependency/1/src/Plugin.cc create mode 100644 testing/btest/plugins/plugin-load-dependency/1/src/Plugin.h create mode 100644 testing/btest/plugins/plugin-load-dependency/2/src/Plugin.cc create mode 100644 testing/btest/plugins/plugin-load-dependency/2/src/Plugin.h create mode 100644 testing/btest/plugins/plugin-load-dependency/3/src/Plugin.cc create mode 100644 testing/btest/plugins/plugin-load-dependency/3/src/Plugin.h diff --git a/testing/btest/Baseline/plugins.plugin-load-dependency/output b/testing/btest/Baseline/plugins.plugin-load-dependency/output new file mode 100644 index 0000000000..e788232bd8 --- /dev/null +++ b/testing/btest/Baseline/plugins.plugin-load-dependency/output @@ -0,0 +1,18 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +Testing::Plugin2 - Plugin2 provides a load dependency for Plugin1 and Plugin3 (dynamic, version 1.0.0) +Testing::Plugin3 - Plugin3 has a load dependency on Plugin2 (dynamic, version 1.0.0) +in Plugin2 +in Plugin3 + +Testing::Plugin1 - Plugin1 has a load dependency on Plugin2 (dynamic, version 1.0.0) +Testing::Plugin2 - Plugin2 provides a load dependency for Plugin1 and Plugin3 (dynamic, version 1.0.0) +in Plugin1 +in Plugin2 + +Testing::Plugin1 - Plugin1 has a load dependency on Plugin2 (dynamic, version 1.0.0) +Testing::Plugin2 - Plugin2 provides a load dependency for Plugin1 and Plugin3 (dynamic, version 1.0.0) +Testing::Plugin3 - Plugin3 has a load dependency on Plugin2 (dynamic, version 1.0.0) +in Plugin1 +in Plugin2 +in Plugin2 +in Plugin3 diff --git a/testing/btest/plugins/plugin-load-dependency.zeek b/testing/btest/plugins/plugin-load-dependency.zeek new file mode 100644 index 0000000000..d9d78d7ebb --- /dev/null +++ b/testing/btest/plugins/plugin-load-dependency.zeek @@ -0,0 +1,31 @@ +# @TEST-EXEC: mkdir 1 +# @TEST-EXEC: cd 1 && ${DIST}/auxil/zeek-aux/plugin-support/init-plugin -u . Testing Plugin1 +# @TEST-EXEC: cp -r %DIR/plugin-load-dependency/1 . +# @TEST-EXEC: cd 1 && ./configure --zeek-dist=${DIST} && make + +# @TEST-EXEC: mkdir 2 +# @TEST-EXEC: cd 2 && ${DIST}/auxil/zeek-aux/plugin-support/init-plugin -u . Testing Plugin2 +# @TEST-EXEC: cp -r %DIR/plugin-load-dependency/2 . +# @TEST-EXEC: cd 2 && ./configure --zeek-dist=${DIST} && make + +# @TEST-EXEC: mkdir 3 +# @TEST-EXEC: cd 3 && ${DIST}/auxil/zeek-aux/plugin-support/init-plugin -u . Testing Plugin3 +# @TEST-EXEC: cp -r %DIR/plugin-load-dependency/3 . +# @TEST-EXEC: cd 3 && ./configure --zeek-dist=${DIST} && make + +# The following run will only work if Zeek loads plugin2 before plugin3 (which +# by alphabetical loading will be the case) +# @TEST-EXEC: ZEEK_PLUGIN_PATH=. zeek -b -N Testing::Plugin3 Testing::Plugin2 | grep -v Zeek:: | sort >> output +# +# @TEST-EXEC: echo >>output +# +# The following run will only work if Zeek loads plugin2 before plugin1 (which +# by alphabetical loading will not be the case). +# @TEST-EXEC: ZEEK_PLUGIN_PATH=. zeek -b -N Testing::Plugin1 Testing::Plugin2 | grep -v Zeek:: | sort >> output +# +# @TEST-EXEC: echo >>output +# +# Finally, try it with self-discovery of all three plugins too. +# @TEST-EXEC: ZEEK_PLUGIN_PATH=. zeek -N | grep -v Zeek:: | sort >> output +# +# @TEST-EXEC: btest-diff output diff --git a/testing/btest/plugins/plugin-load-dependency/.btest-ignore b/testing/btest/plugins/plugin-load-dependency/.btest-ignore new file mode 100644 index 0000000000..e69de29bb2 diff --git a/testing/btest/plugins/plugin-load-dependency/1/src/Plugin.cc b/testing/btest/plugins/plugin-load-dependency/1/src/Plugin.cc new file mode 100644 index 0000000000..76501c4bc9 --- /dev/null +++ b/testing/btest/plugins/plugin-load-dependency/1/src/Plugin.cc @@ -0,0 +1,23 @@ + +#include "Plugin.h" + +namespace btest::plugin::Testing_Plugin1 { Plugin plugin; } + +using namespace btest::plugin::Testing_Plugin1; + +extern void Plugin2_foo(); + +zeek::plugin::Configuration Plugin::Configure() + { + zeek::plugin::Configuration config; + config.name = "Testing::Plugin1"; + config.description = "Plugin1 has a load dependency on Plugin2"; + config.version.major = 1; + config.version.minor = 0; + config.version.patch = 0; + + printf("in Plugin1\n"); + Plugin2_foo(); + + return config; + } diff --git a/testing/btest/plugins/plugin-load-dependency/1/src/Plugin.h b/testing/btest/plugins/plugin-load-dependency/1/src/Plugin.h new file mode 100644 index 0000000000..18ccb8d319 --- /dev/null +++ b/testing/btest/plugins/plugin-load-dependency/1/src/Plugin.h @@ -0,0 +1,17 @@ + +#pragma once + +#include + +namespace btest::plugin::Testing_Plugin1 { + +class Plugin : public zeek::plugin::Plugin +{ +protected: + // Overridden from zeek::plugin::Plugin. + zeek::plugin::Configuration Configure() override; +}; + +extern Plugin plugin; + +} diff --git a/testing/btest/plugins/plugin-load-dependency/2/src/Plugin.cc b/testing/btest/plugins/plugin-load-dependency/2/src/Plugin.cc new file mode 100644 index 0000000000..fd6a28155e --- /dev/null +++ b/testing/btest/plugins/plugin-load-dependency/2/src/Plugin.cc @@ -0,0 +1,21 @@ + +#include "Plugin.h" + +namespace btest::plugin::Testing_Plugin2 { Plugin plugin; } + +using namespace btest::plugin::Testing_Plugin2; + +void Plugin2_foo() { + printf("in Plugin2\n"); +} + +zeek::plugin::Configuration Plugin::Configure() + { + zeek::plugin::Configuration config; + config.name = "Testing::Plugin2"; + config.description = "Plugin2 provides a load dependency for Plugin1 and Plugin3"; + config.version.major = 1; + config.version.minor = 0; + config.version.patch = 0; + return config; + } diff --git a/testing/btest/plugins/plugin-load-dependency/2/src/Plugin.h b/testing/btest/plugins/plugin-load-dependency/2/src/Plugin.h new file mode 100644 index 0000000000..8e9c69aecb --- /dev/null +++ b/testing/btest/plugins/plugin-load-dependency/2/src/Plugin.h @@ -0,0 +1,17 @@ + +#pragma once + +#include + +namespace btest::plugin::Testing_Plugin2 { + +class Plugin : public zeek::plugin::Plugin +{ +protected: + // Overridden from zeek::plugin::Plugin. + zeek::plugin::Configuration Configure() override; +}; + +extern Plugin plugin; + +} diff --git a/testing/btest/plugins/plugin-load-dependency/3/src/Plugin.cc b/testing/btest/plugins/plugin-load-dependency/3/src/Plugin.cc new file mode 100644 index 0000000000..68d878ad55 --- /dev/null +++ b/testing/btest/plugins/plugin-load-dependency/3/src/Plugin.cc @@ -0,0 +1,23 @@ + +#include "Plugin.h" + +namespace btest::plugin::Testing_Plugin3 { Plugin plugin; } + +using namespace btest::plugin::Testing_Plugin3; + +extern void Plugin2_foo(); + +zeek::plugin::Configuration Plugin::Configure() + { + zeek::plugin::Configuration config; + config.name = "Testing::Plugin3"; + config.description = "Plugin3 has a load dependency on Plugin2"; + config.version.major = 1; + config.version.minor = 0; + config.version.patch = 0; + + printf("in Plugin3\n"); + Plugin2_foo(); + + return config; + } diff --git a/testing/btest/plugins/plugin-load-dependency/3/src/Plugin.h b/testing/btest/plugins/plugin-load-dependency/3/src/Plugin.h new file mode 100644 index 0000000000..b6b692f877 --- /dev/null +++ b/testing/btest/plugins/plugin-load-dependency/3/src/Plugin.h @@ -0,0 +1,17 @@ + +#pragma once + +#include + +namespace btest::plugin::Testing_Plugin3 { + +class Plugin : public zeek::plugin::Plugin +{ +protected: + // Overridden from zeek::plugin::Plugin. + zeek::plugin::Configuration Configure() override; +}; + +extern Plugin plugin; + +} From df40e82fd63f47c6032031fe144f865e5985f02a Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Fri, 6 Nov 2020 11:58:41 +0000 Subject: [PATCH 2/4] When attempting to activate a plugin, load dynamic libraries first. Just moving code. This is so that we can abort if dlopen() fails without having changed any other state yet. --- src/plugin/Manager.cc | 104 +++++++++++++++++++++--------------------- 1 file changed, 52 insertions(+), 52 deletions(-) diff --git a/src/plugin/Manager.cc b/src/plugin/Manager.cc index 6f20c7b516..8598e3fe3d 100644 --- a/src/plugin/Manager.cc +++ b/src/plugin/Manager.cc @@ -175,58 +175,6 @@ bool Manager::ActivateDynamicPluginInternal(const std::string& name, bool ok_if_ DBG_LOG(DBG_PLUGINS, "Activating plugin %s", name.c_str()); - // Add the "scripts" and "bif" directories to ZEEKPATH. - std::string scripts = dir + "scripts"; - - if ( util::is_dir(scripts) ) - { - DBG_LOG(DBG_PLUGINS, " Adding %s to ZEEKPATH", scripts.c_str()); - util::detail::add_to_zeek_path(scripts); - } - - string init; - - // First load {scripts}/__preload__.zeek automatically. - for (const string& ext : util::detail::script_extensions) - { - init = dir + "scripts/__preload__" + ext; - - if ( util::is_file(init) ) - { - DBG_LOG(DBG_PLUGINS, " Loading %s", init.c_str()); - util::detail::warn_if_legacy_script(init); - scripts_to_load.push_back(init); - break; - } - } - - // Load {bif,scripts}/__load__.zeek automatically. - for (const string& ext : util::detail::script_extensions) - { - init = dir + "lib/bif/__load__" + ext; - - if ( util::is_file(init) ) - { - DBG_LOG(DBG_PLUGINS, " Loading %s", init.c_str()); - util::detail::warn_if_legacy_script(init); - scripts_to_load.push_back(init); - break; - } - } - - for (const string& ext : util::detail::script_extensions) - { - init = dir + "scripts/__load__" + ext; - - if ( util::is_file(init) ) - { - DBG_LOG(DBG_PLUGINS, " Loading %s", init.c_str()); - util::detail::warn_if_legacy_script(init); - scripts_to_load.push_back(init); - break; - } - } - // Load shared libraries. string dypattern = dir + "/lib/*." + HOST_ARCHITECTURE + DYNAMIC_PLUGIN_SUFFIX; @@ -288,6 +236,58 @@ bool Manager::ActivateDynamicPluginInternal(const std::string& name, bool ok_if_ DBG_LOG(DBG_PLUGINS, " No shared library found"); } + // Add the "scripts" and "bif" directories to ZEEKPATH. + std::string scripts = dir + "scripts"; + + if ( util::is_dir(scripts) ) + { + DBG_LOG(DBG_PLUGINS, " Adding %s to ZEEKPATH", scripts.c_str()); + util::detail::add_to_zeek_path(scripts); + } + + string init; + + // First load {scripts}/__preload__.zeek automatically. + for (const string& ext : util::detail::script_extensions) + { + init = dir + "scripts/__preload__" + ext; + + if ( util::is_file(init) ) + { + DBG_LOG(DBG_PLUGINS, " Loading %s", init.c_str()); + util::detail::warn_if_legacy_script(init); + scripts_to_load.push_back(init); + break; + } + } + + // Load {bif,scripts}/__load__.zeek automatically. + for (const string& ext : util::detail::script_extensions) + { + init = dir + "lib/bif/__load__" + ext; + + if ( util::is_file(init) ) + { + DBG_LOG(DBG_PLUGINS, " Loading %s", init.c_str()); + util::detail::warn_if_legacy_script(init); + scripts_to_load.push_back(init); + break; + } + } + + for (const string& ext : util::detail::script_extensions) + { + init = dir + "scripts/__load__" + ext; + + if ( util::is_file(init) ) + { + DBG_LOG(DBG_PLUGINS, " Loading %s", init.c_str()); + util::detail::warn_if_legacy_script(init); + scripts_to_load.push_back(init); + break; + } + } + // Mark this plugin as activated by clearing the path. m->second.clear(); From b780bc146f88d7a829fa94dd7c7efe0036b4ad30 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Tue, 24 Nov 2020 17:00:20 +0000 Subject: [PATCH 3/4] Fix use of deprecated functionality in test. --- testing/btest/plugins/bifs-and-scripts.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/btest/plugins/bifs-and-scripts.sh b/testing/btest/plugins/bifs-and-scripts.sh index 911d279c11..345c1faa8f 100644 --- a/testing/btest/plugins/bifs-and-scripts.sh +++ b/testing/btest/plugins/bifs-and-scripts.sh @@ -51,7 +51,7 @@ EOF cat >src/foo.bif <("Hello from the plugin!"); %} event plugin_event%(foo: count%); From fe45f5335a782fb60281160f30f24b43377c57c0 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Tue, 24 Nov 2020 17:00:41 +0000 Subject: [PATCH 4/4] Retry loading plugins on failure to resolve to dependencies. Closes #1179. --- src/plugin/Manager.cc | 84 ++++++++++++++++++++++++++++++------------- src/plugin/Manager.h | 34 +++++++++--------- src/zeek-setup.cc | 13 ++----- 3 files changed, 79 insertions(+), 52 deletions(-) diff --git a/src/plugin/Manager.cc b/src/plugin/Manager.cc index 8598e3fe3d..614e100ca1 100644 --- a/src/plugin/Manager.cc +++ b/src/plugin/Manager.cc @@ -140,7 +140,7 @@ void Manager::SearchDynamicPlugins(const std::string& dir) closedir(d); } -bool Manager::ActivateDynamicPluginInternal(const std::string& name, bool ok_if_not_found) +bool Manager::ActivateDynamicPluginInternal(const std::string& name, bool ok_if_not_found, std::vector* errors) { dynamic_plugin_map::iterator m = dynamic_plugins.find(util::strtolower(name)); @@ -160,7 +160,7 @@ bool Manager::ActivateDynamicPluginInternal(const std::string& name, bool ok_if_ return true; } - reporter->Error("plugin %s is not available", name.c_str()); + errors->push_back(util::fmt("plugin %s is not available", name.c_str())); return false; } @@ -192,16 +192,19 @@ bool Manager::ActivateDynamicPluginInternal(const std::string& name, bool ok_if_ current_plugin = nullptr; current_dir = dir.c_str(); current_sopath = path; - void* hdl = dlopen(path, RTLD_LAZY | RTLD_GLOBAL); + void* hdl = dlopen(path, RTLD_NOW | RTLD_GLOBAL); if ( ! hdl ) { const char* err = dlerror(); - reporter->FatalError("cannot load plugin library %s: %s", path, err ? err : ""); + errors->push_back(util::fmt("cannot load plugin library %s: %s", path, err ? err : "")); + return false; } - if ( ! current_plugin ) - reporter->FatalError("load plugin library %s did not instantiate a plugin", path); + if ( ! current_plugin ) { + errors->push_back(util::fmt("load plugin library %s did not instantiate a plugin", path)); + return false; + } current_plugin->SetDynamic(true); current_plugin->DoConfigure(); @@ -217,9 +220,11 @@ bool Manager::ActivateDynamicPluginInternal(const std::string& name, bool ok_if_ // Make sure the name the plugin reports is consistent with // what we expect from its magic file. - if ( util::strtolower(current_plugin->Name()) != util::strtolower(name) ) - reporter->FatalError("inconsistent plugin name: %s vs %s", - current_plugin->Name().c_str(), name.c_str()); + if ( util::strtolower(current_plugin->Name()) != util::strtolower(name) ) { + errors->push_back(util::fmt("inconsistent plugin name: %s vs %s", + current_plugin->Name().c_str(), name.c_str())); + return false; + } current_dir = nullptr; current_sopath = nullptr; @@ -294,37 +299,66 @@ bool Manager::ActivateDynamicPluginInternal(const std::string& name, bool ok_if_ return true; } -bool Manager::ActivateDynamicPlugin(const std::string& name) +void Manager::ActivateDynamicPlugin(const std::string& name) { - if ( ! ActivateDynamicPluginInternal(name) ) - return false; - - UpdateInputFiles(); - return true; + std::vector errors; + if ( ActivateDynamicPluginInternal(name, false, &errors) ) + UpdateInputFiles(); + else + // Reschedule for another attempt later. + requested_plugins.insert(std::move(name)); } -bool Manager::ActivateDynamicPlugins(bool all) - { +void Manager::ActivateDynamicPlugins(bool all) { + // Tracks plugins we need to activate as pairs of their names and booleans + // indicating whether an activation failure is to be deemed a fatal error. + std::set> plugins_to_activate; + + // Activate plugins that were specifically requested. + for ( const auto& x : requested_plugins ) + plugins_to_activate.emplace(x, false); + // Activate plugins that our environment tells us to. vector p; util::tokenize_string(util::zeek_plugin_activate(), ",", &p); - for ( size_t n = 0; n < p.size(); ++n ) - ActivateDynamicPluginInternal(p[n], true); + for ( const auto& x : p ) + plugins_to_activate.emplace(x, true); if ( all ) { - for ( dynamic_plugin_map::const_iterator i = dynamic_plugins.begin(); - i != dynamic_plugins.end(); i++ ) + // Activate all other ones we discovered. + for ( const auto& x : dynamic_plugins ) + plugins_to_activate.emplace(x.first, false); + } + + // Now we keep iterating over all the plugins, trying to load them, for as + // long as we're successful for at least one further of them each round. + // Doing so ensures that we can resolve (non-cyclic) load dependencies + // independent of any particular order. + while ( ! plugins_to_activate.empty() ) { + std::vector errors; + auto plugins_left = plugins_to_activate; + + for ( const auto& x : plugins_to_activate ) { - if ( ! ActivateDynamicPluginInternal(i->first) ) - return false; + if ( ActivateDynamicPluginInternal(x.first, x.second, &errors) ) + plugins_left.erase(x); } + + if ( plugins_left.size() == plugins_to_activate.size() ) + { + // Could not load a single further plugin this round, that's fatal. + for ( const auto& msg : errors ) + reporter->Error("%s", msg.c_str()); + + reporter->FatalError("aborting after plugin errors"); + } + + plugins_to_activate = std::move(plugins_left); } UpdateInputFiles(); - - return true; } void Manager::UpdateInputFiles() diff --git a/src/plugin/Manager.h b/src/plugin/Manager.h index d34c5db07e..268a5b3341 100644 --- a/src/plugin/Manager.h +++ b/src/plugin/Manager.h @@ -2,9 +2,10 @@ #pragma once -#include #include +#include #include +#include #include "zeek/plugin/Plugin.h" #include "zeek/plugin/Component.h" @@ -79,28 +80,25 @@ public: * Activating a plugin involves loading its dynamic module, making its * bifs available, and adding its script paths to ZEEKPATH. * + * This attempts to activiate the plugin immediately. If that fails for + * some reason, we schedule it to be retried later with + * ActivateDynamicPlugins(). + * * @param name The name of the plugin, as found previously by - * SearchPlugin(). - * - * @return True if the plugin has been loaded successfully. - * + ยท* SearchPlugin(). */ - bool ActivateDynamicPlugin(const std::string& name); + void ActivateDynamicPlugin(const std::string& name); /** - * Activates plugins that SearchDynamicPlugins() has previously discovered. - * The effect is the same all calling \a ActivePlugin(name) for each plugin. + * Activates plugins that SearchDynamicPlugins() has previously discovered, + * including any that have failed to load in prior calls to + * ActivateDynamicPlugin(). Aborts if any plugins fails to activate. * * @param all If true, activates all plugins that are found. If false, * activates only those that should always be activated unconditionally, - * as specified via the ZEEK_PLUGIN_ACTIVATE enviroment variable. In other - * words, it's \c true in standard mode and \c false in bare mode. - * - * @return True if all plugins have been loaded successfully. If one - * fails to load, the method stops there without loading any further ones - * and returns false. + * as specified via the ZEEK_PLUGIN_ACTIVATE environment variable. In other */ - bool ActivateDynamicPlugins(bool all); + void ActivateDynamicPlugins(bool all); /** * First-stage initializion of the manager. This is called early on @@ -413,11 +411,15 @@ public: static void RegisterBifFile(const char* plugin, bif_init_func c); private: - bool ActivateDynamicPluginInternal(const std::string& name, bool ok_if_not_found = false); + bool ActivateDynamicPluginInternal(const std::string& name, bool ok_if_not_found, std::vector* errors); void UpdateInputFiles(); void MetaHookPre(HookType hook, const HookArgumentList& args) const; void MetaHookPost(HookType hook, const HookArgumentList& args, HookArgument result) const; + // Plugins that were explicitly requested to be activated, but failed to + // load at first. + std::set requested_plugins; + // All found dynamic plugins, mapping their names to base directory. using dynamic_plugin_map = std::map; dynamic_plugin_map dynamic_plugins; diff --git a/src/zeek-setup.cc b/src/zeek-setup.cc index 2c0aece6bf..42c0abe3cb 100644 --- a/src/zeek-setup.cc +++ b/src/zeek-setup.cc @@ -604,17 +604,8 @@ SetupResult setup(int argc, char** argv, Options* zopts) file_mgr->InitPreScript(); zeekygen_mgr->InitPreScript(); - bool missing_plugin = false; - - for ( set::const_iterator i = requested_plugins.begin(); - i != requested_plugins.end(); i++ ) - { - if ( ! plugin_mgr->ActivateDynamicPlugin(*i) ) - missing_plugin = true; - } - - if ( missing_plugin ) - reporter->FatalError("Failed to activate requested dynamic plugin(s)."); + for ( const auto& x : requested_plugins ) + plugin_mgr->ActivateDynamicPlugin(std::move(x)); plugin_mgr->ActivateDynamicPlugins(! options.bare_mode);