eve: fix pcap_filename race with --pcap-file-recursive - v7#15741
Open
oferda4 wants to merge 3 commits into
Open
eve: fix pcap_filename race with --pcap-file-recursive - v7#15741oferda4 wants to merge 3 commits into
oferda4 wants to merge 3 commits into
Conversation
With --pcap-file-recursive the RX thread advances the global pcap_filename as it moves between files, so an event emitted later for an earlier file would report the wrong name. Remember each flow's source file so such events can use it. Register a per-flow storage holding the flow's PcapFileFileVars, set (and referenced) in FlowInit and released through the storage free callback. It is registered for offline run modes only, so live-capture flows are unaffected. The reference keeps the file vars alive while the flow exists, even after the RX thread has moved on and the file would otherwise be cleaned up. Bug OISF#5255.
Flow-timeout and stream detect-log-flush pseudo packets have no capture file of their own. Set their pcap file vars from the flow so events emitted while processing them (and per-file alert counts) are attributed to the file the flow's packets came from, not the file the RX thread happens to be on. No extra reference is taken: each pseudo packet holds a flow reference and the flow holds the file reference, so it outlives the packet. Bug OISF#5255.
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. Regression tests: SourcePcapFileHelperTest18 (per-packet filename is independent of the global), SourcePcapFileHelperTest19 (stats fallback to global when both p and f are NULL). Fixes OISF#5255.
|
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: #15176
v2:
v3:
v4:
v5:
v6:
v7:
livedevpointer with auint16_t livedev_id, so the previous design (thecaptureunion reusing the livedev pointer slot, the FlowGetLiveDev()/FlowGetPcapFileVars() union accessors, the in_iface guards and the CmpFlowPacket() comment) no longer applies and has been droppedpcap_file_vars), registered for offline run modes only - so live capture is unaffected and the core Flow struct is not grown (addresses the earlier "avoid extending the flow struct" concern). The stored PcapFileFileVars is ref-counted (referenced in FlowInit, released by the storage free callback) so it outlives the source file in --pcap-file-recursive.Redmine ticket: https://redmine.openinfosecfoundation.org/issues/5255
SV_BRANCH=OISF/suricata-verify#3013