eve: fix pcap_filename race with --pcap-file-recursive - v2#14868
Open
eve: fix pcap_filename race with --pcap-file-recursive - v2#14868
Conversation
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.
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 OISF#5255. Regression tests: SourcePcapFileHelperTest16 (per-packet filename is independent of the global), SourcePcapFileHelperTest17 (stats fallback to global when both p and f are NULL).
|
NOTE: This PR may contain new authors. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When processing multiple pcap files with --pcap-file-recursive, the pcap_filename field in EVE JSON events could show the wrong filename. The RX thread updates a global variable as it moves to the next file, but worker threads logging events from the previous file would read the already-updated global.
Use the per-packet PcapFileFileVars reference (p->pcap_v.pfv->filename) that is set at capture time, making the filename immutable for the packet's lifetime. For flow/netflow events where no packet is available (p == NULL), the global fallback is correct as these events are emitted synchronously on the RX thread.
BUGFIX: We also fix in the first commit a bug that we did not increase the ref count in all needed cases. This raised now that we have to have it in this scenario. The first commit handles it.
Previous PR: #14857
v2:
- Avoid extending the flow struct.
Redmine ticket: https://redmine.openinfosecfoundation.org/issues/5255
SV_BRANCH=OISF/suricata-verify#2932