Merge remote-tracking branch 'origin/topic/robin/gh-3881-spicy-ports'

* origin/topic/robin/gh-3881-spicy-ports:
  Spicy: Register well-known ports through an event handler.
  Revert "Remove deprecated port/ports fields for spicy analyzers"

(cherry picked from commit a2079bcda6)

CHANGES

@@ -1,3 +1,24 @@
+7.0.0-11 | 2024-08-30 12:38:59 -0700
+
+  * Spicy: Register well-known ports through an event handler. (Robin Sommer, Corelight)
+
+    This avoids the earlier problem of not tracking ports correctly in
+    scriptland, while still supporting `port` in EVT files and `%port` in
+    Spicy files.
+
+    As it turns out we are already following the same approach for file
+    analyzers' MIME types, so I'm applying the same pattern: it's one
+    event per port, without further customization points. That leaves the
+    patch pretty small after all while fixing the original issue.
+
+    (cherry picked from commit a2079bcda6e40180b888240a281c12cc0ca735be)
+
+  * Revert "Remove deprecated port/ports fields for spicy analyzers" (Robin Sommer, Corelight)
+
+    This reverts commit 15d404dd191a723960e4efd956eec22739d3f1c2.
+
+    (cherry picked from commit a2079bcda6e40180b888240a281c12cc0ca735be)
+
 7.0.0-9 | 2024-08-30 11:47:39 -0700
 
   * ldap: Promote uint8 to uint64 before shifting (Arne Welzel, Corelight)

VERSION

@@ -1 +1 @@
-7.0.0-9
+7.0.0-11

doc

@@ -1 +1 @@
-Subproject commit 4e2413e2fb2f98298430b3e2cb384fc5f18cbde4
+Subproject commit b845bee39fa241892458d274184e2e9f22f4d096

scripts/base/frameworks/spicy/init-framework.zeek

@@ -47,12 +47,18 @@ export {
 
 # Marked with &is_used to suppress complaints when there aren't any
 # Spicy file analyzers loaded, and hence this event can't be generated.
-# The attribute is only supported for Zeek 5.0 and higher.
 event spicy_analyzer_for_mime_type(a: Files::Tag, mt: string) &is_used
     {
     Files::register_for_mime_type(a, mt);
     }
 
+# Marked with &is_used to suppress complaints when there aren't any
+# Spicy protocol analyzers loaded, and hence this event can't be generated.
+event spicy_analyzer_for_port(a: Analyzer::Tag, p: port) &is_used
+    {
+    Analyzer::register_for_port(a, p);
+    }
+
 function enable_protocol_analyzer(tag: Analyzer::Tag) : bool
     {
     return Spicy::__toggle_analyzer(tag, T);

src/spicy/manager.cc

@@ -6,6 +6,7 @@
 #include <glob.h>
 
 #include <exception>
+#include <iterator>
 #include <limits>
 #include <utility>
 
@@ -32,6 +33,7 @@
 #include "zeek/spicy/file-analyzer.h"
 #include "zeek/spicy/packet-analyzer.h"
 #include "zeek/spicy/protocol-analyzer.h"
+#include "zeek/spicy/runtime-support.h"
 #include "zeek/zeek-config-paths.h"
 
 using namespace zeek;
@@ -74,9 +76,13 @@ void Manager::registerProtocolAnalyzer(const std::string& name, hilti::rt::Proto
     info.name_zeek = hilti::rt::replace(name, "::", "_");
     info.name_zeekygen = hilti::rt::fmt("<Spicy-%s>", name);
     info.protocol = proto;
-    info.ports = ports;
     info.linker_scope = linker_scope;
 
+    // Store ports in a deterministic order. We can't (easily) sort the
+    // `hilti::rt::Vector` unfortunately.
+    std::copy(ports.begin(), ports.end(), std::back_inserter(info.ports));
+    std::sort(info.ports.begin(), info.ports.end());
+
     // We may have that analyzer already iff it was previously pre-registered
     // without a linker scope. We'll then only set the scope now.
     if ( auto t = _analyzer_name_to_tag_type.find(info.name_zeek); t != _analyzer_name_to_tag_type.end() ) {
@@ -701,14 +707,25 @@ void Manager::InitPostScript() {
         if ( ! tag )
             reporter->InternalError("cannot get analyzer tag for '%s'", p.name_analyzer.c_str());
 
+        auto register_analyzer_for_port = [&](auto tag, const hilti::rt::Port& port_) {
+            SPICY_DEBUG(hilti::rt::fmt("  Scheduling analyzer for port %s", port_));
+
+            // Well-known ports are registered in scriptland, so we'll raise an
+            // event that will do it for us through a predefined handler.
+            zeek::Args vals = Args();
+            vals.emplace_back(tag.AsVal());
+            vals.emplace_back(zeek::spicy::rt::to_val(port_, base_type(TYPE_PORT)));
+            EventHandlerPtr handler = event_registry->Register("spicy_analyzer_for_port");
+            event_mgr.Enqueue(handler, vals);
+        };
+
         for ( const auto& ports : p.ports ) {
             const auto proto = ports.begin.protocol();
 
             // Port ranges are closed intervals.
             for ( auto port = ports.begin.port(); port <= ports.end.port(); ++port ) {
                 const auto port_ = hilti::rt::Port(port, proto);
-                SPICY_DEBUG(hilti::rt::fmt("  Scheduling analyzer for port %s", port_));
-                analyzer_mgr->RegisterAnalyzerForPort(tag, transport_protocol(port_), port);
+                register_analyzer_for_port(tag, port_);
 
                 // Don't double register in case of single-port ranges.
                 if ( ports.begin.port() == ports.end.port() )
@@ -727,7 +744,7 @@ void Manager::InitPostScript() {
                     continue;
 
                 SPICY_DEBUG(hilti::rt::fmt("  Scheduling analyzer for port %s", port.port));
-                analyzer_mgr->RegisterAnalyzerForPort(tag, transport_protocol(port.port), port.port.port());
+                register_analyzer_for_port(tag, port.port);
             }
         }
     }

src/spicy/manager.h

@@ -85,7 +85,7 @@ public:
      *
      * @param name name of the analyzer as defined in its EVT file
      * @param proto analyzer's transport-layer protocol
-     * @param prts well-known ports for the analyzer; it'll be activated automatically for these
+     * @param ports well-known ports for the analyzer; it'll be activated automatically for these
      * @param parser_orig name of the Spicy parser for the originator side; must match the name that
      * Spicy registers the unit's parser with
      * @param parser_resp name of the Spicy parser for the originator side; must match the name that
@@ -343,7 +343,7 @@ private:
         std::string name_parser_resp;
         std::string name_replaces;
         hilti::rt::Protocol protocol = hilti::rt::Protocol::Undef;
-        hilti::rt::Vector<::zeek::spicy::rt::PortRange> ports;
+        std::vector<::zeek::spicy::rt::PortRange> ports; // we keep this sorted
         std::string linker_scope;
 
         // Computed and available once the analyzer has been registered.

src/spicy/port-range.h

@@ -19,6 +19,11 @@ struct PortRange {
 
     hilti::rt::Port begin; /**< first port in the range */
     hilti::rt::Port end;   /**< last port in the range */
+
+    bool operator<(const PortRange& other) const {
+        // Just get us a deterministic order.
+        return std::tie(begin, end) < std::tie(other.begin, other.end);
+    }
 };
 
 inline bool operator==(const PortRange& a, const PortRange& b) {

testing/btest/Baseline/core.check-unused-event-handlers/.stderr

@@ -22,5 +22,6 @@ warning in <params>, line 1: event handler never invoked: SupervisorControl::res
 warning in <params>, line 1: event handler never invoked: SupervisorControl::status_request
 warning in <params>, line 1: event handler never invoked: SupervisorControl::stop_request
 warning in <params>, line 1: event handler never invoked: spicy_analyzer_for_mime_type
+warning in <params>, line 1: event handler never invoked: spicy_analyzer_for_port
 warning in <params>, line 1: event handler never invoked: terminate_event
 warning in <params>, line 1: event handler never invoked: this_is_never_used

testing/btest/Baseline/spicy.port-deprecated/out.stderr

@@ -1,2 +0,0 @@
-### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-[warning] <...>/udp-test.evt:4: Remove in v7.1: Analyzer spicy::TEST is using the deprecated 'port' or 'ports' keyword to register well-known ports. Use Analyzer::register_for_ports() in the accompanying Zeek script instead.

testing/btest/Baseline/spicy.port-fail/output

@@ -1,3 +1,3 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-[error] <...>/port-fail.evt:9: port outside of valid range
+[error] <...>/port-fail.evt:7: port outside of valid range
 [error] error loading EVT file "<...>/port-fail.evt"

testing/btest/Baseline/spicy.port/output

@@ -0,0 +1,19 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+Analyzer::ANALYZER_SPICY_TEST, 11337/udp
+Analyzer::ANALYZER_SPICY_TEST, 11338/udp
+Analyzer::ANALYZER_SPICY_TEST, 11339/udp
+Analyzer::ANALYZER_SPICY_TEST, 11340/udp
+Analyzer::ANALYZER_SPICY_TEST, 31337/udp
+Analyzer::ANALYZER_SPICY_TEST, 31338/udp
+Analyzer::ANALYZER_SPICY_TEST, 31339/udp
+Analyzer::ANALYZER_SPICY_TEST, 31340/udp
+{
+31339/udp,
+31337/udp,
+31338/udp,
+11339/udp,
+11338/udp,
+11340/udp,
+31340/udp,
+11337/udp
+}

testing/btest/spicy/port-deprecated.evt

@@ -1,21 +0,0 @@
-# @TEST-REQUIRES: have-spicy
-#
-# @TEST-EXEC: spicyz -d -o test.hlto ./udp-test.evt 2>out.stderr
-# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out.stderr
-#
-# @TEST-DOC: Remove with v7.1: Specifying ports is deprecated.
-
-module Test;
-
-import zeek;
-
-public type Message = unit {
-    data: bytes &eod {}
-};
-
-# @TEST-START-FILE udp-test.evt
-protocol analyzer spicy::TEST over UDP:
-    parse with Test::Message,
-    port 11337/udp-11340/udp,
-    ports {31337/udp-31340/udp};
-# @TEST-END-FILE

testing/btest/spicy/port-fail.evt

@@ -2,8 +2,6 @@
 #
 # @TEST-EXEC-FAIL: spicyz %INPUT -d -o x.hlto >output 2>&1
 # @TEST-EXEC: TEST_DIFF_CANONIFIER=diff-canonifier-spicy btest-diff output
-#
-# @TEST-DOC: Remove with v7.1
 
 protocol analyzer spicy::SSH over TCP:
     port 123456/udp;

testing/btest/spicy/port-range-one-port.zeek

@@ -5,7 +5,7 @@
 # @TEST-EXEC: grep -e 'Scheduling analyzer' -e 'error during parsing' < out > out.filtered
 # @TEST-EXEC: btest-diff out.filtered
 
-# @TEST-DOC: Remove with v7.1. Expect a single 'Scheduling analyzer ...' message in the debug output and no parsing errors. There was a bug that 'port 31336/udp' would be wrongly interpreted as a 31336/udp-31337/udp port range. Regression test for #3278.
+# @TEST-DOC: Expect a single 'Scheduling analyzer ...' message in the debug output and no parsing errors. There was a bug that 'port 31336/udp' would be wrongly interpreted as a 31336/udp-31337/udp port range. Regression test for #3278.
 
 # @TEST-START-FILE udp-test.spicy
 module UDPTest;

testing/btest/spicy/port.zeek

@@ -0,0 +1,32 @@
+# @TEST-REQUIRES: have-spicy
+#
+# @TEST-EXEC: spicyz -d -o test.hlto test.spicy test.evt
+# @TEST-EXEC: zeek test.hlto %INPUT >output
+# @TEST-EXEC: btest-diff output
+#
+# @TEST-DOC: Check that we raise port events for Spicy analyzers, and that the ports get correctly registered.
+
+event spicy_analyzer_for_port(a: Analyzer::Tag, p: port){
+    print a, p;
+}
+
+event zeek_done() {
+    print Analyzer::ports[Analyzer::ANALYZER_SPICY_TEST];
+}
+
+# @TEST-START-FILE test.spicy
+module Test;
+
+import zeek;
+
+public type Message = unit {
+    data: bytes &eod {}
+};
+# @TEST-END-FILE
+
+# @TEST-START-FILE test.evt
+protocol analyzer spicy::Test over UDP:
+    parse with Test::Message,
+    port 11337/udp-11340/udp,
+    ports {31337/udp-31340/udp};
+# @TEST-END-FILE