From a21a10fd939dbe2c39692a223998843ef7682d37 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 25 Sep 2026 22:55:18 -0400 Subject: [PATCH] fix(fields): raise ProtocolError for a NumberField negative resolved length (#828) `NumberField.__call__` cached `self._bit_length = self._length * 8` and then computed `1 << self._bit_length` whenever `bit_length` was not supplied. A `length` callback resolving below zero -- the real shape at `pcapkit/protocols/schema/internet/hip.py:734`, `NumberField(length=lambda pkt: pkt['len'] - 4, signed=False)` against a wire `len` under 4 -- raised a bare `ValueError: negative shift count`: not one of `pcapkit.utilities.exceptions`, and raised before `FieldBase.length`'s own `ProtocolError` guard (#805/#811/#827) or `build_template` ever saw the value, since the shift happens several lines earlier. Guard the resolved length in `__call__` itself, right before the shift, and raise `ProtocolError` naming the field and the resolved value, worded as a sibling of `FieldBase.length`'s own message. No sibling field type (`strings`, `misc`, `collections`) performs this eager shift, so the guard stays local to `NumberField` rather than moving into a shared helper. A resolved length of exactly 0 remains legal and is left untouched. Adds `tests/corekit/test_fields_numbers_negative_length.py`: a literal negative length, the real `lambda pkt: pkt['len'] - 4` callable shape, the zero-length boundary, and a positive-length control. `coverage run -m pytest` on `numbers.py` stays at 100% (140->142 statements, 40->42 branches). Closes #828 --- pcapkit/corekit/fields/numbers.py | 24 ++- .../test_fields_numbers_negative_length.py | 176 ++++++++++++++++++ 2 files changed, 199 insertions(+), 1 deletion(-) create mode 100644 tests/corekit/test_fields_numbers_negative_length.py diff --git a/pcapkit/corekit/fields/numbers.py b/pcapkit/corekit/fields/numbers.py index 151cd1d896..051ef5a10c 100644 --- a/pcapkit/corekit/fields/numbers.py +++ b/pcapkit/corekit/fields/numbers.py @@ -9,7 +9,7 @@ import aenum from pcapkit.corekit.fields.field import Field, NoValue -from pcapkit.utilities.exceptions import BaseError, FieldValueError, IntError +from pcapkit.utilities.exceptions import BaseError, FieldValueError, IntError, ProtocolError __all__ = [ 'NumberField', @@ -132,6 +132,23 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': This method will return a new instance of :class:`NumberField` instead of updating the current instance. + Raises: + ProtocolError: If the resolved ``length`` is negative and + ``bit_length`` was not supplied -- e.g. a ``length`` callback + such as ``lambda pkt: pkt['len'] - 4`` resolving below zero + once the wire value it reads is smaller than the subtrahend. + Left alone, ``1 << (length * 8)`` raises a bare, uncatchable + :exc:`ValueError` (``negative shift count``) here, before + :attr:`~pcapkit.corekit.fields.field.FieldBase.length` (see + its own :exc:`ProtocolError` guard, #805/#811/#827) or + :meth:`build_template` ever sees the value: this method sets + ``self._bit_length`` from the resolved length eagerly, as a + cache, and shifts by it immediately, so the crash happens on + *this* line rather than on the later, already-guarded ones. + See GitHub issue #828. A resolved length of exactly ``0`` is a + legitimate empty field (e.g. ``len=4`` above resolving to + ``0``) and is left alone. + Notes: Rebuilding the template here is what applies a callable ``length``, and :meth:`build_template` recomputes ``self._need_process`` as it @@ -142,6 +159,11 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': new_self = super().__call__(packet) if new_self._bit_length < 0: + if new_self._length < 0: + raise ProtocolError( + f'Field {new_self.name} resolved to a negative length; ' + f'length={new_self._length!r}' + ) new_self._bit_length = new_self._length * 8 new_self._bit_mask = (1 << new_self._bit_length) - 1 diff --git a/tests/corekit/test_fields_numbers_negative_length.py b/tests/corekit/test_fields_numbers_negative_length.py new file mode 100644 index 0000000000..36e8bd5809 --- /dev/null +++ b/tests/corekit/test_fields_numbers_negative_length.py @@ -0,0 +1,176 @@ +"""A :class:`~pcapkit.corekit.fields.numbers.NumberField` whose resolved ``length`` is negative. + +GitHub issue #828. :meth:`~pcapkit.corekit.fields.numbers.NumberField.__call__` +caches ``self._bit_length`` from the resolved ``length`` whenever ``bit_length`` +was not supplied, and shifts by it immediately: ``1 << (length * 8)``. A +resolved length below zero -- the real shape is +``pcapkit/protocols/schema/internet/hip.py``'s +``NumberField(length=lambda pkt: pkt['len'] - 4, signed=False)``, which goes +negative for any wire ``len`` under ``4`` -- therefore raised a bare +:exc:`ValueError` (``negative shift count``): not one of +:mod:`pcapkit.utilities.exceptions`, so a caller could not tell it from a bug +of its own, and raised *before* :attr:`~pcapkit.corekit.fields.field. +FieldBase.length`'s own :exc:`ProtocolError` guard (#805, refined by #811 and +#827) ever got a chance to see the value -- that guard fires only once a +:attr:`~pcapkit.corekit.fields.field.FieldBase.template` already exists to +call :func:`struct.calcsize` on, and this method crashes several lines before +building one. + +Every negative case here is run twice: once via a literal negative ``length`` +and once via the real ``lambda pkt: pkt['len'] - 4`` callable shape with a +``packet`` dict, so a fix that only special-cased a literal int would still +fail here. +""" + +from __future__ import annotations + +import importlib.util +import unittest + +from tests._support import purge_modules + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class NegativeResolvedLengthTests(unittest.TestCase): + """The reported defect: a resolved ``length`` below zero, and the two + boundaries either side of it that must keep working. + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_a_literal_negative_length_raises_protocolerror(self) -> None: + """The narrowest reproduction: ``length=-1`` supplied directly. + + No callback is involved at all -- ``Field.__init__`` accepts any + :obj:`int`, so a negative one reaches + :meth:`~pcapkit.corekit.fields.numbers.NumberField.__call__`'s eager + ``1 << (length * 8)`` exactly as a callable resolving to the same + value would. + + """ + from pcapkit.corekit.fields.numbers import NumberField + from pcapkit.utilities.exceptions import ProtocolError + + field = NumberField(length=-1, signed=False) + + with self.assertRaises(ProtocolError) as ctx: + field(dict()) + + self.assertIn('resolved to a negative length', str(ctx.exception)) + self.assertIn('-1', str(ctx.exception)) + + def test_protocolerror_is_a_baseerror_unlike_the_stock_valueerror(self) -> None: + """The defect in one assertion: catchable as a pcapkit error. + + On stock ``21e9588af`` this raises a bare :class:`builtins.ValueError` + (``negative shift count``), which is *not* an instance of + :class:`~pcapkit.utilities.exceptions.BaseError` and so cannot be + caught as one -- measured directly below, since that is the whole + point of the regression. + + """ + from pcapkit.corekit.fields.numbers import NumberField + from pcapkit.utilities.exceptions import BaseError + + field = NumberField(length=-4, signed=False) + + with self.assertRaises(BaseError) as ctx: + field(dict()) + + self.assertNotIsInstance(ctx.exception, type(None)) # sanity: an exception was raised + self.assertIsInstance(ctx.exception, BaseError) + + def test_the_real_callable_shape_from_hip_py_also_raises_protocolerror(self) -> None: + """``lambda pkt: pkt['len'] - 4`` against a truncated ``len``, verbatim. + + This is the exact shape at ``pcapkit/protocols/schema/internet/ + hip.py``'s ``PuzzleParameter.random`` field (line 734 as of + ``21e9588af``): a wire ``len`` of ``3`` -- one octet short of the + 4-octet header this parameter always carries -- resolves the + remaining ``random`` payload to ``-1`` octets, negative because the + packet is malformed rather than because anyone asked for a negative + width. + + """ + from pcapkit.corekit.fields.numbers import NumberField + from pcapkit.utilities.exceptions import BaseError, ProtocolError + + field = NumberField(length=lambda pkt: pkt['len'] - 4, signed=False) + + with self.assertRaises(ProtocolError) as ctx: + field({'len': 3}) + + self.assertIsInstance(ctx.exception, BaseError) + self.assertIn('resolved to a negative length', str(ctx.exception)) + self.assertIn('-1', str(ctx.exception)) + + def test_a_more_negative_resolution_also_raises_protocolerror(self) -> None: + """A wire ``len`` of ``0`` resolves to ``-4``, not just ``-1``. + + Pinned separately from the ``-1`` case so a fix that only checks + ``length == -1`` -- rather than ``length < 0`` -- fails here. + + """ + from pcapkit.corekit.fields.numbers import NumberField + from pcapkit.utilities.exceptions import BaseError, ProtocolError + + field = NumberField(length=lambda pkt: pkt['len'] - 4, signed=False) + + with self.assertRaises(ProtocolError) as ctx: + field({'len': 0}) + + self.assertIsInstance(ctx.exception, BaseError) + self.assertIn('-4', str(ctx.exception)) + + def test_a_resolved_length_of_exactly_zero_still_succeeds(self) -> None: + """The boundary the fix must not break: zero is not negative. + + The same ``lambda pkt: pkt['len'] - 4`` shape with ``len=4`` resolves + to exactly ``0`` -- a legitimate empty field, not a malformed packet -- + and issue #828 is explicit that this must keep working. Checked via + both the literal and the callable shape, matching the negative cases + above. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + literal = NumberField(length=0, signed=False)(dict()) + self.assertEqual(literal._length, 0) + self.assertEqual(literal.template, '>0s') + self.assertEqual(literal.length, 0) + + callable_ = NumberField(length=lambda pkt: pkt['len'] - 4, signed=False) + resolved = callable_({'len': 4}) + self.assertEqual(resolved._length, 0) + self.assertEqual(resolved.template, '>0s') + self.assertEqual(resolved.length, 0) + + def test_a_positive_resolved_length_is_unaffected(self) -> None: + """The control: a well-formed packet is not touched by this guard. + + The same ``hip.py`` shape with a ``len`` of ``8`` -- a well-formed + ``PUZZLE`` parameter, header plus four octets of random data -- + resolves to ``4`` and packs exactly as a static ``length=4`` field + would. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + field = NumberField(length=lambda pkt: pkt['len'] - 4, signed=False) + resolved = field({'len': 8}) + + self.assertEqual(resolved._length, 4) + self.assertEqual(resolved.template, '>I') + self.assertEqual(resolved.length, 4) + + static = NumberField(length=4, signed=False)(dict()) + self.assertEqual(resolved.pack(0xdeadbeef, dict()), + static.pack(0xdeadbeef, dict())) + + +if __name__ == '__main__': + unittest.main()