Conversation
FiveTupleSplitter::getFileNumber() deliberately allocates a new fileNumber when a fresh SYN arrives on a 5-tuple hash that's already been seen, to isolate the new logical session. But FiveTupleSplitter::getFileName() ignored the fileNumber parameter entirely, deriving the output path purely from the packet's own IP/port values -- so the new session produced the exact same path string as the old one. main.cpp's writer cache (outputFiles) is keyed by fileNumber, not by path. On collision, it treats the new fileNumber as never-seen and opens its writer fresh (no append flag) against a path an earlier, still-tracked writer object may still be using -- two independent file handles on one inode, with whichever one flushes last silently discarding the other's data (in unordered_map iteration order at program exit, since no LRU eviction of the earlier writer is required to trigger this). Fold fileNumber into the generated path for both the TCP and UDP branches, matching how every other splitter avoids this by keeping fileNumber and filename 1:1. UDP's getFileNumber() never reallocates on hash reuse today, so it isn't reachable there, but fileNumber is included for naming consistency and to stay correct if that changes. Verified against a synthetic capture with a deliberately reused 5-tuple (same 5-tuple, two SYNs 10 unrelated connections apart): unpatched, one session's data is silently discarded; patched, both land in separate, complete, valid files. Also verified no regression on a real 126,502-packet / 95-connection capture with no reused 5-tuples (95/95 files clean, all packets accounted for, before and after). Fixes seladb#2248 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2249 +/- ##
========================================
Coverage 82.89% 82.90%
========================================
Files 336 336
Lines 61062 61058 -4
Branches 12965 12669 -296
========================================
- Hits 50620 50618 -2
+ Misses 9566 9084 -482
- Partials 876 1356 +480
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@rshayer95 I found the commit when I made this change: 6379100 To be honest, I don't remember why I made this change - in a 5-tuple splitter, it makes sense to include all packets of the same 5-tuple in one file... In any case, I'm not sure your fix is the right one because it add a number to each file generated from now on. Maybe we should just revert my PR and let all packets to be written to the same file? |
@seladb Thanks for finding the original commit. Looking at it, the intent was clearly separate files per session — the comment says as much. The filename side just never got updated to match, which is where the collision comes from. Fair point on adding the number to every file — that changes output for everyone, including captures with no reuse. On reverting: it fixes the collision but merges two distinct sessions into one file with no indication they're separate. Silent correctness issue rather than a visible one. We can have a middle ground by implementing suffixes only when an actual collision occurs. The first session can keep its current filename while subsequent ones are assigned numbers. This way captures without reuse remain unchanged and important sessions stay separated where necessary. Happy to rework it that way, or revert if you had prefer simpler. |
Sure, I think that could work 👍 |
3 participants
Summary
Fixes #2248.
FiveTupleSplitter::getFileNumber()deliberately allocates a newfileNumberwhen a fresh SYN arrives on a 5-tuple hash that's already been seen, to isolate the new logical session. ButFiveTupleSplitter::getFileName()ignored thefileNumberparameter entirely, deriving the output path purely from the packet's own IP/port values — so the new session produced the exact same path string as the old one.main.cpp's writer cache (outputFiles) is keyed byfileNumber, not by path. On collision, it treats the newfileNumberas never-seen and opens its writer fresh (no append flag) against a path an earlier, still-tracked writer object may still be using — two independent file handles on one inode, with whichever one flushes last silently discarding the other's data.Fix
Fold
fileNumberinto the generated path for both the TCP and UDP branches, matching how every other splitter in this file avoids the problem by keepingfileNumberand filename 1:1. UDP'sgetFileNumber()never reallocates on hash reuse today, so the collision isn't reachable there, butfileNumberis included anyway for naming consistency and to stay correct if that ever changes.Verification
Built both an unpatched and patched copy (Alpine 3.22, g++ 14.2.0) and ran both against:
A synthetic capture with a deliberately reused 5-tuple (same 5-tuple, two SYNs 10 unrelated connections apart):
...-0001.pcap,...-0012.pcap), each complete and valid (48/48 packets accounted for across all 12 files, 0 read errors).A real 126,502-packet / 95-connection capture with no reused 5-tuples (regression check): 95/95 files clean, all packets accounted for, identical before and after the patch.
Notes
This is one of two independent resource-ceiling issues found while investigating the corruption reports in #2248 (the other being fd-exhaustion in the underlying pcapng writer on very high connection counts, unrelated to this code path). This PR addresses the file-collision mechanism specifically, which is the one with a deterministic, reproducible root cause and fix.