corekit: stop the option and list loops spinning forever on a truncated area (#431)

pcapkit/corekit/fields/collections.py

@@ -126,6 +126,10 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte
         Returns:
             Unpacked field value.
 
+        Raises:
+            FieldValueError: If the items overrun the field, or if a schema item
+                consumes nothing from ``buffer`` -- see the note below.
+
         """
         length = self._length
         if isinstance(buffer, bytes):
@@ -139,13 +143,47 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte
         from pcapkit.corekit.fields.misc import SchemaField
         is_schema = isinstance(self._item_type, SchemaField)
 
+        # NOTE: The item-typed branch below sizes each item by ``field.length``,
+        # which is what it read, but the schema branch sizes it by ``len(data)``,
+        # which is only what the schema *recorded*. A schema reading a stream that
+        # has already run out records nothing, so ``length -= len(data)`` makes no
+        # progress and the loop spins forever. Reachable from a TCP segment: a
+        # ``SACK`` option declaring more octets than the option area holds leaves
+        # ``sack``'s ``ListField`` reading ``SACKBlock`` off an exhausted stream.
+        # Remembering where the previous item ended is what bounds the iteration
+        # count, since it does not depend on what the schema reports. C.f. #431,
+        # which is the same defect in the ``OptionField`` subclass.
+        #
+        # ``start`` is where the field itself begins. The comparison needs stream
+        # positions, but the diagnostic wants an offset into the field, and the two
+        # only coincide when the field happens to be reading from the front of its
+        # stream -- which it does when handed a :obj:`bytes` buffer and does not
+        # when handed a live file.
+        start = offset = file.tell()
+
         temp = []  # type: list[_TL]
         while length > 0:
             field = self._item_type(packet)
 
             if is_schema:
                 data = cast('SchemaField', self._item_type).unpack(file, packet)
 
+                end = file.tell()
+                if end <= offset:
+                    # NOTE: ``len(temp)`` counts the items already parsed, so it
+                    # names the failing one as a count rather than as an ordinal --
+                    # "after 2 item(s)" rather than "item 2", which would read as
+                    # the second item when it is the third. The ``OptionField``
+                    # message below names the option code in this slot and so has
+                    # no index to be read either way.
+                    raise FieldValueError(
+                        f'Field {self.name} has an item that consumed no data: '
+                        f'after {len(temp)} item(s), at offset {offset - start} of '
+                        f'{self._length}, with {length} octet(s) of the field '
+                        f'left to parse'
+                    )
+                offset = end
+
                 length -= len(data)
                 if length < 0:
                     raise FieldValueError(f'Field {self.name} has invalid length.')
@@ -296,13 +334,41 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list
             as the remaining length to the ``packet`` argument such that
             the next fields can be aware of such informations.
 
+        Raises:
+            FieldValueError: If an option consumes nothing from ``buffer``, since
+                the loop below has then no way to get past it.
+
         """
         length = self._length
         if isinstance(buffer, bytes):
             file = io.BytesIO(buffer)  # type: IO[bytes]
         else:
             file = buffer
 
+        # NOTE: The loop below sizes each option by ``len(data)`` -- the size of
+        # the schema the option reported -- and that is not always the number of
+        # octets the option took from ``file``. The two part company for a schema
+        # whose ``post_process`` returns a *nested* schema, since ``len(data)``
+        # then measures the nested schema rather than what the outer one read. So
+        # ``len(data)`` cannot be the loop's progress measure: an option that
+        # over-reads leaves ``length`` above zero with ``file`` already exhausted,
+        # every field of the next option reads ``b''``, and an option that read
+        # nothing reports ``len(data) == 0`` and leaves ``length`` untouched --
+        # which spins forever, with no exception and no diagnostic. C.f. #431.
+        #
+        # Remembering where the previous option ended gives the loop a measure of
+        # progress that does not depend on what a schema reports, and one octet of
+        # it per iteration is what bounds the iteration count. ``length`` is still
+        # decremented by ``len(data)``, so that an option area which parses today
+        # parses identically.
+        #
+        # ``start`` is where the option area itself begins. The comparison needs
+        # stream positions, but the diagnostic wants an offset into the area, and
+        # the two only coincide when the field happens to be reading from the front
+        # of its stream -- which it does when handed a :obj:`bytes` buffer and does
+        # not when handed a live file.
+        start = offset = file.tell()
+
         # make a copy of the ``packet`` dict so that we can include
         # parsed option schema in the ``packet`` dict
         new_packet = packet.copy()
@@ -362,5 +428,32 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list
             if code == self._eool:
                 break
 
+            # NOTE: The progress check comes *after* the end-of-option-list break,
+            # and that order is not incidental. An area declared longer than the
+            # octets behind it -- an over-long ``ihl``, or a capture cut short by
+            # the snapshot length -- exhausts ``file`` early, and the exhausted
+            # read then decodes the type field as 0. For the IPv4, TCP and PCAP-NG
+            # registries 0 *is* the end-of-option-list code, so the break above has
+            # always absorbed that case and reported the rest of the area as
+            # padding. Checking progress first turned all of those into errors:
+            # measured on ``IPv4(bytes.fromhex('4a00001800010000400600000a0000010a000002'))``,
+            # 20 octets of options declared with none present, and on a TCP segment
+            # with a data offset of 10 and four option octets, both of which parse
+            # on ``main``.
+            #
+            # The registries that spin are the ones where 0 is *not* the
+            # end-of-option-list code, so they never reach the break: HOPOPT,
+            # IPv6-Opts and MH read 0 as ``Pad1``, HIP as an unassigned parameter,
+            # SCTP as a DATA chunk. Those are what this guards, and one octet of
+            # measured progress per surviving iteration is what bounds the loop.
+            end = file.tell()
+            if end <= offset:
+                raise FieldValueError(
+                    f'Field {self.name} has an option that consumed no data: '
+                    f'{code!r} at offset {offset - start} of {self._length}, with '
+                    f'{length} octet(s) of the option area left to parse'
+                )
+            offset = end
+
         self._option_padding = length
         return temp

pcapkit/protocols/schema/internet/hopopt.py

@@ -155,12 +155,22 @@ def smf_dpd_data_selector(pkt: 'dict[str, Any]') -> 'Field':
           wrapped :class:`~pcapkit.protocols.schema.internet.hopopt.SMFHashBasedDPDOption`
           instance.
 
+    Note:
+        The field is sized ``Opt Data Len + 2`` rather than ``Opt Data Len``.
+        ``Opt Data Len`` counts only what follows the option header
+        [:rfc:`8200#section-4.2`], while both schemas this may return inherit
+        :attr:`Option.type` and :attr:`Option.len` and so parse those two octets
+        themselves. Sizing the field at ``Opt Data Len`` handed them an area two
+        octets short of the option they read, which
+        :class:`~pcapkit.corekit.fields.collections.OptionField` then mis-counted
+        against the option area -- c.f. #431.
+
     """
     mode = Enum_SMFDPDMode.get(pkt['test']['mode'])
     schema = SMFDPDOption.registry[mode]
     if schema is None:
         raise FieldValueError(f'HOPOPT: invalid SMF DPD mode: {mode}')
-    return SchemaField(length=pkt['test']['len'], schema=schema)
+    return SchemaField(length=pkt['test']['len'] + 2, schema=schema)
 
 
 def smf_i_dpd_tid_selector(pkt: 'dict[str, Any]') -> 'Field':
@@ -367,11 +377,21 @@ def __init__(self, type: 'Enum_Option', len: 'int', domain: 'int', cmpt_len: 'in
 
 @schema_final
 class _SMFDPDOption(Schema):
-    """Header schema for HOPOPT SMF DPD options with generic representation."""
+    """Header schema for HOPOPT SMF DPD options with generic representation.
+
+    The ``test`` field forward-matches the first three octets of the option --
+    ``Option Type``, ``Opt Data Len`` and the octet carrying the DPD mode bit --
+    without consuming them, so that :func:`smf_dpd_data_selector` can size and
+    choose the schema which then reads the option properly. Its ``namespace``
+    offsets are therefore bit offsets into the *option*, not into any one field
+    of it: ``Opt Data Len`` is octet 1, i.e. bits 8 to 15, and the mode bit is
+    the first bit of octet 2 [:rfc:`6621#section-8.1`].
+
+    """
 
     #: SMF DPD mode.
     test: 'SMFDPDTestFlag' = ForwardMatchField(BitField(length=3, namespace={
-        'len': (1, 8),
+        'len': (8, 8),
         'mode': (16, 1),
     }))
     #: SMF DPD data.

pcapkit/protocols/schema/internet/ipv6_opts.py

@@ -155,12 +155,22 @@ def smf_dpd_data_selector(pkt: 'dict[str, Any]') -> 'Field':
           wrapped :class:`~pcapkit.protocols.schema.internet.ipv6_opts.SMFHashBasedDPDOption`
           instance.
 
+    Note:
+        The field is sized ``Opt Data Len + 2`` rather than ``Opt Data Len``.
+        ``Opt Data Len`` counts only what follows the option header
+        [:rfc:`8200#section-4.2`], while both schemas this may return inherit
+        :attr:`Option.type` and :attr:`Option.len` and so parse those two octets
+        themselves. Sizing the field at ``Opt Data Len`` handed them an area two
+        octets short of the option they read, which
+        :class:`~pcapkit.corekit.fields.collections.OptionField` then mis-counted
+        against the option area -- c.f. #431.
+
     """
     mode = Enum_SMFDPDMode.get(pkt['test']['mode'])
     schema = SMFDPDOption.registry[mode]
     if schema is None:
         raise FieldValueError(f'IPv6-Opts: invalid SMF DPD mode: {mode}')
-    return SchemaField(length=pkt['test']['len'], schema=schema)
+    return SchemaField(length=pkt['test']['len'] + 2, schema=schema)
 
 
 def smf_i_dpd_tid_selector(pkt: 'dict[str, Any]') -> 'Field':
@@ -367,11 +377,21 @@ def __init__(self, type: 'Enum_Option', len: 'int', domain: 'int', cmpt_len: 'in
 
 @schema_final
 class _SMFDPDOption(Schema):
-    """Header schema for IPv6-Opts SMF DPD options with generic representation."""
+    """Header schema for IPv6-Opts SMF DPD options with generic representation.
+
+    The ``test`` field forward-matches the first three octets of the option --
+    ``Option Type``, ``Opt Data Len`` and the octet carrying the DPD mode bit --
+    without consuming them, so that :func:`smf_dpd_data_selector` can size and
+    choose the schema which then reads the option properly. Its ``namespace``
+    offsets are therefore bit offsets into the *option*, not into any one field
+    of it: ``Opt Data Len`` is octet 1, i.e. bits 8 to 15, and the mode bit is
+    the first bit of octet 2 [:rfc:`6621#section-8.1`].
+
+    """
 
     #: SMF DPD mode.
     test: 'SMFDPDTestFlag' = ForwardMatchField(BitField(length=3, namespace={
-        'len': (1, 8),
+        'len': (8, 8),
         'mode': (16, 1),
     }))
     #: SMF DPD data.

tests/_support.py

@@ -2,17 +2,97 @@
 
 import abc
 import collections.abc
+import contextlib
 import importlib.util
 import inspect
+import math
 import pathlib
+import signal
 import sys
+import time
 import types
-from typing import Iterable
+import unittest
+from typing import Iterable, Iterator
 
 from tests._tiers import (ROOT, SAMPLE_ROOT, REGENERATE_SAMPLES_CMD,
                           GeneratedFixtureInUnitTierError, check_unit_tier_read)
 
 
+@contextlib.contextmanager
+def time_limit(seconds: int = 5) -> Iterator[None]:
+    """Fail the calling test if its body has not finished in ``seconds`` seconds.
+
+    A parser defect that degenerates into a loop making no progress -- GitHub
+    issue #431 is one -- offers a test nothing to assert on: the call under test
+    simply never returns. A test written for it without a deadline does not fail,
+    it *wedges*, taking the rest of the run with it, so the deadline is as much a
+    part of the regression test as the assertion is.
+
+    :func:`signal.alarm` is what interrupts the body, rather than a watchdog
+    thread: the loops this guards are pure Python and hold the GIL for the whole
+    of an iteration, so nothing in another thread gets to run and stop them,
+    whereas a signal is delivered between bytecodes. That also rules out
+    :data:`signal.SIGTERM` from an outer :program:`timeout`, which such a loop
+    likewise never gets around to handling.
+
+    There is only ever one pending alarm per process, so arming this one cancels
+    whatever was already scheduled -- an enclosing ``time_limit``, or a deadline the
+    test runner set for itself. Both the handler and that pending alarm are put back
+    on the way out, the alarm with the seconds spent in the body deducted, so an
+    enclosing deadline keeps counting down across the ``with`` rather than being
+    silently dropped.
+
+    An enclosing deadline whose moment falls inside the body is not delivered on
+    time, and the reason is this helper rather than the body: arming an alarm
+    *replaces* the pending one, so the enclosing deadline was already cancelled
+    before the body began and there was nothing left to fire when it came due. It
+    is re-armed for one second on the way out rather than dropped -- honouring it
+    late is the lesser wrong, and dropping it is how an enclosing timeout goes
+    missing altogether. That one-second floor covers every case where the body ran
+    for longer than the enclosing deadline had left.
+
+    Args:
+        seconds: Whole seconds to allow the body. :func:`signal.alarm` counts in
+            whole seconds, so this cannot usefully be fractional.
+
+    Yields:
+        Nothing. The deadline applies to the body of the ``with`` statement.
+
+    Raises:
+        TimeoutError: If the body has not finished within ``seconds`` seconds.
+
+    """
+    # An interval timer is a POSIX facility, and the deadline is the whole point
+    # of this helper: silently running the body without one would restore exactly
+    # the wedged run it exists to prevent, so the test is skipped instead.
+    if not hasattr(signal, 'SIGALRM'):
+        raise unittest.SkipTest('signal.alarm is unavailable on this platform')
+
+    def expire(signum: int, frame: object) -> None:
+        raise TimeoutError(f'did not finish within {seconds}s')
+
+    previous_handler = signal.signal(signal.SIGALRM, expire)
+
+    # NOTE: ``signal.alarm`` returns the seconds left on the alarm it replaces, or
+    # zero when there was none. That return value is the only record of an
+    # enclosing deadline, so it is read here rather than discarded -- there is no
+    # way to ask for it again afterwards.
+    pending = signal.alarm(seconds)
+    started = time.monotonic()
+    try:
+        yield
+    finally:
+        # Cancel first, so that an alarm which fires between here and the handler
+        # being restored cannot be delivered to whatever handler was installed
+        # before -- and so that the alarm re-armed below belongs to that handler
+        # rather than to ``expire``.
+        signal.alarm(0)
+        signal.signal(signal.SIGALRM, previous_handler)
+        if pending:
+            left = pending - (time.monotonic() - started)
+            signal.alarm(max(1, math.ceil(left)))
+
+
 def sample_path(name: str) -> str:
     """Resolve a sample capture file name to its absolute path.
 

tests/protocols/internet/test_ipv4_unit.py

@@ -833,6 +833,34 @@ def test_ipv4_schema_helpers_and_post_process_branches(self) -> None:
         self.assertEqual(tuple(unknown.timestamp), (1,))
         warn.assert_called_once()
 
+    def test_an_option_area_longer_than_the_datagram_still_parses(self) -> None:
+        """An ``ihl`` promising more options than are there is tolerated.
+
+        The header below sets ``ihl`` to 10 -- a 20-octet option area -- and stops
+        after the fixed 20 octets, so there are no option octets at all. Reading
+        past them yields ``b''``, which decodes the option number as 0, and 0 is
+        IPv4's end-of-option-list, so the option loop breaks there and reports the
+        whole area as padding. That is how a datagram cut short by the snapshot
+        length parses at all, and it is why :meth:`OptionField.unpack
+        <pcapkit.corekit.fields.collections.OptionField.unpack>` checks each
+        option's progress *after* its end-of-option-list break rather than before:
+        checking first turns every such header into an error. C.f. #431.
+
+        """
+        from pcapkit.const.ipv4.option_number import OptionNumber
+        from pcapkit.protocols.internet.ipv4 import IPv4
+        from tests._support import time_limit
+
+        raw = bytes.fromhex('4a00001800010000400600000a0000010a000002')
+        with time_limit(5):
+            proto = IPv4(raw, len(raw))
+
+        self.assertEqual(proto.info.hdr_len, 40)
+        self.assertEqual(
+            [(code, opt.length) for code, opt in proto.info.options.items(multi=True)],
+            [(OptionNumber.EOOL, 1)],
+        )
+
 
 if __name__ == '__main__':
     unittest.main()