From db3b2ecad5afa3d385dd42906a8a163518017fda Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 9 Oct 2026 16:20:09 -0400 Subject: [PATCH] fix(corekit): keep a NumberField's bits outside bit_length as captured (#1488) - NumberField.post_process: when the octets carry bits outside bit_length that pre_process would not write back (zeros, or a negative signed value's sign extension), return an int subclass recording them with the capturing field's length, bit_length, sign and byte order. pre_process writes them back only for a field matching all four; any other field, and any other value, is truncated to bit_length bits as before. - EnumField.post_process: carry that record on the pseudo-member, or on an unregistered copy of a registry member, which stays isinstance and equal. - Delete the Gap(1488) row; relax one assertIs pin to isinstance/equality for an octet whose spare bits are set. --- pcapkit/corekit/fields/numbers.py | 153 +++++++++++++- tests/corekit/test_field_roundtrip_unit.py | 11 +- ...est_fields_numbers_bit_length_1488_unit.py | 200 ++++++++++++++++++ .../test_fields_numbers_unassigned_enum.py | 9 +- 4 files changed, 357 insertions(+), 16 deletions(-) create mode 100644 tests/corekit/test_fields_numbers_bit_length_1488_unit.py diff --git a/pcapkit/corekit/fields/numbers.py b/pcapkit/corekit/fields/numbers.py index 113a055a85..2d285275c5 100644 --- a/pcapkit/corekit/fields/numbers.py +++ b/pcapkit/corekit/fields/numbers.py @@ -31,6 +31,92 @@ _T = TypeVar('_T', bound='int') +#: Instance attribute a parsed value records its bits outside +#: :attr:`NumberField.bit_length` under, as ``(outside, length, bit_length, +#: signed, byteorder)``. See GitHub issue :issue:`1488`. +_OUTSIDE_BITS = '_outside_bits_' + + +class _CapturedInt(int): + """An :obj:`int` that also records the bits outside its field's + :attr:`~NumberField.bit_length` as they were captured. + + It equals, hashes and renders as the plain :obj:`int` it holds, and any + arithmetic on it yields a plain :obj:`int`, which is what lets a value a + caller derives from a parsed one keep today's truncating semantics. See + GitHub issue :issue:`1488`. + + """ + + +def _capture_key(field: 'NumberField') -> 'tuple[int, int, bool, str]': + """Return the shape of ``field`` a captured value's bits belong to. + + Args: + field: The field packing or unpacking the value. + + Returns: + ``(length, bit_length, signed, byteorder)``. + + """ + return (field._length, field._bit_length, # pylint: disable=protected-access + field._signed, field._byteorder) # pylint: disable=protected-access + + +def _capture_record(value: 'Any') -> 'Optional[tuple[int, int, int, bool, str]]': + """Return the capture ``value`` records, or :data:`None` if it records none. + + Args: + value: Field value. + + Returns: + ``(outside, length, bit_length, signed, byteorder)``, or :data:`None`. + + The instance dictionary is read rather than :func:`getattr`, so that no + class-level attribute -- an enumeration's member lookup among them -- can + answer for it. + + """ + return getattr(value, '__dict__', {}).get(_OUTSIDE_BITS) + + +def _outside_bits(value: 'Any', field: 'NumberField') -> 'Optional[int]': + """Return the captured bits ``value`` records outside ``field``'s + :attr:`~NumberField.bit_length`, or :data:`None`. + + Args: + value: Field value. + field: The field packing it. + + Returns: + The bits, in place (not shifted down), if ``value`` was captured by a + field of the same length, bit length, sign and byte order as + ``field``; :data:`None` otherwise, so that any other field packs it + exactly as it would the plain value. + + """ + record = _capture_record(value) + if record is None or tuple(record[1:]) != _capture_key(field): + return None + return record[0] + + +def _with_outside_bits(value: 'int', record: 'tuple[int, int, int, bool, str]') -> 'int': + """Return ``value`` recording ``record`` as its captured bits outside + :attr:`~NumberField.bit_length`. + + Args: + value: Field value, the low :attr:`~NumberField.bit_length` bits. + record: ``(outside, length, bit_length, signed, byteorder)``. + + Returns: + A :class:`_CapturedInt` equal to ``value``. + + """ + captured = _CapturedInt(value) + setattr(captured, _OUTSIDE_BITS, record) + return captured + class NumberField(Field[int], Generic[_T]): """Numerical value for protocol fields. @@ -276,9 +362,26 @@ def pre_process(self, value: 'int', packet: 'dict[str, Any]') -> 'int | bytes': :meth:`int.to_bytes` and :func:`struct.pack` both refuse. See GitHub issue :issue:`599`. + A value :meth:`post_process` returned for octets whose bits outside + :attr:`bit_length` were not what packing writes there carries those + bits (see :func:`_outside_bits`), and they are written back as + captured, so ``pack(unpack(b)) == b`` holds for every ``b``. Only a + field of the same length, bit length, sign and byte order as the + one that captured them honours them; any other field, and any other + value -- one a caller built, or the result of arithmetic on a + parsed one, which is a plain :obj:`int` -- is truncated to + :attr:`bit_length` bits exactly as before. See GitHub issue + :issue:`1488`. + """ + outside = _outside_bits(value, self) value = value & self._bit_mask - if self._signed and value > self._bit_mask >> 1: + if outside is not None and self._length >= 0: + value |= outside + width = self._length * 8 + if self._signed and value >> (width - 1): + value -= 1 << width + elif self._signed and value > self._bit_mask >> 1: value -= self._bit_mask + 1 if self._need_process and self._length < 0: @@ -313,15 +416,30 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'int': resolved signed field therefore decodes ``ffffffff`` as ``-1`` rather than ``4294967295``. See GitHub issue :issue:`1274`. + The value is the low :attr:`bit_length` bits, but the octets hold + more. When the bits outside it are not what :meth:`pre_process` + would write there for this value -- zeros, or for a negative signed + value the sign extension -- they are captured but uninterpreted, + and the value records them (see :func:`_with_outside_bits`) so that + packing it writes them back. Otherwise the value is a plain + :obj:`int`, as before. See GitHub issue :issue:`1488`. + """ if not self._need_process: - value = cast('int', value) & self._bit_mask + raw = cast('int', value) else: - value = int.from_bytes( + raw = int.from_bytes( cast('bytes', value), self._byteorder, signed=self._signed - ) & self._bit_mask + ) + value = raw & self._bit_mask if self._signed and value > self._bit_mask >> 1: value -= self._bit_mask + 1 + + if 0 <= self._bit_length < self._length * 8: + spare = ((1 << (self._length * 8)) - 1) & ~self._bit_mask + outside = raw & spare + if outside != (spare if value < 0 else 0): + return _with_outside_bits(value, (outside, *_capture_key(self))) return value @@ -626,18 +744,41 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'Stdli a registry registered from outside :mod:`pcapkit.const`, which has no such obligation to stay quiet. + Octets carrying bits outside :attr:`~NumberField.bit_length` that + :meth:`NumberField.post_process` recorded resolve to a member that + records them too, so that packing it writes them back. A registry + member is shared, so that one is an unregistered copy of it, built + by :meth:`_unregistered_member`: ``isinstance`` against the + registry and equality with the member still hold, identity does + not. See GitHub issue :issue:`1488`. + """ value = super().post_process(value, packet) + record = _capture_record(value) if self._namespace is not None: try: - return self._namespace(value) + member = self._namespace(value) except ValueError as error: # NOTE: An in-library rejection is pcapkit's own decision about # the value, rather than the enumeration library reporting that # no member carries it, so it is not this layer's to absorb. if isinstance(error, BaseError): raise - return self._pseudo_member(value) + else: + if record is None: + return member + # NOTE: A registry member is shared, so the captured bits + # outside ``bit_length`` go on an unregistered copy of it + # rather than on the member itself (:issue:`1488`). + attrs = {key: val for key, val in vars(member).items() + if key not in ('_name_', '_value_', '__reduce_ex__')} + attrs[_OUTSIDE_BITS] = record + return self._unregistered_member( + type(member), member._value_, member._name_, **attrs) # pylint: disable=protected-access + pseudo = self._pseudo_member(int(value)) + if record is not None: + setattr(pseudo, _OUTSIDE_BITS, record) + return pseudo def _pseudo_member(self, value: 'int') -> 'StdlibEnum | AenumEnum': """Build the bounded nameless pseudo-member this method falls back to diff --git a/tests/corekit/test_field_roundtrip_unit.py b/tests/corekit/test_field_roundtrip_unit.py index d7f3c39fd6..bf1dd669c0 100644 --- a/tests/corekit/test_field_roundtrip_unit.py +++ b/tests/corekit/test_field_roundtrip_unit.py @@ -34,7 +34,7 @@ from typing import TYPE_CHECKING, NamedTuple from tests._support import reimport_once_per_class -from tests.corekit._roundtrip import (OK, Gap, KnownFailureTable, Outcome, describe, diff, run, +from tests.corekit._roundtrip import (OK, KnownFailureTable, Outcome, describe, diff, run, skip_without_runtime) if TYPE_CHECKING: @@ -549,14 +549,7 @@ class FieldRoundTripTests(KnownFailureTable, unittest.TestCase): STATUSES = ('OK', 'PACK', 'UNPACK', 'VALUE', 'REPACK', 'WIRE', 'DICT', 'REJECTED', 'TIMEOUT') - KNOWN_FAILURES = ( - Gap(1488, 'NumberField with bit_length narrower than its octets masks the unpacked value ' - 'to bit_length bits, so set high bits are dropped and pack writes them as ' - 'zeros (pcapkit/corekit/fields/numbers.py:318, :322). Shipped use: vlan.TCI ' - '(pcapkit/protocols/schema/link/vlan.py:47-51), which no protocol parses with', - 'WIRE', '', ('NumberField/bit-length-12/wire/ffff', 'NumberField/bit-length-12/wire/f000', - 'EnumField/bit-length-3/wire/ff')), - ) + KNOWN_FAILURES = () def setUp(self) -> None: skip_without_runtime(self) diff --git a/tests/corekit/test_fields_numbers_bit_length_1488_unit.py b/tests/corekit/test_fields_numbers_bit_length_1488_unit.py new file mode 100644 index 0000000000..af26cc204b --- /dev/null +++ b/tests/corekit/test_fields_numbers_bit_length_1488_unit.py @@ -0,0 +1,200 @@ +# -*- coding: utf-8 -*- +"""A number field keeps the bits outside its ``bit_length`` as captured. + +GitHub issue #1488: :meth:`NumberField.post_process +` masked the decoded +value to ``bit_length`` bits and :meth:`~pcapkit.corekit.fields.numbers.NumberField.pre_process` +masked it again, so ``UInt16Field(bit_length=12)`` read ``ffff`` and packed +``0fff``. The parsed value now records the bits it does not interpret, and a +value a caller supplies is still truncated to ``bit_length`` bits. + +Every case builds its own octets in memory and reads no capture. Classes are +imported inside each test, after :func:`~tests._support.reimport_once_per_class`. + +""" + +import copy +import io +import pickle +import unittest + +from tests._support import reimport_once_per_class + +#: Octets of a two-octet field, as hex. +WIRES = ('0000', '0001', '0fff', 'ffff', 'f000', '8000', '0800', 'f7ff', 'a5a5') + + +class TestBitLengthRoundTrip(unittest.TestCase): + """``pack(unpack(b)) == b`` for a field narrower than its octets.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def test_unsigned_wire_round_trips(self) -> None: + from pcapkit.corekit.fields.numbers import UInt16Field + + for order in ('big', 'little'): + for wire in WIRES: + with self.subTest(order=order, wire=wire): + raw = bytes.fromhex(wire) + value = UInt16Field(bit_length=12, byteorder=order)({}).unpack(raw, {}) + self.assertEqual(value, int.from_bytes(raw, order) & 0xFFF) + packed = UInt16Field(bit_length=12, byteorder=order)({}).pack(value, {}) + self.assertEqual(packed.hex(), wire) + + def test_signed_wire_round_trips(self) -> None: + from pcapkit.corekit.fields.numbers import Int16Field + + for wire in WIRES: + with self.subTest(wire=wire): + raw = bytes.fromhex(wire) + low = int.from_bytes(raw, 'big') & 0xFFF + value = Int16Field(bit_length=12)({}).unpack(raw, {}) + self.assertEqual(value, low - 0x1000 if low & 0x800 else low) + self.assertEqual(Int16Field(bit_length=12)({}).pack(value, {}).hex(), wire) + + def test_odd_width_wire_round_trips(self) -> None: + from pcapkit.corekit.fields.numbers import NumberField + + for wire in ('ffffff', 'f00000', '000001', '123456'): + with self.subTest(wire=wire): + raw = bytes.fromhex(wire) + value = NumberField(length=3, bit_length=20)({}).unpack(raw, {}) + self.assertEqual(value, int(wire, 16) & 0xFFFFF) + self.assertEqual(NumberField(length=3, bit_length=20)({}).pack(value, {}).hex(), wire) + + def test_enum_wire_round_trips(self) -> None: + from pcapkit.const.vlan.priority_level import PriorityLevel + from pcapkit.corekit.fields.numbers import EnumField + + for namespace in (None, PriorityLevel): + for octet in range(256): + with self.subTest(namespace=namespace, octet=octet): + raw = bytes([octet]) + value = EnumField(length=1, bit_length=3, namespace=namespace)({}).unpack(raw, {}) + self.assertEqual(value, octet & 7) + if namespace is not None: + self.assertIsInstance(value, PriorityLevel) + self.assertEqual(value.name, PriorityLevel(octet & 7).name) + packed = EnumField(length=1, bit_length=3, namespace=namespace)({}).pack(value, {}) + self.assertEqual(packed, raw) + self.assertEqual(len(PriorityLevel.__members__), 8) + + def test_captured_bits_survive_copy_and_pickle(self) -> None: + from pcapkit.const.vlan.priority_level import PriorityLevel + from pcapkit.corekit.fields.numbers import EnumField, UInt16Field + + number = UInt16Field(bit_length=12)({}).unpack(b'\xff\xff', {}) + member = EnumField(length=1, bit_length=3, namespace=PriorityLevel)({}).unpack(b'\xff', {}) + for clone in (copy.copy, copy.deepcopy, lambda v: pickle.loads(pickle.dumps(v))): + with self.subTest(clone=clone): + self.assertEqual(UInt16Field(bit_length=12)({}).pack(clone(number), {}), b'\xff\xff') + self.assertEqual(EnumField(length=1, bit_length=3, namespace=PriorityLevel)({}) + .pack(clone(member), {}), b'\xff') + + def test_value_without_spare_bits_is_a_plain_int(self) -> None: + from pcapkit.corekit.fields.numbers import Int16Field, UInt16Field + + self.assertIs(type(UInt16Field(bit_length=12)({}).unpack(b'\x0f\xff', {})), int) + self.assertIs(type(Int16Field(bit_length=12)({}).unpack(b'\xff\xff', {})), int) + self.assertIs(type(UInt16Field()({}).unpack(b'\xff\xff', {})), int) + + def test_schema_round_trips(self) -> None: + from pcapkit.corekit.fields.numbers import UInt8Field, UInt16Field + from pcapkit.protocols.schema.schema import Schema + + class T(Schema): + a: 'int' = UInt16Field(bit_length=12) + b: 'int' = UInt8Field(bit_length=1) + + data = b'\xab\xcd\xfe' + schema = T.unpack(io.BytesIO(data), 3, {'__length__': 3}) + self.assertEqual((schema.a, schema.b), (0xBCD, 0)) + self.assertEqual(schema.pack(), data) + self.assertEqual(T.from_dict(schema.to_dict()).pack(), data) + self.assertEqual(T(a=schema.a, b=schema.b).pack({}), data) + + +class TestBitLengthSuppliedValue(unittest.TestCase): + """A value a caller supplies is truncated to ``bit_length`` bits, as before.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def test_unsigned_value_is_truncated(self) -> None: + from pcapkit.corekit.fields.numbers import UInt16Field + + for value, wire in ((0, '0000'), (0xFFF, '0fff'), (0x1FFF, '0fff'), (0xFFFF, '0fff'), + (0xF000, '0000')): + with self.subTest(value=hex(value)): + self.assertEqual(UInt16Field(bit_length=12)({}).pack(value, {}).hex(), wire) + + def test_signed_value_is_sign_extended(self) -> None: + from pcapkit.corekit.fields.numbers import Int16Field + + for value, wire in ((-1, 'ffff'), (0xFFF, 'ffff'), (2047, '07ff'), (-2048, 'f800')): + with self.subTest(value=value): + self.assertEqual(Int16Field(bit_length=12)({}).pack(value, {}).hex(), wire) + + def test_derived_value_drops_the_captured_bits(self) -> None: + from pcapkit.const.vlan.priority_level import PriorityLevel + from pcapkit.corekit.fields.numbers import EnumField, UInt16Field + + value = UInt16Field(bit_length=12)({}).unpack(b'\xff\xfe', {}) + self.assertEqual(UInt16Field(bit_length=12)({}).pack(value + 1, {}), b'\x0f\xff') + self.assertEqual(UInt16Field(bit_length=12)({}).pack(int(value), {}), b'\x0f\xfe') + self.assertEqual(EnumField(length=1, bit_length=3, namespace=PriorityLevel)({}) + .pack(PriorityLevel(7), {}), b'\x07') + + +class TestBitLengthCaptureIsScoped(unittest.TestCase): + """Only a field shaped like the capturing one honours the captured bits.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def test_number_packed_by_another_field(self) -> None: + from pcapkit.corekit.fields.numbers import Int16Field, UInt8Field, UInt16Field + + value = UInt16Field(bit_length=12)({}).unpack(b'\xff\xff', {}) + others = { + 'full width': (lambda: UInt16Field(), '0fff'), + 'narrower bit_length': (lambda: UInt16Field(bit_length=8), '00ff'), + 'shorter octets': (lambda: UInt8Field(), 'ff'), + 'other byte order': (lambda: UInt16Field(bit_length=12, byteorder='little'), 'ff0f'), + 'other sign': (lambda: Int16Field(bit_length=12), 'ffff'), + } + for label, (make, wire) in others.items(): + with self.subTest(label): + want = make()({}).pack(int(value), {}) + self.assertEqual(want.hex(), wire) + self.assertEqual(make()({}).pack(value, {}), want) + + def test_same_shape_through_a_callable_length(self) -> None: + from pcapkit.corekit.fields.numbers import NumberField, UInt16Field + + value = UInt16Field(bit_length=12)({}).unpack(b'\xff\xff', {}) + field = NumberField(length=lambda pkt: pkt['n'], bit_length=12) + self.assertEqual(field({'n': 2}).pack(value, {'n': 2}), b'\xff\xff') + + def test_enum_member_packed_by_another_field(self) -> None: + from pcapkit.const.vlan.priority_level import PriorityLevel + from pcapkit.corekit.fields.numbers import EnumField, UInt8Field + + for namespace in (None, PriorityLevel): + member = EnumField(length=1, bit_length=3, namespace=namespace)({}).unpack(b'\xff', {}) + others = { + 'UInt8Field': lambda: UInt8Field(), + 'full-width EnumField': lambda: EnumField(length=1, namespace=namespace), + 'narrower bit_length': lambda: EnumField(length=1, bit_length=2, namespace=namespace), + } + for label, make in others.items(): + with self.subTest(namespace=namespace, field=label): + self.assertEqual(make()({}).pack(member, {}), + make()({}).pack(member.value, {})) + with self.subTest(namespace=namespace, field='UInt8Field octets'): + self.assertEqual(UInt8Field()({}).pack(member, {}), b'\x07') + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/corekit/test_fields_numbers_unassigned_enum.py b/tests/corekit/test_fields_numbers_unassigned_enum.py index 1ec7544b7f..d83b09d033 100644 --- a/tests/corekit/test_fields_numbers_unassigned_enum.py +++ b/tests/corekit/test_fields_numbers_unassigned_enum.py @@ -304,6 +304,9 @@ def test_bit_length_masking_still_precedes_the_lookup(self) -> None: unmasked ``0xF1`` would have produced a pseudo-member instead and hidden a regression in the masking. + The member is an unregistered copy of ``Nibble.ONE`` rather than the + member itself, since it also records the captured ``0xF0`` (#1488). + """ from pcapkit.corekit.fields.numbers import EnumField @@ -314,7 +317,11 @@ class Nibble(enum.IntEnum): field = EnumField(length=1, namespace=Nibble, bit_length=4) - self.assertIs(field.unpack(b'\xf1', dict()), Nibble.ONE) + resolved = field.unpack(b'\xf1', dict()) + self.assertIsInstance(resolved, Nibble) + self.assertEqual(resolved, Nibble.ONE) + self.assertEqual(resolved.name, 'ONE') + self.assertIs(field.unpack(b'\x01', dict()), Nibble.ONE) def test_no_value_of_any_width_escapes_the_field_layer(self) -> None: """The property, rather than the reported instance.