From 8ae30d8aac22e6c972ba6d3c365de5c6469e41b9 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Fri, 7 Jul 2017 15:44:53 -0700 Subject: [PATCH 1/2] Extend plugin infrastructure to catch Bro version mismatches at link time. People keep running into the problem that they upgrade Bro but forget to recompile their plugins--which can lead to crashes. While the plugins' API version was supposed to catch this, it's not reliable as that check may come too late. This change takes a different tack: We compile a C function into the Bro binary that has Bro's version number encoded into its name. A plugin can then reference that function. If the Bro version changes, the function goes away and the plugin won't load anymore. I've integrated that function reference into the plugin skeleton code so that new plugins get it automatically (unless explicitly removed). I couldn't see a way to do it transparently for already existing plugins unfortunately. The version number used for the function name is slightly normalized to skip any git revision postfixes (i.e., "2.5-xxx" is always treated as "2.5-git") so that one doesn't need to recompile all plugins after every master commit. That seems good enough, usually people run into this when upgrading to a new release. If one loads an old plugin into a new Bro, the error message looks like this: $ bro -NN Demo::Foo fatal error in /home/robin/bro/master/scripts/base/init-bare.bro, line 1: cannot load plugin library /home/robin/tmp/p/build//lib/Demo-Foo.linux-x86_64.so: /home/robin/tmp/p/build//lib/Demo-Foo.linux-x86_64.so: undefined symbol: bro_version_2_5_git_debug Not the prettiest, but better than a crash! TODO: I'm still unsure if we should remove the plugin API version altogetger now. This link-time check should catch everything the API version does, except for master commits. --- CMakeLists.txt | 9 +++++++++ aux/bro-aux | 2 +- bro-config.h.in | 11 +++++++++++ src/plugin/Plugin.h | 9 +++++++++ src/version.c.in | 12 ++++++++++++ 5 files changed, 42 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5a7fba482e..29643c7b84 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -40,12 +40,21 @@ file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/bro-path-dev.csh "setenv PATH \"${CMAKE_CURRENT_BINARY_DIR}/src\":$PATH\n") file(STRINGS "${CMAKE_CURRENT_SOURCE_DIR}/VERSION" VERSION LIMIT_COUNT 1) + string(REPLACE "." " " version_numbers ${VERSION}) separate_arguments(version_numbers) list(GET version_numbers 0 VERSION_MAJOR) list(GET version_numbers 1 VERSION_MINOR) set(VERSION_MAJ_MIN "${VERSION_MAJOR}.${VERSION_MINOR}") +set(VERSION_C_IDENT "${VERSION}") +string(REGEX REPLACE "-[0-9]*$" "_git" VERSION_C_IDENT "${VERSION_C_IDENT}") +string(REGEX REPLACE "[^a-zA-Z0-9_]" "_" VERSION_C_IDENT "${VERSION_C_IDENT}") + +if(${ENABLE_DEBUG}) + set(VERSION_C_IDENT "${VERSION_C_IDENT}_debug") +endif() + ######################################################################## ## Dependency Configuration diff --git a/aux/bro-aux b/aux/bro-aux index 43f4b90bba..7a38763d7a 160000 --- a/aux/bro-aux +++ b/aux/bro-aux @@ -1 +1 @@ -Subproject commit 43f4b90bbaf87dae1a1073e7bf13301e58866011 +Subproject commit 7a38763d7a687dc8974bf6fd212dc75ba4a4b23c diff --git a/bro-config.h.in b/bro-config.h.in index 290dd31cae..003eea88b7 100644 --- a/bro-config.h.in +++ b/bro-config.h.in @@ -229,3 +229,14 @@ #ifndef BRO_PLUGIN_INTERNAL_BUILD #define BRO_PLUGIN_INTERNAL_BUILD @BRO_PLUGIN_INTERNAL_BUILD@ #endif + +/* A C function that has the Bro version encoded into its name. */ +#define BRO_VERSION_FUNCTION bro_version_@VERSION_C_IDENT@ +#ifdef __cplusplus +extern "C" { +#endif +extern const char* BRO_VERSION_FUNCTION(); +#ifdef __cplusplus +} +#endif + diff --git a/src/plugin/Plugin.h b/src/plugin/Plugin.h index aabec22bc4..37f2064644 100644 --- a/src/plugin/Plugin.h +++ b/src/plugin/Plugin.h @@ -18,6 +18,8 @@ #define BRO_PLUGIN_API_VERSION 5 #endif +#define BRO_PLUGIN_BRO_VERSION BRO_VERSION_FUNCTION + class ODesc; class Func; class Event; @@ -93,6 +95,12 @@ public: // strong hint.). The attribute seems generally available. inline Configuration() __attribute__((always_inline)); + /** + * One can assign BRO_PLUGIN_BRO_VERSION to this to catch + * version mismatches at link(!) time. + */ + const char* (*bro_version)(); + private: friend class Plugin; int api_version; // Current BRO_PLUGIN_API_VERSION. Automatically set. @@ -103,6 +111,7 @@ inline Configuration::Configuration() name = ""; description = ""; api_version = BRO_PLUGIN_API_VERSION; + bro_version = 0; } /** diff --git a/src/version.c.in b/src/version.c.in index 86c4b16f24..65df65da00 100644 --- a/src/version.c.in +++ b/src/version.c.in @@ -1 +1,13 @@ + +#include "bro-config.h" + char version[] = "@VERSION@"; + +// A C function that has the current version built into its name. +// One can link a shared library against this to ensure that it won't +// load if the version of the main Bro binary differs compared to +// what the library was compiled against. +const char* BRO_VERSION_FUNCTION() +{ + return "@VERSION_C_IDENT@"; +} From 78f8ff432f984548fa2b88865c904109e121443a Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Fri, 21 Jul 2017 13:23:37 -0700 Subject: [PATCH 2/2] Adding plugin API number into versioned function name, and removing old runtime API version check. --- CMakeLists.txt | 9 +++++++-- src/plugin/Manager.cc | 4 ---- src/plugin/Plugin.cc | 5 ----- src/plugin/Plugin.h | 18 +++--------------- .../plugins.api-version-mismatch/output | 1 - testing/btest/plugins/api-version-mismatch.sh | 8 -------- 6 files changed, 10 insertions(+), 35 deletions(-) delete mode 100644 testing/btest/Baseline/plugins.api-version-mismatch/output delete mode 100644 testing/btest/plugins/api-version-mismatch.sh diff --git a/CMakeLists.txt b/CMakeLists.txt index 29643c7b84..990c09b611 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -40,6 +40,11 @@ file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/bro-path-dev.csh "setenv PATH \"${CMAKE_CURRENT_BINARY_DIR}/src\":$PATH\n") file(STRINGS "${CMAKE_CURRENT_SOURCE_DIR}/VERSION" VERSION LIMIT_COUNT 1) +execute_process(COMMAND grep "^#define *BRO_PLUGIN_API_VERSION" + INPUT_FILE ${CMAKE_CURRENT_SOURCE_DIR}/src/plugin/Plugin.h + OUTPUT_VARIABLE API_VERSION + OUTPUT_STRIP_TRAILING_WHITESPACE) +string(REGEX REPLACE "^#define.*VERSION *" "" API_VERSION "${API_VERSION}") string(REPLACE "." " " version_numbers ${VERSION}) separate_arguments(version_numbers) @@ -47,9 +52,9 @@ list(GET version_numbers 0 VERSION_MAJOR) list(GET version_numbers 1 VERSION_MINOR) set(VERSION_MAJ_MIN "${VERSION_MAJOR}.${VERSION_MINOR}") -set(VERSION_C_IDENT "${VERSION}") +set(VERSION_C_IDENT "${VERSION}_plugin_${API_VERSION}") string(REGEX REPLACE "-[0-9]*$" "_git" VERSION_C_IDENT "${VERSION_C_IDENT}") -string(REGEX REPLACE "[^a-zA-Z0-9_]" "_" VERSION_C_IDENT "${VERSION_C_IDENT}") +string(REGEX REPLACE "[^a-zA-Z0-9_\$]" "_" VERSION_C_IDENT "${VERSION_C_IDENT}") if(${ENABLE_DEBUG}) set(VERSION_C_IDENT "${VERSION_C_IDENT}_debug") diff --git a/src/plugin/Manager.cc b/src/plugin/Manager.cc index 104bcfbdd5..a6c564d4f2 100644 --- a/src/plugin/Manager.cc +++ b/src/plugin/Manager.cc @@ -243,10 +243,6 @@ bool Manager::ActivateDynamicPluginInternal(const std::string& name, bool ok_if_ plugins_by_path.insert(std::make_pair(normalize_path(dir), current_plugin)); - if ( current_plugin->APIVersion() != BRO_PLUGIN_API_VERSION ) - reporter->FatalError("plugin's API version does not match Bro (expected %d, got %d in %s)", - BRO_PLUGIN_API_VERSION, current_plugin->APIVersion(), path); - // We execute the pre-script initialization here; this in // fact could be *during* script initialization if we got // triggered via @load-plugin. diff --git a/src/plugin/Plugin.cc b/src/plugin/Plugin.cc index 7af9b9dfee..f54749f837 100644 --- a/src/plugin/Plugin.cc +++ b/src/plugin/Plugin.cc @@ -242,11 +242,6 @@ VersionNumber Plugin::Version() const return config.version; } -int Plugin::APIVersion() const - { - return config.api_version; - } - bool Plugin::DynamicPlugin() const { return dynamic; diff --git a/src/plugin/Plugin.h b/src/plugin/Plugin.h index 37f2064644..c00831d1fd 100644 --- a/src/plugin/Plugin.h +++ b/src/plugin/Plugin.h @@ -13,10 +13,9 @@ #include "iosource/Component.h" #include "logging/WriterBackend.h" -// We allow to override this externally for testing purposes. -#ifndef BRO_PLUGIN_API_VERSION -#define BRO_PLUGIN_API_VERSION 5 -#endif +// Increase this when making incompatible changes to the plugin API. Note +// that the constant is never used in C code. It's picked up on by CMake. +#define BRO_PLUGIN_API_VERSION 6 #define BRO_PLUGIN_BRO_VERSION BRO_VERSION_FUNCTION @@ -103,14 +102,12 @@ public: private: friend class Plugin; - int api_version; // Current BRO_PLUGIN_API_VERSION. Automatically set. }; inline Configuration::Configuration() { name = ""; description = ""; - api_version = BRO_PLUGIN_API_VERSION; bro_version = 0; } @@ -451,15 +448,6 @@ public: **/ const std::string& PluginPath() const; - /** - * Returns the internal version of the Bro API that this plugin - * relies on. Only plugins that match Bro's current API version can - * be used. For statically compiled plugins this is automatically the - * case, but dynamically loaded plugins may cause a mismatch if they - * were compiled for a different Bro version. - */ - int APIVersion() const; - /** * Returns a list of all components the plugin provides. */ diff --git a/testing/btest/Baseline/plugins.api-version-mismatch/output b/testing/btest/Baseline/plugins.api-version-mismatch/output deleted file mode 100644 index 04f3cdd3a2..0000000000 --- a/testing/btest/Baseline/plugins.api-version-mismatch/output +++ /dev/null @@ -1 +0,0 @@ -fatal error in /home/robin/bro/plugins/scripts/base/init-bare.bro, line 1: plugin's API version does not match Bro (expected 2, got 42 in /home/robin/bro/plugins/testing/btest/.tmp/plugins.api-version-mismatch/build//lib/XXX) diff --git a/testing/btest/plugins/api-version-mismatch.sh b/testing/btest/plugins/api-version-mismatch.sh deleted file mode 100644 index 2483582359..0000000000 --- a/testing/btest/plugins/api-version-mismatch.sh +++ /dev/null @@ -1,8 +0,0 @@ -# @TEST-EXEC: ${DIST}/aux/bro-aux/plugin-support/init-plugin -u . Demo Foo -# @TEST-EXEC: bash %INPUT -# @TEST-EXEC: ./configure --bro-dist=${DIST} && make -# @TEST-EXEC-FAIL: BRO_PLUGIN_PATH=`pwd` bro -NN Demo::Foo >tmp 2>&1 -# @TEST-EXEC: cat tmp | sed 's/Demo-Foo[-a-zA-Z0-9_.]*/XXX/' >>output -# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff output - -( echo '#define BRO_PLUGIN_API_VERSION 42'; cat src/Plugin.cc; ) >src/Plugin.cc.tmp && mv src/Plugin.cc.tmp src/Plugin.cc