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/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 e88db6d51547..785a01922fda 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,154 @@ 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 + * 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 +1216,9 @@ void SourcePcapFileHelperRegisterTests(void) UtRegisterTest("SourcePcapFileHelperTest13", SourcePcapFileHelperTest13); UtRegisterTest("SourcePcapFileHelperTest14", SourcePcapFileHelperTest14); UtRegisterTest("SourcePcapFileHelperTest15", SourcePcapFileHelperTest15); + UtRegisterTest("SourcePcapFileHelperTest16", SourcePcapFileHelperTest16); + UtRegisterTest("SourcePcapFileHelperTest17", SourcePcapFileHelperTest17); + 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 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);