fix(reassembly): only the final fragment's partial block counts, and an oversized datagram never completes (#1566, #1567)

docs/source/pcapkit/foundation/reassembly/ip/index.rst

@@ -93,3 +93,11 @@ Algorithm
 Because completion frees the buffer (:rfc:`791#section-3.2`), a fragment that
 arrives after its datagram has completed opens a new buffer, which is later
 reported as a separate incomplete datagram; this is intended (:issue:`1507`).
+
+Two departures from the procedure above:
+
+* Only the final fragment's partial last ``RCVBT`` block is set. A non-final
+  fragment carries a multiple of 8 octets, so one ending mid-block was cut or
+  is malformed, and setting its last block would complete the datagram with
+  octets nobody sent (:issue:`1567`).
+* A datagram longer than the data buffer is never complete (:issue:`1566`).

docs/source/pcapkit/foundation/reassembly/ip/ipv6.rst

@@ -63,6 +63,17 @@ Terminology
           back off. All four adapters (``pcap``, ``pcapng``, ``dpkt`` and
           ``scapy``) agree on the three fields.
 
+       .. note::
+
+          ``tl`` is derived from the captured payload, not from the Payload
+          Length field, so a fragment the snapshot length cut does not show it
+          in ``tl``. A non-final fragment must carry a multiple of 8 octets
+          (:rfc:`8200#section-4.5`), so reassembly does not count the partial
+          last block of one that ends mid-block as received: the datagram
+          stays incomplete, rather than completing with zeros where the cut
+          octets were (:issue:`1567`). A cut *final* fragment cannot be told
+          from a whole one.
+
        .. note::
 
           The reassembly key is the Fragment header's *Identification*

pcapkit/foundation/reassembly/ip.py

@@ -35,14 +35,16 @@ class IP(ReassemblyBase[Packet[_AT], Datagram[_AT], BufferID, Buffer[_AT]], Gene
 
     Args:
         strict: if :data:`True`, report a datagram that is not completely
-            reassembled as the tuple of its received runs; otherwise as one
-            contiguous payload, its holes zero-filled -- or, while the total
-            length is unknown, only the prefix up to the first hole. Receipt
-            is tracked in 8-octet blocks, so the partial last block of a
-            snaplen-truncated fragment is not counted as received: it is left
-            out of the runs and ends the prefix, but once the total length is
-            known the contiguous payload keeps its real octets, with only the
-            rest of the hole zero-filled
+            reassembled as the tuple of its received runs, empty if none was;
+            otherwise as one contiguous payload, its holes zero-filled -- or,
+            while the total length is unknown, only the prefix up to the first
+            hole. Receipt is tracked in 8-octet blocks, and only the final
+            fragment's partial last block counts as received -- a non-final
+            fragment's, or a snaplen-truncated or buffer-clipped fragment's,
+            does not: it is left out of the runs and ends the prefix, but once
+            the total length is known the contiguous payload keeps its real
+            octets, with only the rest of the hole zero-filled. A datagram
+            longer than the data buffer is never complete
         store: if store reassembled datagram in memory, i.e.,
             :attr:`self._dtgram <pcapkit.foundation.reassembly.reassembly.Reassembly._dtgram>`
             (if not, datagram will be discarded after callback)
@@ -168,12 +170,12 @@ def reassembly(self, info: 'Packet[_AT]') -> 'None':
         # grows to hold such a datagram rather than clipping it, ``RCVBT`` in
         # step with it.
         #
-        # With nothing declared, a snaplen cut cannot be seen in the length.
-        # A fragment with **MF** set must carry a multiple of 8 octets, so its
-        # partial last block is a cut and is left unmarked below. Any other cut
-        # -- an unfragmented frame's, or a final fragment's -- cannot be told
-        # from a whole one, since the frame's original length is not part of
-        # the input, and the datagram comes out ``COMPLETE`` but short.
+        # With nothing declared, a snaplen cut cannot be seen in the length. A
+        # non-final fragment's cut still shows, as a partial last block, which
+        # is left unmarked below. Any other cut -- an unfragmented frame's, or
+        # a final fragment's -- cannot be told from a whole one, since the
+        # frame's original length is not part of the input, and the datagram
+        # comes out ``COMPLETE`` but short.
         if TL == 0:
             length = len(info.payload)
             extent = FO + length
@@ -206,13 +208,23 @@ def reassembly(self, info: 'Packet[_AT]') -> 'None':
         buf.datagram[start:stop] = payload
 
         # Set RCVBT bits (in 8 octets) for the blocks this fragment actually
-        # filled. A fragment held in full marks its last block even when it is
-        # short, as the final fragment's is; a truncated one leaves its partial
-        # last block clear, since the octets after the cut are a real hole --
-        # as does a non-final fragment with Total Length 0, whose partial last
-        # block can only be a cut.
+        # filled. Only the final fragment, held in full, marks a partial last
+        # block: the octets after its end are past the datagram's. Every other
+        # partial last block is left clear, as a hole --
+        #
+        # * a truncated fragment's, since the octets after the cut are missing.
+        #   That includes one clipped by the data buffer's end, whose octets
+        #   past it cannot be held, so it counts as cut there (:issue:`1566`).
+        # * a non-final fragment's. :rfc:`791#section-3.2` and
+        #   :rfc:`8200#section-4.5` make every fragment but the last carry a
+        #   multiple of 8 octets, so one that ends mid-block was cut or is
+        #   malformed, and nobody sent the rest of its block. Marking it would
+        #   complete the datagram with zeros in their place. This is the only
+        #   way a cut shows when the length is not declared: under Total
+        #   Length 0, and on IPv6, whose adapters derive it from the captured
+        #   payload (:issue:`1567`).
         start = FO // 8
-        if held == length and not (TL == 0 and MF):
+        if held == length and not MF:
             stop = (FO + held + 7) // 8
         else:
             stop = (FO + held) // 8
@@ -224,16 +236,33 @@ def reassembly(self, info: 'Packet[_AT]') -> 'None':
         # fragment with MF clear has arrived, and ``0`` is a valid length
         if not MF:
             buf.__update__(TDL=length + FO)
-        TDL = buf.TDL
 
         # when datagram is reassembled in whole
-        start = 0
-        stop = (TDL + 7) // 8
-        if TDL >= 0 and all(buf.RCVBT[start:stop]):
+        if self._is_complete(buf):
             self._dtgram.extend(
                 self.submit(self._buffer.pop(BUFID), bufid=BUFID, checked=True)
             )
 
+    @staticmethod
+    def _is_complete(buf: 'Buffer[_AT]') -> 'bool':
+        """Tell whether a buffer holds its whole datagram.
+
+        Arguments:
+            buf: buffer to check
+
+        Returns:
+            Whether the total data length is known, fits the data buffer, and
+            every 8-octet block up to it is marked received.
+
+        A datagram longer than the data buffer cannot be held, so it is never
+        complete. Checking ``RCVBT`` alone does not show that: slicing it past
+        its end stops silently, so the blocks past the buffer would read as
+        received (:issue:`1566`).
+
+        """
+        TDL = buf.TDL
+        return 0 <= TDL <= len(buf.datagram) and all(buf.RCVBT[:(TDL + 7) // 8])
+
     @staticmethod
     def _detect_conflicts(rcvbt: 'bytearray', tdl: 'int', datagram: 'bytearray', payload: 'bytearray',
                            start: 'int', stop: 'int') -> 'list[tuple[int, int]]':
@@ -331,9 +360,7 @@ def submit(self, buf: 'Buffer[_AT]', *, bufid: 'tuple[_AT, _AT, int, TransType]'
         datagram = buf.datagram
         conflict = tuple(buf.conflict)
 
-        start = 0
-        stop = (TDL + 7) // 8
-        flag = checked or (TDL >= 0 and all(RCVBT[start:stop]))
+        flag = checked or self._is_complete(buf)
         ret = []  # type: list[Datagram[_AT]]
 
         # How completely this datagram came out, and why it stopped. Derived once,
@@ -361,23 +388,24 @@ def submit(self, buf: 'Buffer[_AT]', *, bufid: 'tuple[_AT, _AT, int, TransType]'
             # the last run may reach the end of the bit table
             if byte:
                 data.append(bytes(byte))
-            # strip empty packets
-            if data or header:
-                packet = Datagram(
-                    completed=completion,
-                    id=DatagramID(
-                        src=bufid[0],
-                        dst=bufid[1],
-                        id=bufid[2],
-                        proto=bufid[3],
-                    ),
-                    index=tuple(index),
-                    header=header,
-                    payload=tuple(data),
-                    packet=None,
-                    conflict=conflict,
-                )
-                ret.append(packet)
+            # Report it even with no run received, its payload ``()``, as
+            # ``strict=False`` reports it with an empty payload: ``index``
+            # still says which frames were its fragments (:issue:`1566`).
+            packet = Datagram(
+                completed=completion,
+                id=DatagramID(
+                    src=bufid[0],
+                    dst=bufid[1],
+                    id=bufid[2],
+                    proto=bufid[3],
+                ),
+                index=tuple(index),
+                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``
         # asked for one contiguous payload rather than the received runs
         else:

tests/foundation/reassembly/test_ip.py

@@ -98,16 +98,18 @@ class TestIP(IP):
         self.assertEqual(pending._buffer[self._packet(num=0, fo=0, mf=True, payload=b'').bufid].header,
                          b'updated')
 
+        # a buffer with no run received is still reported, as strict=False
+        # reports it (#1566)
         empty = TestIP()
         src = ip_address('192.0.2.1')
         dst = ip_address('198.51.100.2')
-        self.assertEqual(
-            empty.submit(
-                Buffer(-1, bytearray(b'\x00\x00'), [], b'', bytearray(b''), 1000.0, []),
-                bufid=(src, dst, 42, TransType.UDP),
-            ),
-            [],
+        datagram, = empty.submit(
+            Buffer(-1, bytearray(b'\x00\x00'), [], b'', bytearray(b''), 1000.0, []),
+            bufid=(src, dst, 42, TransType.UDP),
         )
+        self.assertFalse(datagram.completed)
+        self.assertEqual(datagram.payload, ())
+        self.assertIsNone(datagram.packet)
 
 
 @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')

tests/foundation/reassembly/test_ip_cut_non_final_fragment_1567_unit.py

@@ -0,0 +1,108 @@
+# -*- coding: utf-8 -*-
+"""A non-final IP fragment that ends mid-block leaves a hole, not zeros.
+
+GitHub issue #1567. The IPv6 adapters derive ``tl`` from the captured payload,
+so a fragment the snapshot length cut declares exactly what was captured, and
+the reassembler marked its partial last block received. A 3-fragment datagram
+with its first fragment cut from 64 octets to 60 then came out ``COMPLETE``,
+octets 60-63 zero-filled; cut to 52, strict mode's first run carried 4
+zero-filled octets.
+
+Every fragment but the last carries a multiple of 8 octets
+(:rfc:`8200#section-4.5`, :rfc:`791#section-3.2`), so only the final fragment's
+partial last block is now counted received. That covers an IPv4 fragment whose
+Total Length declares a non-multiple of 8 on a non-final fragment as well.
+
+Every case builds its fragments in memory and reads no capture. Classes are
+imported inside each test, after :func:`~tests._support.reimport_once_per_class`.
+
+"""
+
+from ipaddress import ip_address
+from itertools import permutations
+import unittest
+
+from tests._support import reimport_once_per_class
+
+#: The datagram, with no zero octet in it, so a hole shows.
+DATA = bytes((i * 7 + 3) % 251 + 1 for i in range(200))
+
+#: Its three fragments, as ``(fragment offset, more fragments, payload)``.
+FRAGMENTS = ((0, True, DATA[0:64]), (64, True, DATA[64:128]), (128, False, DATA[128:200]))
+
+
+class TestIPReassemblyCutNonFinalFragment(unittest.TestCase):
+    """Pin that a cut non-final fragment leaves the datagram incomplete."""
+
+    def setUp(self) -> None:
+        reimport_once_per_class(self)
+
+    def _packet(self, *, num: int, fo: int, mf: bool, payload: bytes, tl: 'int | None' = None):
+        """A fragment whose ``tl`` is, as the IPv6 adapters pass it, its captured length."""
+        from pcapkit.const.reg.transtype import TransType
+        from pcapkit.foundation.reassembly.data.ip import Packet
+
+        bufid = (ip_address('2001:db8::1'), ip_address('2001:db8::2'), 9, TransType.UDP)
+        return Packet(bufid, num, fo, 40, mf, 40 + len(payload) if tl is None else tl,
+                      b'H' * 40, bytearray(payload), 1000.0 + num)
+
+    def _reassemble(self, cls, packets, strict: bool):
+        reasm = cls(strict=strict)
+        for packet in packets:
+            reasm(packet)
+        datagram, = reasm.datagram
+        return datagram
+
+    def test_a_cut_first_ipv6_fragment_leaves_a_hole(self) -> None:
+        from pcapkit.foundation.reassembly.data.data import Completion
+        from pcapkit.foundation.reassembly.ipv6 import IPv6
+
+        for cut in (60, 52):
+            for strict in (True, False):
+                with self.subTest(cut=cut, strict=strict):
+                    packets = [self._packet(num=num, fo=fo, mf=mf, payload=payload[:cut] if fo == 0 else payload)
+                               for num, (fo, mf, payload) in enumerate(FRAGMENTS, 1)]
+                    datagram = self._reassemble(IPv6, packets, strict)
+                    self.assertIs(datagram.completed, Completion.PARTIAL)
+                    self.assertEqual(datagram.index, (1, 2, 3))
+                    if strict:
+                        # the cut fragment's whole blocks only
+                        self.assertEqual(datagram.payload, (DATA[:cut // 8 * 8], DATA[64:]))
+                    else:
+                        # its real octets kept, the rest of the hole zero-filled
+                        self.assertEqual(datagram.payload, DATA[:cut] + bytes(64 - cut) + DATA[64:])
+
+    def test_a_non_final_ipv4_fragment_ending_mid_block_leaves_a_hole(self) -> None:
+        # Total Length 100 declares 60 octets on a non-final fragment, all of
+        # them captured: malformed rather than cut, but nobody sent 60-63.
+        from pcapkit.foundation.reassembly.data.data import Completion
+        from pcapkit.foundation.reassembly.ipv4 import IPv4
+
+        for strict in (True, False):
+            with self.subTest(strict=strict):
+                packets = [self._packet(num=num, fo=fo, mf=mf, payload=payload[:60] if fo == 0 else payload)
+                           for num, (fo, mf, payload) in enumerate(FRAGMENTS, 1)]
+                datagram = self._reassemble(IPv4, packets, strict)
+                self.assertIs(datagram.completed, Completion.PARTIAL)
+                self.assertEqual(datagram.payload,
+                                 (DATA[:56], DATA[64:]) if strict else DATA[:60] + bytes(4) + DATA[64:])
+
+    def test_an_uncut_datagram_completes_byte_exact_in_any_order(self) -> None:
+        from pcapkit.foundation.reassembly.data.data import Completion
+        from pcapkit.foundation.reassembly.ipv4 import IPv4
+        from pcapkit.foundation.reassembly.ipv6 import IPv6
+
+        for cls in (IPv4, IPv6):
+            for order in permutations(range(3)):
+                for strict in (True, False):
+                    with self.subTest(cls=cls.__name__, order=order, strict=strict):
+                        packets = [self._packet(num=index + 1, fo=FRAGMENTS[index][0], mf=FRAGMENTS[index][1],
+                                                payload=FRAGMENTS[index][2]) for index in order]
+                        datagram = self._reassemble(cls, packets, strict)
+                        self.assertIs(datagram.completed, Completion.COMPLETE)
+                        self.assertEqual(datagram.payload, DATA)
+                        self.assertEqual(datagram.conflict, ())
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/foundation/reassembly/test_ip_last_fragment_offset_1565_unit.py

@@ -65,7 +65,13 @@ def test_a_lone_fragment_at_the_largest_offset_is_buffered(self) -> None:
                             datagram, = reasm.datagram
                             self.assertIs(datagram.completed, Completion.PARTIAL)
                             self.assertEqual(datagram.index, (1,))
-                            if strict:
+                            if strict and mf:
+                                # a non-final fragment ending mid-block was cut
+                                # or is malformed, so its block is not received
+                                # (#1567) -- and the datagram is still reported
+                                # (#1566)
+                                self.assertEqual(datagram.payload, ())
+                            elif strict:
                                 run, = datagram.payload
                                 self.assertEqual(run[:size], tail)
                             elif mf:
@@ -112,7 +118,9 @@ def test_a_non_final_fragment_at_the_largest_offset_leaves_the_datagram_open(sel
                             run, = datagram.payload
                         else:
                             run = datagram.payload
-                        self.assertEqual(run[:LAST + size], BODY + tail)
+                        # the tail ends mid-block on a non-final fragment, so
+                        # its block is not received (#1567)
+                        self.assertEqual(run, BODY)
 
     def test_a_tso_fragment_at_the_largest_offset_composes_with_the_buffer_growth(self) -> None:
         # Total Length 0 (#1555): 7 octets end exactly at the preallocated