Fix TCP reassembly coordinates (#349); add an end-to-end test tier

examples/generators/legacy.py

@@ -58,10 +58,12 @@
    data of its own. Both fixtures therefore keep each direction of each
    connection to a single HTTP message, and let the peer answer only with pure
    acknowledgements until that message is complete.
-3. A datagram counts as complete when the hole descriptor list is down to two
-   entries or fewer, which an ordered run of segments plus a FIN or RST
-   achieves; an out-of-order segment adds a third entry that the missing
-   segment's arrival then removes.
+3. A datagram counts as complete when no hole in the descriptor list falls
+   inside the octets that direction actually received. An ordered run of
+   segments leaves only the open-ended hole past the last octet, which is
+   outside the payload buffer and so does not count; an out-of-order segment
+   opens a hole in the middle, and the missing segment's arrival closes it
+   again.
 4. The payload of a complete datagram goes to
    :meth:`pcapkit.protocols.transport.transport.Transport.analyze`, which
    picks the application protocol from the two port numbers. Port 80 is what
@@ -71,17 +73,28 @@
 oversight and is not: neither fixture contains a datagram that reassembles
 *incompletely*, even though ``test_reassembly.py`` and ``test_analyse.py`` both
 have a branch for one -- a payload that is a tuple of received fragments, and a
-``packet`` of :data:`None`. That branch cannot be reached from a realistic
-capture. ``submit`` slices the payload buffer with the bounds of each hole, but
-those bounds are absolute TCP sequence numbers (``first=tcp_info.seq`` in
-``pcapkit/toolkit/pcap.py``) while the buffer is indexed from the start of the
-direction's data, so with any real initial sequence number every slice starts
-far beyond the end of the buffer, every fragment comes out empty, and ``if
-data:`` discards the datagram without a word. Measured on a probe capture with
-one segment permanently missing: a realistic initial sequence number yields no
-datagram for that direction at all, and only an initial sequence number of zero
-produces the tuple these scripts print. A fixture cannot have both a real
-handshake and that branch, so it has the real handshake.
+``packet`` of :data:`None`. Nothing prevents that branch any more; it is simply
+that every stream in these two captures arrives whole. Each direction here is
+sent in full and every segment eventually delivered, some of them out of order
+and one retransmitted, so the holes that open all close again before the FIN or
+RST that submits the buffer.
+
+It used to be that no capture could reach that branch at all, which is how the
+absence started. ``submit`` sliced the payload buffer with the bounds of each
+hole, but those bounds were absolute TCP sequence numbers
+(``first=tcp_info.seq`` in ``pcapkit/toolkit/pcap.py``) while the buffer is
+indexed from the start of the direction's data, so with any real initial
+sequence number every slice started far beyond the end of the buffer, every
+fragment came out empty, and ``if data:`` discarded the datagram without a word.
+That is fixed -- ``submit`` now converts each hole's absolute bounds into
+offsets into the buffer it is reading, and decides completeness from whether any
+hole survives that conversion -- and GitHub issue #349 records the whole of it.
+Regenerating these two fixtures to carry a permanently lost segment as well was
+considered and rejected: they are pinned byte-for-byte by the test suite, and a
+stream with a hole in it is cheaper to build segment by segment than to read out
+of a capture. ``tests/foundation/reassembly/test_tcp.py`` therefore covers the
+incomplete branch directly, with realistic initial sequence numbers, while
+these captures go on covering the complete one end to end.
 
 """
 

pcapkit/foundation/reassembly/data/tcp.py

@@ -45,9 +45,12 @@ class Packet(Info):
     rst: 'bool'
     #: Payload length, header excluded.
     len: 'int'
-    #: This sequence number.
+    #: Sequence number of the first octet of :attr:`payload`, i.e. the segment's
+    #: own sequence number. Absolute, not an offset into any payload buffer.
     first: 'int'
-    #: Next (wanted) sequence number.
+    #: Sequence number of the last octet of :attr:`payload`, i.e. ``first +
+    #: len - 1``. **Inclusive**, so a segment carrying no payload at all has
+    #: :attr:`last` one below :attr:`first`.
     last: 'int'
     #: Raw :obj:`bytes` type header.
     header: 'bytes'
@@ -102,11 +105,21 @@ def __init__(self, completed: 'bool', id: 'DatagramID[_AT]', index: 'tuple[int,
 
 @info_final
 class HoleDescriptor(Info):
-    """Data model for :term:`TCP <reasm.tcp.buffer>` hole descriptor."""
+    """Data model for :term:`TCP <reasm.tcp.buffer>` hole descriptor.
 
-    #: Start of hole.
+    Both bounds are **absolute TCP sequence numbers** and both are
+    **inclusive**, so a hole covers ``last - first + 1`` octets. They are not
+    offsets into :attr:`Fragment.raw`: the descriptor list is kept once per
+    buffer ID, whereas each acknowledgement number's payload buffer carries an
+    initial sequence number of its own, so only
+    :meth:`TCP.submit <pcapkit.foundation.reassembly.tcp.TCP.submit>` -- which
+    knows which buffer it is looking at -- can convert one to the other.
+
+    """
+
+    #: Sequence number of the first missing octet.
     first: 'int'
-    #: Stop of hole.
+    #: Sequence number of the last missing octet, inclusive.
     last: 'int'
 
     if TYPE_CHECKING:
@@ -119,7 +132,11 @@ class Fragment(Info):
 
     #: List of reassembled packets.
     ind: 'list[int]'
-    #: ISN of payload buffer.
+    #: Sequence number of the octet held in ``raw[0]``, i.e. the origin this
+    #: buffer is indexed from: ``raw[n]`` holds the octet whose sequence number
+    #: is ``isn + n``. Revised downwards whenever a segment turns up below the
+    #: data already buffered, so it is not necessarily the connection's own
+    #: initial sequence number.
     isn: 'int'
     #: Length of payload buffer.
     len: 'int'

pcapkit/foundation/reassembly/tcp.py

@@ -42,6 +42,24 @@ class TCP(Reassembly[Packet, Datagram, BufferID, Buffer]):
         # Fetch result:
         >>> result = tcp_reassembly.datagram
 
+    Note:
+        There are two coordinate systems in play here, and keeping them apart
+        matters. The :term:`hole descriptor list <reasm.tcp.buffer>` of
+        :rfc:`815` is kept in **absolute TCP sequence numbers**, inclusive of
+        both bounds, because a hole belongs to the connection's sequence space
+        for that direction and not to any one payload buffer: the list is held
+        once per buffer ID, while each acknowledgement number gets a payload
+        buffer of its own with an initial sequence number of its own, and that
+        initial sequence number is revised whenever a segment turns up below
+        the data already buffered. A payload buffer, on the other hand, is
+        indexed from zero, such that
+        :attr:`buffer.raw[n] <pcapkit.foundation.reassembly.data.tcp.Fragment.raw>`
+        holds the octet with sequence number
+        :attr:`buffer.isn <pcapkit.foundation.reassembly.data.tcp.Fragment.isn>`
+        ``+ n``. :meth:`submit` is therefore the one place that converts
+        between the two, subtracting that buffer's initial sequence number from
+        each hole bound.
+
     """
     if TYPE_CHECKING:
         protocol: 'Type[TCP_Protocol]'
@@ -77,6 +95,14 @@ def reassembly(self, info: 'Packet') -> 'None':
         RST = info.rst      # Reset Connection Flag (Termination)
         SYN = info.syn      # Synchronise Flag (Establishment)
 
+        # Sequence number of the first octet of this segment's payload. A SYN
+        # occupies a sequence number of its own (:rfc:`793`), so payload sent
+        # by or after a SYN starts at ``dsn + 1`` rather than at ``dsn``.
+        # Without this the octet the SYN spends becomes a zero byte at the head
+        # of the payload buffer, and every complete datagram of a connection
+        # whose handshake was captured comes back one octet too long.
+        PSN = DSN + 1 if SYN else DSN
+
         # when SYN is set, reset buffer of existing session
         if SYN and BUFID in self._buffer:
             self._dtgram.extend(
@@ -88,7 +114,11 @@ def reassembly(self, info: 'Packet') -> 'None':
             self._buffer[BUFID] = Buffer(
                 hdl=[
                     HoleDescriptor(
-                        first=info.len,
+                        # everything from the octet after this segment onwards
+                        # is still missing -- in absolute sequence numbers, so
+                        # that the bound stays valid for every payload buffer
+                        # under this buffer ID
+                        first=PSN + info.len,
                         last=sys.maxsize,
                     ),
                 ],
@@ -98,7 +128,7 @@ def reassembly(self, info: 'Packet') -> 'None':
                         ind=[
                             info.num,
                         ],
-                        isn=info.dsn,
+                        isn=PSN,
                         len=info.len,
                         raw=info.payload,
                     ),
@@ -111,7 +141,7 @@ def reassembly(self, info: 'Packet') -> 'None':
                     ind=[
                         info.num,
                     ],
-                    isn=info.dsn,
+                    isn=PSN,
                     len=info.len,
                     raw=info.payload,
                 )
@@ -126,18 +156,18 @@ def reassembly(self, info: 'Packet') -> 'None':
                 # record fragment payload
                 ISN = self._buffer[BUFID].ack[ACK].isn       # Initial Sequence Number
                 RAW = self._buffer[BUFID].ack[ACK].raw       # Raw Payload Data
-                if DSN >= ISN:  # if fragment goes after existing payload
+                if PSN >= ISN:  # if fragment goes after existing payload
                     LEN = self._buffer[BUFID].ack[ACK].len
-                    GAP = DSN - (ISN + LEN)     # gap length between payloads
+                    GAP = PSN - (ISN + LEN)     # gap length between payloads
                     if GAP >= 0:    # if fragment goes after existing payload
                         RAW += bytearray(GAP) + info.payload
                     else:           # if fragment partially overlaps existing payload
-                        RAW[DSN - ISN:DSN - ISN + info.len] = info.payload
+                        RAW[PSN - ISN:PSN - ISN + info.len] = info.payload
                 else:           # if fragment exceeds existing payload
                     LEN = info.len
-                    GAP = ISN - (DSN + LEN)     # gap length between payloads
+                    GAP = ISN - (PSN + LEN)     # gap length between payloads
                     self._buffer[BUFID].ack[ACK].__update__(
-                        isn=DSN,
+                        isn=PSN,
                     )
                     if GAP >= 0:    # if fragment exceeds existing payload
                         RAW = info.payload + bytearray(GAP) + RAW
@@ -151,28 +181,36 @@ def reassembly(self, info: 'Packet') -> 'None':
                 )
 
             # update hole descriptor list
-            HDL = self._buffer[BUFID].hdl                          # HDL alias
-            for (index, hole) in enumerate(HDL):                   # step one
-                if info.first > hole.last:                         # step two
-                    continue
-                if info.last < hole.first:                         # step three
-                    continue
-                del HDL[index]                                     # step four
-                if info.first > hole.first:                        # step five
-                    new_hole = HoleDescriptor(
-                        first=hole.first,
-                        last=info.first - 1,
-                    )
-                    HDL.insert(index, new_hole)
-                    index += 1
-                if info.last < hole.last and not FIN and not RST:  # step six
-                    new_hole = HoleDescriptor(
-                        first=info.last + 1,
-                        last=hole.last
-                    )
-                    HDL.insert(index, new_hole)
-                break                                              # step seven
-            #self._buffer[BUFID].hdl = HDL                         # update HDL
+            #
+            # A segment carrying no payload -- a bare acknowledgement, SYN, FIN
+            # or RST -- fills no hole, so it must not be run through the
+            # :rfc:`815` algorithm: its ``last`` lies one below its ``first``,
+            # and letting that through would split whichever hole contains it
+            # into two adjacent holes covering the very same octets, growing
+            # the list without bound on a long-lived connection.
+            if info.len > 0:
+                HDL = self._buffer[BUFID].hdl                          # HDL alias
+                for (index, hole) in enumerate(HDL):                   # step one
+                    if info.first > hole.last:                         # step two
+                        continue
+                    if info.last < hole.first:                         # step three
+                        continue
+                    del HDL[index]                                     # step four
+                    if info.first > hole.first:                        # step five
+                        new_hole = HoleDescriptor(
+                            first=hole.first,
+                            last=info.first - 1,
+                        )
+                        HDL.insert(index, new_hole)
+                        index += 1
+                    if info.last < hole.last and not FIN and not RST:  # step six
+                        new_hole = HoleDescriptor(
+                            first=info.last + 1,
+                            last=hole.last
+                        )
+                        HDL.insert(index, new_hole)
+                    break                                              # step seven
+                #self._buffer[BUFID].hdl = HDL                         # update HDL
 
         # when FIN/RST is set, submit buffer of this session
         if FIN or RST:
@@ -196,17 +234,34 @@ def submit(self, buf: 'Buffer', *, bufid: 'BufferID') -> 'list[Datagram]':  # ty
 
         # check through every buffer with ACK
         for (ack, buffer) in buf.ack.items():
+            # Translate the hole descriptor list, which is kept in absolute
+            # sequence numbers for the whole direction, into offsets into this
+            # payload buffer, which is indexed from its own initial sequence
+            # number. Holes lying wholly outside this buffer -- the open-ended
+            # one past the last octet received, and any belonging to a
+            # different acknowledgement number's data -- drop out here; those
+            # straddling an edge are clipped to it rather than being allowed to
+            # index from the far end of the buffer as a negative bound would.
+            length = len(buffer.raw)
+            holes = []  # type: list[tuple[int, int]]
+            for hole in HDL:
+                start = hole.first - buffer.isn       # inclusive lower bound
+                stop = hole.last - buffer.isn + 1     # exclusive upper bound
+                if stop <= 0 or start >= length:
+                    continue                          # hole misses this buffer
+                holes.append((max(start, 0), min(stop, length)))
+            holes.sort()
+
             # if this buffer is not implemented
             # go through every hole and extract received payload
-            if len(HDL) > 2 and self._flag_s:
+            if holes and self._flag_s:
                 data = []  # type: list[bytes]
-                start = stop = 0
-                for hole in HDL:
-                    stop = hole.first
-                    byte = buffer.raw[start:stop]
-                    start = hole.last + 1
+                start = 0
+                for (hole_start, hole_stop) in holes:
+                    byte = buffer.raw[start:hole_start]
                     if byte:    # strip empty payload
-                        data.append(byte)
+                        data.append(bytes(byte))
+                    start = max(start, hole_stop)
                 byte = buffer.raw[start:]
                 if byte:    # strip empty payload
                     data.append(bytes(byte))

pcapkit/toolkit/dpkt.py

@@ -251,8 +251,8 @@ def tcp_reassembly(packet: 'Packet', *, count: 'int' = -1) -> 'TCP_Packet | None
             fin=bool(int(flags[7])),                            # finish flag
             header=tcp.pack()[:tcp.__hdr_len__],                # raw bytes type header
             payload=bytearray(tcp.pack()[tcp.__hdr_len__:]),    # raw bytearray type payload
-            first=tcp.seq,                                      # this sequence number
-            last=tcp.seq + raw_len,                             # next (wanted) sequence number
+            first=tcp.seq,                                      # first sequence number of payload
+            last=tcp.seq + raw_len - 1,                         # last sequence number of payload
             len=raw_len,                                        # payload length, header excludes
         )
         return data

pcapkit/toolkit/pcap.py

@@ -163,8 +163,8 @@ def tcp_reassembly(frame: 'Frame') -> 'TCP_Packet | None':
             rst=tcp_info.flags.rst,                 # reset connection flag
             header=tcp.packet.header,               # raw bytes type header
             payload=bytearray(tcp.packet.payload),  # raw bytearray type payload
-            first=tcp_info.seq,                     # this sequence number
-            last=tcp_info.seq + raw_len,            # next (wanted) sequence number
+            first=tcp_info.seq,                     # first sequence number of payload
+            last=tcp_info.seq + raw_len - 1,        # last sequence number of payload
             len=raw_len,                            # payload length, header excludes
         )
         return data

pcapkit/toolkit/pcapng.py

@@ -169,8 +169,8 @@ def tcp_reassembly(frame: 'PCAPNG') -> 'TCP_Packet | None':
             rst=tcp_info.flags.rst,                 # reset connection flag
             header=tcp.packet.header,               # raw bytes type header
             payload=bytearray(tcp.packet.payload),  # raw bytearray type payload
-            first=tcp_info.seq,                     # this sequence number
-            last=tcp_info.seq + raw_len,            # next (wanted) sequence number
+            first=tcp_info.seq,                     # first sequence number of payload
+            last=tcp_info.seq + raw_len - 1,        # last sequence number of payload
             len=raw_len,                            # payload length, header excludes
         )
         return data

pcapkit/toolkit/scapy.py

@@ -250,8 +250,8 @@ def tcp_reassembly(packet: 'Packet', *, count: 'int' = -1) -> 'TCP_Packet | None
             rst=bool(tcp.flags.R),                  # reset connection flag
             header=bytes(tcp)[:tcp.dataofs * 4],    # raw bytes type header
             payload=bytearray(bytes(tcp.payload)),  # raw bytearray type payload
-            first=tcp.seq,                          # this sequence number
-            last=tcp.seq + raw_len,                 # next (wanted) sequence number
+            first=tcp.seq,                          # first sequence number of payload
+            last=tcp.seq + raw_len - 1,             # last sequence number of payload
             len=raw_len,                            # payload length, header excludes
         )
         return data