# The tool said it wrote 48 packets. Only 44 were there

## Recap

I have been developing a tool wrapped around tshark. The [first blog post](https://robinhayer.hashnode.dev/the-2-5-gb-wall) was about hitting a wall on a 2.5 GB file. Later, I talked about parallelizing the PCAP processing in my [second blog post](https://robinhayer.hashnode.dev/concurrency-without-a-parallel-parser) where I ran into a file corruption bug.

Now here comes the real culprit: the PcapSplitter's `FiveTupleSplitter` caused file truncation/corruption on TCP session reuse (i.e., a new `SYN` packet arrives for an already tracked 5-tuple hash).

## How I found it

[Post two](https://robinhayer.hashnode.dev/concurrency-without-a-parallel-parser) ended with me replacing PcapSplitter. *This is what came before that replacement.*

The initial signal came from "**Total Block Length**" errors thrown by tshark on some output files. That told me something was wrong, not what.

The actual signal came when I built a small reproduction. PcapSplitter reported 12 files and 48 packets, but on disk, there were 11 files and 44 packets. Exit code zero and printed "Finished" on standard output.

## Theory One: File Descriptor Exhaustion, wrong

Someone on Reddit suggested file descriptor exhaustion. It was plausible with one output file per flow, where I had 95-125 flows, and a failed `open()` with an unchecked return would produce silent drops.

But instead of relying on theory, I tested it. Then I found that at low `ulimit -n` it silently drops most packets and still exits zero. Sharp threshold, reproducible. It wasn't my corruption. Raising the limit didn't fix the original problem.

## Theory Two: Hardcoded LRU Limit, again wrong

After testing file descriptor exhaustion, I found that the PcapSplitter library has a hardcoded `MAX_NUMBER_OF_CONCURRENT_OPEN_FILES = 250` with an LRU that closes and reopens handles past it. It was a perfect candidate.

I tested it too. It held up fine. My minimal reproduction was 13 connections, nowhere near the cap.

Two good theories, one found a different real bug, the other held up fine. Neither was mine.

## What it was exactly

The correct part of the splitter was assigning a new file number when a TCP session reuses a 5-tuple, but the filename function builds the name from IP and port only. So, in this case, both sessions get the same filenames. Then `main.cpp` sees a file number it has never seen, and opens that file fresh, without append. This truncates the existing file or causes a race condition between two active file writer handlers.

### 1\. File Number Allocation on Session Reuse (`ConnectionSplitters.h`)

When a fresh `SYN` arrives for a 5-tuple hash that has been seen before, `FiveTupleSplitter::getFileNumber` deliberately assigns a **new file number** to isolate the logical session:

```cpp
if (isSyn && m_TcpFlowTable.find(hash) != m_TcpFlowTable.end() && m_TcpFlowTable[hash] == false)
{
    m_FlowTable[hash] = getNextFileNumber(filesToClose);   // <-- Allocates a NEW file number
}
```

### 2\. The Filename Disconnect (`ConnectionSplitters.h`)

Right below this logic, `FiveTupleSplitter::getFileName` overrides the base class implementation but completely ignores the `fileNumber` parameter passed into it:

```cpp
std::string getFileName(pcpp::Packet& packet, const std::string& outputPcapBasePath, int fileNumber)
{
    // ...
    sstream << "connection-";
    if (packet.isPacketOfType(pcpp::TCP)) {
        // ...
        // Filename is derived strictly from the packet's IP/Port values:
        updateStringStream(sstream, getSrcIPString(packet), srcPort, getDstIPString(packet), dstPort);
        return outputPcapBasePath + sstream.str();   // <-- fileNumber parameter is UNUSED on the TCP/UDP path
    }
    // ...
}
```

Because `fileNumber` is dropped, the new session produces the **identical filename string** as the older session.

### 3\. The Truncation / Race Condition (`main.cpp`)

In `main.cpp`, the active writer cache map (`outputFiles`) is keyed by the integer `fileNum`, not the filepath string. When the newly allocated `fileNum` is evaluated, it goes down the fresh initialization branch:

```cpp
// Since fileNum is a newly generated integer, it won't be found in the map
if (outputFiles.find(fileNum) == outputFiles.end()) 
{
    std::string fileName = splitter->getFileName(parsedPacket, outputPcapFileName, fileNum) + outputFileExtension;

    if (isReaderPcapng) {
        outputFiles[fileNum].reset(new pcpp::PcapNgFileWriterDevice(fileName));
    } else {
        outputFiles[fileNum].reset(new pcpp::PcapFileWriterDevice(fileName, rawPacket.getLinkLayerType()));
    }

    // CRITICAL: Plain open — no append flag — on a path that already holds valid session data!
    if (!outputFiles[fileNum]->open()) 
        break;
}
```

### Impact & Consequences

1.  **File Truncation:** If the previous file representing the same session was closed by the LRU mechanism, the new file writer triggers a standard file open on the existing path, that truncates the existing PCAP data captured from the previous session.
    
2.  **Write Race / Corruption:** If duplicate files representing the same session are still open and active, it creates a race condition between two active file writer handlers. This breaks the linear layout of the PCAP/PCAPNG format and leaves the file corrupted.
    

## Reporting the bug and fixing upstream

I filed the issue with a 13-connection reproduction in [#2248](https://github.com/seladb/PcapPlusPlus/issues/2248).

The maintainer found his own commit [6379100](https://github.com/seladb/PcapPlusPlus/commit/63791006db94bc07aeb88f55a649d80dc121cbc2) and said he couldn't remember why he'd made the change. He pushed back on my first fix, where I suggested appending the number to every filename. But it changes output for everyone, which is why he pushed back. Fair point.

Then I proposed a middle ground where we only suffix on actual collision, first session keeps its name. He accepted it, asked for tests. I wrote the code and tests.

Merged [#2249](https://github.com/seladb/PcapPlusPlus/pull/2249)

![The maintainer approving and merging the fix, with a thank-you comment.](https://cdn.hashnode.com/uploads/covers/6a792e914054dda7adcb15a7/36acfe09-c421-442f-88bd-06c6fdfc49eb.png align="center")

## Takeaways

1.  Exit code zero is not always success. Verify what a tool claims against what it produced.
    
2.  A possible theory that reproduces a real bug can still be the wrong theory.
    
3.  Bugs usually come from two reasonable components disagreeing about a definition. Not necessarily a broken piece.