From b95b9191549be3cf9e8da4c2a621b3a9ab59c332 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 20 Sep 2026 03:57:54 -0400 Subject: [PATCH] fix(corekit): reject a signed= that contradicts a field's fixed sign (#545) Closes #545. All eight `*IntField` subclasses documented a `signed` constructor argument that `__signed__` then discarded -- in both directions, measured on 691f12ab5: `UInt8Field(signed=True)._signed` was `False` and `Int8Field(signed=False)._signed` was `True`. A caller who asked for a signed field got unsigned parsing with no warning, and the values only look wrong once the high bit is set, which for a length or an identifier may never happen in testing. Deleting the docstring line would have left the trap in place with its advertisement removed, so the argument is rejected instead. No in-tree caller is affected: every `signed=` in the tree goes to the base `NumberField`, where `__signed__` is unset and the argument always worked, and no call site passes it positionally. - numbers.py: `signed` defaults to `None` rather than `False`. That is what lets an explicit contradiction be told apart from the default, and both directions need it -- `Int8Field(signed=False)` is otherwise indistinguishable from `Int8Field()`. A contradiction raises `FieldValueError` naming the class and the sign it fixes; an agreeing or omitted value behaves exactly as before. `FieldValueError` rather than `BoolError`, which means "must *be* a bool", and rather than `FieldError`, which this package raises for a missing or wrong-kind argument (`SchemaField` with no schema); `BitField.__init__` already rejects a bad `namespace` *value* with `FieldValueError`. - numbers.py: the struct template is built from the resolved `self._signed` rather than from the raw argument. Same defect one level down -- a subclass fixing `__signed__` without `__template__` declared itself signed and then unpacked unsigned, giving `>I` and parsing `b'\xff\xff\xff\xff'` as `4294967295` instead of `-1`. Nothing in the tree does this today; the base class allows it, and `__call__` and `pre_process` already used `self._signed`. - numbers.py: all ten docstrings that documented `signed` now say what it does, and the eight fixed classes document the `Raises:`. - tests: `tests/corekit/test_fields_numbers.py`, the first test module for this file, covering both directions across all eight classes, the agreeing and omitted cases, the census by introspection so a ninth subclass cannot escape it, and the template defect through a parse rather than a string compare. Unit tier green: 1048 passed, 8 skipped. --- pcapkit/corekit/fields/numbers.py | 109 ++++++++++++++--- tests/corekit/test_fields_numbers.py | 175 +++++++++++++++++++++++++++ 2 files changed, 269 insertions(+), 15 deletions(-) create mode 100644 tests/corekit/test_fields_numbers.py diff --git a/pcapkit/corekit/fields/numbers.py b/pcapkit/corekit/fields/numbers.py index fb18018a68..c921a7c482 100644 --- a/pcapkit/corekit/fields/numbers.py +++ b/pcapkit/corekit/fields/numbers.py @@ -8,7 +8,7 @@ import aenum from pcapkit.corekit.fields.field import Field, NoValue -from pcapkit.utilities.exceptions import IntError +from pcapkit.utilities.exceptions import FieldValueError, IntError __all__ = [ 'NumberField', @@ -38,12 +38,28 @@ class NumberField(Field[int], Generic[_T]): length: Field size (in bytes); if a callable is given, it should return an integer value and accept the current packet as its only argument. default: Field default value, if any. - signed: Whether the field is signed. + signed: Whether the field is signed; :data:`None` defers to the + class-level ``__signed__``, which this class leaves unset and so + means unsigned. byteorder: Field byte order. bit_length: Field bit length. callback: Callback function to be called upon :meth:`self.__call__ `. + Raises: + 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. + + Notes: + A subclass such as :class:`UInt32Field` fixes the sign through + ``__signed__``, so ``signed`` there is at best redundant. It used to be + discarded outright, in both directions, which meant + ``UInt32Field(signed=True)`` handed back an unsigned field whose values + only looked wrong once the high bit was set -- see GitHub issue #545. A + contradicting value is now rejected instead; omitting it, or passing the + sign the class already fixes, stays legal. + """ __length__ = None # type: Optional[int] @@ -56,7 +72,7 @@ def bit_length(self) -> 'int': return self._bit_length def __init__(self, length: 'Optional[int | Callable[[dict[str, Any]], int]]' = None, - default: 'int | NoValueType' = NoValue, signed: 'bool' = False, + default: 'int | NoValueType' = NoValue, signed: 'Optional[bool]' = None, byteorder: 'Literal["little", "big"]' = 'big', bit_length: 'Optional[int]' = None, callback: 'Callable[[Self, dict[str, Any]], None]' = lambda *_: None) -> 'None': @@ -72,7 +88,23 @@ def __init__(self, length: 'Optional[int | Callable[[dict[str, Any]], int]]' = N else: self._bit_length, self._bit_mask = -1, -1 - self._signed = signed if self.__signed__ is None else self.__signed__ + # NOTE: ``__signed__`` fixes the sign for a subclass such as + # :class:`UInt32Field`, and used to *discard* the ``signed`` argument to + # do it -- in both directions, so ``UInt32Field(signed=True)`` returned + # an unsigned field and ``Int8Field(signed=False)`` a signed one, both + # without a word. ``None`` is what "not given" looks like, which is what + # lets a contradicting value be told apart from the default and rejected + # while leaving an agreeing one alone. See #545. + if self.__signed__ is None: + self._signed = False if signed is None else bool(signed) + elif signed is None or bool(signed) == self.__signed__: + self._signed = self.__signed__ + else: + raise FieldValueError( + f'{type(self).__name__}: field is fixed as ' + f'{"signed" if self.__signed__ else "unsigned"}, ' + f'but signed={signed!r} was given' + ) self._byteorder = byteorder self._need_process = False @@ -80,7 +112,11 @@ def __init__(self, length: 'Optional[int | Callable[[dict[str, Any]], int]]' = N if self.__template__ is not None: struct_fmt = self.__template__ else: - struct_fmt = self.build_template(self._length, signed) + # NOTE: ``self._signed``, not the ``signed`` argument. A subclass + # that fixes ``__signed__`` without also fixing ``__template__`` + # would otherwise build its template from the argument and parse + # with the opposite sign to the one it declared. + struct_fmt = self.build_template(self._length, self._signed) self._template = f'{endian}{struct_fmt}' def __call__(self, packet: 'dict[str, Any]') -> 'Self': @@ -194,12 +230,17 @@ class Int32Field(NumberField): Args: length: Field size (in bytes). default: Field default value, if any. - signed: Whether the field is signed. + signed: Whether the field is signed; fixed as :data:`True` here, so a + contradicting :data:`False` is rejected rather than ignored. byteorder: Field byte order. bit_length: Field bit length. callback: Callback function to be called upon :meth:`self.__call__ `. + Raises: + FieldValueError: If ``signed`` is given as :data:`False`, contradicting + the sign this class fixes. + """ __length__ = 4 @@ -213,12 +254,17 @@ class UInt32Field(NumberField): Args: length: Field size (in bytes). default: Field default value, if any. - signed: Whether the field is signed. + signed: Whether the field is signed; fixed as :data:`False` here, so a + contradicting :data:`True` is rejected rather than ignored. byteorder: Field byte order. bit_length: Field bit length. callback: Callback function to be called upon :meth:`self.__call__ `. + Raises: + FieldValueError: If ``signed`` is given as :data:`True`, contradicting + the sign this class fixes. + """ __length__ = 4 @@ -232,12 +278,17 @@ class Int16Field(NumberField): Args: length: Field size (in bytes). default: Field default value, if any. - signed: Whether the field is signed. + signed: Whether the field is signed; fixed as :data:`True` here, so a + contradicting :data:`False` is rejected rather than ignored. byteorder: Field byte order. bit_length: Field bit length. callback: Callback function to be called upon :meth:`self.__call__ `. + Raises: + FieldValueError: If ``signed`` is given as :data:`False`, contradicting + the sign this class fixes. + """ __length__ = 2 @@ -251,12 +302,17 @@ class UInt16Field(NumberField): Args: length: Field size (in bytes). default: Field default value, if any. - signed: Whether the field is signed. + signed: Whether the field is signed; fixed as :data:`False` here, so a + contradicting :data:`True` is rejected rather than ignored. byteorder: Field byte order. bit_length: Field bit length. callback: Callback function to be called upon :meth:`self.__call__ `. + Raises: + FieldValueError: If ``signed`` is given as :data:`True`, contradicting + the sign this class fixes. + """ __length__ = 2 @@ -270,12 +326,17 @@ class Int64Field(NumberField): Args: length: Field size (in bytes). default: Field default value, if any. - signed: Whether the field is signed. + signed: Whether the field is signed; fixed as :data:`True` here, so a + contradicting :data:`False` is rejected rather than ignored. byteorder: Field byte order. bit_length: Field bit length. callback: Callback function to be called upon :meth:`self.__call__ `. + Raises: + FieldValueError: If ``signed`` is given as :data:`False`, contradicting + the sign this class fixes. + """ __length__ = 8 @@ -289,12 +350,17 @@ class UInt64Field(NumberField): Args: length: Field size (in bytes). default: Field default value, if any. - signed: Whether the field is signed. + signed: Whether the field is signed; fixed as :data:`False` here, so a + contradicting :data:`True` is rejected rather than ignored. byteorder: Field byte order. bit_length: Field bit length. callback: Callback function to be called upon :meth:`self.__call__ `. + Raises: + FieldValueError: If ``signed`` is given as :data:`True`, contradicting + the sign this class fixes. + """ __length__ = 8 @@ -308,12 +374,17 @@ class Int8Field(NumberField): Args: length: Field size (in bytes). default: Field default value, if any. - signed: Whether the field is signed. + signed: Whether the field is signed; fixed as :data:`True` here, so a + contradicting :data:`False` is rejected rather than ignored. byteorder: Field byte order. bit_length: Field bit length. callback: Callback function to be called upon :meth:`self.__call__ `. + Raises: + FieldValueError: If ``signed`` is given as :data:`False`, contradicting + the sign this class fixes. + """ __length__ = 1 @@ -327,12 +398,17 @@ class UInt8Field(NumberField): Args: length: Field size (in bytes). default: Field default value, if any. - signed: Whether the field is signed. + signed: Whether the field is signed; fixed as :data:`False` here, so a + contradicting :data:`True` is rejected rather than ignored. byteorder: Field byte order. bit_length: Field bit length. callback: Callback function to be called upon :meth:`self.__call__ `. + Raises: + FieldValueError: If ``signed`` is given as :data:`True`, contradicting + the sign this class fixes. + """ __length__ = 1 @@ -347,7 +423,9 @@ class EnumField(NumberField[Union[enum.IntEnum, aenum.IntEnum]]): length: Field size (in bytes); if a callable is given, it should return an integer value and accept the current packet as its only argument. default: Field default value, if any. - signed: Whether the field is signed. + signed: Whether the field is signed; :data:`None` defers to the + class-level ``__signed__``, which this class leaves unset and so + means unsigned. byteorder: Field byte order. bit_length: Field bit length. namespace: Field namespace (a :class:`enum.IntEnum` class). @@ -357,7 +435,8 @@ class EnumField(NumberField[Union[enum.IntEnum, aenum.IntEnum]]): """ def __init__(self, length: 'int | Callable[[dict[str, Any]], int]', - default: 'StdlibEnum | AenumEnum | NoValueType' = NoValue, signed: 'bool' = False, + default: 'StdlibEnum | AenumEnum | NoValueType' = NoValue, + signed: 'Optional[bool]' = None, byteorder: 'Literal["little", "big"]' = 'big', bit_length: 'Optional[int]' = None, namespace: 'Optional[Type[StdlibEnum] | Type[AenumEnum]]' = None, diff --git a/tests/corekit/test_fields_numbers.py b/tests/corekit/test_fields_numbers.py new file mode 100644 index 0000000000..d76be8bae9 --- /dev/null +++ b/tests/corekit/test_fields_numbers.py @@ -0,0 +1,175 @@ +from __future__ import annotations + +import importlib.util +import inspect +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) + +#: The eight :class:`~pcapkit.corekit.fields.numbers.NumberField` subclasses that +#: fix their sign through ``__signed__``, each paired with the sign it fixes. +#: Derived by introspection in :meth:`FixedSignTests.test_the_census_is_complete` +#: rather than only written down here, so a ninth subclass added tomorrow fails +#: that test instead of quietly escaping every other test in this module. +FIXED_SIGN = { + 'Int8Field': True, 'Int16Field': True, 'Int32Field': True, 'Int64Field': True, + 'UInt8Field': False, 'UInt16Field': False, 'UInt32Field': False, 'UInt64Field': False, +} + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class FixedSignTests(unittest.TestCase): + """``signed=`` on a subclass whose ``__signed__`` fixes the sign. + + GitHub issue #545. The argument was accepted, documented on every one of + these eight classes, and then discarded -- in *both* directions, so + ``UInt8Field(signed=True)._signed`` was :data:`False` and + ``Int8Field(signed=False)._signed`` was :data:`True`. A caller who asked for + a signed field got unsigned parsing with no warning, and the values only + look wrong once the high bit is set, which for a length or an identifier may + never happen in testing. + + Both directions are asserted separately because the defect was symmetrical: + a fix that only guarded one of them would pass a test that only checked one. + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_the_census_is_complete(self) -> None: + """:data:`FIXED_SIGN` is every subclass that fixes a sign, by introspection.""" + from pcapkit.corekit.fields import numbers + + found = { + name: obj.__signed__ + for name, obj in vars(numbers).items() + if inspect.isclass(obj) and issubclass(obj, numbers.NumberField) + and obj is not numbers.NumberField and obj.__signed__ is not None + } + self.assertEqual(found, FIXED_SIGN) + + def test_an_unsigned_field_rejects_a_contradicting_signed_true(self) -> None: + from pcapkit.corekit.fields import numbers + from pcapkit.utilities.exceptions import FieldValueError + + for name, fixed in FIXED_SIGN.items(): + if fixed: + continue + with self.assertRaises(FieldValueError, msg=f'{name}(signed=True) was accepted'): + getattr(numbers, name)(signed=True) + + def test_a_signed_field_rejects_a_contradicting_signed_false(self) -> None: + from pcapkit.corekit.fields import numbers + from pcapkit.utilities.exceptions import FieldValueError + + for name, fixed in FIXED_SIGN.items(): + if not fixed: + continue + with self.assertRaises(FieldValueError, msg=f'{name}(signed=False) was accepted'): + getattr(numbers, name)(signed=False) + + def test_a_signed_agreeing_with_the_class_is_accepted(self) -> None: + """Redundant is not wrong: only a *contradiction* is rejected.""" + from pcapkit.corekit.fields import numbers + + for name, fixed in FIXED_SIGN.items(): + field = getattr(numbers, name)(signed=fixed) + self.assertIs(field._signed, fixed, f'{name}(signed={fixed})') + + def test_omitting_signed_keeps_the_class_sign(self) -> None: + from pcapkit.corekit.fields import numbers + + for name, fixed in FIXED_SIGN.items(): + self.assertIs(getattr(numbers, name)()._signed, fixed, name) + + def test_the_rejection_names_the_class_and_the_sign_it_fixes(self) -> None: + """An error a caller cannot act on is barely better than silence.""" + from pcapkit.corekit.fields.numbers import Int16Field, UInt32Field + from pcapkit.utilities.exceptions import FieldValueError + + with self.assertRaisesRegex(FieldValueError, r'UInt32Field: field is fixed as unsigned'): + UInt32Field(signed=True) + with self.assertRaisesRegex(FieldValueError, r'Int16Field: field is fixed as signed'): + Int16Field(signed=False) + + def test_a_contradiction_is_judged_by_truth_value(self) -> None: + """``signed`` is documented as a :obj:`bool` and is read as one. + + ``signed=1`` contradicts an unsigned field exactly as ``signed=True`` + does, and ``signed=0`` agrees with it exactly as ``signed=False`` does. + Pinned because the check could as easily have been an identity test, + which would let ``UInt8Field(signed=1)`` through. + + """ + from pcapkit.corekit.fields.numbers import UInt8Field + from pcapkit.utilities.exceptions import FieldValueError + + with self.assertRaises(FieldValueError): + UInt8Field(signed=1) + self.assertIs(UInt8Field(signed=0)._signed, False) + + def test_the_fixed_classes_still_parse_as_they_always_did(self) -> None: + """The rejection must not have moved the sign of a default construction.""" + from pcapkit.corekit.fields.numbers import Int8Field, UInt8Field + + self.assertEqual(Int8Field().unpack(b'\xff', {}), -1) + self.assertEqual(UInt8Field().unpack(b'\xff', {}), 255) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class DeferredSignTests(unittest.TestCase): + """``signed=`` where no ``__signed__`` fixes it, which is where it works.""" + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_the_base_class_still_honours_signed(self) -> None: + from pcapkit.corekit.fields.numbers import NumberField + + self.assertIs(NumberField(length=4, signed=True)._signed, True) + self.assertEqual(NumberField(length=4, signed=True).template, '>i') + self.assertIs(NumberField(length=4, signed=False)._signed, False) + self.assertEqual(NumberField(length=4, signed=False).template, '>I') + + def test_the_base_class_defaults_to_unsigned(self) -> None: + """:data:`None` is the new default and has to mean what ``False`` meant.""" + from pcapkit.corekit.fields.numbers import NumberField + + self.assertIs(NumberField(length=4)._signed, False) + self.assertEqual(NumberField(length=4).template, '>I') + self.assertEqual(NumberField(length=4).unpack(b'\xff\xff\xff\xff', {}), 0xffffffff) + + def test_an_enum_field_still_honours_signed(self) -> None: + """:class:`EnumField` fixes no sign, so its own ``signed`` is real.""" + from pcapkit.corekit.fields.numbers import EnumField + + self.assertIs(EnumField(length=4, signed=True)._signed, True) + self.assertIs(EnumField(length=4, signed=False)._signed, False) + self.assertIs(EnumField(length=4)._signed, False) + + def test_a_class_fixing_a_sign_without_a_template_parses_with_that_sign(self) -> None: + """The second instance of the same defect, one level down. + + ``__signed__`` was resolved into :attr:`_signed`, but the struct + template was built from the raw ``signed`` *argument*. A subclass that + fixes ``__signed__`` and leaves ``__template__`` unset -- which the + base class explicitly allows, and which nothing in ``pcapkit`` happens + to do today -- therefore declared itself signed and then unpacked + unsigned: ``>I`` for a class saying ``__signed__ = True``, so + ``b'\\xff\\xff\\xff\\xff'`` parsed as ``4294967295`` instead of ``-1``. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + class SignedNoTemplate(NumberField): + __length__ = 4 + __signed__ = True + + field = SignedNoTemplate() + self.assertIs(field._signed, True) + self.assertEqual(field.template, '>i') + self.assertEqual(field.unpack(b'\xff\xff\xff\xff', {}), -1)