diff --git a/pcapkit/corekit/fields/field.py b/pcapkit/corekit/fields/field.py index 20bc553031..77c63436d7 100644 --- a/pcapkit/corekit/fields/field.py +++ b/pcapkit/corekit/fields/field.py @@ -4,6 +4,7 @@ import abc import contextlib import contextvars +import re import struct from typing import TYPE_CHECKING, Generic, TypeVar, cast @@ -198,6 +199,31 @@ def _zero_pad_budget() -> 'Iterator[list[int]]': _zero_pad_ledger.reset(token) +#: Pattern[str]: Matches a leading, optionally byte-order-prefixed negative +#: count in a :attr:`FieldBase.template`, e.g. the ``-1`` in ``'-1s'`` or in +#: ``'>-1s'``. +#: +#: Every template this package builds from a resolved field length is +#: ``f'{length}s'`` (see :mod:`pcapkit.corekit.fields.strings`, +#: :mod:`~pcapkit.corekit.fields.misc`, :mod:`~pcapkit.corekit.fields.collections` +#: and, before it gets byte-order-prefixed, :mod:`~pcapkit.corekit.fields.numbers` +#: -- :meth:`NumberField.build_template `'s ``else`` arm), so a negative resolved length +#: always leaves that literal ``-`` immediately before the digits. What +#: :class:`~pcapkit.corekit.fields.numbers.NumberField` adds is a byte-order +#: prefix -- one of ``'@=<>!'`` -- *in front of* that whole template +#: (``f'{endian}{struct_fmt}'``), which is what a plain ``^-\d+`` misses: the +#: prefix, not the minus sign, is what starts the string. The optional +#: character class here accounts for exactly that one prefix position, so a +#: negative count is recognised whether or not it was byte-order-prefixed, while +#: still refusing anything else -- a bare byte-order character with no minus +#: sign after it (``'>Xs'``), or any other malformed template -- which is what +#: makes checking for it a reliable way to tell those cases apart from each +#: other *before* :func:`struct.calcsize` is asked to size either one. See +#: #825, #827. +_RE_NEGATIVE_LENGTH_TEMPLATE = re.compile(r'^[@=<>!]?-\d+') + + class FieldMeta(abc.ABCMeta, Generic[_T]): """Meta class to add dynamic support to :class:`FieldBase`. @@ -276,16 +302,34 @@ def length(self) -> 'int': (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. + :func:`struct.calcsize` cannot size such a template and raises + a bare :exc:`struct.error`, uncatchable as a pcapkit-specific + error; this re-raises it as the negative-length message below. + See #805. + ProtocolError: If :attr:`template` is otherwise malformed -- + anything else :func:`struct.calcsize` cannot size, such as a + typo'd format character -- rather than the negative-length + message above, which would misreport the actual cause. + :func:`struct.calcsize` raises the identical bare + :exc:`struct.error` for both cases (measured: + ``calcsize('-1s')`` and ``calcsize('Xs')`` both raise ``bad + char in struct format``), so the two are told apart by + :data:`_RE_NEGATIVE_LENGTH_TEMPLATE` against :attr:`template` + itself -- which is known already, without needing anything + :func:`struct.calcsize`'s own error says -- rather than by the + error message. See #825. """ try: return struct.calcsize(self.template) except struct.error as error: + if _RE_NEGATIVE_LENGTH_TEMPLATE.match(self.template) is not None: + raise ProtocolError( + f'Field {self.name} resolved to a negative length; ' + f'template={self.template!r}' + ) from error raise ProtocolError( - f'Field {self.name} resolved to a negative length; ' + f'Field {self.name} has a malformed template; ' f'template={self.template!r}' ) from error diff --git a/tests/corekit/test_fields_field.py b/tests/corekit/test_fields_field.py index b00465cec2..c0531a0ece 100644 --- a/tests/corekit/test_fields_field.py +++ b/tests/corekit/test_fields_field.py @@ -879,10 +879,12 @@ class FieldBaseLengthNegativeResolvedLengthTests(unittest.TestCase): def setUp(self) -> None: purge_modules(['pcapkit']) + from pcapkit.corekit.fields.numbers import NumberField from pcapkit.corekit.fields.strings import BytesField from pcapkit.utilities.exceptions import ProtocolError self.BytesField = BytesField + self.NumberField = NumberField self.ProtocolError = ProtocolError def test_a_negative_resolved_length_raises_protocolerror_not_structerror(self) -> None: @@ -919,6 +921,91 @@ def test_a_zero_or_positive_resolved_length_is_unaffected(self) -> None: {'__length__': resolved}) self.assertEqual(field.length, resolved) + def test_a_malformed_template_raises_protocolerror_naming_the_template(self) -> None: + """A typo'd template is a category error, not a negative length (#825). + + :func:`struct.calcsize` raises the identical bare :exc:`struct.error` + -- ``bad char in struct format`` -- for a malformed template as it + does for a negative one (measured on 3.14.7: ``calcsize('-1s')`` and + ``calcsize('Xs')`` both raise it). Pre-fix, :attr:`FieldBase.length + ` caught that blanket + and always reported "resolved to a negative length", which would have + misdiagnosed this case. The template here (``'Xs'``) never matches + the leading-minus-sign shape a resolved negative count takes, so it + must fall to the distinct, template-naming message instead. + """ + field = self.BytesField(length=4) + field.name = 'weird' + field._template = 'Xs' # a typo, not a resolved negative count + + with self.assertRaises(self.ProtocolError) as ctx: + field.length # noqa: B018 -- property access is the point + + message = str(ctx.exception) + self.assertIn('Xs', message) + self.assertNotIn('negative length', message) + + # must not be a bare struct.error, and must still chain to the real + # one struct.calcsize actually raised. + self.assertNotIsInstance(ctx.exception, struct.error) + self.assertIsInstance(ctx.exception.__cause__, struct.error) + + def test_a_byte_order_prefixed_negative_length_is_still_reported_as_negative(self) -> None: + """A prefixed negative template must not be misdiagnosed as malformed (#827). + + :class:`~pcapkit.corekit.fields.numbers.NumberField` builds its + template as ``f'{endian}{struct_fmt}'`` + (:meth:`NumberField.__init__ `) over the same ``f'{length}s'`` fall-through + every other field uses (:meth:`NumberField.build_template + `'s + ``else`` arm) -- so a negative resolved length comes out prefixed, + e.g. ``'>-1s'``, not bare ``'-1s'``. Uses the real ``NumberField`` + construction path rather than hand-setting ``_template``, so this + would catch a future change to how templates are assembled, not + just to the regex. + """ + field = self.NumberField(length=-1) + field.name = 'number' + + self.assertEqual(field.template, '>-1s') + with self.assertRaises(self.ProtocolError) as ctx: + field.length # noqa: B018 -- property access is the point + + message = str(ctx.exception) + self.assertIn('negative length', message) + self.assertNotIn('malformed', message) + self.assertNotIsInstance(ctx.exception, struct.error) + self.assertIsInstance(ctx.exception.__cause__, struct.error) + + def test_negative_and_malformed_templates_are_reported_distinctly(self) -> None: + """The two categories never trade messages, and neither leaks a bare + :exc:`struct.error` -- across both, not just one. + """ + cases = { + 'negative': (self.BytesField(length=lambda pkt: pkt['__length__'])( + {'__length__': -1}), None), + 'malformed': (self.BytesField(length=4), 'Xs'), + } + for label, (field, forced_template) in cases.items(): + with self.subTest(case=label): + field.name = 'field' + if forced_template is not None: + field._template = forced_template + + with self.assertRaises(self.ProtocolError) as ctx: + field.length # noqa: B018 -- property access is the point + + message = str(ctx.exception) + if label == 'negative': + self.assertIn('negative length', message) + else: + self.assertNotIn('negative length', message) + self.assertIn(field.template, message) + + self.assertNotIsInstance(ctx.exception, struct.error) + self.assertIsInstance(ctx.exception.__cause__, struct.error) + if __name__ == '__main__': unittest.main()