diff --git a/pcapkit/corekit/fields/numbers.py b/pcapkit/corekit/fields/numbers.py index 051ef5a10c..7cec395021 100644 --- a/pcapkit/corekit/fields/numbers.py +++ b/pcapkit/corekit/fields/numbers.py @@ -51,6 +51,14 @@ class NumberField(Field[int], Generic[_T]): IntError: If no ``length`` is given and ``__length__`` fixes none either. FieldValueError: If ``signed`` contradicts a sign already fixed by ``__signed__`` -- never from this class, which fixes none. + ProtocolError: If ``bit_length`` is given negative. Left alone, + ``(1 << bit_length) - 1`` raises a bare, uncatchable + :exc:`ValueError` (``negative shift count``) here, before + :meth:`__call__`'s own negative-``length`` guard (#828/#829) or + :attr:`~pcapkit.corekit.fields.field.FieldBase.length`'s (#805) + ever see anything -- this one fires at construction time, on the + argument itself rather than on a resolved wire length. See + GitHub issue #831. Notes: A subclass such as :class:`UInt32Field` fixes the sign through @@ -84,6 +92,11 @@ def __init__(self, length: 'Optional[int | Callable[[dict[str, Any]], int]]' = N super().__init__(length, default, callback) if bit_length is not None: + if bit_length < 0: + raise ProtocolError( + f'Field {self.name} resolved to a negative length; ' + f'bit_length={bit_length!r}' + ) self._bit_length = bit_length self._bit_mask = (1 << bit_length) - 1 else: @@ -133,19 +146,26 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': 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 + ProtocolError: If the resolved ``length`` is negative -- 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``) when ``bit_length`` was not supplied, 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 + See GitHub issue #828. This guard runs regardless of whether + ``bit_length`` was supplied, so a field constructed with a + fixed ``bit_length`` *and* a callable ``length`` that resolves + negative raises the identical message as one with no + ``bit_length`` at all, rather than falling through to a + ``template='...-1s'`` :exc:`ProtocolError` from + :attr:`~pcapkit.corekit.fields.field.FieldBase.length` later -- + see GitHub issue #831. A resolved length of exactly ``0`` is a legitimate empty field (e.g. ``len=4`` above resolving to ``0``) and is left alone. @@ -158,12 +178,13 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': """ new_self = super().__call__(packet) + if new_self._length < 0: + raise ProtocolError( + f'Field {new_self.name} resolved to a negative length; ' + f'length={new_self._length!r}' + ) + 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 index 36e8bd5809..80ca037031 100644 --- a/tests/corekit/test_fields_numbers_negative_length.py +++ b/tests/corekit/test_fields_numbers_negative_length.py @@ -172,5 +172,159 @@ def test_a_positive_resolved_length_is_unaffected(self) -> None: static.pack(0xdeadbeef, dict())) +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class NegativeBitLengthTests(unittest.TestCase): + """GitHub issue #831: the sibling eager shift, in ``__init__`` rather than + ``__call__``. + + ``NumberField.__init__`` caches ``self._bit_mask`` from ``bit_length`` + whenever one is supplied, and shifts by it immediately: + ``(1 << bit_length) - 1``. #829 guarded the *resolved-length* shift in + :meth:`~pcapkit.corekit.fields.numbers.NumberField.__call__` but left this + one, at ``pcapkit/corekit/fields/numbers.py:88`` on ``5576708d4``, open -- + a negative ``bit_length`` still raised a bare, uncatchable + :exc:`ValueError` (``negative shift count``) rather than a + :class:`~pcapkit.utilities.exceptions.ProtocolError`. + + The last two cases below are the second half of #831: with ``bit_length`` + supplied, #829's guard in :meth:`__call__` sits entirely inside the + ``bit_length``-not-supplied branch and so never runs, and a field combining + a fixed ``bit_length`` with a callable ``length`` that resolves negative + used to fall through to ``template='...-1s'`` and a *different* + :exc:`ProtocolError` message from + :attr:`~pcapkit.corekit.fields.field.FieldBase.length` -- the same wire + condition, two different messages, depending on an unrelated constructor + argument. The fix validates the resolved ``length`` unconditionally in + ``__call__``, so both shapes now raise the identical message. + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_a_negative_bit_length_raises_protocolerror(self) -> None: + """The narrowest reproduction: ``bit_length=-1`` supplied directly. + + Unlike the resolved-``length`` case, this one does not even need a + call: :meth:`~pcapkit.corekit.fields.numbers.NumberField.__init__` + shifts by ``bit_length`` eagerly, so the raise happens at + construction. + + """ + from pcapkit.corekit.fields.numbers import NumberField + from pcapkit.utilities.exceptions import ProtocolError + + with self.assertRaises(ProtocolError) as ctx: + NumberField(length=4, bit_length=-1) + + self.assertIn('resolved to a negative length', str(ctx.exception)) + self.assertIn('bit_length=-1', str(ctx.exception)) + + def test_negative_bit_length_protocolerror_is_a_baseerror(self) -> None: + """The defect in one assertion: catchable as a pcapkit error. + + On stock ``5576708d4`` this raises a bare :class:`builtins.ValueError` + (``negative shift count``), which is *not* an instance of + :class:`~pcapkit.utilities.exceptions.BaseError` -- measured directly + below, mirroring + :meth:`NegativeResolvedLengthTests. + test_protocolerror_is_a_baseerror_unlike_the_stock_valueerror` + for the ``__init__`` shift rather than the ``__call__`` one. + + """ + from pcapkit.corekit.fields.numbers import NumberField + from pcapkit.utilities.exceptions import BaseError + + with self.assertRaises(BaseError) as ctx: + NumberField(length=4, bit_length=-3) + + self.assertIsInstance(ctx.exception, BaseError) + + def test_the_reported_value_is_the_bit_length_not_a_byte_length(self) -> None: + """The message must name what is actually wrong: ``bit_length``. + + A resolved byte ``length`` of ``4`` is perfectly legitimate here -- + it is the *bit* length that is negative -- so the message must not + report ``length=4``, which would send a reader looking at the wrong + argument. + + """ + from pcapkit.corekit.fields.numbers import NumberField + from pcapkit.utilities.exceptions import ProtocolError + + with self.assertRaises(ProtocolError) as ctx: + NumberField(length=4, bit_length=-2) + + self.assertIn('bit_length=-2', str(ctx.exception)) + self.assertNotIn('length=4', str(ctx.exception)) + + def test_a_positive_bit_length_is_unaffected(self) -> None: + """The control: the ordinary, well-formed case is not touched. + + The exact shape at ``pcapkit/protocols/schema/link/vlan.py``'s + ``TCI.pcp``/``dei``/``vid`` fields (lines 47, 49, 51 on + ``5576708d4``): a positive literal ``bit_length`` alongside a fixed + byte ``length``. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + field = NumberField(length=1, bit_length=3) + self.assertEqual(field.bit_length, 3) + self.assertEqual(field._bit_mask, 0b111) + + called = field(dict()) + self.assertEqual(called.bit_length, 3) + self.assertEqual(called._bit_mask, 0b111) + + def test_bit_length_zero_still_works_exactly_as_it_does_on_main(self) -> None: + """The boundary the fix must not break: ``bit_length=0`` is not negative. + + Checked against stock behaviour rather than assumed: on ``5576708d4``, + prior to any change here, ``bit_length=0`` already constructs and + calls cleanly with a mask of ``0``, and that has to keep being true. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + field = NumberField(length=4, bit_length=0) + self.assertEqual(field.bit_length, 0) + self.assertEqual(field._bit_mask, 0) + + called = field(dict()) + self.assertEqual(called.bit_length, 0) + self.assertEqual(called._bit_mask, 0) + + def test_bit_length_supplied_plus_negative_callable_length_matches_the_unsupplied_message(self) -> None: + """The second half of #831: one wire condition, one message. + + No in-tree call site combines a fixed ``bit_length`` with a callable + ``length`` -- ``vlan.py``'s three ``bit_length`` fields all take + positive literal ``length`` -- so this is a constructed reproduction, + not a real wire shape. Before the fix, #829's guard in ``__call__`` + sits inside ``if new_self._bit_length < 0:`` and this combination + never enters that branch, so no ``ProtocolError`` fires there at all; + the field instead degrades to ``template='>-1s'`` and the error is + deferred to :attr:`~pcapkit.corekit.fields.field.FieldBase.length`, + which reports ``template='>-1s'`` rather than ``length=-1`` -- + a different message for the same underlying defect. + + """ + from pcapkit.corekit.fields.numbers import NumberField + from pcapkit.utilities.exceptions import BaseError, ProtocolError + + with_bit_length = NumberField(length=lambda pkt: pkt['len'] - 4, bit_length=8, signed=False) + without_bit_length = NumberField(length=lambda pkt: pkt['len'] - 4, signed=False) + + with self.assertRaises(ProtocolError) as ctx_with: + with_bit_length({'len': 3}) + with self.assertRaises(ProtocolError) as ctx_without: + without_bit_length({'len': 3}) + + self.assertIsInstance(ctx_with.exception, BaseError) + self.assertEqual(str(ctx_with.exception), str(ctx_without.exception)) + self.assertIn('resolved to a negative length', str(ctx_with.exception)) + self.assertIn('length=-1', str(ctx_with.exception)) + + if __name__ == '__main__': unittest.main()