Add initial TCP reconstruction implementation - #163
Conversation
📝 WalkthroughWalkthroughChangesThe PR adds a TCP stream reassembly
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PacketSource
participant TcpStreamReassemblyPool
participant TcpStreamReassemblyBuf
participant Example
PacketSource->>TcpStreamReassemblyPool: process_sliced_packet(SlicedPacket)
TcpStreamReassemblyPool->>TcpStreamReassemblyPool: identify TCP stream and process flags
TcpStreamReassemblyPool->>TcpStreamReassemblyBuf: add(sequence, payload, FIN)
TcpStreamReassemblyBuf-->>TcpStreamReassemblyPool: contiguous payload
TcpStreamReassemblyPool-->>Example: mutable reassembly buffer
Example->>TcpStreamReassemblyBuf: consume(processed length)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
etherparse/src/tcp_reassembly/tcp_stream_reassembly_pool.rs (1)
30-34: 🧹 Nitpick | 🔵 TrivialRecycled buffer pools retain full capacity indefinitely.
Buffers pushed onto
finished_data_bufs/finished_section_bufskeep their allocated capacity (a data buffer can be as large asmax_capacity, 1 MiB by default). While the count is bounded by peak concurrent streams, a single traffic spike keeps that memory resident for the pool's lifetime. Consider capping the number of retained buffers (and/or shrinking oversized ones) so idle memory can be released after churn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@etherparse/src/tcp_reassembly/tcp_stream_reassembly_pool.rs` around lines 30 - 34, Bound the recycled-buffer pools finished_data_bufs and finished_section_bufs so they cannot retain all peak-concurrency allocations indefinitely. When returning buffers to these pools, retain only a capped number and discard or shrink oversized buffers, while preserving reuse for appropriately sized buffers and allowing idle memory to be released after traffic churn.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog.md`:
- Line 6: Update the TcpStreamReassemblyBuf changelog description to hyphenate
the compound adjectives: use “32-bit sequence number” and “monotonic 64-bit”
while preserving the rest of the wording.
In `@etherparse/src/tcp_reassembly/tcp_stream_reassembly_buf.rs`:
- Around line 228-264: Defer FIN recording in the add flow until after the
max-window validation succeeds for non-empty payloads. Keep the existing
fin_offset update for the empty-payload fast path, remove the earlier
unconditional update, and after the SegmentBeyondMaxWindow check set fin_offset
from the validated post-trim end_abs when fin is true.
---
Nitpick comments:
In `@etherparse/src/tcp_reassembly/tcp_stream_reassembly_pool.rs`:
- Around line 30-34: Bound the recycled-buffer pools finished_data_bufs and
finished_section_bufs so they cannot retain all peak-concurrency allocations
indefinitely. When returning buffers to these pools, retain only a capped number
and discard or shrink oversized buffers, while preserving reuse for
appropriately sized buffers and allowing idle memory to be released after
traffic churn.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a2add71-fdeb-4864-8363-af8b1e6422f7
📒 Files selected for processing (11)
changelog.mdetherparse/examples/tcp_reassembly.rsetherparse/proptest-regressions/tcp_reassembly/tcp_stream_reassembly_buf.txtetherparse/src/lib.rsetherparse/src/tcp_reassembly/mod.rsetherparse/src/tcp_reassembly/tcp_reassemble_error.rsetherparse/src/tcp_reassembly/tcp_segment_range.rsetherparse/src/tcp_reassembly/tcp_stream_id.rsetherparse/src/tcp_reassembly/tcp_stream_ip_id.rsetherparse/src/tcp_reassembly/tcp_stream_reassembly_buf.rsetherparse/src/tcp_reassembly/tcp_stream_reassembly_pool.rs
| ## Unreleased | ||
|
|
||
| * Added TCP stream reassembly support (in the `std`-only `tcp_reassembly` module, contains allocations): | ||
| * `TcpStreamReassemblyBuf`, a re-usable buffer that reconstructs the payload byte stream of a single direction of a TCP connection. It is robust against retransmits, re-ordered & duplicated/overlapping segments, big offset jumps and 32 bit sequence number wrap-around (all bookkeeping is done in a monotonic 64 bit "absolute stream offset" space). Data is exposed in-place via `contiguous` and freed via `consume` ("collect then clean"). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate the compound adjectives. "32 bit sequence number" → "32-bit sequence number" and "monotonic 64 bit" → "monotonic 64-bit".
🧰 Tools
🪛 LanguageTool
[grammar] ~6-~6: Use a hyphen to join words.
Context: ...apping segments, big offset jumps and 32 bit sequence number wrap-around (all boo...
(QB_NEW_EN_HYPHEN)
[grammar] ~6-~6: Use a hyphen to join words.
Context: ...ll bookkeeping is done in a monotonic 64 bit "absolute stream offset" space). Dat...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@changelog.md` at line 6, Update the TcpStreamReassemblyBuf changelog
description to hyphenate the compound adjectives: use “32-bit sequence number”
and “monotonic 64-bit” while preserving the rest of the wording.
Source: Linters/SAST tools
| let end_abs = start_abs + payload.len() as i128; | ||
|
|
||
| // record the end of the stream if the FIN flag is set (FIN occupies the | ||
| // sequence number right after the payload) | ||
| if fin { | ||
| let fin_pos = if end_abs < 0 { 0 } else { end_abs as u64 }; | ||
| self.fin_offset = Some(fin_pos); | ||
| } | ||
|
|
||
| // nothing to store for an empty payload (e.g. pure SYN/ACK/FIN segments) | ||
| if payload.is_empty() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // trim the part that lies before the read cursor (already consumed / | ||
| // retransmitted data) | ||
| let cursor = self.base_offset as i128; | ||
| let (payload, start_abs) = if start_abs < cursor { | ||
| let trim = (cursor - start_abs) as usize; | ||
| if trim >= payload.len() { | ||
| // fully before the cursor -> nothing to store | ||
| return Ok(()); | ||
| } | ||
| (&payload[trim..], cursor) | ||
| } else { | ||
| (payload, start_abs) | ||
| }; | ||
| let end_abs = start_abs + payload.len() as i128; | ||
|
|
||
| // enforce the maximum buffer window (measured from the read cursor) | ||
| let end_offset = (end_abs - cursor) as u64; | ||
| if end_offset > self.max_capacity as u64 { | ||
| return Err(SegmentBeyondMaxWindow { | ||
| end_offset, | ||
| max_capacity: self.max_capacity, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
fin_offset is mutated before the max-window check, so a rejected FIN segment still records the FIN.
For a FIN-bearing segment with a non-empty payload that lands beyond max_capacity, add sets self.fin_offset (Line 234) and then returns Err(SegmentBeyondMaxWindow) (Line 260). On an existing stream (retained on error by the pool at tcp_stream_reassembly_pool.rs Line 204) this leaves the buffer believing the stream ends at an offset whose data was never stored. Defer the FIN recording until after the segment is accepted; keep it for the empty-payload fast path where there is no window concern.
🐛 Proposed fix
let end_abs = start_abs + payload.len() as i128;
- // record the end of the stream if the FIN flag is set (FIN occupies the
- // sequence number right after the payload)
- if fin {
- let fin_pos = if end_abs < 0 { 0 } else { end_abs as u64 };
- self.fin_offset = Some(fin_pos);
- }
-
- // nothing to store for an empty payload (e.g. pure SYN/ACK/FIN segments)
- if payload.is_empty() {
- return Ok(());
- }
+ // nothing to store for an empty payload (e.g. pure SYN/ACK/FIN segments)
+ if payload.is_empty() {
+ if fin {
+ self.fin_offset = Some(if end_abs < 0 { 0 } else { end_abs as u64 });
+ }
+ return Ok(());
+ }Then, after the window check passes (following Line 264, where the post-trim end_abs is >= cursor >= 0):
// record the end of the stream only once the segment is accepted
if fin {
self.fin_offset = Some(end_abs as u64);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@etherparse/src/tcp_reassembly/tcp_stream_reassembly_buf.rs` around lines 228
- 264, Defer FIN recording in the add flow until after the max-window validation
succeeds for non-empty payloads. Keep the existing fin_offset update for the
empty-payload fast path, remove the earlier unconditional update, and after the
SegmentBeyondMaxWindow check set fin_offset from the validated post-trim end_abs
when fin is true.
1 participant
Summary by CodeRabbit