From a7343ee019174cd12b5fb081207b5acdbe7d98ac Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Thu, 15 Jul 2021 09:42:34 +0200 Subject: [PATCH] Fix registration of protocol analyzers from inside plugins. With the recent packet manager work, it broke to register a protocol analyzer for a specific port from inside a plugin's initialization code. That's because that registration now depends on the packet manager being set up, which isn't case at that time a plugin's `InitPostInit()` runs. This fix contains two parts: - Initialize the packet manager before the analyzer manager, so that the latter's `InitPostScript()` can rely on the former being ready. - Change the analyzer manager to (only) record port registrations happening before it's fully initialized. Its `InitPostScript()` then performs the actual registrations, knowing it can use the packet manager now. This comes with a `cmake/` to add a missing include directory. --- cmake | 2 +- src/analyzer/Manager.cc | 28 ++++++++++++++++++ src/analyzer/Manager.h | 7 +++++ src/zeek-setup.cc | 12 ++++---- .../btest/Baseline/plugins.protocol/output | 2 ++ testing/btest/Traces/port4243.trace | Bin 0 -> 868 bytes .../plugins/protocol-plugin/src/Plugin.cc | 11 +++++++ .../plugins/protocol-plugin/src/Plugin.h | 2 ++ testing/btest/plugins/protocol.zeek | 2 ++ 9 files changed, 59 insertions(+), 7 deletions(-) create mode 100644 testing/btest/Traces/port4243.trace diff --git a/cmake b/cmake index 9d762b4cac..4d1990f0e4 160000 --- a/cmake +++ b/cmake @@ -1 +1 @@ -Subproject commit 9d762b4cacf299a2e54e0f7f258868ee217f1d36 +Subproject commit 4d1990f0e4c273cf51ec52278add6ff256f9c889 diff --git a/src/analyzer/Manager.cc b/src/analyzer/Manager.cc index 5bfd77f539..c23d329d9f 100644 --- a/src/analyzer/Manager.cc +++ b/src/analyzer/Manager.cc @@ -86,6 +86,15 @@ void Manager::InitPostScript() for ( auto i = 0; i < port_list->Length(); ++i ) vxlan_ports.emplace_back(port_list->Idx(i)->AsPortVal()->Port()); + + for ( const auto& p : pending_analyzers_for_ports ) { + if ( ! RegisterAnalyzerForPort(p) ) + reporter->Warning("cannot register analyzer for port %u", std::get<2>(p)); + } + + pending_analyzers_for_ports.clear(); + + initialized = true; } void Manager::DumpDebug() @@ -231,6 +240,22 @@ bool Manager::UnregisterAnalyzerForPort(EnumVal* val, PortVal* port) bool Manager::RegisterAnalyzerForPort(const Tag& tag, TransportProto proto, uint32_t port) { + if ( initialized ) + return RegisterAnalyzerForPort(std::make_tuple(tag, proto, port)); + else + { + // Cannot register these before PostScriptInit() has run because we + // depend on packet analyis having been set up. That also means we don't have + // a reliable return value, for now we just assume it's working. + pending_analyzers_for_ports.emplace(tag, proto, port); + return true; + } + } + +bool Manager::RegisterAnalyzerForPort(const std::tuple& p) + { + const auto& [tag, proto, port] = p; + // TODO: this class is becoming more generic and removing a lot of the // checks for protocols, but this part might need to stay like this. packet_analysis::AnalyzerPtr analyzer; @@ -249,6 +274,9 @@ bool Manager::RegisterAnalyzerForPort(const Tag& tag, TransportProto proto, uint bool Manager::UnregisterAnalyzerForPort(const Tag& tag, TransportProto proto, uint32_t port) { + if ( auto i = pending_analyzers_for_ports.find(std::make_tuple(tag, proto, port)); i != pending_analyzers_for_ports.end() ) + pending_analyzers_for_ports.erase(i); + // TODO: this class is becoming more generic and removing a lot of the // checks for protocols, but this part might need to stay like this. packet_analysis::AnalyzerPtr analyzer; diff --git a/src/analyzer/Manager.h b/src/analyzer/Manager.h index 7b1ac05be4..17a0075a2d 100644 --- a/src/analyzer/Manager.h +++ b/src/analyzer/Manager.h @@ -335,6 +335,8 @@ public: { return vxlan_ports; } private: + // Internal version that must be used only once InitPostScript has completed. + bool RegisterAnalyzerForPort(const std::tuple& p); friend class packet_analysis::IP::IPBasedAnalyzer; @@ -372,11 +374,16 @@ private: }; }; + + using protocol_analyzers = std::set>; using conns_map = std::multimap; using conns_queue = std::priority_queue, ScheduledAnalyzer::Comparator>; + bool initialized = false; + protocol_analyzers pending_analyzers_for_ports; + conns_map conns; conns_queue conns_by_timeout; std::vector vxlan_ports; diff --git a/src/zeek-setup.cc b/src/zeek-setup.cc index 838bf86ef8..2f7ca0d014 100644 --- a/src/zeek-setup.cc +++ b/src/zeek-setup.cc @@ -87,8 +87,8 @@ int perftools_profile = 0; #endif zeek::ValManager* zeek::val_mgr = nullptr; -zeek::analyzer::Manager* zeek::analyzer_mgr = nullptr; zeek::packet_analysis::Manager* zeek::packet_mgr = nullptr; +zeek::analyzer::Manager* zeek::analyzer_mgr = nullptr; zeek::plugin::Manager* zeek::plugin_mgr = nullptr; zeek::detail::RuleMatcher* zeek::detail::rule_matcher = nullptr; @@ -253,8 +253,8 @@ static void done_with_network() run_state::terminating = true; - analyzer_mgr->Done(); packet_mgr->Done(); + analyzer_mgr->Done(); timer_mgr->Expire(); dns_mgr->Flush(); event_mgr.Drain(); @@ -324,8 +324,8 @@ static void terminate_bro() plugin_mgr->FinishPlugins(); delete zeekygen_mgr; - delete analyzer_mgr; delete packet_mgr; + delete analyzer_mgr; delete file_mgr; // broker_mgr, timer_mgr, and supervisor are deleted via iosource_mgr delete iosource_mgr; @@ -577,8 +577,8 @@ SetupResult setup(int argc, char** argv, Options* zopts) iosource_mgr = new iosource::Manager(); event_registry = new EventRegistry(); - analyzer_mgr = new analyzer::Manager(); packet_mgr = new packet_analysis::Manager(); + analyzer_mgr = new analyzer::Manager(); log_mgr = new logging::Manager(); input_mgr = new input::Manager(); file_mgr = new file_analysis::Manager(); @@ -708,8 +708,8 @@ SetupResult setup(int argc, char** argv, Options* zopts) exit(success ? 0 : 1); } - analyzer_mgr->InitPostScript(); packet_mgr->InitPostScript(); + analyzer_mgr->InitPostScript(); file_mgr->InitPostScript(); dns_mgr->InitPostScript(); @@ -916,8 +916,8 @@ SetupResult setup(int argc, char** argv, Options* zopts) reporter->FatalError("errors occurred while initializing"); run_state::detail::zeek_init_done = true; - analyzer_mgr->DumpDebug(); packet_mgr->DumpDebug(); + analyzer_mgr->DumpDebug(); run_state::detail::have_pending_timers = ! run_state::reading_traces && timer_mgr->Size() > 0; diff --git a/testing/btest/Baseline/plugins.protocol/output b/testing/btest/Baseline/plugins.protocol/output index 9ae01cc2c7..06f824361c 100644 --- a/testing/btest/Baseline/plugins.protocol/output +++ b/testing/btest/Baseline/plugins.protocol/output @@ -5,3 +5,5 @@ Demo::Foo - A Foo test analyzer (dynamic, version 1.0.0) === foo_message, [orig_h=::1, orig_p=37927/tcp, resp_h=::1, resp_p=4242/tcp], Hello, Foo!\x0a +=== +foo_message, [orig_h=::1, orig_p=37927/tcp, resp_h=::1, resp_p=4243/tcp], Hello, Foo!\x0a diff --git a/testing/btest/Traces/port4243.trace b/testing/btest/Traces/port4243.trace new file mode 100644 index 0000000000000000000000000000000000000000..ddd2202314aeaf1735dc06c4064fe593d5417c9b GIT binary patch literal 868 zcmca|c+)~A1{MYw`2U}Qff2}goBcS@;yx!s9FPsd$e`_R0*I@@<^ZCQ07xS;V4R{Z zFu77NyBj39fN9k#kOC%_|3_GuIJi#l^VJ23Gcq%?gKP_<*fxR5Q`Emb=8OZ{*t0-r z)vD(QP;CR*2(zsbXj>T2b`VAeP`@blvzzAX|bc vwgs9pW`ILa5Eyz-F>RSju`Nh`nb`ocX(^7za)S|49yk*L_M5} literal 0 HcmV?d00001 diff --git a/testing/btest/plugins/protocol-plugin/src/Plugin.cc b/testing/btest/plugins/protocol-plugin/src/Plugin.cc index 67d094a747..4c6fcb0352 100644 --- a/testing/btest/plugins/protocol-plugin/src/Plugin.cc +++ b/testing/btest/plugins/protocol-plugin/src/Plugin.cc @@ -1,6 +1,7 @@ #include "Plugin.h" #include "analyzer/Component.h" +#include "analyzer/Manager.h" #include "Foo.h" @@ -20,3 +21,13 @@ zeek::plugin::Configuration Plugin::Configure() config.version.patch = 0; return config; } + + +void Plugin::InitPostScript() + { + auto tag = ::zeek::analyzer_mgr->GetAnalyzerTag("Foo"); + if ( ! tag ) + ::zeek::reporter->FatalError("cannot get analyzer Tag"); + + zeek::analyzer_mgr->RegisterAnalyzerForPort(tag, TransportProto::TRANSPORT_TCP, 4243); + } diff --git a/testing/btest/plugins/protocol-plugin/src/Plugin.h b/testing/btest/plugins/protocol-plugin/src/Plugin.h index a8cd802746..5b2fb584e2 100644 --- a/testing/btest/plugins/protocol-plugin/src/Plugin.h +++ b/testing/btest/plugins/protocol-plugin/src/Plugin.h @@ -10,6 +10,8 @@ class Plugin : public zeek::plugin::Plugin protected: // Overridden from zeek::plugin::Plugin. zeek::plugin::Configuration Configure() override; + + void InitPostScript() override; }; extern Plugin plugin; diff --git a/testing/btest/plugins/protocol.zeek b/testing/btest/plugins/protocol.zeek index 5c03900cc7..7668da4140 100644 --- a/testing/btest/plugins/protocol.zeek +++ b/testing/btest/plugins/protocol.zeek @@ -4,6 +4,8 @@ # @TEST-EXEC: ZEEK_PLUGIN_PATH=`pwd` zeek -NN Demo::Foo >>output # @TEST-EXEC: echo === >>output # @TEST-EXEC: ZEEK_PLUGIN_PATH=`pwd` zeek -r $TRACES/port4242.trace %INPUT >>output +# @TEST-EXEC: echo === >>output +# @TEST-EXEC: ZEEK_PLUGIN_PATH=`pwd` zeek -r $TRACES/port4243.trace %INPUT >>output # @TEST-EXEC: TEST_DIFF_CANONIFIER= btest-diff output event foo_message(c: connection, data: string)