Merge remote-tracking branch 'origin/topic/awelzel/broker-deprecate-valp-list-interface'

* origin/topic/awelzel/broker-deprecate-valp-list-interface:
  broker: Deprecate MakeEvent(ValPList*)
  Span: Remove deduction guideline for Iter, Iter, include cleanup

CHANGES

@@ -1,3 +1,35 @@
+7.1.0-dev.209 | 2024-08-20 10:10:46 +0200
+
+  * broker: Deprecate MakeEvent(ValPList*) (Arne Welzel, Corelight)
+
+    The variadic broker messaging BIFs currently convert @ARGS@ into a
+    ValPList before passing it on to MakeEvent(). This appears historic
+    plumbing. Implement the same functionality using Span<const ValPtr>
+    and do the extra copying in the now deprecated MakeEvent().
+
+    Further, make passing a frame optional as not all callers may
+    have one available.
+
+  * Span: Remove deduction guideline for Iter, Iter, include cleanup (Arne Welzel, Corelight)
+
+    We don't have a constructor for that, so that's confusing, also
+    <array> isn't used.
+
+  * ScriptOpt: Ensure global statements have non-null scope (Arne Welzel, Corelight)
+
+    The ProfileFunc() logic assumed that GetScope() returned a non-nullptr.
+    This holds except for the synthetic global statements function.
+
+    Fix the latter and add an assert, also add a name to the type so it's
+    easier to recognize in a debugger what's going on, otherwise the name
+    is "".
+
+    This was found by UBSAN due to it seeing the ->OrderedVars() call on a
+    nullptr. Elsewhere, num_params == 0 shielded from that access and so
+    didn't lead to crashes.
+
+  * Update gen-zam submodule [nomail] [skip ci] (Tim Wojtulewicz, Corelight)
+
 7.1.0-dev.202 | 2024-08-16 09:06:41 -0700
 
   * Remove unused wrapper packet analyzer (Jan Grashoefer, Corelight)

VERSION

@@ -1 +1 @@
-7.1.0-dev.202
+7.1.0-dev.209

src/Span.h

@@ -2,7 +2,6 @@
 
 #pragma once
 
-#include <array>
 #include <cstddef>
 #include <iterator>
 #include <type_traits>
@@ -128,9 +127,6 @@ private:
 template<class T>
 Span(T*, size_t) -> Span<T>;
 
-template<class Iter>
-Span(Iter, Iter) -> Span<typename std::iterator_traits<Iter>::value_type>;
-
 template<class T, size_t N>
 Span(T (&)[N]) -> Span<T>;
 

src/broker/Manager.cc

@@ -853,16 +853,29 @@ bool Manager::AutoUnpublishEvent(const string& topic, Val* event) {
 }
 
 RecordVal* Manager::MakeEvent(ValPList* args, zeek::detail::Frame* frame) {
-    auto rval = new RecordVal(BifType::Record::Broker::Event);
+    // Deprecated MakeEvent() version using ValPList - requires extra copy.
+    zeek::Args cargs;
+    cargs.reserve(args->size());
+    for ( auto* a : *args )
+        cargs.push_back({zeek::NewRef{}, a});
+
+    return MakeEvent(ArgsSpan{cargs}, frame)->Ref()->AsRecordVal();
+}
+
+zeek::RecordValPtr Manager::MakeEvent(ArgsSpan args, zeek::detail::Frame* frame) {
+    scoped_reporter_location srl{frame};
+    return MakeEvent(args);
+}
+
+zeek::RecordValPtr Manager::MakeEvent(ArgsSpan args) {
+    auto rval = zeek::make_intrusive<RecordVal>(BifType::Record::Broker::Event);
     auto arg_vec = make_intrusive<VectorVal>(vector_of_data_type);
     rval->Assign(1, arg_vec);
-    Func* func = nullptr;
+    const Func* func = nullptr;
-    scoped_reporter_location srl{frame};
 
-    for ( auto i = 0; i < args->length(); ++i ) {
+    for ( size_t index = 0; index < args.size(); index++ ) {
-        auto arg_val = (*args)[i];
+        const auto& arg_val = args[index];
-
+        if ( index == 0 ) {
-        if ( i == 0 ) {
             // Event val must come first.
 
             if ( arg_val->GetType()->Tag() != TYPE_FUNC ) {
@@ -877,10 +890,10 @@ RecordVal* Manager::MakeEvent(ValPList* args, zeek::detail::Frame* frame) {
                 return rval;
             }
 
-            auto num_args = func->GetType()->Params()->NumFields();
+            auto num_args = static_cast<size_t>(func->GetType()->Params()->NumFields());
 
-            if ( num_args != args->length() - 1 ) {
+            if ( num_args != args.size() - 1 ) {
-                Error("bad # of arguments: got %d, expect %d", args->length(), num_args + 1);
+                Error("bad # of arguments: got %zu, expect %zu", args.size() - 1, num_args + 1);
                 return rval;
             }
 
@@ -888,12 +901,12 @@ RecordVal* Manager::MakeEvent(ValPList* args, zeek::detail::Frame* frame) {
             continue;
         }
 
-        const auto& got_type = (*args)[i]->GetType();
+        const auto& got_type = arg_val->GetType();
-        const auto& expected_type = func->GetType()->ParamList()->GetTypes()[i - 1];
+        const auto& expected_type = func->GetType()->ParamList()->GetTypes()[index - 1];
 
         if ( ! same_type(got_type, expected_type) ) {
             rval->Remove(0);
-            Error("event parameter #%d type mismatch, got %s, expect %s", i, type_name(got_type->Tag()),
+            Error("event parameter #%zu type mismatch, got %s, expect %s", index, type_name(got_type->Tag()),
                   type_name(expected_type->Tag()));
             return rval;
         }
@@ -901,17 +914,17 @@ RecordVal* Manager::MakeEvent(ValPList* args, zeek::detail::Frame* frame) {
         RecordValPtr data_val;
 
         if ( same_type(got_type, detail::DataVal::ScriptDataType()) )
-            data_val = {NewRef{}, (*args)[i]->AsRecordVal()};
+            data_val = {NewRef{}, arg_val->AsRecordVal()};
         else
-            data_val = BrokerData::ToRecordVal((*args)[i]);
+            data_val = BrokerData::ToRecordVal(arg_val);
 
         if ( ! data_val->HasField(0) ) {
             rval->Remove(0);
-            Error("failed to convert param #%d of type %s to broker data", i, type_name(got_type->Tag()));
+            Error("failed to convert param #%zu of type %s to broker data", index, type_name(got_type->Tag()));
             return rval;
         }
 
-        arg_vec->Assign(i - 1, std::move(data_val));
+        arg_vec->Assign(index - 1, std::move(data_val));
     }
 
     return rval;

src/broker/Manager.h

@@ -14,6 +14,7 @@
 #include <unordered_map>
 
 #include "zeek/IntrusivePtr.h"
+#include "zeek/Span.h"
 #include "zeek/broker/Data.h"
 #include "zeek/iosource/IOSource.h"
 #include "zeek/logging/WriterBackend.h"
@@ -262,7 +263,27 @@ public:
      * @return an `Event` record value.  If an invalid event or arguments
      * were supplied the optional "name" field will not be set.
      */
-    RecordVal* MakeEvent(ValPList* args, zeek::detail::Frame* frame);
+    [[deprecated("Remove in v8.1: Use the ArgsSpan version instead")]] RecordVal* MakeEvent(ValPList* args,
+                                                                                            zeek::detail::Frame* frame);
+
+    using ArgsSpan = Span<const ValPtr>;
+
+    /**
+     * Create an `Event` record value from an event and its arguments.
+     * @param args A span pointing at the event arguments.
+     * @return an `Event` record value.  If an invalid event or arguments
+     * were supplied the optional "name" field will not be set.
+     */
+    zeek::RecordValPtr MakeEvent(ArgsSpan args);
+
+    /**
+     * Create an `Event` record value from an event and its arguments.
+     * @param args A span pointing at the event arguments.
+     * @param frame the calling frame, used to report location info upon error
+     * @return an `Event` record value.  If an invalid event or arguments
+     * were supplied the optional "name" field will not be set.
+     */
+    zeek::RecordValPtr MakeEvent(ArgsSpan args, zeek::detail::Frame* frame);
 
     /**
      * Register interest in peer event messages that use a certain topic prefix.

src/broker/messaging.bif

@@ -5,9 +5,16 @@
 #include <set>
 #include <string>
 
+#include "zeek/Span.h"
 #include "zeek/broker/Manager.h"
 #include "zeek/logging/Manager.h"
 
+namespace {
+
+using ArgsSpan = zeek::Span<const zeek::ValPtr>;
+
+}
+
 static bool is_string_set(const zeek::Type* type)
 	{
 	if ( ! type->IsSet() )
@@ -46,7 +53,7 @@ std::set<std::string> val_to_topic_set(zeek::Val* val)
 	return rval;
 	}
 
-static bool publish_event_args(zeek::ValPList& args, const zeek::String* topic,
+static bool publish_event_args(ArgsSpan args, const zeek::String* topic,
                                zeek::detail::Frame* frame)
 	{
 	zeek::Broker::Manager::ScriptScopeGuard ssg;
@@ -57,9 +64,8 @@ static bool publish_event_args(zeek::ValPList& args, const zeek::String* topic,
 		                                      args[0]->AsRecordVal());
 	else
 		{
-		auto ev = zeek::broker_mgr->MakeEvent(&args, frame);
+		auto ev = zeek::broker_mgr->MakeEvent(args, frame);
-		rval = zeek::broker_mgr->PublishEvent(topic->CheckString(), ev);
+		rval = zeek::broker_mgr->PublishEvent(topic->CheckString(), ev->AsRecordVal());
-		Unref(ev);
 		}
 
 	return rval;
@@ -92,13 +98,9 @@ type Broker::Event: record;
 function Broker::make_event%(...%): Broker::Event
 	%{
 	zeek::Broker::Manager::ScriptScopeGuard ssg;
-	const auto& bif_args = @ARGS@;
-	ValPList args(bif_args->size());
 
-	for ( auto i = 0u; i < bif_args->size(); ++i )
+	auto ev = zeek::broker_mgr->MakeEvent(ArgsSpan{*@ARGS@});
-		args.push_back((*bif_args)[i].get());
+	return zeek::cast_intrusive<RecordVal>(ev);
-
-	return RecordValPtr{zeek::AdoptRef{}, zeek::broker_mgr->MakeEvent(&args, frame)};
 	%}
 
 ## Publishes an event at a given topic.
@@ -112,13 +114,8 @@ function Broker::make_event%(...%): Broker::Event
 ## Returns: true if the message is sent.
 function Broker::publish%(topic: string, ...%): bool
 	%{
-	const auto& bif_args = @ARGS@;
+	auto rval = publish_event_args(ArgsSpan{*@ARGS@}.subspan(1),
-	ValPList args(bif_args->size() - 1);
+	                               topic->AsString(), frame);
-
-	for ( auto i = 1u; i < bif_args->size(); ++i )
-		args.push_back((*bif_args)[i].get());
-
-	auto rval = publish_event_args(args, topic->AsString(), frame);
 	return zeek::val_mgr->Bool(rval);
 	%}
 
@@ -208,13 +205,8 @@ function Cluster::publish_rr%(pool: Pool, key: string, ...%): bool
 	if ( ! topic->AsString()->Len() )
 		return zeek::val_mgr->False();
 
-	const auto& bif_args = @ARGS@;
+	auto rval = publish_event_args(ArgsSpan{*@ARGS@}.subspan(2),
-	ValPList args(bif_args->size() - 2);
+	                               topic->AsString(), frame);
-
-	for ( auto i = 2u; i < bif_args->size(); ++i )
-		args.push_back((*bif_args)[i].get());
-
-	auto rval = publish_event_args(args, topic->AsString(), frame);
 	return zeek::val_mgr->Bool(rval);
 	%}
 
@@ -251,12 +243,7 @@ function Cluster::publish_hrw%(pool: Pool, key: any, ...%): bool
 	if ( ! topic->AsString()->Len() )
 		return zeek::val_mgr->False();
 
-	const auto& bif_args = @ARGS@;
+	auto rval = publish_event_args(ArgsSpan{*@ARGS@}.subspan(2),
-	ValPList args(bif_args->size() - 2);
+	                               topic->AsString(), frame);
-
-	for ( auto i = 2u; i < bif_args->size(); ++i )
-		args.push_back((*bif_args)[i].get());
-
-	auto rval = publish_event_args(args, topic->AsString(), frame);
 	return zeek::val_mgr->Bool(rval);
 	%}