eve: fix pcap_filename race with --pcap-file-recursive - v2

src/flow-timeout.c

@@ -42,6 +42,7 @@
 #include "flow-timeout.h"
 #include "pkt-var.h"
 #include "host.h"
+#include "source-pcap-file-helper.h"
 
 #include "stream-tcp-private.h"
 #include "stream-tcp-reassemble.h"
@@ -98,6 +99,17 @@ static inline Packet *FlowPseudoPacketSetup(
         DecodeSetNoPayloadInspectionFlag(p);
     }
 
+    PcapFileFileVars *pfv = FlowGetPcapFileVars(f);
+    if (pfv != NULL) {
+        /* Assign pfv for filename tracking. No extra PcapFileRef here: the
+         * flow already holds a reference (from FlowInit) that keeps pfv alive
+         * for the entire duration of FlowFinish, which completes before
+         * FlowClearMemory releases the flow's reference.  Using
+         * PacketPoolReturnPacket directly (as FlowFinish does) would skip
+         * p->ReleasePacket anyway, so adding a ref here just leaks ref_cnt. */
+        p->pcap_v.pfv = pfv;
+    }
+
     if (direction == 0)
         p->flowflags |= FLOW_PKT_TOSERVER;
     else

src/flow-util.c

@@ -24,6 +24,7 @@
  */
 
 #include "suricata-common.h"
+#include "suricata.h"
 #include "threads.h"
 
 #include "flow.h"
@@ -45,6 +46,8 @@
 #include "decode-icmpv4.h"
 
 #include "util-validate.h"
+#include "source-pcap-file-helper.h"
+#include "runmodes.h"
 
 /** \brief allocate a flow
  *
@@ -214,9 +217,28 @@ void FlowInit(ThreadVars *tv, Flow *f, const Packet *p)
 
     SCFlowRunInitCallbacks(tv, f, p);
 
+    PcapFileFileVars *pfv = FlowGetPcapFileVars(f);
+    if (pfv != NULL) {
+        PcapFileRef(pfv);
+    }
+
     SCReturn;
 }
 
+struct PcapFileFileVars_ *FlowGetPcapFileVars(const Flow *f)
+{
+    if (IsRunModeOffline(SCRunmodeGet()))
+        return f->pcap_file_vars;
+    return NULL;
+}
+
+struct LiveDevice_ *FlowGetLiveDev(const Flow *f)
+{
+    if (!IsRunModeOffline(SCRunmodeGet()))
+        return f->livedev;
+    return NULL;
+}
+
 FlowStorageId g_bypass_info_id = { .id = -1 };
 
 FlowStorageId GetFlowBypassInfoID(void)

src/flow-util.h

@@ -141,6 +141,24 @@ uint8_t FlowGetProtoMapping(uint8_t);
 void FlowInit(ThreadVars *, Flow *, const Packet *);
 uint8_t FlowGetReverseProtoMapping(uint8_t rproto);
 
+/**
+ * \brief Return the pcap file vars for a flow, or NULL.
+ *
+ * Returns f->pcap_file_vars only when running in an offline (pcap-file) run
+ * mode.  In live-capture modes the union slot holds a LiveDevice pointer, so
+ * callers must never interpret it as a PcapFileFileVars without this guard.
+ */
+struct PcapFileFileVars_ *FlowGetPcapFileVars(const Flow *f);
+
+/**
+ * \brief Return the live device for a flow, or NULL.
+ *
+ * Returns f->livedev only in live-capture run modes.  In offline (pcap-file)
+ * mode the same union slot holds a PcapFileFileVars pointer, so callers must
+ * never interpret it as a LiveDevice without this guard.
+ */
+struct LiveDevice_ *FlowGetLiveDev(const Flow *f);
+
 /* flow end counter logic */
 
 typedef struct FlowEndCounters_ {

src/flow.c

@@ -46,6 +46,9 @@
 #include "flow-bypass.h"
 #include "flow-spare-pool.h"
 #include "flow-callbacks.h"
+#include "source-pcap-file-helper.h"
+#include "flow-timeout.h"
+#include "tmqh-packetpool.h"
 
 #include "stream-tcp-private.h"
 
@@ -1131,6 +1134,12 @@ int FlowClearMemory(Flow* f, uint8_t proto_map)
         flow_freefuncs[proto_map].Freefunc(f->protoctx);
     }
 
+    PcapFileFileVars *pfv = FlowGetPcapFileVars(f);
+    if (pfv != NULL) {
+        PcapFileUnref(pfv);
+        /* f->livedev (union) is cleared by FLOW_RECYCLE below */
+    }
+
     FlowFreeStorage(f);
 
     FLOW_RECYCLE(f);
@@ -1461,6 +1470,201 @@ static int FlowTest09 (void)
     return result;
 }
 
+/**
+ * \test Verify that pcap_file_vars remains NULL when a non-pcap
+ *       packet creates a flow (no pfv on the packet).
+ */
+static int FlowTest10(void)
+{
+    FlowInitConfig(FLOW_QUIET);
+
+    Packet *p = UTHBuildPacket((uint8_t *)"a", 1, IPPROTO_TCP);
+    FAIL_IF_NULL(p);
+
+    Flow *f = FlowAlloc();
+    FAIL_IF_NULL(f);
+
+    ThreadVars tv;
+    memset(&tv, 0, sizeof(tv));
+    FlowInit(&tv, f, p);
+
+    /* pfv must stay NULL for non-pcap packets */
+    FAIL_IF(FlowGetPcapFileVars(f) != NULL);
+
+    FlowClearMemory(f, f->protomap);
+    FlowFree(f);
+    UTHFreePacket(p);
+    FlowShutdown();
+    PASS;
+}
+
+/**
+ * \test Verify that FlowInit increments the pfv ref_cnt and
+ *       FlowClearMemory decrements it correctly.
+ */
+static int FlowTest11(void)
+{
+    FlowInitConfig(FLOW_QUIET);
+
+    PcapFileFileVars pfv;
+    memset(&pfv, 0, sizeof(pfv));
+    SC_ATOMIC_INIT(pfv.ref_cnt);
+    SC_ATOMIC_SET(pfv.ref_cnt, 1);
+    pfv.cleanup_requested = false;
+
+    Packet *p = UTHBuildPacket((uint8_t *)"a", 1, IPPROTO_TCP);
+    FAIL_IF_NULL(p);
+    p->pcap_v.pfv = &pfv;
+    /* livedev carries pfv so FlowInit sets f->pcap_file_vars via the union */
+    p->livedev = (struct LiveDevice_ *)&pfv;
+
+    Flow *f = FlowAlloc();
+    FAIL_IF_NULL(f);
+
+    /* offline mode so FlowInit and FlowClearMemory use the pcap_file_vars path */
+    SCRunMode saved_mode = SCRunmodeGet();
+    SCRunmodeSet(RUNMODE_PCAP_FILE);
+
+    ThreadVars tv;
+    memset(&tv, 0, sizeof(tv));
+    FlowInit(&tv, f, p);
+
+    /* FlowInit should have incremented ref_cnt from 1 to 2 */
+    FAIL_IF_NOT(SC_ATOMIC_GET(pfv.ref_cnt) == 2);
+    FAIL_IF_NOT(FlowGetPcapFileVars(f) == &pfv);
+
+    /* FlowClearMemory should decrement ref_cnt from 2 to 1 */
+    FlowClearMemory(f, f->protomap);
+    FlowFree(f);
+
+    FAIL_IF_NOT(SC_ATOMIC_GET(pfv.ref_cnt) == 1);
+
+    SCRunmodeSet(saved_mode);
+    p->pcap_v.pfv = NULL;
+    p->livedev = NULL;
+    UTHFreePacket(p);
+    FlowShutdown();
+    PASS;
+}
+
+/**
+ * \test Verify that FlowPseudoPacketGet assigns pfv to the pseudo-packet
+ *       without adding an extra ref_cnt (the flow's existing reference is
+ *       sufficient to keep pfv alive for the packet's inline lifetime).
+ */
+static int FlowTest12(void)
+{
+    FlowInitConfig(FLOW_QUIET);
+    PacketPoolInit();
+
+    PcapFileFileVars pfv;
+    memset(&pfv, 0, sizeof(pfv));
+    SC_ATOMIC_INIT(pfv.ref_cnt);
+    SC_ATOMIC_SET(pfv.ref_cnt, 1);
+    pfv.cleanup_requested = false;
+
+    Flow *f = FlowAlloc();
+    FAIL_IF_NULL(f);
+    f->flags |= FLOW_IPV4; // Ensure flow is IPv4 to avoid crash in FlowPseudoPacketSetup
+    f->src.addr_data32[0] = 0x01020304;
+    f->dst.addr_data32[0] = 0x05060708;
+
+    f->pcap_file_vars = &pfv;
+    PcapFileRef(&pfv); /* simulate flow's own ref; ref_cnt now 2 */
+
+    /* offline mode so FlowPseudoPacketSetup sets p->pcap_v.pfv and
+     * FlowClearMemory calls PcapFileUnref */
+    SCRunMode saved_mode = SCRunmodeGet();
+    SCRunmodeSet(RUNMODE_PCAP_FILE);
+
+    TcpSession ssn;
+    memset(&ssn, 0, sizeof(ssn));
+
+    Packet *p = FlowPseudoPacketGet(0, f, &ssn);
+    FAIL_IF_NULL(p);
+
+    /* FlowPseudoPacketGet must set pfv but must NOT add an extra ref:
+     * the flow already holds one, and FlowFinish uses PacketPoolReturnPacket
+     * which never calls p->ReleasePacket, so adding a ref here would leak. */
+    FAIL_IF_NOT(SC_ATOMIC_GET(pfv.ref_cnt) == 2);
+    FAIL_IF_NOT(p->pcap_v.pfv == &pfv);
+
+    /* Return packet to pool (as FlowFinish does); ref_cnt must stay at 2. */
+    PacketPoolReturnPacket(p);
+
+    FAIL_IF_NOT(SC_ATOMIC_GET(pfv.ref_cnt) == 2);
+
+    FlowClearMemory(f, f->protomap);
+    FlowFree(f);
+
+    SCRunmodeSet(saved_mode);
+    PacketPoolDestroy();
+    FlowShutdown();
+    PASS;
+}
+
+/**
+ * \test FlowGetPcapFileVars returns NULL in live mode even when the union
+ *       field is non-NULL, and returns the real pointer in offline mode.
+ */
+static int FlowTest13(void)
+{
+    FlowInitConfig(FLOW_QUIET);
+
+    PcapFileFileVars pfv;
+    memset(&pfv, 0, sizeof(pfv));
+    SC_ATOMIC_INIT(pfv.ref_cnt);
+
+    Flow *f = FlowAlloc();
+    FAIL_IF_NULL(f);
+    f->pcap_file_vars = &pfv;
+
+    /* RUNMODE_UNITTEST is not offline: accessor must hide the union value */
+    FAIL_IF_NOT(FlowGetPcapFileVars(f) == NULL);
+
+    /* Switch to offline: accessor must expose it */
+    SCRunMode saved = SCRunmodeGet();
+    SCRunmodeSet(RUNMODE_PCAP_FILE);
+    FAIL_IF_NOT(FlowGetPcapFileVars(f) == &pfv);
+    SCRunmodeSet(saved);
+
+    f->pcap_file_vars = NULL;
+    FlowFree(f);
+    FlowShutdown();
+    PASS;
+}
+
+/**
+ * \test FlowGetLiveDev returns the livedev in live mode and NULL in offline
+ *       mode, even when the union field is non-NULL.
+ */
+static int FlowTest14(void)
+{
+    FlowInitConfig(FLOW_QUIET);
+
+    /* Use a PcapFileFileVars as a stand-in to avoid needing a real LiveDevice */
+    PcapFileFileVars dummy;
+    memset(&dummy, 0, sizeof(dummy));
+
+    Flow *f = FlowAlloc();
+    FAIL_IF_NULL(f);
+    f->livedev = (struct LiveDevice_ *)&dummy;
+
+    /* RUNMODE_UNITTEST is not offline: accessor must return livedev */
+    FAIL_IF_NOT(FlowGetLiveDev(f) == (struct LiveDevice_ *)&dummy);
+
+    /* Switch to offline: accessor must hide livedev (it's actually a pfv) */
+    SCRunMode saved = SCRunmodeGet();
+    SCRunmodeSet(RUNMODE_PCAP_FILE);
+    FAIL_IF_NOT(FlowGetLiveDev(f) == NULL);
+    SCRunmodeSet(saved);
+
+    f->livedev = NULL;
+    FlowFree(f);
+    FlowShutdown();
+    PASS;
+}
+
 #endif /* UNITTESTS */
 
 /**
@@ -1478,7 +1682,11 @@ void FlowRegisterTests (void)
                    FlowTest08);
     UtRegisterTest("FlowTest09 -- Test flow Allocations when it reach memcap",
                    FlowTest09);
-
+    UtRegisterTest("FlowTest10 -- pcap_file_vars NULL on non-pcap packet", FlowTest10);
+    UtRegisterTest("FlowTest11 -- pcap_file_vars refcounting", FlowTest11);
+    UtRegisterTest("FlowTest12 -- pcap_file_vars pseudo packet refcounting", FlowTest12);
+    UtRegisterTest("FlowTest13 -- FlowGetPcapFileVars run-mode guard", FlowTest13);
+    UtRegisterTest("FlowTest14 -- FlowGetLiveDev run-mode guard", FlowTest14);
     RegisterFlowStorageTests();
 #endif /* UNITTESTS */
 }

src/flow.h

@@ -343,6 +343,8 @@ typedef uint16_t FlowThreadId;
  *  of a flow. This is why we can access those without protection of the lock.
  */
 
+struct PcapFileFileVars_;
+
 typedef struct Flow_
 {
     /* flow "header", used for hashing and flow lookup. Static after init,
@@ -388,8 +390,13 @@ typedef struct Flow_
      *  the flow_id. */
     uint32_t flow_hash;
 
-    /** Incoming interface */
-    struct LiveDevice_ *livedev;
+    /** Incoming interface (live) or source pcap file vars (pcap-file-recursive).
+     *  These run modes are mutually exclusive; use FlowGetLiveDev() /
+     *  FlowGetPcapFileVars() rather than accessing the union directly. */
+    union {
+        struct LiveDevice_ *livedev;
+        struct PcapFileFileVars_ *pcap_file_vars;
+    };
 
     struct Flow_ *next; /* (hash) list next */
 

src/output-json-flow.c

@@ -52,6 +52,7 @@
 #include "stream-tcp-private.h"
 #include "flow-storage.h"
 #include "util-exception-policy.h"
+#include "flow-util.h"
 
 static SCJsonBuilder *CreateEveHeaderFromFlow(const Flow *f)
 {
@@ -102,8 +103,9 @@ static SCJsonBuilder *CreateEveHeaderFromFlow(const Flow *f)
 #endif
 
     /* input interface */
-    if (f->livedev) {
-        SCJbSetString(jb, "in_iface", f->livedev->dev);
+    struct LiveDevice_ *ldev = FlowGetLiveDev(f);
+    if (ldev != NULL) {
+        SCJbSetString(jb, "in_iface", ldev->dev);
     }
 
     JB_SET_STRING(jb, "event_type", "flow");

src/output-json-netflow.c

@@ -49,6 +49,7 @@
 #include "output-json-netflow.h"
 
 #include "stream-tcp-private.h"
+#include "flow-util.h"
 
 static SCJsonBuilder *CreateEveHeaderFromNetFlow(const Flow *f, int dir)
 {
@@ -105,8 +106,9 @@ static SCJsonBuilder *CreateEveHeaderFromNetFlow(const Flow *f, int dir)
 #endif
 
     /* input interface */
-    if (f->livedev) {
-        SCJbSetString(js, "in_iface", f->livedev->dev);
+    struct LiveDevice_ *ldev = FlowGetLiveDev(f);
+    if (ldev != NULL) {
+        SCJbSetString(js, "in_iface", ldev->dev);
     }
 
     JB_SET_STRING(js, "event_type", "netflow");