diff --git a/pcapkit/corekit/fields/strings.py b/pcapkit/corekit/fields/strings.py index fcf93254b9..d7387f1138 100644 --- a/pcapkit/corekit/fields/strings.py +++ b/pcapkit/corekit/fields/strings.py @@ -265,8 +265,17 @@ class BitField(_TextField[Dict[str, Any]]): callback: Callback function to be called upon :meth:`self.__call__ `. + Bits that no subfield names are kept as captured: where any of them is + set, the unpacked value carries them under an ``__unnamed__`` key, as an + integer over the whole field with every named bit cleared, and packing + writes them back (:issue:`1487`). The key is absent when they are all + zero, so a value built without it packs them as zeros. + """ + #: Key of the unpacked value that carries the unnamed bits. + UNNAMED = '__unnamed__' + def __init__(self, length: 'int', default: 'dict[str, Any] | NoValueType' = NO_VALUE, namespace: 'Optional[dict[str, NamespaceEntry]]' = None, @@ -288,6 +297,11 @@ def __init__(self, length: 'int', f'{start}-{start + size - 1} of a {width}-bit field' ) + #: Mask of the bits no subfield names, most significant bit first. + self._unnamed = (1 << width) - 1 + for start, size in self._namespace.values(): + self._unnamed &= ~(((1 << size) - 1) << (width - start - size)) + def pre_process(self, value: 'dict[str, Any]', packet: 'dict[str, Any]') -> 'bytes': # pylint: disable=unused-argument """Process field value before construction (packing). @@ -300,7 +314,8 @@ def pre_process(self, value: 'dict[str, Any]', packet: 'dict[str, Any]') -> 'byt Raises: FieldValueError: If a subfield value does not fit the bits it was - declared to occupy. + declared to occupy, or the ``__unnamed__`` value is not an + integer. """ # NOTE: The buffer holds one ASCII digit per bit, so it must be seeded with @@ -308,7 +323,15 @@ def pre_process(self, value: 'dict[str, Any]', packet: 'dict[str, Any]') -> 'byt # is the *character* ``b'0'``, which is itself non-zero, so a truthiness test # over the buffer cannot tell a cleared bit from a set one and would report # every named bit as set. - buffer = bytearray(b'0' * (self.length * 8)) + # + # The unnamed bits seed it, so that they come back as captured; masking + # them keeps a stray named bit in the record from overriding a subfield. + unnamed = value.get(self.UNNAMED, 0) + if isinstance(unnamed, bool) or not isinstance(unnamed, int): + raise FieldValueError( + f'{type(self).__name__}: {self.UNNAMED} must be an int, not {unnamed!r}' + ) + buffer = bytearray(f'{unnamed & self._unnamed:0{self.length * 8}b}'.encode()) for name, (start, size) in self._namespace.items(): bits = f'{value[name]:0{size}b}' if len(bits) > size: @@ -335,6 +358,9 @@ def post_process(self, value: 'bytes', packet: 'dict[str, Any]') -> 'dict[str, A for name, (start, len) in self._namespace.items(): end = start + len buffer[name] = int(binary[start:end], 2) + unnamed = int.from_bytes(value, 'big') & self._unnamed + if unnamed: + buffer[self.UNNAMED] = unnamed return buffer diff --git a/tests/corekit/test_bitfield_unnamed_bits_1487_unit.py b/tests/corekit/test_bitfield_unnamed_bits_1487_unit.py new file mode 100644 index 0000000000..9afbbcf367 --- /dev/null +++ b/tests/corekit/test_bitfield_unnamed_bits_1487_unit.py @@ -0,0 +1,89 @@ +# -*- coding: utf-8 -*- +"""A BitField packs back the bits its namespace does not name. + +GitHub issue #1487: :class:`~pcapkit.corekit.fields.strings.BitField` read only +its named subfields and packed from all zeros, so octets with a bit outside +every subfield set came back with it cleared. The unpacked value now carries +those bits under ``__unnamed__`` when any is set, and a value without the key +packs them as zeros, as before. + +:mod:`pcapkit` is imported inside each test, after +:func:`~tests._support.reimport_once_per_class`, not at module load. + +""" + +import unittest + +from tests._support import reimport_once_per_class + +#: ``a`` is bit 0 and ``c`` bits 8-11; bits 1-7 and 12-15 are unnamed. +NAMESPACE = {'a': (0, 1), 'c': (8, 4)} + + +class BitFieldUnnamedBitsTests(unittest.TestCase): + """GitHub issue #1487.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + from pcapkit.corekit.fields.strings import BitField + + self.field = BitField(length=2, namespace=NAMESPACE)({}) + + def _round_trip(self, raw: bytes) -> bytes: + return self.field.pack(self.field.unpack(raw, {}), {}) + + def test_unnamed_bits_survive_unpack_then_pack(self) -> None: + for raw in (b'\xff\xff', b'\x01\x00', b'\x7f\x0f', b'\x00\x01', b'\x80\xf0', b'\x00\x00'): + with self.subTest(raw=raw.hex()): + self.assertEqual(self._round_trip(raw), raw) + + def test_unpacked_value_names_the_unnamed_bits(self) -> None: + self.assertEqual(self.field.unpack(b'\xff\xff', {}), + {'a': 1, 'c': 15, '__unnamed__': 0x7f0f}) + + def test_key_is_absent_when_the_unnamed_bits_are_zero(self) -> None: + self.assertEqual(self.field.unpack(b'\x80\xf0', {}), {'a': 1, 'c': 15}) + + def test_a_value_without_the_key_packs_unnamed_bits_as_zero(self) -> None: + self.assertEqual(self.field.pack({'a': 1, 'c': 15}, {}), b'\x80\xf0') + self.assertEqual(self.field.pack({'a': 0, 'c': 0}, {}), b'\x00\x00') + + def test_named_subfields_win_over_the_record(self) -> None: + """A named bit set in ``__unnamed__`` does not override its subfield.""" + self.assertEqual(self.field.pack({'a': 0, 'c': 0, '__unnamed__': 0xffff}, {}), b'\x7f\x0f') + + def test_edited_subfield_keeps_the_captured_unnamed_bits(self) -> None: + value = self.field.unpack(b'\xff\xff', {}) + value['c'] = 0 + self.assertEqual(self.field.pack(value, {}), b'\xff\x0f') + + def test_a_non_int_record_is_rejected(self) -> None: + from pcapkit.utilities.exceptions import FieldValueError + + for bad in ('1', 1.0, True, None): + with self.subTest(bad=bad), self.assertRaises(FieldValueError): + self.field.pack({'a': 0, 'c': 0, '__unnamed__': bad}, {}) + + def test_schema_to_dict_from_dict_keeps_the_unnamed_bits(self) -> None: + from pcapkit.corekit.fields.strings import BitField + from pcapkit.protocols.schema.schema import Schema, schema_final + + @schema_final + class Flags(Schema): + flags: dict = BitField(length=2, namespace=NAMESPACE) + + raw = b'\x7f\x0f' + parsed = Flags.unpack(raw, len(raw), None) + self.assertEqual(parsed.pack(), raw) + self.assertEqual(Flags.from_dict(parsed.to_dict()).pack(), raw) + + def test_full_namespace_never_gains_the_key(self) -> None: + from pcapkit.corekit.fields.strings import BitField + + field = BitField(length=1, namespace={'hi': (0, 4), 'lo': (4, 4)})({}) + self.assertEqual(field.unpack(b'\xff', {}), {'hi': 15, 'lo': 15}) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/corekit/test_field_roundtrip_unit.py b/tests/corekit/test_field_roundtrip_unit.py index d1b6c5aa4a..d7f3c39fd6 100644 --- a/tests/corekit/test_field_roundtrip_unit.py +++ b/tests/corekit/test_field_roundtrip_unit.py @@ -550,13 +550,6 @@ class FieldRoundTripTests(KnownFailureTable, unittest.TestCase): STATUSES = ('OK', 'PACK', 'UNPACK', 'VALUE', 'REPACK', 'WIRE', 'DICT', 'REJECTED', 'TIMEOUT') KNOWN_FAILURES = ( - Gap(1487, 'BitField.post_process reads only the bits its namespace names, and ' - 'pre_process seeds the rest as zeros, so octets with a bit outside every ' - 'subfield set pack back with it cleared ' - '(pcapkit/corekit/fields/strings.py:311, :334). Not reachable from a shipped ' - 'schema: all 102 BitFields cover every bit ' - '(test_every_schema_bitfield_covers_every_bit)', - 'WIRE', '', ('BitField/partial/wire/ffff', 'BitField/partial/wire/0100')), 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 '