From 15c8bc98e37bf851775cc43d32a7fc25e9b38b51 Mon Sep 17 00:00:00 2001 From: Ofer Dagan Date: Thu, 19 Feb 2026 16:40:03 +0100 Subject: [PATCH 1/2] flow: fix ref_cnt leak and use-after-free Reuse the existing f->livedev slot as an anonymous union with f->pcap_file_vars (pcap-file and live-capture modes are mutually exclusive, so the struct does not grow). Set p->livedev = pfv in PcapFileCallbackLoop so FlowInit's existing f->livedev = p->livedev assignment propagates pfv into the union automatically. Keep pfv alive (via PcapFileRef) for the lifetime of the flow in FlowInit and release it (PcapFileUnref) in FlowClearMemory, fixing the ref_cnt leak and preventing use-after-free when a flow outlives its source file in recursive mode. Add FlowGetPcapFileVars() and FlowGetLiveDev() typed accessors that centralise the IsRunModeOffline() run-mode discriminant, so no call site can misinterpret the union value. Guard the in_iface EVE field in JsonFlowLogger and JsonNetFlowLogger with FlowGetLiveDev() to prevent a bogus interface name in pcap-file mode. Add FlowTest13/14 to verify each accessor hides the union value when the run mode does not match. --- src/flow-timeout.c | 12 ++ src/flow-util.c | 22 ++++ src/flow-util.h | 18 +++ src/flow.c | 210 +++++++++++++++++++++++++++++++++- src/flow.h | 11 +- src/output-json-flow.c | 6 +- src/output-json-netflow.c | 6 +- src/source-pcap-file-helper.c | 131 ++++++++++++++++++++- src/source-pcap-file-helper.h | 6 + 9 files changed, 412 insertions(+), 10 deletions(-) diff --git a/src/flow-timeout.c b/src/flow-timeout.c index 35aef3d5dc5e..ae7401835ca7 100644 --- a/src/flow-timeout.c +++ b/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 diff --git a/src/flow-util.c b/src/flow-util.c index 9a77a41a4a19..987b5201c2e9 100644 --- a/src/flow-util.c +++ b/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) diff --git a/src/flow-util.h b/src/flow-util.h index 11bddefc8527..7a191ff48e65 100644 --- a/src/flow-util.h +++ b/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_ { diff --git a/src/flow.c b/src/flow.c index 6d81fa18d3c5..50369023e6dd 100644 --- a/src/flow.c +++ b/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 */ } diff --git a/src/flow.h b/src/flow.h index 772b85de62fb..f0135b2207c5 100644 --- a/src/flow.h +++ b/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 */ diff --git a/src/output-json-flow.c b/src/output-json-flow.c index 2bd5d0129b1f..7be589cf4b08 100644 --- a/src/output-json-flow.c +++ b/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"); diff --git a/src/output-json-netflow.c b/src/output-json-netflow.c index 6a0d1d2e60a0..6175d8af210c 100644 --- a/src/output-json-netflow.c +++ b/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"); diff --git a/src/source-pcap-file-helper.c b/src/source-pcap-file-helper.c index e88db6d51547..03354b93edba 100644 --- a/src/source-pcap-file-helper.c +++ b/src/source-pcap-file-helper.c @@ -131,6 +131,7 @@ void PcapFileCallbackLoop(char *user, struct pcap_pkthdr *h, u_char *pkt) p->pcap_v.tenant_id = ptv->shared->tenant_id; p->pcap_v.pfv = ptv; + p->livedev = (struct LiveDevice_ *)ptv; ptv->shared->pkts++; ptv->shared->bytes += h->caplen; @@ -305,6 +306,9 @@ TmEcode InitPcapFile(PcapFileFileVars *pfv) pfv->cleanup_requested = false; + /* Cache delete_mode so deferred cleanup can read it after shared is freed. */ + pfv->delete_mode = (pfv->shared != NULL) ? pfv->shared->delete_mode : PCAP_FILE_DELETE_NONE; + pfv->datalink = pcap_datalink(pfv->pcap_handle); SCLogDebug("datalink %" PRId32 "", pfv->datalink); DatalinkSetGlobalType(pfv->datalink); @@ -357,15 +361,18 @@ TmEcode ValidateLinkType(int datalink, DecoderFunc *DecoderFn) bool PcapFileShouldDeletePcapFile(PcapFileFileVars *pfv) { - if (pfv == NULL || pfv->shared == NULL) { + if (pfv == NULL) { return false; } - if (pfv->shared->delete_mode == PCAP_FILE_DELETE_NONE) { + /* Use the cached delete_mode: pfv->shared may already be freed when this + * is called from the deferred-cleanup path (after ReceivePcapFileThreadDeinit + * frees ptv which contains shared). */ + if (pfv->delete_mode == PCAP_FILE_DELETE_NONE) { return false; } - if (pfv->shared->delete_mode == PCAP_FILE_DELETE_ALWAYS) { + if (pfv->delete_mode == PCAP_FILE_DELETE_ALWAYS) { return true; } @@ -457,6 +464,23 @@ PcapFileDeleteMode PcapFileParseDeleteMode(void) return delete_mode; } +void PcapFileRef(PcapFileFileVars *pfv) +{ + if (pfv != NULL) { + SC_ATOMIC_ADD(pfv->ref_cnt, 1); + } +} + +void PcapFileUnref(PcapFileFileVars *pfv) +{ + if (pfv != NULL) { + uint32_t prev = SC_ATOMIC_SUB(pfv->ref_cnt, 1); + if (prev == 1 && pfv->cleanup_requested) { + CleanupPcapFileFileVars(pfv); + } + } +} + #ifdef UNITTESTS #include "util-unittest-helper.h" /** @@ -472,6 +496,7 @@ static int SourcePcapFileHelperTest01(void) PcapFileFileVars pfv; memset(&pfv, 0, sizeof(pfv)); pfv.shared = &shared; + pfv.delete_mode = shared.delete_mode; /* cache, as InitPcapFile would */ pfv.filename = SCStrdup("test.pcap"); SC_ATOMIC_INIT(pfv.alerts_count); SC_ATOMIC_SET(pfv.alerts_count, 0); @@ -482,6 +507,7 @@ static int SourcePcapFileHelperTest01(void) /* Test case 2: Non-alerts mode with no alerts */ shared.delete_mode = PCAP_FILE_DELETE_NON_ALERTS; + pfv.delete_mode = PCAP_FILE_DELETE_NON_ALERTS; int result2 = PcapFileShouldDeletePcapFile(&pfv); FAIL_IF_NOT(result2); @@ -492,11 +518,13 @@ static int SourcePcapFileHelperTest01(void) /* Test case 4: Always delete mode with alerts */ shared.delete_mode = PCAP_FILE_DELETE_ALWAYS; + pfv.delete_mode = PCAP_FILE_DELETE_ALWAYS; int result4 = PcapFileShouldDeletePcapFile(&pfv); FAIL_IF_NOT(result4); /* Test case 5: No delete mode */ shared.delete_mode = PCAP_FILE_DELETE_NONE; + pfv.delete_mode = PCAP_FILE_DELETE_NONE; int result5 = PcapFileShouldDeletePcapFile(&pfv); FAIL_IF(result5); @@ -687,6 +715,7 @@ static int SourcePcapFileHelperTest07(void) PcapFileFileVars pfv; memset(&pfv, 0, sizeof(pfv)); pfv.shared = &shared; + pfv.delete_mode = PCAP_FILE_DELETE_NON_ALERTS; /* as InitPcapFile would cache */ pfv.filename = SCStrdup("test.pcap"); SC_ATOMIC_INIT(pfv.alerts_count); SC_ATOMIC_SET(pfv.alerts_count, UINT64_MAX); /* max value */ @@ -772,6 +801,7 @@ static int SourcePcapFileHelperTest09(void) PcapFileFileVars *pfv = SCCalloc(1, sizeof(*pfv)); FAIL_IF_NULL(pfv); pfv->shared = shared; + pfv->delete_mode = PCAP_FILE_DELETE_NON_ALERTS; /* as InitPcapFile would cache */ pfv->filename = SCStrdup("unit_del_test.pcap"); FAIL_IF_NULL(pfv->filename); @@ -819,6 +849,7 @@ static int SourcePcapFileHelperTest10(void) PcapFileFileVars *pfv1 = SCCalloc(1, sizeof(*pfv1)); FAIL_IF_NULL(pfv1); pfv1->shared = shared1; + pfv1->delete_mode = PCAP_FILE_DELETE_ALWAYS; /* as InitPcapFile would cache */ pfv1->filename = SCStrdup(tmpname); FAIL_IF_NULL(pfv1->filename); @@ -851,6 +882,7 @@ static int SourcePcapFileHelperTest10(void) PcapFileFileVars *pfv2 = SCCalloc(1, sizeof(*pfv2)); FAIL_IF_NULL(pfv2); pfv2->shared = shared2; + pfv2->delete_mode = PCAP_FILE_DELETE_ALWAYS; /* as InitPcapFile would cache */ pfv2->filename = SCStrdup(tmpname); FAIL_IF_NULL(pfv2->filename); @@ -1016,6 +1048,97 @@ static int SourcePcapFileHelperTest15(void) PASS; } +/** + * \test Verify that PcapFileShouldDeletePcapFile uses the cached pfv->delete_mode + * and does not touch pfv->shared, which may be NULL (freed) in the deferred + * cleanup path (Bug 2 regression test). + */ +static int SourcePcapFileHelperTest18(void) +{ + /* ALWAYS: shared freed (NULL) but delete_mode cached -> must delete */ + PcapFileFileVars pfv; + memset(&pfv, 0, sizeof(pfv)); + pfv.delete_mode = PCAP_FILE_DELETE_ALWAYS; + pfv.shared = NULL; /* simulates ptv freed by ReceivePcapFileThreadDeinit */ + SC_ATOMIC_INIT(pfv.alerts_count); + SC_ATOMIC_SET(pfv.alerts_count, 0); + + FAIL_IF_NOT(PcapFileShouldDeletePcapFile(&pfv)); + + /* NON_ALERTS, no alerts: shared freed -> must delete */ + pfv.delete_mode = PCAP_FILE_DELETE_NON_ALERTS; + FAIL_IF_NOT(PcapFileShouldDeletePcapFile(&pfv)); + + /* NON_ALERTS, with alerts: shared freed -> must NOT delete */ + SC_ATOMIC_ADD(pfv.alerts_count, 1); + FAIL_IF(PcapFileShouldDeletePcapFile(&pfv)); + + /* NONE: shared freed -> must NOT delete */ + pfv.delete_mode = PCAP_FILE_DELETE_NONE; + SC_ATOMIC_SET(pfv.alerts_count, 0); + FAIL_IF(PcapFileShouldDeletePcapFile(&pfv)); + + PASS; +} + +/** + * \test Verify the full deferred-cleanup-with-freed-shared scenario (Bug 2). + * + * Timeline mimicked: + * 1. pfv set up, delete_mode cached as NON_ALERTS, no alerts. + * 2. CleanupPcapFileFileVars called while ref_cnt > 0 -> deferred. + * 3. The "shared" struct is freed (ReceivePcapFileThreadDeinit frees ptv). + * 4. Last reference released (PcapFileUnref from FlowClearMemory) -> + * final cleanup fires using only pfv->delete_mode -> file deleted. + */ +static int SourcePcapFileHelperTest19(void) +{ + const char *tmpname = "suri_ut_deferred_no_shared.pcap"; + const uint8_t dummy[] = { 0x00 }; + int rc = TestHelperBufferToFile(tmpname, dummy, sizeof(dummy)); + FAIL_IF_NOT(rc >= 0); + + /* Allocate shared separately so we can free it independently of pfv. */ + PcapFileSharedVars *shared = SCCalloc(1, sizeof(*shared)); + FAIL_IF_NULL(shared); + shared->delete_mode = PCAP_FILE_DELETE_NON_ALERTS; + + PcapFileFileVars *pfv = SCCalloc(1, sizeof(*pfv)); + FAIL_IF_NULL(pfv); + pfv->shared = shared; + pfv->delete_mode = PCAP_FILE_DELETE_NON_ALERTS; /* cached as InitPcapFile does */ + pfv->filename = SCStrdup(tmpname); + FAIL_IF_NULL(pfv->filename); + pfv->pcap_handle = pcap_open_dead(DLT_EN10MB, 65535); + FAIL_IF_NULL(pfv->pcap_handle); + SC_ATOMIC_INIT(pfv->alerts_count); + SC_ATOMIC_SET(pfv->alerts_count, 0); + SC_ATOMIC_INIT(pfv->ref_cnt); + SC_ATOMIC_SET(pfv->ref_cnt, 1); /* flow holds a reference */ + pfv->cleanup_requested = false; + + /* Step 2: thread deinit triggers cleanup while flow ref is still active. */ + CleanupPcapFileFileVars(pfv); + FAIL_IF_NOT(pfv->cleanup_requested); /* must be deferred */ + + /* Step 3: simulate ptv freed – pfv->shared is now a dangling pointer. + * We free it and NULL it out; the fix must not read pfv->shared below. */ + SCFree(shared); + pfv->shared = NULL; + + /* Step 4: flow releases its reference -> triggers final cleanup. + * PcapFileShouldDeletePcapFile must use pfv->delete_mode, not pfv->shared. */ + PcapFileUnref(pfv); /* prev==1, cleanup_requested==true -> CleanupPcapFileFileVars */ + + /* File must be deleted (NON_ALERTS mode, zero alerts). */ + FILE *f = fopen(tmpname, "rb"); + FAIL_IF_NOT_NULL(f); + if (f != NULL) + fclose(f); + + PASS; +} + /** * \brief Register unit tests for pcap file helper */ @@ -1036,5 +1159,7 @@ void SourcePcapFileHelperRegisterTests(void) UtRegisterTest("SourcePcapFileHelperTest13", SourcePcapFileHelperTest13); UtRegisterTest("SourcePcapFileHelperTest14", SourcePcapFileHelperTest14); UtRegisterTest("SourcePcapFileHelperTest15", SourcePcapFileHelperTest15); + UtRegisterTest("SourcePcapFileHelperTest18", SourcePcapFileHelperTest18); + UtRegisterTest("SourcePcapFileHelperTest19", SourcePcapFileHelperTest19); } #endif /* UNITTESTS */ diff --git a/src/source-pcap-file-helper.h b/src/source-pcap-file-helper.h index 7d59d4fc7505..904205a6be79 100644 --- a/src/source-pcap-file-helper.h +++ b/src/source-pcap-file-helper.h @@ -82,6 +82,9 @@ typedef struct PcapFileFileVars_ struct bpf_program filter; PcapFileSharedVars *shared; + /** Cached copy of shared->delete_mode, valid even after shared is freed + * (deferred cleanup path). Set in InitPcapFile. */ + PcapFileDeleteMode delete_mode; SC_ATOMIC_DECLARE(uint64_t, alerts_count); /* Reference count for outstanding users (e.g., pseudo packets created @@ -151,6 +154,9 @@ PcapFileFileVars *PcapFileGetCurrentPfv(void); void PcapFileInstallCaptureHooks(void); +void PcapFileRef(PcapFileFileVars *pfv); +void PcapFileUnref(PcapFileFileVars *pfv); + #ifdef UNITTESTS void SourcePcapFileHelperRegisterTests(void); #endif From 795aa9b7717ced7eb3a852ee1427620453cabab0 Mon Sep 17 00:00:00 2001 From: Ofer Dagan Date: Thu, 19 Feb 2026 16:43:27 +0100 Subject: [PATCH 2/2] eve: fix pcap_filename race with --pcap-file-recursive When running with --pcap-file-recursive, EVE output was using the global pcap_filename string to populate the "pcap_filename" field. This string is updated by the RX thread as it moves from file to file, so by the time a worker thread emits a log entry the global may already point to the next file, recording the wrong filename. Fix: read the filename from p->pcap_v.pfv->filename instead, which is set per-packet by the RX thread and is stable for the lifetime of the packet. The previous commit stores a pfv reference in the Flow so that flow-timeout pseudo packets also carry the correct filename. For stream pseudo-packets (PKT_SRC_STREAM_TCP_DETECTLOG_FLUSH), also propagate p->pcap_v.pfv from the flow so that http/fileinfo events emitted during stream flushing report the correct per-flow filename. For events where p == NULL (flow and netflow records), use the pfv stored in the flow (FlowGetPcapFileVars) so that flow events report the file the flow's packets came from, not the last file the RX thread processed. Fall back to PcapFileGetFilename() only when f is also NULL (e.g. stats events). A BUG_ON guards against p != NULL with a NULL pfv, which would indicate a missing pfv assignment in the capture path. Fixes #5255. Regression tests: SourcePcapFileHelperTest16 (per-packet filename is independent of the global), SourcePcapFileHelperTest17 (stats fallback to global when both p and f are NULL). --- src/output-json.c | 18 ++++++++++- src/source-pcap-file-helper.c | 59 +++++++++++++++++++++++++++++++++++ src/stream-tcp.c | 1 + 3 files changed, 77 insertions(+), 1 deletion(-) diff --git a/src/output-json.c b/src/output-json.c index 1b7464e35ee4..8a3d95c81fa5 100644 --- a/src/output-json.c +++ b/src/output-json.c @@ -60,6 +60,7 @@ #include "flow-var.h" #include "flow-bit.h" #include "flow-storage.h" +#include "flow-util.h" #include "source-pcap-file-helper.h" @@ -1007,7 +1008,22 @@ void OutputJsonBuilderBuffer( } if (file_ctx->is_pcap_offline) { - SCJbSetString(js, "pcap_filename", PcapFileGetFilename()); + /* Use the per-packet filename to avoid a race where the RX thread + * has already moved to the next pcap file while workers still + * process packets from the previous one. + * Flow/netflow events pass p == NULL; use the pfv stored in the flow + * so they report the correct file even after the RX thread has moved + * on to the next file. Fall back to the global only when f is also + * NULL (e.g. stats events). */ + const char *filename; + if (p != NULL) { + BUG_ON(p->pcap_v.pfv == NULL); + filename = p->pcap_v.pfv->filename; + } else { + PcapFileFileVars *pfv = (f != NULL) ? FlowGetPcapFileVars(f) : NULL; + filename = pfv ? pfv->filename : PcapFileGetFilename(); + } + SCJbSetString(js, "pcap_filename", filename); } SCEveRunCallbacks(tv, p, f, js); diff --git a/src/source-pcap-file-helper.c b/src/source-pcap-file-helper.c index 03354b93edba..785a01922fda 100644 --- a/src/source-pcap-file-helper.c +++ b/src/source-pcap-file-helper.c @@ -1048,6 +1048,63 @@ static int SourcePcapFileHelperTest15(void) PASS; } +/** + * \test Verify that per-packet pfv->filename is independent of the global + * pcap_filename -- simulates the race condition from ticket #5255. + */ +static int SourcePcapFileHelperTest16(void) +{ + /* Set global to "file_B.pcap" (as if RX thread moved on) */ + extern char pcap_filename[PATH_MAX]; + strlcpy(pcap_filename, "file_B.pcap", sizeof(pcap_filename)); + + /* Create a pfv for "file_A.pcap" (the file this packet actually belongs to) */ + PcapFileFileVars *pfv = SCCalloc(1, sizeof(*pfv)); + FAIL_IF_NULL(pfv); + pfv->filename = SCStrdup("file_A.pcap"); + FAIL_IF_NULL(pfv->filename); + SC_ATOMIC_INIT(pfv->ref_cnt); + SC_ATOMIC_SET(pfv->ref_cnt, 0); + SC_ATOMIC_INIT(pfv->alerts_count); + SC_ATOMIC_SET(pfv->alerts_count, 0); + + Packet *p = PacketGetFromAlloc(); + FAIL_IF_NULL(p); + p->pcap_v.pfv = pfv; + + /* Per-packet filename must be "file_A.pcap" even though global says B */ + FAIL_IF_NOT(strcmp(p->pcap_v.pfv->filename, "file_A.pcap") == 0); + FAIL_IF_NOT(strcmp(PcapFileGetFilename(), "file_B.pcap") == 0); + + /* Cleanup */ + PacketFreeOrRelease(p); + CleanupPcapFileFileVars(pfv); + strlcpy(pcap_filename, "unknown", sizeof(pcap_filename)); + PASS; +} + +/** + * \test Verify the global fallback: when both p == NULL and f == NULL + * (e.g. stats events), PcapFileGetFilename() is used. + */ +static int SourcePcapFileHelperTest17(void) +{ + extern char pcap_filename[PATH_MAX]; + strlcpy(pcap_filename, "current_file.pcap", sizeof(pcap_filename)); + + /* With no packet and no flow (stats events), the global is the + * correct source of truth. */ + FAIL_IF_NOT(strcmp(PcapFileGetFilename(), "current_file.pcap") == 0); + + /* Verify the global tracks file changes correctly */ + strlcpy(pcap_filename, "next_file.pcap", sizeof(pcap_filename)); + FAIL_IF_NOT(strcmp(PcapFileGetFilename(), "next_file.pcap") == 0); + + /* Cleanup */ + strlcpy(pcap_filename, "unknown", sizeof(pcap_filename)); + PASS; +} + /** * \test Verify that PcapFileShouldDeletePcapFile uses the cached pfv->delete_mode * and does not touch pfv->shared, which may be NULL (freed) in the deferred @@ -1159,6 +1216,8 @@ void SourcePcapFileHelperRegisterTests(void) UtRegisterTest("SourcePcapFileHelperTest13", SourcePcapFileHelperTest13); UtRegisterTest("SourcePcapFileHelperTest14", SourcePcapFileHelperTest14); UtRegisterTest("SourcePcapFileHelperTest15", SourcePcapFileHelperTest15); + UtRegisterTest("SourcePcapFileHelperTest16", SourcePcapFileHelperTest16); + UtRegisterTest("SourcePcapFileHelperTest17", SourcePcapFileHelperTest17); UtRegisterTest("SourcePcapFileHelperTest18", SourcePcapFileHelperTest18); UtRegisterTest("SourcePcapFileHelperTest19", SourcePcapFileHelperTest19); } diff --git a/src/stream-tcp.c b/src/stream-tcp.c index 299e071d0fb1..5535aa7b0ef5 100644 --- a/src/stream-tcp.c +++ b/src/stream-tcp.c @@ -6936,6 +6936,7 @@ static void StreamTcpPseudoPacketCreateDetectLogFlush(ThreadVars *tv, memcpy(&np->vlan_id[0], &f->vlan_id[0], sizeof(np->vlan_id)); np->vlan_idx = f->vlan_idx; np->livedev = (struct LiveDevice_ *)f->livedev; + np->pcap_v.pfv = FlowGetPcapFileVars(f); if (parent->flags & PKT_NOPACKET_INSPECTION) { DecodeSetNoPacketInspectionFlag(np);