foundation: give IP reassembly the RFC timeout, and trace TCP flows bidirectionally - #435
Conversation
Review of #435 at
|
…mode claim Two findings from the review of #435. ## A flow closed one packet too early, and reused ports merged (blocking) `closed = len(buffer.fin) >= 2` submitted a bidirectional flow on the second FIN of its four-way close. The close is FIN, ACK, FIN, ACK, so the final ACK arrived after the buffer had been popped and opened a fresh buffer under the same canonical BUFID -- which a later connection reusing those endpoints then merged into. The NOTE above that line predicted exactly this defect for closing on the *first* FIN and was only half-applied. Reproduced on the reviewed head with a four-way close plus reuse: frames (6, 7, 8) came back as one flow, mixing connection one's final ACK with connection two. It was visible in the corpus all along. http.pcap traced 220 flows, of which **109 were single-frame stray tails** beside 109 nine-frame flows -- each conversation split in two. It now traces 111, every one two-way, none of one frame, all 1117 frames still in exactly one flow. My earlier report read those 109 as one-directional conversations; they were the bug. The fix is not a later close condition but a different question. Observing a teardown is not knowing that nothing more will arrive: duplicates of the final ACK can follow, so no rule naming the last packet of the exchange can hold. So a teardown -- FIN from both endpoints, or RST from either -- is *recorded* and finalises nothing; the flow keeps the packets that belong to it. It is finalised only by proof that no more can come: a new connection's SYN on the same endpoints, or the end of the capture, via the new `TraceFlow.finish()` that `Extractor._cleanup` calls. Callbacks fire there, so they see the whole conversation. `submit()` still reports an unfinalised flow, so reading `index` mid-capture cannot strand the rest of a conversation in a second flow. Telling that SYN from the peer's SYN-ACK is what the recorded teardown is for -- a SYN-ACK cannot follow a completed teardown. That also closes the RST gap `pep.rst` documents: `rst` is now on the traceflow packet model and reported by all six adapters that build one. `bidirectional=False` still closes on FIN, ignores RST, and reproduces main exactly at 355 flows. ## The IP loose-mode payload *did* change (correcting the earlier claim) The previous commit message said of both `strict=False` fixes that "the payload shape is unchanged, so follow_tcp_stream still reconstructs what it did". That is true of the TCP fix and **not** of the IP one, where the payload changed. Stated properly: - **TCP**: payload unchanged -- the buffer with its holes zero-filled, which is what `follow_tcp_stream` reconstructs a stream from. Only `completed` stopped claiming a holed datagram was complete. - **IP, final fragment received** (`TDL > 0`): payload unchanged, holes zero-filled, as TCP does. Only `completed` became honest. - **IP, final fragment never received** (`TDL` still `-1`): the payload changed. It was `datagram[:-1]`, 65534 octets of preallocated buffer reported complete. It is now the **contiguous prefix** -- offset zero to the first hole -- rather than the `b''` an earlier draft of this branch returned, because those octets really did arrive and a blob cannot convey the offset of a run that starts after an unmeasured gap. `strict=True`, the default, still lists the runs. Both branches now have tests; the IP one had none. Suite 901 passed / 17 skipped / 848 subtests, 0 failed. All 15 sample captures still give byte-identical tree, json and reassembly output against the branch point, and still regenerate byte-identically; 1479 datagrams, all COMPLETE. mypy 127 against 128; pylint 4686 against 4706.
|
Reviewed at head CI at What I checkedFinding 1 (traceflow redesign). Read
Finding 2 (IP Everywhere Toolkit adapters: confirmed all 6 engines that support TCP trace (dpkt, pcap, pcapng, pypcapfile, pyshark, scapy) now populate Claims independently reproducedChecked out
Findings postedOne inline comment on Not coveredDid not review: the VerdictNo blocking defects found. One minor, non-blocking documentation issue posted inline (duplicate paragraph in |
…ding timestamps Five review comments on #435. ## The DPKT engine was throwing the timestamp away (substantive) The fix was not where the comment sat. DPKT's reader yields ``(timestamp, bytes)`` and only the octets became a packet, so a *stored* frame did not know when it was captured and ``follow_tcp_stream`` had nothing to pass its reassembler but a bound ``functools.partial(timestamp=0.0)``. The timestamp was never unavailable -- it was being discarded at the engine. ``DPKT.read_frame`` now attaches it (``pcapkit.toolkit.dpkt.attach_timestamp``, read back by ``packet2timestamp``, which raises rather than defaulting for a frame that never came through the engine), the partial is gone, and the NOTE that asserted DPKT "cannot read a frame's capture timestamp off the frame" -- the framing that led to the wrong fix -- is corrected here and in the three adapter docstrings that repeated it. **No behaviour change**, as expected: TCP reassembly has no timeout by default, so the value reaches no decision. ``follow_tcp_stream`` on the DPKT engine returns the same streams and conversations as the default engine, which is now pinned by a test; the timestamps attached to ``in.pcap`` are the capture's own (1511106545.471719 …), not zeros. ## The application layer is wired into flow tracing (scope addition) Previously declined on the grounds that traceflow buffers no payload so there is no second parse to postpone. That is true, which is why this is a capability rather than a deferral -- and of the two designs the page posed, the tracer **delegates to** ``reassembly.tcp.TCP`` rather than growing a payload buffer of its own: a buffer concatenating payloads in capture order is silently wrong on the first retransmission or reordered segment, where RFC 815's hole-descriptor algorithm already in the reassembler is not. A test delivers segments out of order and asserts sequence order comes back. So the traceflow packet carries the four segment fields that reassembler needs (``seq``, ``ack``, ``header``, ``payload``) from exactly where each engine's sibling ``tcp_reassembly`` adapter already reads them, each flow owns a reassembler, and ``Index.packet`` postpones twice: reading it flushes the flow's reassembler, and each datagram's own ``packet`` is parsed later still -- the same ``Deferred`` arrangement as the reassembly side, mirrored in ``traceflow/data/data.py`` for the same reason those two modules already mirror each other. **Opt-in** (``analyse=True``, ``trace_analyse=True`` on Extractor, extract() and follow_tcp_stream): buffering every traced payload is a cost tracing does not otherwise pay, and tracing's per-packet cost is something this package has deliberately driven down. Off by default nothing is buffered, reassembled or parsed, and flow counts are unchanged. Refused on the ``pyshark`` engine, which reports dissected fields rather than octets -- the same reason it has no reassembly adapter. ## Completion uses StrEnum (question) ``pcapkit.utilities.compat.StrEnum``, as ``httpv1.Type`` and ``pcapng.TLSKeyLabel`` do. Verified on both the stdlib and aenum paths that it keeps every property the contract needs: only COMPLETE truthy, ``== True`` and ``== False`` both false. It adds ``json.dumps`` support -- a plain Enum raises TypeError, and ``to_dict()`` hands the field straight out -- and ``completed == 'timeout'``. The one cost is documented in a Warning: a non-empty string that tests false, so ``bool(x)`` and ``bool(str(x))`` disagree. The redundant ``__str__`` is gone, StrEnum already giving it. ## Quoting and alignment (questions) ``Buffer[_AT]`` unquoted, matching ``Packet[_AT]`` beside it and the sibling ``reassembly.ip.IP``. Comment columns re-aligned in both reassembly modules, where ``TS = info.timestamp`` had pushed its own comment a column right of the rest. Also silences, on both DeferredPacket mixins, the mypy ``[misc]`` and pylint ``no-member`` false positives about ``super()`` calls undefined on a mixin -- pylint rates those as *errors*. Suite 930 passed / 17 skipped / 929 subtests, 0 failed; 947 collected against 916 on origin/main, the difference being this branch's own tests. All 15 sample captures give byte-identical tree, json and reassembly output against origin/main and still regenerate byte-identically; 1479 datagrams, all COMPLETE. Flow counts unchanged at 122 bidirectional / 355 unidirectional, 1222 traced frames either way. mypy 124 against 128; pylint 4712 against 4723.
Review of PR #435 at head
|
Review of PR #435 at head
|
…directionally Both items are asked for on the Help Wanted page (docs/source/pep.rst, from #419) and in discussion #106. Reassembly timeout - The clock is the capture's own timestamps, never the host's: an offline parser has no other notion of time passing, and keying on time.time() would make the same file reassemble differently on every run. Packet and Buffer models gained a `timestamp`, and Reassembly.expire() abandons a buffer whose first-arriving fragment is older than `timeout` seconds, swept when a packet is handed over -- the only evidence capture time has advanced. - 60s for IPv4 and IPv6. RFC 1122 s3.3.2 supersedes RFC 791's TTL-derived timer ("SHOULD be a fixed value, not set from the remaining TTL... between 60 and 120 seconds") and RFC 8200 s4.5 mandates 60; RFC 791's 15s is an initial lower bound that MAX(TIMER,TTL) raises, not a deadline. TCP gets no timeout: no specification gives stream reassembly one, and an idle connection is ordinary. - Datagram.completed widens from bool to Completion -- COMPLETE, PARTIAL, TIMEOUT -- so an abandoned datagram is distinguishable from one that was merely unfinished. Only COMPLETE is truthy, so `if datagram.completed` is unchanged. - Fixes two cases where strict=False reported an incomplete datagram as *complete*: TCP passed off zero-filled holes as received data, and IP sliced datagram[:TDL] with TDL still -1, handing back 65534 octets of preallocated buffer. Both now report PARTIAL; the payload shape is unchanged, so follow_tcp_stream still reconstructs what it did. Bidirectional flow tracing - TCP.make_bufid() orders the two endpoints canonically, so both halves of a conversation share one buffer, label and output file. Index gained forward and reverse so per-direction ordering stays recoverable. A flow closes once *both* halves have FINed, since a connection is not over while one direction is still sending. - Default; bidirectional=False (trace_bidirectional= on Extractor, extract() and follow_tcp_stream) restores per-direction flows and reproduces main exactly. - Also fixes the Scapy adapter reporting time.time() as a flow timestamp, which put the moment of parsing into every label. Verified against a git archive of the branch point: all 15 sample captures give byte-identical tree, json and reassembly output with ip/tcp/reassembly enabled, and no capture hits a timeout (1479 datagrams, all COMPLETE). Flow tracing goes 355 -> 234 flows over the corpus with traced frames unchanged at 1222, the drop equalling the 121 two-way conversations exactly. make_samples.py regenerates byte-identically. Suite 893 passed / 17 skipped; mypy 127 errors against 128 on the branch point.
…sable reaches ``# pylint: disable`` is line-scoped, and ``unused-argument`` is reported against the ``def`` -- so wrapping a signature leaves every parameter on a continuation line outside the disable's reach. Splitting Buffer's and Index's stubs to fit the new fields therefore leaked seven unused-argument messages, and the shorter form already there was leaking three of its own plus two super-init-not-called. Both stubs go back on one line, as every other data model in pcapkit writes them, with a note saying why the long line is deliberate. Pylint: 4689 messages against 4706 on the branch point.
…mode claim Two findings from the review of #435. ## A flow closed one packet too early, and reused ports merged (blocking) `closed = len(buffer.fin) >= 2` submitted a bidirectional flow on the second FIN of its four-way close. The close is FIN, ACK, FIN, ACK, so the final ACK arrived after the buffer had been popped and opened a fresh buffer under the same canonical BUFID -- which a later connection reusing those endpoints then merged into. The NOTE above that line predicted exactly this defect for closing on the *first* FIN and was only half-applied. Reproduced on the reviewed head with a four-way close plus reuse: frames (6, 7, 8) came back as one flow, mixing connection one's final ACK with connection two. It was visible in the corpus all along. http.pcap traced 220 flows, of which **109 were single-frame stray tails** beside 109 nine-frame flows -- each conversation split in two. It now traces 111, every one two-way, none of one frame, all 1117 frames still in exactly one flow. My earlier report read those 109 as one-directional conversations; they were the bug. The fix is not a later close condition but a different question. Observing a teardown is not knowing that nothing more will arrive: duplicates of the final ACK can follow, so no rule naming the last packet of the exchange can hold. So a teardown -- FIN from both endpoints, or RST from either -- is *recorded* and finalises nothing; the flow keeps the packets that belong to it. It is finalised only by proof that no more can come: a new connection's SYN on the same endpoints, or the end of the capture, via the new `TraceFlow.finish()` that `Extractor._cleanup` calls. Callbacks fire there, so they see the whole conversation. `submit()` still reports an unfinalised flow, so reading `index` mid-capture cannot strand the rest of a conversation in a second flow. Telling that SYN from the peer's SYN-ACK is what the recorded teardown is for -- a SYN-ACK cannot follow a completed teardown. That also closes the RST gap `pep.rst` documents: `rst` is now on the traceflow packet model and reported by all six adapters that build one. `bidirectional=False` still closes on FIN, ignores RST, and reproduces main exactly at 355 flows. ## The IP loose-mode payload *did* change (correcting the earlier claim) The previous commit message said of both `strict=False` fixes that "the payload shape is unchanged, so follow_tcp_stream still reconstructs what it did". That is true of the TCP fix and **not** of the IP one, where the payload changed. Stated properly: - **TCP**: payload unchanged -- the buffer with its holes zero-filled, which is what `follow_tcp_stream` reconstructs a stream from. Only `completed` stopped claiming a holed datagram was complete. - **IP, final fragment received** (`TDL > 0`): payload unchanged, holes zero-filled, as TCP does. Only `completed` became honest. - **IP, final fragment never received** (`TDL` still `-1`): the payload changed. It was `datagram[:-1]`, 65534 octets of preallocated buffer reported complete. It is now the **contiguous prefix** -- offset zero to the first hole -- rather than the `b''` an earlier draft of this branch returned, because those octets really did arrive and a blob cannot convey the offset of a run that starts after an unmeasured gap. `strict=True`, the default, still lists the runs. Both branches now have tests; the IP one had none. Suite 901 passed / 17 skipped / 848 subtests, 0 failed. All 15 sample captures still give byte-identical tree, json and reassembly output against the branch point, and still regenerate byte-identically; 1479 datagrams, all COMPLETE. mypy 127 against 128; pylint 4686 against 4706.
…r verbatim A ``code`` span is not markup inside a ``.. code-block:: text``, so the two lines added for 'forward' and 'reverse' were showing their backticks in the rendered page. Nothing else in that diagram uses them.
…ding timestamps Five review comments on #435. ## The DPKT engine was throwing the timestamp away (substantive) The fix was not where the comment sat. DPKT's reader yields ``(timestamp, bytes)`` and only the octets became a packet, so a *stored* frame did not know when it was captured and ``follow_tcp_stream`` had nothing to pass its reassembler but a bound ``functools.partial(timestamp=0.0)``. The timestamp was never unavailable -- it was being discarded at the engine. ``DPKT.read_frame`` now attaches it (``pcapkit.toolkit.dpkt.attach_timestamp``, read back by ``packet2timestamp``, which raises rather than defaulting for a frame that never came through the engine), the partial is gone, and the NOTE that asserted DPKT "cannot read a frame's capture timestamp off the frame" -- the framing that led to the wrong fix -- is corrected here and in the three adapter docstrings that repeated it. **No behaviour change**, as expected: TCP reassembly has no timeout by default, so the value reaches no decision. ``follow_tcp_stream`` on the DPKT engine returns the same streams and conversations as the default engine, which is now pinned by a test; the timestamps attached to ``in.pcap`` are the capture's own (1511106545.471719 …), not zeros. ## The application layer is wired into flow tracing (scope addition) Previously declined on the grounds that traceflow buffers no payload so there is no second parse to postpone. That is true, which is why this is a capability rather than a deferral -- and of the two designs the page posed, the tracer **delegates to** ``reassembly.tcp.TCP`` rather than growing a payload buffer of its own: a buffer concatenating payloads in capture order is silently wrong on the first retransmission or reordered segment, where RFC 815's hole-descriptor algorithm already in the reassembler is not. A test delivers segments out of order and asserts sequence order comes back. So the traceflow packet carries the four segment fields that reassembler needs (``seq``, ``ack``, ``header``, ``payload``) from exactly where each engine's sibling ``tcp_reassembly`` adapter already reads them, each flow owns a reassembler, and ``Index.packet`` postpones twice: reading it flushes the flow's reassembler, and each datagram's own ``packet`` is parsed later still -- the same ``Deferred`` arrangement as the reassembly side, mirrored in ``traceflow/data/data.py`` for the same reason those two modules already mirror each other. **Opt-in** (``analyse=True``, ``trace_analyse=True`` on Extractor, extract() and follow_tcp_stream): buffering every traced payload is a cost tracing does not otherwise pay, and tracing's per-packet cost is something this package has deliberately driven down. Off by default nothing is buffered, reassembled or parsed, and flow counts are unchanged. Refused on the ``pyshark`` engine, which reports dissected fields rather than octets -- the same reason it has no reassembly adapter. ## Completion uses StrEnum (question) ``pcapkit.utilities.compat.StrEnum``, as ``httpv1.Type`` and ``pcapng.TLSKeyLabel`` do. Verified on both the stdlib and aenum paths that it keeps every property the contract needs: only COMPLETE truthy, ``== True`` and ``== False`` both false. It adds ``json.dumps`` support -- a plain Enum raises TypeError, and ``to_dict()`` hands the field straight out -- and ``completed == 'timeout'``. The one cost is documented in a Warning: a non-empty string that tests false, so ``bool(x)`` and ``bool(str(x))`` disagree. The redundant ``__str__`` is gone, StrEnum already giving it. ## Quoting and alignment (questions) ``Buffer[_AT]`` unquoted, matching ``Packet[_AT]`` beside it and the sibling ``reassembly.ip.IP``. Comment columns re-aligned in both reassembly modules, where ``TS = info.timestamp`` had pushed its own comment a column right of the rest. Also silences, on both DeferredPacket mixins, the mypy ``[misc]`` and pylint ``no-member`` false positives about ``super()`` calls undefined on a mixin -- pylint rates those as *errors*. Suite 930 passed / 17 skipped / 929 subtests, 0 failed; 947 collected against 916 on origin/main, the difference being this branch's own tests. All 15 sample captures give byte-identical tree, json and reassembly output against origin/main and still regenerate byte-identically; 1479 datagrams, all COMPLETE. Flow counts unchanged at 122 bidirectional / 355 unidirectional, 1222 traced frames either way. mypy 124 against 128; pylint 4712 against 4723.
Three review findings, none of which changes behaviour -- the behaviour was right and the documentation overpromised. The class docstring said `Index.packet` "holds one reassembled datagram per direction". It holds one per direction *per acknowledgement number*: the reassembler buckets as `_buffer[BUFID].ack[ACK]` and emits one datagram per bucket, and `_make_segment` passes `ack` through untouched. Measured on three request/response round trips over one connection: six datagrams, three each way. That is the useful shape rather than an accident -- the acknowledgement number advances exactly when the peer has spoken, so each datagram's payload is one application message that `Datagram.packet` can parse alone, where merging a direction would hand the parser several concatenated messages and have it read only the first. The docstring now says so and shows the six. The existing test could not see this: every segment it sends carries the default `ack=0`, which is the one shape where "one per direction" is the whole truth. Its assertion message said the general rule; it now says the reason, and a new test drives three real round trips and pins all six datagrams by (source port, acknowledgement number). `submit()` did not say that an open flow's `packet` is a mid-capture snapshot. The `Deferred` resolves on first read and is fixed there, so an `Index` a caller kept cannot see later segments and carries no marker distinguishing it from a final result. Measured: read at two frames gives `b'first'`, stays `b'first'` after the third arrives, and is `b'firstsecond'` after `finish()`. Documented, and pinned by a test. This method's own cache is not the cause -- `trace()` and `finish()` both clear it -- and the docstring says that too, so the note is not mistaken for a caching bug later. Docs: `Deferred` and `DeferredPacket` move from `traceflow/tcp.rst` to `traceflow/index.rst`, next to `TraceFlowData`, which is where reassembly declares its equivalents. They live in `traceflow/data/data.py`, so declaring them under `tcp.rst`'s `data.tcp` module directive was wrong twice over. A fourth finding is pre-existing and filed as #443 rather than folded in here: conflicting retransmissions are resolved last-write-wins and still reported COMPLETE. `git diff origin/main` over `reassembly/tcp.py` touches neither overlap branch. Full suite on 3.14.7: 932 passed, 17 skipped (930 before, plus the two tests added here).
…boundary Review finding, and correct. The docstring added in 9089131 said bucketing on the acknowledgement number "splits a conversation at its message boundaries", so each datagram's payload is one parseable application message. That holds only where an exchange is one request and one reply. Under pipelining it does not, because several requests in flight before any reply all carry the same acknowledgement number and so share a bucket. Measured on this tree: two requests back to back on one ACK come back as a single datagram carrying b'req1req2', where the wording implied two. So the docstring now says the boundary is the peer's turn -- one datagram per exchange -- and states plainly that this narrows the concatenation to within an exchange rather than eliminating it, and that a parser handed a pipelined datagram still sees only the first message. The reason for documenting the behaviour rather than changing it survives that correction: merging a whole direction concatenates every message it ever sent, which is strictly worse than concatenating within one exchange. The claim was too absolute, not wrong about which option is better. Pinned by test_pipelined_sends_share_a_datagram_because_the_ack_never_moved, since a limitation that lives only in prose is one the next reader has to rediscover. tests/foundation/traceflow: 18 passed.
1c8d975 to
c8e72b8
Compare
1 participant
Closes the two reassembly and traceflow items on the Help Wanted page,
docs/source/pep.rstunder "Reassembly Beyond IP and TCP" (added by #419, mirroring discussion #106#discussioncomment-18470233):What the timeout means for an offline parser
The clock is the capture's own timestamps. Wall clock says nothing about a file, and
time.time()would make the same capture reassemble differently on every run — which would break both the fixtures and any reproducible comparison. A fragment arriving is the only evidence that capture time advanced, so the sweep runs when a packet is handed over rather than on a timer.That has a consequence worth documenting rather than hiding: the clock only advances while that particular reassembler is fed. For IPv4 and TCP that is nearly every frame; for IPv6 it is only fragments. So a stalled IPv6 buffer with nothing behind it reports
PARTIAL, notTIMEOUT— the honest answer, since the capture never shows the deadline passing.60 seconds for both IPv4 and IPv6, and the page's 15 seconds was wrong. RFC 1122 §3.3.2 supersedes RFC 791 here: the timeout "SHOULD be a fixed value, not set from the remaining TTL… between 60 seconds and 120 seconds". RFC 791's 15 s is an initial setting that
MAX(TIMER, TTL)raises toward the 4.25-minute TTL ceiling, so it is not a deadline — and TTL is not in the packet model anyway.pep.rstis corrected in this PR. TCP gets no timeout by default: no specification gives stream reassembly one and an idle connection is ordinary, buttimeout=enables one.Two behaviour changes, both opt-out-able
1.
Datagram.completedwidens fromboolto aCompletionenum —COMPLETE/PARTIAL/TIMEOUT, same field, same channel, so a caller learns why a datagram is incomplete without a parallel vocabulary. OnlyCOMPLETEis truthy, soif datagram.completed:andassertTrue/assertFalseare unchanged.datagram.completed == Trueno longer holds — that is the breaking shape.2. Bidirectional TCP flow tracing, on by default, with
trace_bidirectional=Falseonextract(),Extractorandfollow_tcp_stream(), andbidirectional=FalseonTraceFlow.TCP.make_bufid()orders the endpoints canonically so both halves reduce to one key; the label still comes from the packet that opened the flow, so labels are unchanged.Indexgainsforwardandreverse. A flow closes once both halves have FINed — closing on the first would cut the peer's FIN and final ACK out, and they would then open a second flow under the same ID, which is the very split being removed.Default-on because
pep.rstlists per-direction keying as a defect, beside "closes on FIN but never on RST", rather than as a missing option — and because the module is called "Follow TCP Stream", which everywhere else means the conversation.Flow output changes, and the argument for it
An earlier revision of this description had these numbers wrong, and the way they were wrong is worth recording: it reported
http.pcapat 220 flows, of which 109 were single-frame, and read those 109 as one-directional conversations. They were not — they were the defect. Each conversation was a 9-frame flow plus a 1-frame stray tail, created when the flow was finalised on the teardown and the final acknowledgement then opened a second buffer under the same ID. Fixing the close condition removes the tails.http.pcaptcp.pcapin.pcaphttp.pcaphas 111 conversations and every one of them now carries both directions in a single flow.bidirectional=Falsereproducesmainexactly at 355 flows.traced_framesis identical in every capture — 1222 total, nothing lost and nothing double-counted, which is what makes the flow-count drop a merge rather than a loss. Concretely onin.pcap: frame 3 is123.129.210.135:80 → 192.168.1.100:55232and frame 4 is the same connection reversed; they now form one flow withforward=(3,)andreverse=(4,). Same address pair, same port pair, opposite directions — one TCP connection.Verified independently through
follow_tcp_streamonin.pcap, with payload conserved in every configuration:So the merged stream carries both directions' payload, which is what following a stream means, and the opt-out reproduces
mainexactly.Also fixed, beyond the brief
Three
strict=Falsecases, not two, and they are not symmetric — an earlier draft of this description got that wrong.follow_tcp_streamreconstructs a stream from. Onlycompletedstopped claiming a holed datagram was complete.TDL > 0): payload unchanged, holes zero-filled, as TCP does. Onlycompletedbecame honest.TDLstill-1): the payload changed. It wasdatagram[:-1]— 65534 octets of preallocated buffer, reported complete. It is now the contiguous prefix, offset zero to the first hole; notb'', because those octets really did arrive.strict=True, the default, still lists the runs.The original wording, kept for the record of what was wrong: TCP reported a hole-ridden buffer as complete, passing zero-filled gaps off as received data; there the payload shape really is unchanged, a test pins it and
follow_tcp_streamdepends on the blob, so onlycompletedbecame honest. IP is different. It sliceddatagram[:TDL]withTDLstill-1, handing back 65534 octets of preallocated buffer — almost all of them zeros the sender never sent — and calling it complete. That payload could not be left as it was. It now reports the contiguous prefix: every octet from offset zero up to the first hole. That is the longest run whose extent is known without knowing the total length, it is all genuinely received, and it is the part a caller asking for one blob can actually use, since a parser reading from the start cannot use a run that begins after an unmeasured gap. Reportingb''was considered and rejected as discarding data that really arrived; anything past the first hole is still reported bystrict=True, which lists the runs precisely because their offsets cannot be conveyed in a blob.RCVBTrecords receipt in 8-octet units, so the prefix ends on that boundary.Scapy's traceflow reported
time.time()as the flow timestamp, putting the parse moment into every label and output filename. Nowfloat(packet.time), i.e. deterministic.Datagram.__init__overloads removed. They promised complete ⇒bytesand incomplete ⇒tuple, which only held because loose mode was lying, and could not be selected from a runtime-computed value anyway.DPKT is the one adapter that cannot self-supply a timestamp — it returns
(ts, buf)and the engine keeps only the packet — so its three reassembly adapters take it positionally.follow_tcp_streambinds0.0there viafunctools.partial, safe because TCP has no timeout by default, and documented at the call site.Deliberately not done
Wiring traceflow's
analyzeis not in this PR, and the record says it should not be. PR #424#discussion_r4031491063settled it: traceflow buffers no parsed protocol at all — only a dumper, frame indices and a label — andgrepfinds noanalyze()call anywhere underfoundation/traceflow/. So there is no second parse to postpone; adding one is a new capability needing a design decision first (does the tracer grow a per-direction payload buffer, or delegate toTCP_Reassemblyasfollow_tcp_streamalready does?). Recorded inpep.rstas the remaining open item there.Verification
Suite 893 passed / 17 skipped / 848 subtests / 0 failed, against a branch-point baseline of 859 / 35 / 844 — collection 910 versus 894, exactly the +16 tests added. The 18 extra baseline skips are all
tests/test_tier_guard.py, because agit archivescratch tree is not a checkout; worth knowing for anyone measuring a baseline that way.All 15 captures produce byte-identical
tree,jsonand reassembly output withip/tcp/reassembly=True; no capture hits a timeout — 1479 datagrams, allCOMPLETE.make_samples.pyregenerates byte-identically. mypy 127 errors versus 128 on the baseline; pylint 4689 versus 4706, rating 8.76 → 8.77. Docs build clean for the changed content.