Merge remote-tracking branch 'origin/topic/awelzel/2403-reject-confli…

…cting-plugins'

* origin/topic/awelzel/2403-reject-conflicting-plugins:
  plugins: Reject dynamic plugins matching names of built-in ones

CHANGES

@@ -1,3 +1,13 @@
+5.2.0-dev.103 | 2022-10-16 17:49:24 -0700
+
+  * plugins: Reject dynamic plugins matching names of built-in ones (Arne Welzel, Corelight)
+
+    This goes the hard-exit on conflicts route as IMO it provides better
+    messaging that something is wrong, rather than defaulting to something
+    the user may not expect.
+
+    Fixes #2403
+
 5.2.0-dev.101 | 2022-10-16 17:48:26 -0700
 
   * Add &ordered attribute for tables/sets (Tim Wojtulewicz, Corelight)

VERSION

@@ -1 +1 @@
-5.2.0-dev.101
+5.2.0-dev.103

src/plugin/Manager.cc

@@ -164,6 +164,8 @@ bool Manager::ActivateDynamicPluginInternal(const std::string& name, bool ok_if_
 
 	dynamic_plugin_map::iterator m = dynamic_plugins.find(util::strtolower(name));
 
+	plugin_list* all_plugins = Manager::ActivePluginsInternal();
+
 	if ( m == dynamic_plugins.end() )
 		{
 		if ( ok_if_not_found )
@@ -172,8 +174,6 @@ bool Manager::ActivateDynamicPluginInternal(const std::string& name, bool ok_if_
 		// Check if it's a static built-in plugin; they are always
 		// active, so just ignore. Not the most efficient way, but
 		// this should be rare to begin with.
-		plugin_list* all_plugins = Manager::ActivePluginsInternal();
-
 		for ( const auto& p : *all_plugins )
 			{
 			if ( p->Name() == name )
@@ -195,6 +195,23 @@ bool Manager::ActivateDynamicPluginInternal(const std::string& name, bool ok_if_
 
 	DBG_LOG(DBG_PLUGINS, "Activating plugin %s", name.c_str());
 
+	// If there's a plugin with the same name already, report an error and let
+	// the user do the conflict resolution.
+	auto lower_name = util::strtolower(name);
+	for ( const auto& p : *all_plugins )
+		{
+		if ( util::strtolower(p->Name()) == lower_name )
+			{
+			auto v = p->Version();
+			auto error = util::fmt(
+				"dynamic plugin %s from directory %s conflicts with %s plugin %s (%d.%d.%d)",
+				name.c_str(), dir.c_str(), p->DynamicPlugin() ? "dynamic" : "built-in",
+				p->Name().c_str(), v.major, v.minor, v.patch);
+			errors->push_back(error);
+			return false;
+			}
+		}
+
 	// Load shared libraries.
 
 	string dypattern = dir + "/lib/*." + HOST_ARCHITECTURE + DYNAMIC_PLUGIN_SUFFIX;

testing/btest/Baseline/plugins.conflict-plugin/output

@@ -0,0 +1,2 @@
+error in <...>/init-bare.zeek, line 1: dynamic plugin zeek::asciireader from directory <...>/build/ conflicts with built-in plugin Zeek::AsciiReader (-1.-1.0)
+fatal error in <...>/init-bare.zeek, line 1: aborting after plugin errors

testing/btest/plugins/conflict-plugin.zeek

@@ -0,0 +1,5 @@
+# @TEST-EXEC: ${DIST}/auxil/zeek-aux/plugin-support/init-plugin  -u . Zeek AsciiReader 2>&1 > /dev/null
+# @TEST-EXEC: cp -r %DIR/conflict-plugin/* .
+# @TEST-EXEC: ./configure --zeek-dist=${DIST} && make
+# @TEST-EXEC-FAIL: ZEEK_PLUGIN_PATH=`pwd` zeek -NN >> output 2>&1
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff output

testing/btest/plugins/conflict-plugin/CMakeLists.txt

@@ -0,0 +1,16 @@
+
+project(Zeek-Plugin-Conflict-Plugin)
+
+cmake_minimum_required(VERSION 3.5)
+
+if ( NOT ZEEK_DIST )
+    message(FATAL_ERROR "ZEEK_DIST not set")
+endif ()
+
+set(CMAKE_MODULE_PATH ${ZEEK_DIST}/cmake)
+
+include(ZeekPlugin)
+
+zeek_plugin_begin(Zeek AsciiReader)
+zeek_plugin_cc(src/Plugin.cc)
+zeek_plugin_end()

testing/btest/plugins/conflict-plugin/src/Plugin.cc

@@ -0,0 +1,19 @@
+#include "Plugin.h"
+
+namespace btest::plugin::Demo_Foo
+	{
+Plugin plugin;
+	}
+
+using namespace btest::plugin::Demo_Foo;
+
+zeek::plugin::Configuration Plugin::Configure()
+	{
+	zeek::plugin::Configuration config;
+	config.name = "Zeek::AsciiReader";
+	config.description = "Conflicts with the built-in AsciiReader";
+	config.version.major = 1;
+	config.version.minor = 0;
+	config.version.patch = 0;
+	return config;
+	}

testing/btest/plugins/conflict-plugin/src/Plugin.h

@@ -0,0 +1,17 @@
+#pragma once
+
+#include <plugin/Plugin.h>
+
+namespace btest::plugin::Demo_Foo
+	{
+
+class Plugin : public zeek::plugin::Plugin
+	{
+protected:
+	// Overridden from plugin::Plugin.
+	virtual zeek::plugin::Configuration Configure();
+	};
+
+extern Plugin plugin;
+
+	}