fix(fields): raise ProtocolError, not struct.error, on a negative resolved field length

pcapkit/corekit/fields/field.py

@@ -8,7 +8,7 @@
 from typing import TYPE_CHECKING, Generic, TypeVar, cast
 
 from pcapkit.utilities.compat import final
-from pcapkit.utilities.exceptions import FieldValueError, NoDefaultValue
+from pcapkit.utilities.exceptions import FieldValueError, NoDefaultValue, ProtocolError
 
 __all__ = ['Field']
 
@@ -269,8 +269,25 @@ def template(self) -> 'str':
 
     @property
     def length(self) -> 'int':
-        """Field size."""
-        return struct.calcsize(self.template)
+        """Field size.
+
+        Raises:
+            ProtocolError: If :attr:`template` resolves to a negative count
+                (e.g. ``'-5s'``, from a ``length`` callback such as
+                ``lambda pkt: pkt['__length__']`` resolving below zero once
+                the buffer ran short of what the schema declared).
+                :func:`struct.calcsize` cannot size such a template and would
+                otherwise raise a bare :exc:`struct.error`, uncatchable as a
+                pcapkit-specific error. See #805.
+
+        """
+        try:
+            return struct.calcsize(self.template)
+        except struct.error as error:
+            raise ProtocolError(
+                f'Field {self.name} resolved to a negative length; '
+                f'template={self.template!r}'
+            ) from error
 
     @property
     def optional(self) -> 'bool':

tests/corekit/test_fields_field.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import struct
 import threading
 import unittest
 
@@ -855,3 +856,69 @@ def test_a_short_read_round_trips_back_to_the_padded_wire_form(self) -> None:
                     with self.subTest(width=width, byteorder=byteorder, kept=kept):
                         self.assertEqual(field.pack(read, {}),
                                          buffer.ljust(width, b'\x00'))
+
+
+class FieldBaseLengthNegativeResolvedLengthTests(unittest.TestCase):
+    """:attr:`FieldBase.length <pcapkit.corekit.fields.field.FieldBase.length>`
+    on a negative resolved length.
+
+    A ``length=lambda pkt: pkt['__length__']``-style callback is
+    attacker/corruption-controlled the same way :attr:`FieldBase.unpack`'s
+    ``length`` argument is (see the class above): :meth:`Schema.unpack
+    <pcapkit.protocols.schema.schema.Schema.unpack>` decrements
+    ``packet['__length__']`` by each field's nominal width regardless of how
+    many octets the buffer actually held, and a subsequent field's callback
+    can resolve to that negative remainder. :class:`~pcapkit.corekit.fields.
+    strings._TextField.__call__` then builds a template such as ``'-5s'``
+    from it with no lower bound, and :func:`struct.calcsize` cannot size
+    that -- pre-fix this surfaced as a bare, uncatchable :exc:`struct.error`
+    (GitHub issue #805).
+
+    """
+
+    def setUp(self) -> None:
+        purge_modules(['pcapkit'])
+
+        from pcapkit.corekit.fields.strings import BytesField
+        from pcapkit.utilities.exceptions import ProtocolError
+
+        self.BytesField = BytesField
+        self.ProtocolError = ProtocolError
+
+    def test_a_negative_resolved_length_raises_protocolerror_not_structerror(self) -> None:
+        """The crash reproduction from #805, isolated to the field property.
+
+        Pre-fix, ``struct.error: bad char in struct format`` -- a bare
+        stdlib exception, not even a :exc:`ValueError`, uncatchable by
+        ordinary caller code -- escaped here. This is the exact shape
+        HTTP/2's ``GoawayFrame.debug``/``PushPromiseFrame.fragment`` fields
+        hit once their fixed-width siblings have already driven
+        ``pkt['__length__']`` negative.
+        """
+        field = self.BytesField(length=lambda pkt: pkt['__length__'])({'__length__': -5})
+
+        self.assertEqual(field.template, '-5s')
+        with self.assertRaises(self.ProtocolError) as ctx:
+            field.length  # noqa: B018 -- property access is the point
+
+        # must not be a bare struct.error: ProtocolError is a ValueError
+        # subclass in this library's hierarchy, never struct.error itself.
+        self.assertIsInstance(ctx.exception, ValueError)
+        self.assertNotIsInstance(ctx.exception, struct.error)
+
+    def test_a_zero_or_positive_resolved_length_is_unaffected(self) -> None:
+        """The fix must not change the answer for any non-negative length.
+
+        The property is a thin ``try/except`` around the same
+        :func:`struct.calcsize` call as before; a resolved length that never
+        raises must return exactly what it always returned.
+        """
+        for resolved in (0, 1, 5, 1024):
+            with self.subTest(resolved=resolved):
+                field = self.BytesField(length=lambda pkt: pkt['__length__'])(
+                    {'__length__': resolved})
+                self.assertEqual(field.length, resolved)
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/protocols/application/test_http_unit.py

@@ -951,15 +951,16 @@ def test_guess_version_does_not_suppress_struct_error_on_the_http1_arm(self) ->
 
         #799 closed the *outer*-header slice of the class this suppression
         exists for (``httpv2.HTTP.unpack`` now rejects a buffer under nine
-        octets before the schema layer runs), but did not retire the
-        suppression: a buffer that clears nine octets can still carry a frame
-        type -- ``GOAWAY``, ``PUSH_PROMISE``, a padded ``DATA``/``HEADERS`` --
-        whose own fixed-width fields exceed what is left, and that still raises
-        a bare :exc:`struct.error` one field further in. See
+        octets before the schema layer runs), and #805 closed the *inner*
+        slice (a resolved field length going negative now raises
+        ``ProtocolError`` rather than a bare :exc:`struct.error`), but neither
+        retired the suppression: it stays as defence in depth for any
+        ``__length__``-keyed field #805 did not individually verify, across
+        the ten schema modules that share the pattern. See
         ``test_guess_version_reports_unknown_version_for_a_short_payload`` for
-        the outer-header case #799 did close, and
-        ``test_guess_version_still_leaks_a_bare_struct_error_for_an_inner_field_shortfall``
-        below for the class it did not.
+        the outer-header case #799 closed, and
+        ``test_guess_version_no_longer_leaks_a_bare_struct_error_for_an_inner_field_shortfall``
+        below for the inner one #805 closed.
 
         It is also unnecessary: nothing reaches a :exc:`struct.error` through
         ``httpv1.HTTP``. Nine byte patterns over lengths 0-24, on both the direct
@@ -999,24 +1000,26 @@ def test_guess_version_does_not_suppress_struct_error_on_the_http1_arm(self) ->
                     with self.assertRaises(struct.error):
                         HTTP(io.BytesIO(raw), len(raw))
 
-    def test_guess_version_still_leaks_a_bare_struct_error_for_an_inner_field_shortfall(self) -> None:
-        """A buffer that clears nine octets can still crash one field further in.
+    def test_guess_version_no_longer_leaks_a_bare_struct_error_for_an_inner_field_shortfall(self) -> None:
+        """A buffer that clears nine octets used to still crash one field further in.
 
         #799's ``httpv2.HTTP.unpack`` guard only protects the fixed nine-octet
         *outer* header. A ``GOAWAY`` frame's own fixed ``stream`` (4 octets) and
         ``error`` (4 octets) fields consume eight more octets before ``debug``
         is even reached, so a sixteen-octet buffer -- nine for the header, seven
-        for the rest -- drives ``pkt['__length__']`` to ``-1`` at ``debug`` and
-        still raises a bare :exc:`struct.error` straight through ``httpv2.HTTP``.
-        Filed as its own issue (#805) rather than fixed here: the actual root is
-        generic ``Schema.unpack``/``FieldBase.length`` machinery shared by at
-        least ten schema modules, not something httpv2-specific.
-
-        This is exactly why ``_guess_version``'s last arm keeps suppressing
-        :exc:`struct.error` alongside ``ProtocolError`` -- narrowing it, as an
-        earlier revision of this fix did, regressed the direct ``HTTP()`` guess
-        path: the same bytes came back a bare :exc:`struct.error` instead of
-        ``ProtocolError``, which a caller catching protocol errors cannot catch.
+        for the rest -- drives ``pkt['__length__']`` to ``-1`` at ``debug``.
+        Pre-#805, that raised a bare :exc:`struct.error` straight through
+        ``httpv2.HTTP``; #805 closed the actual root, generic
+        ``FieldBase.length`` (``struct.calcsize`` on a negative-count template),
+        shared by at least ten schema modules, not something httpv2-specific --
+        so direct construction now raises :exc:`ProtocolError` too.
+
+        ``_guess_version``'s last arm keeps suppressing :exc:`struct.error`
+        alongside ``ProtocolError`` regardless: narrowing it, as an earlier
+        revision of this fix did, regressed the direct ``HTTP()`` guess path
+        for a *different* input class (the sub-nine-octet outer-header one),
+        and #805 does not touch every ``__length__``-keyed field in every one
+        of those ten modules, so the suppression stays as defence in depth.
         This test drives real wire bytes through the actual guess path -- no
         ``mock.patch`` -- specifically because a mocked fault cannot see a
         regression in the *un-mocked* route the fault is meant to stand in for.
@@ -1033,15 +1036,16 @@ def test_guess_version_still_leaks_a_bare_struct_error_for_an_inner_field_shortf
         raw = b'\x00\x00\x15\x07\x00\x00\x00\x00\x00' + b'\xff' * 7
         self.assertEqual(len(raw), 16)
 
-        # Direct construction is documented to leak the unwrapped struct.error
-        # (see the module docstring above and #805) -- pinned so a fix to #805
-        # is noticed here rather than silently changing this contract too.
+        # Direct construction now raises ProtocolError, not a bare
+        # struct.error -- #805's fix, pinned here so a regression is noticed.
         import struct
-        with self.assertRaises(struct.error):
+        with self.assertRaises(ProtocolError) as direct_ctx:
             HTTPv2(io.BytesIO(raw), len(raw))
+        self.assertNotIsInstance(direct_ctx.exception, struct.error)
 
-        # The guess path must not repeat that leak: _guess_version's arm 2
-        # suppresses struct.error precisely so this comes back catchable.
+        # The guess path must answer the same way: _guess_version's arm 2
+        # suppresses struct.error/ProtocolError precisely so this comes back
+        # catchable either way.
         for label, kwargs in (('guessed', {}), ('explicit version=2', {'version': 2})):
             with self.subTest(path=label):
                 with self.assertRaises(ProtocolError) as ctx:

tests/protocols/application/test_httpv2_negative_length_unit.py

@@ -0,0 +1,153 @@
+# -*- coding: utf-8 -*-
+"""A negative resolved field length must raise ``ProtocolError``, not ``struct.error``.
+
+GitHub issue #805. :meth:`~pcapkit.protocols.schema.schema.Schema.unpack`
+decrements ``packet['__length__']`` by each field's *nominal* width
+regardless of how many octets the buffer actually had, and a field whose own
+``length=lambda pkt: pkt['__length__']``-style callback resolves to that
+negative remainder builds a struct template such as ``'-5s'``.
+:func:`struct.calcsize` cannot size that and raised a bare
+:exc:`struct.error` for it -- not a :exc:`ProtocolError`, not even a
+:exc:`ValueError`, uncatchable by ordinary caller code.
+
+Scope, per the issue's own correction of its body: the fix belongs at the
+*resolved field length* -- :attr:`~pcapkit.corekit.fields.field.FieldBase.length`
+-- not at the running-counter warning (``pcapkit/protocols/schema/schema.py``
+around ``packet['__length__'] < 0``), which this module does not touch. See
+:mod:`tests.corekit.test_fields_field`'s ``FieldBaseLengthNegativeResolvedLengthTests``
+for the property-level unit tests and the unaffected non-negative control.
+
+``read()``'s own ``schema.length > length`` guard (`httpv2.py`) cannot see
+this class of input: the crash happens while resolving the *inner* frame's
+own fields, during :meth:`Schema.unpack`, before that comparison ever runs.
+So these are built by constructing :class:`HTTP` (HTTP/2) directly, which is
+how the issue itself reproduces the defect.
+
+"""
+
+from __future__ import annotations
+
+import importlib.util
+import io
+import struct
+import unittest
+import warnings
+
+RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper')
+HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS)
+
+
+def http2_frame_bytes(type_: 'int', flags: 'int', sid: 'int', payload: 'bytes') -> 'bytes':
+    """Build the wire octets of one HTTP/2 frame.
+
+    Args:
+        type_: Frame type octet.
+        flags: Frame flags octet.
+        sid: Stream identifier.
+        payload: The frame payload, header excluded.
+
+    Returns:
+        The packed frame, its 3-octet length field counting the whole frame
+        (header included), this library's convention.
+
+    """
+    return (
+        (len(payload) + 9).to_bytes(3, 'big')
+        + bytes([type_, flags])
+        + sid.to_bytes(4, 'big')
+        + payload
+    )
+
+
+@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
+class NegativeResolvedLengthUnitTests(unittest.TestCase):
+    """The #805 reproduction, and the well-formed inputs it must not touch."""
+
+    def test_goaway_at_sixteen_octets_raises_protocolerror_not_structerror(self) -> None:
+        """The issue's own repro: a 16-octet ``GOAWAY`` frame.
+
+        ``stream``+``error`` alone are eight fixed octets, so a 16-octet
+        buffer (7 octets of frame-specific payload after the 9-octet header)
+        drives ``debug``'s ``length=lambda pkt: pkt['__length__']`` to -1.
+        Pre-fix: ``struct.error: bad char in struct format``.
+        """
+        from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2
+        from pcapkit.utilities.exceptions import ProtocolError
+
+        data = b'\x00\x00\x15\x07\x00\x00\x00\x00\x00' + b'\xff' * 7
+        self.assertEqual(len(data), 16)
+
+        with self.assertRaises(ProtocolError) as ctx:
+            HTTPv2(io.BytesIO(data), 16)
+        self.assertNotIsInstance(ctx.exception, struct.error)
+
+    def test_goaway_buflens_nine_through_sixteen_all_raise_protocolerror(self) -> None:
+        """Every ``GOAWAY`` buffer length the issue names as an escape.
+
+        ``buflen`` 9 through 16: the fixed ``stream``/``error`` fields alone
+        need eight payload octets, so every one of these drives ``debug``
+        negative.
+        """
+        from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2
+        from pcapkit.utilities.exceptions import ProtocolError
+
+        for buflen in range(9, 17):
+            payload = b'\x00' * (buflen - 9)
+            raw = http2_frame_bytes(0x07, 0x00, 0, payload)
+            with self.subTest(buflen=buflen):
+                with self.assertRaises(ProtocolError) as ctx:
+                    HTTPv2(io.BytesIO(raw), buflen)
+                self.assertNotIsInstance(ctx.exception, struct.error)
+
+    def test_push_promise_buflens_nine_through_twelve_all_raise_protocolerror(self) -> None:
+        """Every ``PUSH_PROMISE`` buffer length the issue names as an escape.
+
+        The promised stream identifier alone is four octets, so buffers 9
+        through 12 leave ``fragment`` negative.
+        """
+        from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2
+        from pcapkit.utilities.exceptions import ProtocolError
+
+        for buflen in range(9, 13):
+            payload = b'\x00' * (buflen - 9)
+            raw = http2_frame_bytes(0x05, 0x04, 1, payload)
+            with self.subTest(buflen=buflen):
+                with self.assertRaises(ProtocolError) as ctx:
+                    HTTPv2(io.BytesIO(raw), buflen)
+                self.assertNotIsInstance(ctx.exception, struct.error)
+
+    def test_an_over_padded_data_frame_raises_protocolerror(self) -> None:
+        """``pad_len`` exceeding the payload area, for a ``DATA`` frame."""
+        from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2
+        from pcapkit.utilities.exceptions import ProtocolError
+
+        payload = bytes([20])  # PADDED, pad_len=20, no data and no padding octets follow
+        raw = http2_frame_bytes(0x00, 0x08, 1, payload)
+
+        with self.assertRaises(ProtocolError) as ctx:
+            HTTPv2(io.BytesIO(raw), len(raw))
+        self.assertNotIsInstance(ctx.exception, struct.error)
+
+    def test_a_well_formed_goaway_frame_still_parses_cleanly(self) -> None:
+        """Control: a ``GOAWAY`` frame whose buffer matches its declared length.
+
+        The fix is a ``try``/``except struct.error`` around the same
+        :func:`struct.calcsize` call as before, so a resolved length that
+        never goes negative must parse identically to pre-fix -- no
+        exception, no warning.
+        """
+        from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2
+
+        debug = b'ok'
+        payload = (5).to_bytes(4, 'big') + (0).to_bytes(4, 'big') + debug
+        raw = http2_frame_bytes(0x07, 0x00, 1, payload)
+
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter('always')
+            info = HTTPv2(io.BytesIO(raw), len(raw)).info
+
+        self.assertEqual(info.debug_data, debug)
+        self.assertEqual([str(w.message) for w in caught], [])
+
+if __name__ == '__main__':
+    unittest.main()