Merge remote-tracking branch 'origin/topic/awelzel/2935-publish-hrw-rr-type-check'

* origin/topic/awelzel/2935-publish-hrw-rr-type-check:
  zeek.bif: Remove cat_sep() fully var_arg changes
  broker/messaging: Runtime type checks for pool
  bifcl: Bump for runtime-type checks in var_arg bifs

CHANGES

@@ -1,3 +1,21 @@
+6.0.0-dev.391 | 2023-04-19 19:48:50 +0200
+
+  * zeek.bif: Remove cat_sep() fully var_arg changes (Arne Welzel, Corelight)
+
+    These were introduced to better catch type violations at runtime. With
+    bifcl doing these checks, revert to a better documented version.
+
+  * GH-2935: broker/messaging: Runtime type checks for pool (Arne Welzel, Corelight)
+
+    publish_hrw() and publish_rr() are excluded from type checking due to their
+    variadic nature. Passing a wrong type for the pool argument previously triggered
+    an abort, now the result is runtime errors. This isn't great, but it's
+    better than crashing Zeek.
+
+    Closes #2935
+
+  * bifcl: Bump for runtime-type checks in var_arg bifs (Arne Welzel, Corelight)
+
 6.0.0-dev.387 | 2023-04-19 09:01:32 -0700
 
   * Add call stacks to script profiler output (Tim Wojtulewicz, Corelight)

VERSION

@@ -1 +1 @@
-6.0.0-dev.387
+6.0.0-dev.391

auxil/bifcl

@@ -1 +1 @@
-Subproject commit 14f393117f63ef7327a40ade435f9021787b4f45
+Subproject commit b6f138be79f7d4408302b1297b0c63092b019773

src/broker/messaging.bif

@@ -65,6 +65,16 @@ static bool publish_event_args(zeek::ValPList& args, const zeek::String* topic,
 	return rval;
 	}
 
+static bool is_cluster_pool(zeek::Val* pool)
+	{
+	static zeek::RecordTypePtr pool_type = nullptr;
+
+	if ( ! pool_type )
+		pool_type = zeek::id::find_type<zeek::RecordType>("Cluster::Pool");
+
+	return pool->GetType() == pool_type;
+	}
+
 %%}
 
 module Broker;
@@ -186,6 +196,12 @@ function Cluster::publish_rr%(pool: Pool, key: string, ...%): bool
 	if ( ! topic_func )
 		topic_func = zeek::detail::global_scope()->Find("Cluster::rr_topic")->GetVal()->AsFunc();
 
+	if ( ! is_cluster_pool(pool) )
+		{
+		zeek::emit_builtin_error("expected type Cluster::Pool for pool");
+		return zeek::val_mgr->False();
+		}
+
 	zeek::Args vl{{zeek::NewRef{}, pool}, {zeek::NewRef{}, key}};
 	auto topic = topic_func->Invoke(&vl);
 
@@ -223,6 +239,12 @@ function Cluster::publish_hrw%(pool: Pool, key: any, ...%): bool
 	if ( ! topic_func )
 		topic_func = zeek::detail::global_scope()->Find("Cluster::hrw_topic")->GetVal()->AsFunc();
 
+	if ( ! is_cluster_pool(pool) )
+		{
+		zeek::emit_builtin_error("expected type Cluster::Pool for pool");
+		return zeek::val_mgr->False();
+		}
+
 	zeek::Args vl{{zeek::NewRef{}, pool}, {zeek::NewRef{}, key}};
 	auto topic = topic_func->Invoke(&vl);
 

src/zeek.bif

@@ -1541,44 +1541,18 @@ function cat%(...%): string
 ## Concatenates all arguments, with a separator placed between each one. This
 ## function is similar to :zeek:id:`cat`, but places a separator between each
 ## given argument. If any of the variable arguments is an empty string it is
-## replaced by the given default string instead. This function takes at least
+## replaced by the given default string instead.
-## two arguments. The first argument is the separator *sep* placed between
+##
-## each argument, the second argument is the default string *def* used when
+## sep: The separator to place between each argument.
-## an argument is the empty string.
+##
+## def: The default string to use when an argument is the empty string.
 ##
 ## Returns: A concatenation of all arguments with *sep* between each one and
 ##          empty strings replaced with *def*.
 ##
 ## .. zeek:see:: cat string_cat
-function cat_sep%(...%): string
+function cat_sep%(sep: string, def: string, ...%): string
 	%{
-	if ( @ARGC@ < 2 )
-		{
-		zeek::emit_builtin_error(zeek::util::fmt("cat_sep() takes at least 2 arguments, got %ld",
-		                                         @ARGC@));
-		return nullptr;
-		}
-
-	zeek::ValPtr sep_arg = @ARG@[0];
-	zeek::ValPtr def_arg = @ARG@[1];
-
-	if ( sep_arg->GetType()->Tag() != zeek::TYPE_STRING )
-		{
-		zeek::emit_builtin_error(zeek::util::fmt("expected type string for separator, got %s",
-		                                         zeek::type_name(sep_arg->GetType()->Tag())));
-		return nullptr;
-		}
-
-	if ( def_arg->GetType()->Tag() != zeek::TYPE_STRING )
-		{
-		zeek::emit_builtin_error(zeek::util::fmt("expected type string for default, got %s",
-		                                         zeek::type_name(def_arg->GetType()->Tag())));
-		return nullptr;
-		}
-
-	zeek::StringVal* sep = sep_arg->AsStringVal();
-	zeek::StringVal* def = def_arg->AsStringVal();
-
 	zeek::ODesc d;
 	d.SetStyle(RAW_STYLE);
 	int pre_size = 0;

testing/btest/Baseline/bifs.cat_sep_errors-2/.stderr

@@ -1,2 +1,2 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-error in <...>/cat_sep_errors.zeek, line 3: cat_sep() takes at least 2 arguments, got 1 (cat_sep(sep))
+error in <...>/cat_sep_errors.zeek, line 3: cat_sep() takes at least 2 argument(s), got 1 (cat_sep(sep))

testing/btest/Baseline/bifs.cat_sep_errors-3/.stderr

@@ -1,2 +1,2 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-error in <...>/cat_sep_errors.zeek, line 4: expected type string for default, got count (cat_sep(sep, 1))
+error in <...>/cat_sep_errors.zeek, line 4: expected type string for def, got count (cat_sep(sep, 1))

testing/btest/Baseline/bifs.cat_sep_errors-4/.stderr

@@ -1,2 +1,2 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-error in <...>/cat_sep_errors.zeek, line 4: expected type string for separator, got count (cat_sep(1, default))
+error in <...>/cat_sep_errors.zeek, line 4: expected type string for sep, got count (cat_sep(1, default))

testing/btest/Baseline/bifs.cat_sep_errors-5/.stderr

@@ -1,2 +1,2 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-error in <...>/cat_sep_errors.zeek, line 3: expected type string for separator, got record (cat_sep([$a=1], default))
+error in <...>/cat_sep_errors.zeek, line 3: expected type string for sep, got record (cat_sep([$a=1], default))

testing/btest/Baseline/bifs.cat_sep_errors/.stderr

@@ -1,2 +1,2 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-error in <...>/cat_sep_errors.zeek, line 6: cat_sep() takes at least 2 arguments, got 0 (cat_sep())
+error in <...>/cat_sep_errors.zeek, line 6: cat_sep() takes at least 2 argument(s), got 0 (cat_sep())

testing/btest/Baseline/scripts.base.frameworks.cluster.publish-hrw-type-check/.stderr

@@ -0,0 +1,5 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/publish-hrw-type-check.zeek, line 15: expected type Cluster::Pool for pool (Cluster::publish_hrw(/topic, 1234/tcp, test_send, [$a=a]))
+error in <...>/publish-hrw-type-check.zeek, line 17: expected type Cluster::Pool for pool (Cluster::publish_hrw(0, 1234/tcp, test_send, [$a=a]))
+error in <...>/publish-hrw-type-check.zeek, line 19: expected type Cluster::Pool for pool (Cluster::publish_rr(/topic, val, test_send, [$a=b]))
+error in <...>/publish-hrw-type-check.zeek, line 21: expected type string for key, got port (Cluster::publish_rr(Cluster::Pool(), 1234/tcp, test_send, [$a=c]))

testing/btest/scripts/base/frameworks/cluster/publish-hrw-type-check.zeek

@@ -0,0 +1,22 @@
+# @TEST-DOC: Check that Cluster::publish_hrw() and Cluster::publish_rr() do not cause an abort when provided with wrongly typed arguments.
+# @TEST-EXEC: zeek -b %INPUT
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr
+
+@load base/frameworks/cluster
+
+type R: record {
+	a: string;
+};
+
+event test_send(r: R) { }
+
+event zeek_init()
+	{
+	Cluster::publish_hrw("/topic", 1234/tcp, test_send, [$a="a"]);
+
+	Cluster::publish_hrw(0, 1234/tcp, test_send, [$a="a"]);
+
+	Cluster::publish_rr("/topic", "val", test_send, [$a="b"]);
+
+	Cluster::publish_rr(Cluster::Pool(), 1234/tcp, test_send, [$a="c"]);
+	}