From 6fbfb4185dcac0830e1fa267c9154ac5dc807094 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 15:23:50 -0400 Subject: [PATCH] reassembly: record IP fragment overlap conflicts instead of losing them silently (#477) `pcapkit/foundation/reassembly/ip.py:133` slice-assigned an arriving fragment's payload into the datagram buffer unconditionally, with no check of `RCVBT` beforehand and no record kept when two fragments claimed the same octets with different bytes. This is the IP analogue of #443 (fixed for TCP in unmerged PR #478), but is a separate implementation and, per research below, a different resolution. - pcapkit/foundation/reassembly/data/ip.py: add `Buffer.conflict` (list, mutable, accumulated) and `Datagram.conflict` (tuple, additive) -- `(first, last)` absolute, inclusive octet ranges, mirroring #478's TCP `conflict` field/convention. - pcapkit/foundation/reassembly/ip.py: add `IP._detect_conflicts`, called before the existing overwrite. It consults `RCVBT` block bits clipped by `TDL` (see below) to find which octets already held real data, then compares bytes directly against the arriving payload; a differing run is recorded, an equal one (a duplicate) is not. The overwrite itself is UNCHANGED -- last-write-wins, not first-write-wins. - tests/foundation/reassembly/test_ip.py: 7 new cases -- identical duplicate (uncontested), full block-aligned conflict, partial overlap extending each direction, a conflict confined to the final fragment's partial 8-octet block (the RCVBT-granularity edge case), a three-way conflict, and a conflict resolved before a later fragment completes the datagram. tests/foundation/reassembly/data/test_models.py updated for the new constructor field (IP section only). - docs/source/pcapkit/foundation/reassembly/ip/{ipv4,ipv6}.rst: document `conflict` on both Datagram and Buffer, and the RCVBT/TDL clipping. RFC 791 answers the "which fragment wins" question explicitly, and the answer is the OPPOSITE of TCP's RFC 9293 first-write-wins: "In the case that two or more fragments contain the same data either identically or through a partial overlap, this procedure will use the more recently arrived copy in the data buffer and datagram delivered." RFC 815's hole-descriptor algorithm says nothing about differing content directly, but its unconditional "copy the data from the fragment into the reassembly buffer" is consistent with the same last-write-wins resolution. So this fix keeps the existing overwrite direction and adds only the missing record of disagreement -- it does not port #478's first-write-wins merge logic, which would contradict RFC 791's own text. RCVBT records receipt in 8-octet blocks, coarser than an octet conflict needs. Every fragment but the last is required to be block-aligned (and FO is always a multiple of 8 -- it is wire-encoded in 8-octet units), so the only block that can be partially real is the one holding the final fragment's own tail. `_detect_conflicts` treats octet `pos` as genuinely already-received iff `rcvbt[pos // 8]` is set AND (`tdl < 0` or `pos < tdl`) -- `tdl` being `Buffer.TDL` as it stood before this fragment's own update. Verified against a case built for exactly this: a final fragment covering only 3 of an 8-octet block's octets, then a later fragment claiming the full block -- conflict is reported as (8, 10), not (8, 15); bytes 11-15 were never really sent by anyone and are correctly treated as new territory, not a conflict. Comparing bytes directly (per the brief's suggestion) was sufficient once combined with this TDL clip -- no new per-octet bytearray needed. Reproduced pre-fix through the public API: two conflicting 8-octet fragments at the same offset silently produced a complete datagram with the second fragment's bytes and `AttributeError` on `.conflict` (field did not exist). Post-fix: same payload (last-write-wins, per RFC 791), plus `conflict == ((0, 7),)`. Reverted the fix via a tagged stash and confirmed all 7 new boundary cases fail immediately with `AttributeError: 'Datagram' object has no attribute 'conflict'`, restored via `git stash apply` + `git stash drop`. Build/test: tests/foundation/reassembly/ -- 63 passed (7 new), 0 failed. tests/foundation/ -- 200 passed, 11 skipped. mypy pcapkit, mypy --config-file mypy.ini pcapkit, and the Makefile's --follow-imports=silent --ignore-missing-imports --show-column-numbers --show-error-codes pcapkit all report zero errors in the two files touched here (122 total errors in 39 files, all pre-existing and unrelated, e.g. toolkit/scapy.py, protocols/internet/ipv6.py). Accepted behaviour break, same reasoning as #478: a capture that hits this path is anomalous by definition, and callers reading `.conflict` on an existing `Datagram` for the first time is the point of the fix. Noticed but not fixed here (out of scope, filing separately): pcapkit/toolkit/scapy.py:160 sets `fo=ipv4.frag` with no `* 8`, but scapy's `IP.frag` is the raw 13-bit wire field in 8-octet units, not a byte offset -- confirmed by constructing `scapy.all.IP(frag=5)` and observing the raw fragment-offset field on the wire is `5`, not `40`. Every other adapter (`dpkt.py`, `pypcapfile.py`, `pcap.py`, `pcapng.py`, native `ipv4.py`) multiplies by 8 or uses an already-scaled `.offset`. This looks like a real defect in the scapy IPv4 fragment-offset handling, separate from #477 and not yet filed as its own issue. --- .../pcapkit/foundation/reassembly/ip/ipv4.rst | 38 ++++- .../pcapkit/foundation/reassembly/ip/ipv6.rst | 36 ++++ pcapkit/foundation/reassembly/data/ip.py | 39 ++++- pcapkit/foundation/reassembly/ip.py | 101 ++++++++++- .../foundation/reassembly/data/test_models.py | 6 +- tests/foundation/reassembly/test_ip.py | 158 +++++++++++++++++- 6 files changed, 365 insertions(+), 13 deletions(-) diff --git a/docs/source/pcapkit/foundation/reassembly/ip/ipv4.rst b/docs/source/pcapkit/foundation/reassembly/ip/ipv4.rst index 14ae11ebb6..4bbe6266fc 100644 --- a/docs/source/pcapkit/foundation/reassembly/ip/ipv4.rst +++ b/docs/source/pcapkit/foundation/reassembly/ip/ipv4.rst @@ -68,6 +68,9 @@ Terminology | |--> 'header' : (bytes) IPv4 header | |--> 'payload' : (bytes) reassembled IPv4 payload | |--> 'packet' : (Protocol) parsed reassembled payload + | |--> 'conflict' : (tuple) octet ranges on which two fragments disagreed + | | |--> (tuple) (first, last), absolute and inclusive + | | |--> ... |--> (Info) data | |--> 'completed' : (Completion) PARTIAL or TIMEOUT --> incomplete | |--> 'id' : (Info) original packet identifier @@ -82,6 +85,9 @@ Terminology | | |--> (bytes) IPv4 payload fragment | | |--> ... | |--> 'packet' : (None) + | |--> 'conflict' : (tuple) octet ranges on which two fragments disagreed + | | |--> (tuple) (first, last), absolute and inclusive + | | |--> ... |--> (Info) data ... .. note:: @@ -95,6 +101,18 @@ Terminology ``repr()``), runs it and keeps the result; see :class:`~pcapkit.foundation.reassembly.data.data.Deferred`. + .. note:: + + ``completed`` and ``conflict`` are independent signals: a datagram + can be :attr:`~pcapkit.foundation.reassembly.data.data.Completion.COMPLETE` + and still carry a non-empty ``conflict``. :rfc:`791` resolves an + overlapping fragment's disagreement itself -- "this procedure will + use the more recently arrived copy in the data buffer" -- the + opposite resolution from TCP's first-write-wins + (:rfc:`9293#section-3.10`, fixed for TCP by #443) -- so a contested + range never leaves a hole on its own, and ``conflict`` is what lets + a caller tell a clean datagram from a contested one. See #477. + reasm.ipv4.buffer Data structure for internal buffering when performing reassembly algorithms (:attr:`IPv4._buffer `) @@ -118,9 +136,27 @@ Terminology | |--> 'header' : (bytes) header buffer | |--> 'datagram' : (bytearray) data buffer, holes set to b'\\x00' | |--> 'timestamp' : (float) capture timestamp of the - | first-arriving fragment + | | first-arriving fragment + | |--> 'conflict' : (list) octet ranges on which an arriving + | | fragment disagreed with bytes already in + | | 'datagram' + | | |--> (tuple) (first, last), absolute and + | | inclusive + | | |--> ... |--> (tuple) BUFID ... + .. note:: + + ``conflict`` is only ever appended to, and is checked against + ``datagram`` and ``RCVBT`` *before* an arriving fragment's own write + and bookkeeping touch them -- see + :meth:`IP._detect_conflicts `. + ``RCVBT`` records receipt in 8-octet blocks, which is coarser than + the octet a conflict needs; ``TDL`` is what recovers the exact + extent for the one block that can be partially real -- the final + fragment's own tail -- so a conflict here is never wider than the + octets that genuinely disagreed, even inside that block. + .. note:: A buffer is abandoned once the reassembly timeout elapses on the diff --git a/docs/source/pcapkit/foundation/reassembly/ip/ipv6.rst b/docs/source/pcapkit/foundation/reassembly/ip/ipv6.rst index 9f3bd73798..1b9bb37b5c 100644 --- a/docs/source/pcapkit/foundation/reassembly/ip/ipv6.rst +++ b/docs/source/pcapkit/foundation/reassembly/ip/ipv6.rst @@ -90,6 +90,9 @@ Terminology | |--> 'header' : (bytes) header before IPv6-Frag | |--> 'payload' : (bytes) reassembled IPv6 payload | |--> 'packet' : (Protocol) parsed reassembled payload + | |--> 'conflict' : (tuple) octet ranges on which two fragments disagreed + | | |--> (tuple) (first, last), absolute and inclusive + | | |--> ... |--> (Info) data | |--> 'completed' : (Completion) PARTIAL or TIMEOUT --> incomplete | |--> 'id' : (Info) original packet identifier @@ -104,6 +107,9 @@ Terminology | | |--> (bytes) IPv6 payload fragment | | |--> ... | |--> 'packet' : (None) + | |--> 'conflict' : (tuple) octet ranges on which two fragments disagreed + | | |--> (tuple) (first, last), absolute and inclusive + | | |--> ... |--> (Info) data ... .. note:: @@ -128,6 +134,18 @@ Terminology ``repr()``), runs it and keeps the result; see :class:`~pcapkit.foundation.reassembly.data.data.Deferred`. + .. note:: + + ``completed`` and ``conflict`` are independent signals: a datagram + can be :attr:`~pcapkit.foundation.reassembly.data.data.Completion.COMPLETE` + and still carry a non-empty ``conflict``. :rfc:`791` resolves an + overlapping fragment's disagreement itself -- "this procedure will + use the more recently arrived copy in the data buffer" -- the + opposite resolution from TCP's first-write-wins + (:rfc:`9293#section-3.10`, fixed for TCP by #443) -- so a contested + range never leaves a hole on its own, and ``conflict`` is what lets + a caller tell a clean datagram from a contested one. See #477. + reasm.ipv6.buffer Data structure for internal buffering when performing reassembly algorithms (:attr:`IPv6._buffer `) @@ -150,4 +168,22 @@ Terminology | | |--> (int) packet range number | |--> 'header' : (bytes) header buffer | |--> 'datagram' : (bytearray) data buffer, holes set to b'\\x00' + | |--> 'conflict' : (list) octet ranges on which an arriving + | | fragment disagreed with bytes already in + | | 'datagram' + | | |--> (tuple) (first, last), absolute and + | | inclusive + | | |--> ... |--> (tuple) BUFID ... + + .. note:: + + ``conflict`` is only ever appended to, and is checked against + ``datagram`` and ``RCVBT`` *before* an arriving fragment's own write + and bookkeeping touch them -- see + :meth:`IP._detect_conflicts `. + ``RCVBT`` records receipt in 8-octet blocks, which is coarser than + the octet a conflict needs; ``TDL`` is what recovers the exact + extent for the one block that can be partially real -- the final + fragment's own tail -- so a conflict here is never wider than the + octets that genuinely disagreed, even inside that block. diff --git a/pcapkit/foundation/reassembly/data/ip.py b/pcapkit/foundation/reassembly/data/ip.py index 349d7f4463..821df38ba9 100644 --- a/pcapkit/foundation/reassembly/data/ip.py +++ b/pcapkit/foundation/reassembly/data/ip.py @@ -105,6 +105,34 @@ class Datagram(DeferredPacket, Info, Generic[_AT]): #: :class:`Deferred` may be passed in its place, and reading this attribute #: then runs it and keeps the result. packet: 'Optional[Protocol]' + #: Octet ranges, absolute into the reassembled payload and both + #: **inclusive** -- the same convention as :attr:`Packet.fo` combined with + #: its length -- on which two fragments disagreed, i.e. an arriving + #: fragment overlapped octets already buffered but did not repeat them. + #: Empty when the datagram never saw a contested octet. + #: + #: :rfc:`791` resolves the disagreement itself: "In the case that two or + #: more fragments contain the same data either identically or through a + #: partial overlap, this procedure will use the more recently arrived copy + #: in the data buffer and datagram delivered." So :attr:`payload` always + #: holds whichever fragment arrived *last* over a contested range -- the + #: opposite resolution from TCP's first-write-wins + #: (:rfc:`9293#section-3.10`) -- and this field is what lets a caller tell + #: a clean datagram from a contested one, since a resolved conflict does + #: not, on its own, leave a hole for ``completed`` to report. + #: + #: :attr:`Buffer.RCVBT ` + #: only records receipt in 8-octet blocks, coarser than the octet + #: granularity a conflict needs. Every fragment but the last is required + #: to be block-aligned (and :attr:`Packet.fo` is *always* a multiple of 8, + #: being wire-encoded in 8-octet units), so the only block that can be + #: partially real is the one holding the final fragment's own tail -- and + #: a range reported here never extends past + #: :attr:`Buffer.TDL ` + #: for exactly that reason, even though a whole ``RCVBT`` block straddling + #: it reads as "received". See + #: :meth:`IP._detect_conflicts `. + conflict: 'tuple[tuple[int, int], ...]' if TYPE_CHECKING: # NOTE: one signature, not a pair of ``@overload``\\ s keyed on @@ -118,7 +146,7 @@ class Datagram(DeferredPacket, Info, Generic[_AT]): # from. Overloads keyed on a literal cannot be selected from a ``completed`` # computed at runtime anyway, so they only made the reassemblers' own calls # untypeable while promising a correlation the code does not keep. - def __init__(self, completed: 'Completion', id: 'DatagramID[_AT]', index: 'tuple[int, ...]', header: 'bytes', payload: 'bytes | tuple[bytes, ...]', packet: 'Optional[Protocol | Deferred]') -> 'None': ... # pylint: disable=unused-argument,super-init-not-called,multiple-statements,line-too-long,redefined-builtin + def __init__(self, completed: 'Completion', id: 'DatagramID[_AT]', index: 'tuple[int, ...]', header: 'bytes', payload: 'bytes | tuple[bytes, ...]', packet: 'Optional[Protocol | Deferred]', conflict: 'tuple[tuple[int, int], ...]') -> 'None': ... # pylint: disable=unused-argument,super-init-not-called,multiple-statements,line-too-long,redefined-builtin @info_final class Buffer(Info, Generic[_AT]): @@ -141,6 +169,13 @@ class Buffer(Info, Generic[_AT]): #: first-arriving fragment", so a later fragment does not extend the #: deadline and this field is never revised once set. timestamp: 'float' + #: Octet ranges, absolute into :attr:`datagram` and both **inclusive**, on + #: which an arriving fragment disagreed with bytes already placed there by + #: an earlier one. Accumulated across every fragment merged into this + #: buffer, in the order the conflicts were found; carried onto + #: :attr:`Datagram.conflict ` + #: verbatim when the buffer is submitted. + conflict: 'list[tuple[int, int]]' if TYPE_CHECKING: - def __init__(self, TDL: 'int', RCVBT: 'bytearray', index: 'list[int]', header: 'bytes', datagram: 'bytearray', timestamp: 'float') -> 'None': ... # pylint: disable=unused-argument,super-init-not-called,multiple-statements,line-too-long,redefined-builtin + def __init__(self, TDL: 'int', RCVBT: 'bytearray', index: 'list[int]', header: 'bytes', datagram: 'bytearray', timestamp: 'float', conflict: 'list[tuple[int, int]]') -> 'None': ... # pylint: disable=unused-argument,super-init-not-called,multiple-statements,line-too-long,redefined-builtin diff --git a/pcapkit/foundation/reassembly/ip.py b/pcapkit/foundation/reassembly/ip.py index 908ddf3e3e..a3737aa29f 100644 --- a/pcapkit/foundation/reassembly/ip.py +++ b/pcapkit/foundation/reassembly/ip.py @@ -118,39 +118,127 @@ def reassembly(self, info: 'Packet[_AT]') -> 'None': header=header, # header buffer datagram=bytearray(65535), # data buffer timestamp=TS, # first-arriving fragment's clock reading + conflict=[], # conflicting octet ranges ) else: # put header into header buffer if not FO: # pylint: disable=else-if-used self._buffer[BUFID].__update__(header=header) + buf = self._buffer[BUFID] + # append packet index - self._buffer[BUFID].index.append(info.num) + buf.index.append(info.num) # put data into data buffer start = FO stop = TL - IHL + FO - self._buffer[BUFID].datagram[start:stop] = info.payload + + # Find where this fragment disagrees with what the buffer already + # holds *before* writing it -- ``buf.RCVBT`` and ``buf.TDL`` still + # describe the state as every earlier fragment left it, which is + # exactly what :meth:`_detect_conflicts` needs. + conflicts = self._detect_conflicts(buf.RCVBT, buf.TDL, buf.datagram, info.payload, start, stop) + if conflicts: + buf.conflict.extend(conflicts) + + # :rfc:`791` is explicit that an overlapping fragment's data "will use + # the more recently arrived copy in the data buffer" -- the opposite of + # TCP's first-write-wins (:rfc:`9293#section-3.10`) -- so the arriving + # payload always overwrites here; ``conflicts`` above is what records + # that it *disagreed* with what it overwrote, which is the part RFC 791 + # leaves unrecorded and this fix adds. + buf.datagram[start:stop] = info.payload # set RCVBT bits (in 8 octets) start = FO // 8 stop = FO // 8 + (TL - IHL + 7) // 8 - self._buffer[BUFID].RCVBT[start:stop] = b'\x01' * (stop - start) + buf.RCVBT[start:stop] = b'\x01' * (stop - start) # get total data length (header excludes) TDL = 0 if not MF: TDL = TL - IHL + FO - self._buffer[BUFID].__update__(TDL=TDL) + buf.__update__(TDL=TDL) # when datagram is reassembled in whole start = 0 stop = (TDL + 7) // 8 - if TDL and all(self._buffer[BUFID].RCVBT[start:stop]): + if TDL and all(buf.RCVBT[start:stop]): self._dtgram.extend( self.submit(self._buffer.pop(BUFID), bufid=BUFID, checked=True) ) + @staticmethod + def _detect_conflicts(rcvbt: 'bytearray', tdl: 'int', datagram: 'bytearray', payload: 'bytearray', + start: 'int', stop: 'int') -> 'list[tuple[int, int]]': + """Find where an arriving fragment disagrees with already-received bytes. + + Arguments: + rcvbt: this buffer's :attr:`~pcapkit.foundation.reassembly.data.ip.Buffer.RCVBT` + as it stood *before* the arriving fragment's own bits are set, + i.e. what earlier fragments had already claimed, in 8-octet + blocks. + tdl: this buffer's :attr:`~pcapkit.foundation.reassembly.data.ip.Buffer.TDL` + as it stood *before* the arriving fragment's own update -- + ``-1`` while the final fragment (``MF=0``) has not yet + arrived. + datagram: this buffer's data buffer, read *before* the arriving + fragment's payload is written into it. + payload: the arriving fragment's payload. + start: absolute octet offset of ``payload[0]`` in ``datagram``, + i.e. this fragment's ``FO``. + stop: absolute octet offset one past ``payload[-1]``, i.e. + ``start + len(payload)``. + + Returns: + ``(first, last)`` absolute octet ranges, inclusive, where + ``datagram`` and ``payload`` disagree over octets this buffer had + already genuinely received. + + :rfc:`791` marks receipt in 8-octet blocks (``RCVBT``), coarser than + the octet granularity a conflict needs: every fragment but the last is + required to be a multiple of 8 octets, and a fragment's ``FO`` is + *always* a multiple of 8 -- it is wire-encoded in 8-octet units -- so a + non-final fragment's range is always exactly block-aligned. The only + block that can be *partially* real is therefore the one holding the + final fragment's own tail: the ``RCVBT`` update in :meth:`reassembly` + sets that block's bit across its full 8 octets even though only the + octets up to ``tdl`` were ever actually written, the rest still being + ``datagram``'s zero-fill. + + So once ``tdl`` is known, an octet at or past it is excluded here + regardless of its block's bit -- comparing it would manufacture a + conflict against a byte nothing ever really sent, over a distinction + :meth:`~pcapkit.foundation.reassembly.ip.IP.submit` does not need + anyway, since it never reports a payload past ``tdl``. Before ``tdl`` + is known (``tdl < 0``), every set ``rcvbt`` bit came from a non-final, + block-aligned fragment and is exact on its own, with nothing to clip. + + """ + conflicts = [] # type: list[tuple[int, int]] + length = stop - start + index = 0 + while index < length: + pos = start + index + if not (rcvbt[pos // 8] and (tdl < 0 or pos < tdl)): + index += 1 + continue + if datagram[pos] == payload[index]: + index += 1 + continue + run_stop = index + 1 + while run_stop < length: + pos = start + run_stop + if not (rcvbt[pos // 8] and (tdl < 0 or pos < tdl)): + break + if datagram[pos] == payload[run_stop]: + break + run_stop += 1 + conflicts.append((start + index, start + run_stop - 1)) + index = run_stop + return conflicts + def submit(self, buf: 'Buffer[_AT]', *, bufid: 'tuple[_AT, _AT, int, TransType]', # type: ignore[override] # pylint: disable=arguments-differ checked: 'bool' = False, timeout: 'bool' = False) -> 'list[Datagram[_AT]]': """Submit reassembled payload. @@ -176,6 +264,7 @@ def submit(self, buf: 'Buffer[_AT]', *, bufid: 'tuple[_AT, _AT, int, TransType]' index = buf.index header = buf.header datagram = buf.datagram + conflict = tuple(buf.conflict) start = 0 stop = (TDL + 7) // 8 @@ -216,6 +305,7 @@ def submit(self, buf: 'Buffer[_AT]', *, bufid: 'tuple[_AT, _AT, int, TransType]' header=header, payload=tuple(data), packet=None, + conflict=conflict, ) ret.append(packet) # if datagram is reassembled in whole -- or if it is not, and ``strict`` @@ -275,6 +365,7 @@ def submit(self, buf: 'Buffer[_AT]', *, bufid: 'tuple[_AT, _AT, int, TransType]' # result most of them never read. ``Deferred`` postpones it to the # first read of ``Datagram.packet``. packet=Deferred(self.protocol.analyze, bufid[3], payload), + conflict=conflict, ) ret.append(packet) diff --git a/tests/foundation/reassembly/data/test_models.py b/tests/foundation/reassembly/data/test_models.py index b8b67e8bf0..33fed7da14 100644 --- a/tests/foundation/reassembly/data/test_models.py +++ b/tests/foundation/reassembly/data/test_models.py @@ -32,17 +32,19 @@ def test_ip_data_models_and_package_aliases(self) -> None: self.assertEqual(packet.timestamp, 1000.0) datagram_id = DatagramID(src, dst, 123, TransType.UDP) - datagram = Datagram(Completion.PARTIAL, datagram_id, (7,), b'ip-header', (b'payload',), None) + datagram = Datagram(Completion.PARTIAL, datagram_id, (7,), b'ip-header', (b'payload',), None, ()) self.assertIsInstance(datagram.id, IP_DatagramID) self.assertIsInstance(datagram, IP_Datagram) self.assertFalse(datagram.completed) self.assertIs(datagram.completed, Completion.PARTIAL) self.assertEqual(datagram.to_dict()['payload'], (b'payload',)) + self.assertEqual(datagram.conflict, ()) - buffer = Buffer(-1, bytearray(b'\x01'), [7], b'ip-header', bytearray(b'payload'), 1000.0) + buffer = Buffer(-1, bytearray(b'\x01'), [7], b'ip-header', bytearray(b'payload'), 1000.0, []) self.assertIsInstance(buffer, IP_Buffer) self.assertEqual(buffer.index, [7]) self.assertEqual(buffer.timestamp, 1000.0) + self.assertEqual(buffer.conflict, []) storage = ReassemblyData((datagram,), (), ()) self.assertEqual(storage.ipv4, (datagram,)) diff --git a/tests/foundation/reassembly/test_ip.py b/tests/foundation/reassembly/test_ip.py index 2e0b6d4e1d..4305a6e29f 100644 --- a/tests/foundation/reassembly/test_ip.py +++ b/tests/foundation/reassembly/test_ip.py @@ -16,13 +16,13 @@ def setUp(self) -> None: purge_modules(['pcapkit']) def _packet(self, *, num: int, fo: int, mf: bool, payload: bytes, - header: bytes = b'ip-header', timestamp: float = 1000.0): + header: bytes = b'ip-header', timestamp: float = 1000.0, ident: int = 42): from pcapkit.const.reg.transtype import TransType from pcapkit.foundation.reassembly.data.ip import Packet src = ip_address('192.0.2.1') dst = ip_address('198.51.100.2') - return Packet((src, dst, 42, TransType.UDP), num, fo, 20, mf, + return Packet((src, dst, ident, TransType.UDP), num, fo, 20, mf, 20 + len(payload), header, bytearray(payload), timestamp) def test_complete_fragmented_datagram_is_submitted_and_analyzed(self) -> None: @@ -103,13 +103,165 @@ class TestIP(IP): dst = ip_address('198.51.100.2') self.assertEqual( empty.submit( - Buffer(-1, bytearray(b'\x00\x00'), [], b'', bytearray(b''), 1000.0), + Buffer(-1, bytearray(b'\x00\x00'), [], b'', bytearray(b''), 1000.0, []), bufid=(src, dst, 42, TransType.UDP), ), [], ) +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class IPOverlapConflictTests(unittest.TestCase): + """:issue:`477` -- an overlapping fragment carrying different bytes must + have the disagreement recorded, not silently overwrite the earlier one. + + :rfc:`791` resolves *which* bytes win itself: "this procedure will use the + more recently arrived copy in the data buffer" -- the opposite of TCP's + first-write-wins (:rfc:`9293#section-3.10`, fixed for TCP by #443/#478). + So every case below still expects the *arriving* fragment's bytes in the + reassembled payload; what changes is that ``datagram.conflict`` now + records where that overwrite disagreed with what was already there. + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def _packet(self, *, num: int, fo: int, mf: bool, payload: bytes, + header: bytes = b'ip-header', timestamp: float = 1000.0, ident: int = 42): + from pcapkit.const.reg.transtype import TransType + from pcapkit.foundation.reassembly.data.ip import Packet + + src = ip_address('192.0.2.1') + dst = ip_address('198.51.100.2') + return Packet((src, dst, ident, TransType.UDP), num, fo, 20, mf, + 20 + len(payload), header, bytearray(payload), timestamp) + + def _reasm(self): + from pcapkit.foundation.reassembly.ip import IP + + class Analyzer: + @classmethod + def analyze(cls, proto: object, payload: bytes) -> object: + return {'proto': proto, 'payload': payload} + + class TestIP(IP): + __protocol_type__ = Analyzer + + return TestIP() + + def test_identical_duplicate_fragment_stays_uncontested(self) -> None: + """A byte-for-byte retransmission is not a conflict.""" + reasm = self._reasm() + reasm(self._packet(num=1, fo=0, mf=True, payload=b'AAAAAAAA', ident=1)) + reasm(self._packet(num=2, fo=0, mf=True, payload=b'AAAAAAAA', ident=1)) + reasm(self._packet(num=3, fo=8, mf=False, payload=b'ZZZZ', ident=1)) + + datagram, = reasm.datagram + self.assertTrue(datagram.completed) + self.assertEqual(datagram.payload, b'AAAAAAAAZZZZ') + self.assertEqual(datagram.conflict, ()) + + def test_conflicting_full_overlap_records_conflict_and_last_write_wins(self) -> None: + """Two block-aligned fragments claiming the same octets, with different data.""" + reasm = self._reasm() + reasm(self._packet(num=1, fo=0, mf=True, payload=b'AAAAAAAA', ident=2)) + reasm(self._packet(num=2, fo=0, mf=True, payload=b'BBBBBBBB', ident=2)) + reasm(self._packet(num=3, fo=8, mf=False, payload=b'ZZZZ', ident=2)) + + datagram, = reasm.datagram + self.assertTrue(datagram.completed) + # per RFC 791, the more recently arrived copy wins + self.assertEqual(datagram.payload, b'BBBBBBBBZZZZ') + self.assertEqual(datagram.conflict, ((0, 7),)) + + def test_conflicting_partial_overlap_extending_left(self) -> None: + """The arriving fragment starts before the buffered one and overlaps its head.""" + reasm = self._reasm() + # base: octets 8-15 + reasm(self._packet(num=1, fo=8, mf=True, payload=b'CCCCCCCC', ident=3)) + # arriving: octets 0-15 -- overlaps 8-15 with different data + reasm(self._packet(num=2, fo=0, mf=True, payload=b'0123456789ABCDEF'.replace( + b'89ABCDEF', b'DDDDDDDD'), ident=3)) + reasm(self._packet(num=3, fo=16, mf=False, payload=b'ZZZZ', ident=3)) + + datagram, = reasm.datagram + self.assertTrue(datagram.completed) + self.assertEqual(datagram.payload, b'01234567DDDDDDDDZZZZ') + self.assertEqual(datagram.conflict, ((8, 15),)) + + def test_conflicting_partial_overlap_extending_right(self) -> None: + """The arriving fragment starts inside the buffered one and overlaps its tail.""" + reasm = self._reasm() + # base: octets 0-15 + reasm(self._packet(num=1, fo=0, mf=True, payload=b'E' * 16, ident=4)) + # arriving: octets 8-23 -- overlaps 8-15 with different data + reasm(self._packet(num=2, fo=8, mf=True, payload=b'F' * 16, ident=4)) + reasm(self._packet(num=3, fo=24, mf=False, payload=b'ZZZZ', ident=4)) + + datagram, = reasm.datagram + self.assertTrue(datagram.completed) + self.assertEqual(datagram.payload, b'E' * 8 + b'F' * 16 + b'ZZZZ') + self.assertEqual(datagram.conflict, ((8, 15),)) + + def test_conflict_inside_final_partial_block_does_not_over_report(self) -> None: + """The genuinely coarse case: a conflict inside the *final* fragment's + partial 8-octet block, where ``RCVBT`` rounds a 3-octet tail up to a + whole 8-octet block. + + Fragment B (the final one) covers only octets 8, 9 and 10, but + ``RCVBT`` marks the whole block covering octets 8-15 as received. + Fragment C then claims the full block (octets 8-15): it genuinely + conflicts with B over 8-10, but 11-15 were never really received by + anyone before C -- they are past ``TDL`` -- so they must not be + reported as conflicting, even though their ``RCVBT`` block reads as + "received". + + """ + reasm = self._reasm() + # keep the buffer open past fragment B below by leaving octets 0-7 + # unreceived until the very end + reasm(self._packet(num=1, fo=16, mf=True, payload=b'A' * 8, ident=5)) + # final fragment: octets 8, 9, 10 only -- sets TDL=11, rounds the + # RCVBT block covering 8-15 to fully "received" + reasm(self._packet(num=2, fo=8, mf=False, payload=b'BBB', ident=5)) + # conflicts with B at 8, 9, 10; 11-15 is new territory, not a conflict + reasm(self._packet(num=3, fo=8, mf=True, payload=b'XXXXXXXX', ident=5)) + # completes the datagram + reasm(self._packet(num=4, fo=0, mf=True, payload=b'D' * 8, ident=5)) + + datagram, = reasm.datagram + self.assertTrue(datagram.completed) + self.assertEqual(datagram.payload, b'D' * 8 + b'XXX') + # exactly the real conflict, not the whole 8-15 block + self.assertEqual(datagram.conflict, ((8, 10),)) + + def test_three_way_conflict_records_each_disagreement(self) -> None: + """A third fragment disagreeing with the second's already-resolved overwrite.""" + reasm = self._reasm() + reasm(self._packet(num=1, fo=0, mf=True, payload=b'A' * 8, ident=6)) + reasm(self._packet(num=2, fo=0, mf=True, payload=b'B' * 8, ident=6)) + reasm(self._packet(num=3, fo=0, mf=True, payload=b'C' * 8, ident=6)) + reasm(self._packet(num=4, fo=8, mf=False, payload=b'ZZZZ', ident=6)) + + datagram, = reasm.datagram + self.assertTrue(datagram.completed) + self.assertEqual(datagram.payload, b'C' * 8 + b'ZZZZ') + self.assertEqual(datagram.conflict, ((0, 7), (0, 7))) + + def test_conflict_is_recorded_even_when_a_later_fragment_completes_the_datagram(self) -> None: + """``completed`` and ``conflict`` are independent: a clean completion + can still carry a recorded conflict from earlier in reassembly.""" + reasm = self._reasm() + reasm(self._packet(num=1, fo=0, mf=True, payload=b'A' * 8, ident=7)) + reasm(self._packet(num=2, fo=0, mf=True, payload=b'B' * 8, ident=7)) + reasm(self._packet(num=3, fo=8, mf=False, payload=b'ZZZZ', ident=7)) + + datagram, = reasm.datagram + self.assertTrue(datagram.completed) + self.assertEqual(datagram.conflict, ((0, 7),)) + + @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class DeferredAnalysisTests(unittest.TestCase): """``Datagram.packet`` is analysed on first read, not at submit time.