More fixes

src/Conn.cc

@@ -29,7 +29,8 @@ uint64_t Connection::total_connections = 0;
 uint64_t Connection::current_connections = 0;
 
 Connection::Connection(IPBasedConnKeyPtr k, zeek::ConnTuple& ct, double t, uint32_t flow, const Packet* pkt)
-    : Session(t, connection_timeout, connection_status_update, detail::connection_status_update_interval), key(k) {
+    : Session(t, connection_timeout, connection_status_update, detail::connection_status_update_interval),
+      key(std::move(k)) {
     orig_addr = ct.src_addr;
     resp_addr = ct.dst_addr;
     orig_port = ct.src_port;

src/Conn.h

@@ -54,7 +54,7 @@ enum ConnEventToFlag {
 };
 
 class IPBasedConnKey;
-using IPBasedConnKeyPtr = zeek::IntrusivePtr<IPBasedConnKey>;
+using IPBasedConnKeyPtr = std::unique_ptr<IPBasedConnKey>;
 
 static inline int addr_port_canon_lt(const IPAddr& addr1, uint32_t p1, const IPAddr& addr2, uint32_t p2) {
     return addr1 < addr2 || (addr1 == addr2 && p1 < p2);
@@ -64,7 +64,6 @@ static inline int addr_port_canon_lt(const IPAddr& addr1, uint32_t p1, const IPA
 class Connection final : public session::Session {
 public:
     Connection(zeek::IPBasedConnKeyPtr k, zeek::ConnTuple& ct, double t, uint32_t flow, const Packet* pkt);
-    // Connection(const detail::ConnKey& k, double t, const ConnTuple* id, uint32_t flow, const Packet* pkt);
     ~Connection() override;
 
     /**

src/analyzer/Analyzer.cc

@@ -814,9 +814,9 @@ TEST_SUITE("Analyzer management") {
 
         zeek::Packet p;
         zeek::ConnTuple ct;
-        zeek::IPBasedConnKeyPtr kp = zeek::make_intrusive<zeek::IPConnKey>();
+        zeek::IPBasedConnKeyPtr kp = std::make_unique<zeek::IPConnKey>();
         // auto conn = std::make_unique<zeek::Connection>(zeek::detail::OLD_ConnKey(t), 0, &t, 0, &p);
-        auto conn = std::make_unique<zeek::Connection>(kp, ct, 0, 0, &p);
+        auto conn = std::make_unique<zeek::Connection>(std::move(kp), ct, 0, 0, &p);
         auto* tcp = new zeek::packet_analysis::TCP::TCPSessionAdapter(conn.get());
         conn->SetSessionAdapter(tcp, nullptr);
 
@@ -848,8 +848,8 @@ TEST_SUITE("Analyzer management") {
 
         zeek::Packet p;
         zeek::ConnTuple ct;
-        zeek::IPBasedConnKeyPtr kp = zeek::make_intrusive<zeek::IPConnKey>();
+        zeek::IPBasedConnKeyPtr kp = std::make_unique<zeek::IPConnKey>();
-        auto conn = std::make_unique<zeek::Connection>(kp, ct, 0, 0, &p);
+        auto conn = std::make_unique<zeek::Connection>(std::move(kp), ct, 0, 0, &p);
 
         auto ssh = zeek::analyzer_mgr->InstantiateAnalyzer("SSH", conn.get());
         REQUIRE(ssh);

src/analyzer/protocol/smtp/BDAT.cc

@@ -329,8 +329,8 @@ private:
 TEST_CASE("line forward testing") {
     zeek::Packet p;
     zeek::ConnTuple ct;
-    zeek::IPBasedConnKeyPtr kp = zeek::make_intrusive<zeek::IPConnKey>();
+    zeek::IPBasedConnKeyPtr kp = std::make_unique<zeek::IPConnKey>();
-    auto conn = std::make_unique<zeek::Connection>(kp, ct, 0, 0, &p);
+    auto conn = std::make_unique<zeek::Connection>(std::move(kp), ct, 0, 0, &p);
     auto smtp_analyzer =
         std::unique_ptr<zeek::analyzer::Analyzer>(zeek::analyzer_mgr->InstantiateAnalyzer("SMTP", conn.get()));
     auto mail = std::make_unique<Test_MIME_Message>(smtp_analyzer.get());

src/conntuple/Builder.cc

@@ -2,6 +2,7 @@
 
 #include "zeek/conntuple/Builder.h"
 
+#include "zeek/Val.h"
 #include "zeek/packet_analysis/protocol/ip/IPBasedAnalyzer.h"
 
 namespace zeek::conntuple {
@@ -9,6 +10,81 @@ namespace zeek::conntuple {
 Builder::Builder() {}
 Builder::~Builder() {}
 
-zeek::ConnKeyPtr Builder::NewConnKey() { return zeek::make_intrusive<zeek::IPConnKey>(); }
+bool fill_from_val(const IPBasedConnKey* ck, const zeek::ValPtr& v) {
+    auto& t = ck->RawTuple();
+
+    const auto& vt = v->GetType();
+    if ( ! IsRecord(vt->Tag()) ) {
+        t.transport = detail::INVALID_CONN_KEY_IP_PROTO;
+        assert(ck->Error().has_value());
+        return false;
+    }
+
+    RecordType* vr = vt->AsRecordType();
+    auto vl = v->As<RecordVal*>();
+
+    int orig_h, orig_p; // indices into record's value list
+    int resp_h, resp_p;
+    int proto;
+
+    if ( vr == id::conn_id ) {
+        orig_h = 0;
+        orig_p = 1;
+        resp_h = 2;
+        resp_p = 3;
+        proto = 4;
+    }
+    else {
+        // While it's not a conn_id, it may have equivalent fields.
+        orig_h = vr->FieldOffset("orig_h");
+        resp_h = vr->FieldOffset("resp_h");
+        orig_p = vr->FieldOffset("orig_p");
+        resp_p = vr->FieldOffset("resp_p");
+        proto = vr->FieldOffset("proto");
+
+        if ( orig_h < 0 || resp_h < 0 || orig_p < 0 || resp_p < 0 || proto < 0 ) {
+            t.transport = detail::INVALID_CONN_KEY_IP_PROTO;
+            assert(ck->Error().has_value());
+            return false;
+        }
+
+        // TODO we ought to check that the fields have the right
+        // types, too.
+    }
+
+    if ( ! vl->HasField(orig_h) || ! vl->HasField(resp_h) || ! vl->HasField(orig_p) || ! vl->HasField(resp_p) ) {
+        t.transport = detail::INVALID_CONN_KEY_IP_PROTO;
+        assert(ck->Error().has_value());
+        return false;
+    }
+
+    const IPAddr& orig_addr = vl->GetFieldAs<AddrVal>(orig_h);
+    const IPAddr& resp_addr = vl->GetFieldAs<AddrVal>(resp_h);
+
+    const auto& orig_portv = vl->GetFieldAs<PortVal>(orig_p);
+    const auto& resp_portv = vl->GetFieldAs<PortVal>(resp_p);
+
+    const auto& protov = vl->GetField<CountVal>(proto);
+
+    auto ct = ConnTuple{orig_addr, resp_addr, ntohs(orig_portv->Port()), ntohs(resp_portv->Port()),
+                        static_cast<uint16_t>(protov->AsCount())};
+
+    detail::init_raw_tuple(t, ct);
+    assert(! ck->Error().has_value());
+    return true;
+}
+
+zeek::ConnKeyPtr Builder::NewConnKey() { return std::make_unique<zeek::IPConnKey>(); }
+
+// Creating a ConnKey instance from a ValPtr, assuming conn_id.
+zeek::ConnKeyPtr Builder::FromVal(const zeek::ValPtr& v) {
+    auto ck = NewConnKey();
+    auto* k = static_cast<zeek::IPBasedConnKey*>(ck.get());
+    if ( ! fill_from_val(k, v) ) {
+        assert(ck->Error().has_value());
+    }
+
+    return ck;
+}
 
 } // namespace zeek::conntuple

src/conntuple/Builder.h

@@ -14,12 +14,19 @@ namespace conntuple {
 class Builder;
 using BuilderPtr = std::unique_ptr<Builder>;
 
+/**
+ * Fill an IPBasedConnKey from a Zeek script value.
+ */
+bool fill_from_val(const IPBasedConnKey* k, const zeek::ValPtr& v);
+
 class Builder {
 public:
     Builder();
     virtual ~Builder();
 
+    // TODO: Should these better be abstract?
     virtual zeek::ConnKeyPtr NewConnKey();
+    virtual zeek::ConnKeyPtr FromVal(const zeek::ValPtr& v);
 
     static zeek::conntuple::BuilderPtr Instantiate() { return std::make_unique<Builder>(); }
 };

src/conntuple/vlan/Builder.cc

@@ -2,7 +2,10 @@
 
 #include "zeek/conntuple/vlan/Builder.h"
 
+#include <memory>
+
 #include "zeek/ID.h"
+#include "zeek/conntuple/Builder.h"
 #include "zeek/packet_analysis/protocol/ip/IPBasedAnalyzer.h"
 #include "zeek/session/Session.h"
 
@@ -20,18 +23,11 @@ public:
         key.inner_vlan = pkt.inner_vlan;
     }
 
-    bool FromConnIdVal(const zeek::RecordValPtr& rv) override {
-        if ( ! zeek::IPBasedConnKey::FromConnIdVal(rv) )
-            return false;
-
-        // TODO: Also load vlan and inner_vlan from the record!
-    }
-
     zeek::Span<const std::byte> Key() const override {
         return {reinterpret_cast<const std::byte*>(&key), reinterpret_cast<const std::byte*>(&key) + sizeof(key)};
     }
 
-    detail::RawConnTuple& RawTuple() override { return key.tuple; }
+    detail::RawConnTuple& RawTuple() const override { return key.tuple; }
 
     virtual void FillConnIdVal(RecordValPtr& conn_id) override {
         if ( conn_id->NumFields() <= 5 )
@@ -47,14 +43,41 @@ public:
     };
 
 private:
+    friend class Builder;
+
     // Key bytes.
     struct {
-        struct detail::RawConnTuple tuple;
+        // mutable for non-const RawTuple() return value.
+        mutable struct detail::RawConnTuple tuple;
         // Add 802.1Q vlan tags to connection tuples. The tag representation here is as
         // in the Packet class, since that's where we learn the tag values from.
         uint32_t vlan;
         uint32_t inner_vlan;
-    } key;
+    } __attribute__((packed, aligned)) key;
 };
 
+zeek::ConnKeyPtr Builder::NewConnKey() { return std::make_unique<IPVlanConnKey>(); }
+
+zeek::ConnKeyPtr Builder::FromVal(const zeek::ValPtr& v) {
+    auto ck = NewConnKey();
+    auto* k = static_cast<IPVlanConnKey*>(ck.get());
+    if ( ! zeek::conntuple::fill_from_val(k, v) ) {
+        assert(ck->Error().has_value());
+        return ck;
+    }
+
+    auto rt = v->GetType()->AsRecordType();
+    auto vl = v->As<RecordVal*>();
+
+    // XXX: Use static field offsets
+    if ( rt->GetFieldType(5)->Tag() == TYPE_INT && vl->HasField(5) )
+        k->key.vlan = vl->GetFieldAs<zeek::IntVal>(5);
+
+    if ( rt->GetFieldType(6)->Tag() == TYPE_INT && vl->HasField(6) )
+        k->key.inner_vlan = vl->GetFieldAs<zeek::IntVal>(6);
+
+
+    return ck;
+}
+
 } // namespace zeek::plugin::Zeek_Conntuple_VLAN

src/conntuple/vlan/Builder.h

@@ -8,6 +8,7 @@ namespace zeek::plugin::Zeek_Conntuple_VLAN {
 class Builder : public conntuple::Builder {
 public:
     virtual zeek::ConnKeyPtr NewConnKey() override;
+    virtual zeek::ConnKeyPtr FromVal(const zeek::ValPtr& v) override;
 };
 
 } // namespace zeek::plugin::Zeek_Conntuple_VLAN

src/packet_analysis/protocol/gtpv1/GTPv1.cc

@@ -22,12 +22,12 @@ bool GTPv1_Analyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pack
     const auto& key = conn->Key();
     auto sk = key.SessionKey();
 
-
     auto cm_it = conn_map.find(sk);
     if ( cm_it == conn_map.end() ) {
-        sk.CopyData();
+        sk.CopyData(); // Copy key data to store in map.
-        ConnMap::value_type p{std::move(sk), std::make_unique<binpac::GTPv1::GTPv1_Conn>(this)};
+        auto [it, inserted] = conn_map.emplace(std::move(sk), std::make_unique<binpac::GTPv1::GTPv1_Conn>(this));
-        // cm_it = conn_map.insert(cm_it, std::move(p));
+        assert(inserted);
+        cm_it = it;
 
         // Let script land know about the state we created, so it will
         // register a conn removal hook for cleanup.

src/packet_analysis/protocol/gtpv1/functions.bif

@@ -13,8 +13,8 @@ function remove_gtpv1_connection%(cid: conn_id%) : bool
 	zeek::packet_analysis::AnalyzerPtr gtpv1 = zeek::packet_mgr->GetAnalyzer("GTPv1");
 	if ( gtpv1 )
 		{
-		auto ck = zeek::conntuple_mgr->GetBuilder().NewConnKey();
+		// XXX: Should be IP specific builder!
-		// ck->LoadConnIdVal(v);
+		auto ck = zeek::conntuple_mgr->GetBuilder().FromVal({zeek::NewRef{}, cid});
 		auto sk = ck->SessionKey();
 		static_cast<zeek::packet_analysis::gtpv1::GTPv1_Analyzer*>(gtpv1.get())->RemoveConnection(sk);
 		}

src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc

@@ -2,6 +2,8 @@
 
 #include "zeek/packet_analysis/protocol/ip/IPBasedAnalyzer.h"
 
+#include <memory>
+
 #include "zeek/Conn.h"
 #include "zeek/RunState.h"
 #include "zeek/Val.h"
@@ -14,65 +16,6 @@
 using namespace zeek;
 using namespace zeek::packet_analysis::IP;
 
-// Populate a ConnKey from a conn_id record instance.
-bool IPBasedConnKey::FromConnIdVal(const zeek::RecordValPtr& rv) {
-    auto& t = RawTuple();
-    const auto& vt = rv->GetType();
-    if ( ! IsRecord(vt->Tag()) ) {
-        t.transport = detail::INVALID_CONN_KEY_IP_PROTO;
-        return false;
-    }
-
-    RecordType* vr = vt->AsRecordType();
-    auto vl = rv->As<RecordVal*>();
-
-    int orig_h, orig_p; // indices into record's value list
-    int resp_h, resp_p;
-    int proto;
-
-    if ( vr == id::conn_id ) {
-        orig_h = 0;
-        orig_p = 1;
-        resp_h = 2;
-        resp_p = 3;
-        proto = 4;
-    }
-    else {
-        // While it's not a conn_id, it may have equivalent fields.
-        orig_h = vr->FieldOffset("orig_h");
-        resp_h = vr->FieldOffset("resp_h");
-        orig_p = vr->FieldOffset("orig_p");
-        resp_p = vr->FieldOffset("resp_p");
-        proto = vr->FieldOffset("proto");
-
-        if ( orig_h < 0 || resp_h < 0 || orig_p < 0 || resp_p < 0 || proto < 0 ) {
-            t.transport = detail::INVALID_CONN_KEY_IP_PROTO;
-            return false;
-        }
-
-        // TODO we ought to check that the fields have the right
-        // types, too.
-    }
-
-    if ( ! vl->HasField(orig_h) || ! vl->HasField(resp_h) || ! vl->HasField(orig_p) || ! vl->HasField(resp_p) ) {
-        t.transport = detail::INVALID_CONN_KEY_IP_PROTO;
-        return false;
-    }
-
-    const IPAddr& orig_addr = vl->GetFieldAs<AddrVal>(orig_h);
-    const IPAddr& resp_addr = vl->GetFieldAs<AddrVal>(resp_h);
-
-    const auto& orig_portv = vl->GetFieldAs<PortVal>(orig_p);
-    const auto& resp_portv = vl->GetFieldAs<PortVal>(resp_p);
-
-    const auto& protov = vl->GetField<CountVal>(proto);
-
-    auto ct = ConnTuple{orig_addr, resp_addr, orig_portv->Port(), resp_portv->Port(),
-                        static_cast<uint16_t>(protov->AsCount())};
-    InitRawConnTuple(ct);
-    return true;
-}
-
 IPBasedAnalyzer::IPBasedAnalyzer(const char* name, TransportProto proto, uint32_t mask, bool report_unknown_protocols)
     : zeek::packet_analysis::Analyzer(name, report_unknown_protocols), transport(proto), server_port_mask(mask) {}
 
@@ -87,17 +30,21 @@ bool IPBasedAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pkt
     if ( ! BuildConnTuple(len, data, pkt, tuple) )
         return false;
 
-    // The IPBasedAnalyzer requires a builder that produces IPConnKey instances.
+    static IPBasedConnKeyPtr key; // Watch: This is static for reuse!
-    // We could check with dynamic_cast, but that's probably slow, so assume
+    if ( ! key ) {
-    // plugin providers know what they're doing here and anyhow, we don't really
+        ConnKeyPtr ck = conntuple_mgr->GetBuilder().NewConnKey();
-    // have analyzers that instantiate non-IP connections today and definitely
+
-    // not here!
+        // The IPBasedAnalyzer requires a builder that produces IPBasedConnKey instances.
-    auto key = cast_intrusive<IPConnKey>(zeek::conntuple_mgr->GetBuilder().NewConnKey());
+        // We could check with dynamic_cast, but that's probably slow, so assume plugin
+        // providers know what they're doing here and anyhow, we don't really have analyzers
+        // that instantiate non-IP connections today and definitely not here!
+        key = std::unique_ptr<IPBasedConnKey>(static_cast<IPBasedConnKey*>(ck.release()));
+    }
 
     // Initialize the key with the IP conn tuple and the packet as additional context.
     //
     // Custom IPConnKey implementations can fiddle with the Key through
-    // the DoInit(const Packet& pkt) hook at this point.
+    // the DoInit(const Packet& pkt) hook called at this point.
     key->Init(tuple, *pkt);
 
     const std::shared_ptr<IP_Hdr>& ip_hdr = pkt->ip_hdr;
@@ -108,6 +55,8 @@ bool IPBasedAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pkt
         conn = NewConn(std::move(key), tuple, pkt);
         if ( conn )
             session_mgr->Insert(conn, false);
+
+        assert(! key);
     }
     else {
         if ( conn->IsReuse(run_state::processing_start_time, ip_hdr->Payload()) ) {
@@ -117,16 +66,14 @@ bool IPBasedAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pkt
             conn = NewConn(std::move(key), tuple, pkt);
             if ( conn )
                 session_mgr->Insert(conn, false);
+
+            assert(! key);
         }
         else {
             conn->CheckEncapsulation(pkt->encap);
 
-            // We could give back the ConnKey for re-use to avoid the malloc/memset()
+            // The next time we get here, the key instance
-            // overhead if we already knew about the session and don't had a use for
+            assert(key);
-            // the key other than facilitating the lookup of the session. Not sure
-            // that's worth it.
-            //
-            // GetBuilder().ReturnKey(std::move(key));
         }
     }
 

src/packet_analysis/protocol/ip/IPBasedAnalyzer.h

@@ -4,6 +4,7 @@
 
 #include <netinet/in.h>
 #include <map>
+#include <optional>
 #include <set>
 
 #include "zeek/Conn.h"
@@ -30,13 +31,36 @@ namespace detail {
 // UNKNOWN_IP_PROTO is 65535
 constexpr uint16_t INVALID_CONN_KEY_IP_PROTO = 65534;
 
+/**
+ * Struct for embedding into a IPBasedConnKey.
+ */
 struct RawConnTuple {
     in6_addr ip1;
     in6_addr ip2;
     uint16_t port1 = 0;
     uint16_t port2 = 0;
     uint16_t transport = detail::INVALID_CONN_KEY_IP_PROTO;
-};
+} __attribute__((packed, aligned));
+
+/**
+ * Initialize a raw conn tuple from a conn tuple in canonicalized form.
+ */
+inline void init_raw_tuple(RawConnTuple& t, const ConnTuple& ct) {
+    if ( ct.is_one_way || addr_port_canon_lt(ct.src_addr, ct.src_port, ct.dst_addr, ct.dst_port) ) {
+        ct.src_addr.CopyIPv6(&t.ip1);
+        ct.dst_addr.CopyIPv6(&t.ip2);
+        t.port1 = ct.src_port;
+        t.port2 = ct.dst_port;
+    }
+    else {
+        ct.dst_addr.CopyIPv6(&t.ip1);
+        ct.src_addr.CopyIPv6(&t.ip2);
+        t.port1 = ct.dst_port;
+        t.port2 = ct.src_port;
+    }
+
+    t.transport = ct.proto;
+}
 
 } // namespace detail
 
@@ -56,44 +80,31 @@ public:
      * subclasses to hook into the initialization of the key.
      */
     void Init(const ConnTuple& ct, const Packet& pkt) {
-        InitRawConnTuple(ct);
+        init_raw_tuple(RawTuple(), ct);
         ConnKey::Init(pkt);
     }
 
-    bool FromConnIdVal(const zeek::RecordValPtr& conn_id) override;
+    std::optional<std::string> Error() const override {
+        auto& rt = RawTuple();
+        if ( rt.transport == detail::INVALID_CONN_KEY_IP_PROTO )
+            return "invalid connection ID record";
+        if ( rt.transport == UNKNOWN_IP_PROTO )
+            return "invalid connection ID record: the proto field has the \"unknown\" 65535 value. Did you forget to "
+                   "set it?";
+
+        return std::nullopt;
+    }
 
     /**
-     * Return a modifiable version of the raw Conn Tuple.
+     * Return a modifiable version of the embedded RawConnTuple.
      *
      * This is virtual such that subclasses can control where
-     * their own ConnTuple instance to allow.
+     * they'd like to place the RawConnTuple within the key.
      */
-    virtual detail::RawConnTuple& RawTuple() = 0;
+    virtual detail::RawConnTuple& RawTuple() const = 0;
-
-protected:
-    /**
-     * Helper for subclasses that override Init() to initialize this key's tuple in normalized form.
-     */
-    void InitRawConnTuple(const ConnTuple& ct) {
-        auto& t = RawTuple();
-        if ( ct.is_one_way || addr_port_canon_lt(ct.src_addr, ct.src_port, ct.dst_addr, ct.dst_port) ) {
-            ct.src_addr.CopyIPv6(&t.ip1);
-            ct.dst_addr.CopyIPv6(&t.ip2);
-            t.port1 = ct.src_port;
-            t.port2 = ct.dst_port;
-        }
-        else {
-            ct.dst_addr.CopyIPv6(&t.ip1);
-            ct.src_addr.CopyIPv6(&t.ip2);
-            t.port1 = ct.dst_port;
-            t.port2 = ct.src_port;
-        }
-
-        t.transport = ct.proto;
-    }
 };
 
-using IPBasedConnKeyPtr = zeek::IntrusivePtr<IPBasedConnKey>;
+using IPBasedConnKeyPtr = std::unique_ptr<IPBasedConnKey>;
 
 
 /**
@@ -103,7 +114,6 @@ class IPConnKey : public IPBasedConnKey {
 public:
     IPConnKey() {
         // Fill holes as we use the full tuple as a Key!
-        // Could we h
         memset(static_cast<void*>(&key), '\0', sizeof(key));
     }
 
@@ -111,11 +121,12 @@ public:
         return {reinterpret_cast<const std::byte*>(&key), reinterpret_cast<const std::byte*>(&key) + sizeof(key)};
     }
 
-    detail::RawConnTuple& RawTuple() override { return key.tuple; }
+    detail::RawConnTuple& RawTuple() const override { return key.tuple; }
 
 private:
     struct {
-        struct detail::RawConnTuple tuple;
+        // mutable for non-const RawTuple() return value.
+        mutable struct detail::RawConnTuple tuple;
     } key;
 };
 

src/packet_analysis/protocol/teredo/Teredo.cc

@@ -185,10 +185,8 @@ bool TeredoAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pack
         return false;
     }
 
-    auto& k = conn->Key();
+    const auto& k = conn->Key();
     auto sk = k.SessionKey();
-
-    // zeek::detail::OLD_ConnKey conn_key = conn->Key();
     OrigRespMap::iterator or_it = orig_resp_map.find(sk);
 
     // The first time a teredo packet is parsed successfully, insert
@@ -196,9 +194,10 @@ bool TeredoAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pack
     // see valid Teredo packets. Further, raise an event so that script
     // layer can install a connection removal hooks to cleanup later.
     if ( or_it == orig_resp_map.end() ) {
-        sk.CopyData();
+        sk.CopyData(); // Copy key data to store in map.
-        // OrigRespMap::value_type p{std::move(sk), OrigResp{}};
+        auto [it, inserted] = orig_resp_map.emplace(std::move(sk), OrigResp{});
-        auto [it, inserted] = orig_resp_map.insert_or_assign(std::move(sk), OrigResp{});
+        assert(inserted);
+        or_it = it;
 
         packet->session->EnqueueEvent(new_teredo_state, nullptr, packet->session->GetVal());
     }

src/packet_analysis/protocol/teredo/Teredo.h

@@ -54,7 +54,7 @@ protected:
         bool valid_resp = false;
         bool confirmed = false;
     };
-    using OrigRespMap = std::unordered_map<zeek::session::detail::Key, OrigResp, zeek::session::detail::KeyHash>;
+    using OrigRespMap = std::map<zeek::session::detail::Key, OrigResp>;
     OrigRespMap orig_resp_map;
 
     std::unique_ptr<zeek::detail::Specific_RE_Matcher> pattern_re;

src/packet_analysis/protocol/teredo/functions.bif

@@ -13,8 +13,8 @@ function remove_teredo_connection%(cid: conn_id%) : bool
 	zeek::packet_analysis::AnalyzerPtr teredo = zeek::packet_mgr->GetAnalyzer("Teredo");
 	if ( teredo )
 		{
-		auto ck = zeek::conntuple_mgr->GetBuilder().NewConnKey();
+		// XXX: Should be IP specific builder!
-		// ck->LoadConnIdVal(v);
+		auto ck = zeek::conntuple_mgr->GetBuilder().FromVal({zeek::NewRef{}, cid});
 		auto sk = ck->SessionKey();
 		static_cast<zeek::packet_analysis::teredo::TeredoAnalyzer*>(teredo.get())->RemoveConnection(sk);
 		}

src/session/Key.h

@@ -4,6 +4,7 @@
 
 #include <cstddef>
 #include <cstdint>
+#include <optional>
 
 #include "zeek/Hash.h"
 #include "zeek/IntrusivePtr.h"
@@ -100,7 +101,14 @@ class ConnKey {
 public:
     virtual ~ConnKey() {}
 
-    // Init function called by analyzers or subclassed.
+    /**
+     * ConnKeys created from Vals may be invalid, Error() can be used to determine validity.
+     */
+    virtual std::optional<std::string> Error() const = 0;
+
+    /**
+     * Initialization of this key with the current packet.
+     */
     void Init(const Packet& pkt) { DoInit(pkt); }
 
     /**
@@ -117,11 +125,6 @@ public:
      */
     virtual void FillConnIdVal(RecordValPtr& conn_id) {};
 
-    /**
-     * Populate a ConnKey instance from a script layer record value.
-     */
-    virtual bool FromConnIdVal(const RecordValPtr& conn_id) = 0;
-
     /**
      * They Key over which to compute a hash or use for comparison with other keys.
      *
@@ -130,28 +133,16 @@ public:
     virtual zeek::Span<const std::byte> Key() const = 0;
 
     /**
-     * View over the key data as a SessionKey.
+     * View over key data as returned by Key() as session::detail::Key instance.
      *
-     * Not sure this makes much sense, it might also be possible to switch
+     * Mostly for plumbing the session/Manager.h code.
-     * the session_map to hold ConnKeyPtr instead with specialized hash and
-     * equals functions instead.
      */
     zeek::session::detail::Key SessionKey() const {
         auto span = Key();
         return zeek::session::detail::Key(span.data(), span.size(), session::detail::Key::CONNECTION_KEY_TYPE);
     }
-
-    // Support usage as IntrusivePtr.
-    int ref_cnt = 1;
 };
 
-inline void Ref(ConnKey* k) { ++k->ref_cnt; }
+using ConnKeyPtr = std::unique_ptr<ConnKey>;
-
-inline void Unref(ConnKey* k) {
-    if ( --k->ref_cnt == 0 )
-        delete k;
-}
-
-using ConnKeyPtr = zeek::IntrusivePtr<ConnKey>;
 
 } // namespace zeek

src/session/Manager.cc

@@ -91,31 +91,21 @@ Manager::~Manager() {
 void Manager::Done() {}
 
 Connection* Manager::FindConnection(Val* v) {
-    // zeek::detail::OLD_ConnKeyPtr conn_key = conntuple_mgr->GetBuilder().GetKey(v);
+    // XXX: Technically we should probably dispatch between different builders for different kinds of Val instances.
+    // conn_id is IP specific, so if ``v`` is something else, maybe we'd like to use a different builder.
+    auto conn_key = conntuple_mgr->GetBuilder().FromVal({zeek::NewRef{}, v});
 
-    auto conn_key = conntuple_mgr->GetBuilder().NewConnKey();
+    if ( conn_key->Error() ) {
-    /**
-    // conn_key->LoadConnIdVal(v);
-
-    This is IP specific stuff!
-
-    if ( ! conn_key->Valid() ) {
         // Produce a loud error for invalid script-layer conn_id records.
-        const char* extra = "";
+        zeek::emit_builtin_error(conn_key->Error()->c_str());
-        if ( conn_key->transport == UNKNOWN_IP_PROTO )
-            extra = ": the proto field has the \"unknown\" 65535 value. Did you forget to set it?";
-
-        zeek::emit_builtin_error(zeek::util::fmt("invalid connection ID record encountered%s", extra));
         return nullptr;
     }
 
-    */
-
     return FindConnection(*conn_key);
 }
 
 Connection* Manager::FindConnection(const zeek::ConnKey& conn_key) {
-    detail::Key key{conn_key.SessionKey()};
+    auto key = conn_key.SessionKey();
 
     auto it = session_map.find(key);
     if ( it != session_map.end() )

src/session/Manager.h

@@ -93,7 +93,7 @@ public:
     size_t CurrentSessions() { return session_map.size(); }
 
 private:
-    using SessionMap = std::unordered_map<zeek::session::detail::Key, Session*, detail::KeyHash>;
+    using SessionMap = std::unordered_map<detail::Key, Session*, detail::KeyHash>;
 
     // Inserts a new connection into the sessions map. If a connection with
     // the same key already exists in the map, it will be overwritten by