diff --git a/pcapkit/protocols/link/arp.py b/pcapkit/protocols/link/arp.py index 0a28c32398..b291ec3250 100644 --- a/pcapkit/protocols/link/arp.py +++ b/pcapkit/protocols/link/arp.py @@ -52,7 +52,7 @@ from pcapkit.protocols.link.link import Link from pcapkit.protocols.schema.link.arp import ARP as Schema_ARP from pcapkit.utilities.compat import cached_property -from pcapkit.utilities.exceptions import ProtocolError +from pcapkit.utilities.exceptions import FieldValueError, ProtocolError if TYPE_CHECKING: from enum import IntEnum as StdlibEnum @@ -403,15 +403,31 @@ def _make_addr_resolve(self, addr: 'str | bytes', htype: 'int') -> 'bytes': is validated and packed, one octet per group (an empty string packs as no octets); otherwise, a :class:`str` is taken as the *hex* encoded address that :meth:`_read_addr_resolve` - returns, and :obj:`bytes` are returned as given. + returns, and :class:`bytes`/:class:`bytearray` are returned as + :class:`bytes`. Raises: + FieldValueError: If ``addr`` is a :obj:`bool`, whatever ``htype`` + is. ProtocolError: If ``htype`` is ``1`` and ``addr`` is not a - well-formed MAC address, or if ``htype`` is not ``1`` and - ``addr`` is a :class:`str` that is not *hex* encoded. + well-formed MAC address given as :class:`str`, + :class:`bytes` or :class:`bytearray`, or if ``htype`` is not + ``1`` and ``addr`` is either a :class:`str` that is not *hex* + encoded or none of :class:`str`, :class:`bytes` and + :class:`bytearray`. """ + # NOTE: checked before the ``htype`` dispatch, for the same reason as + # in ``_make_proto_resolve``: without it a ``bool`` reaches the MAC + # pattern or falls through unchanged, and fails later as a + # ``TypeError`` (#2166). + if isinstance(addr, bool): + raise FieldValueError( + f'{self.__class__.__name__}: invalid hardware address: must not be a bool, ' + f'not {addr!r} -- pass int({addr!r}) if the numeric value is what is wanted') if htype == Enum_Hardware.Ethernet: + if not isinstance(addr, (str, bytes, bytearray)): + raise ProtocolError(f'Invalid MAC address: {addr!r}') _addr = addr.encode() if isinstance(addr, str) else addr if PAT_MAC_ADDR.fullmatch(_addr) is not None: return bytes.fromhex(_addr.replace(b':', b'').replace(b'-', b'').decode()) @@ -421,7 +437,9 @@ def _make_addr_resolve(self, addr: 'str | bytes', htype: 'int') -> 'bytes': return bytes.fromhex(addr.replace(':', '').replace('-', '')) except ValueError: raise ProtocolError(f'Invalid hardware address: {addr!r}') from None - return addr + if isinstance(addr, (bytes, bytearray)): + return bytes(addr) + raise ProtocolError(f'Invalid hardware address: {addr!r}') def _make_proto_resolve(self, addr: 'IPv4Address | IPv6Address | str | bytes', ptype: 'int', plen: 'Optional[int]' = None) -> 'bytes': @@ -439,15 +457,18 @@ def _make_proto_resolve(self, addr: 'IPv4Address | IPv6Address | str | bytes', p octets; otherwise, a :class:`str` is taken as the *hex* encoded address that :meth:`_read_proto_resolve` returns, an :class:`~ipaddress.IPv4Address`/:class:`~ipaddress.IPv6Address` is - packed, and anything else is returned as given. + packed, and :class:`bytes`/:class:`bytearray` are returned as + :class:`bytes`. Raises: - FieldValueError: If ``addr`` is packed as an IPv4 or IPv6 address - and is a :obj:`bool`, not a valid address, or not of that - version (see + FieldValueError: If ``addr`` is a :obj:`bool`, whatever ``ptype`` + is, or is packed as an IPv4 or IPv6 address and is not a + valid address or not of that version (see :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address`). - ProtocolError: If ``ptype`` is neither IPv4 nor IPv6 and ``addr`` - is a :class:`str` that is not *hex* encoded. + ProtocolError: If the address is not packed as IPv4 or IPv6, and + ``addr`` is either a :class:`str` that is not *hex* encoded or + none of :class:`str`, an IP address, :class:`bytes` and + :class:`bytearray`. Notes: Through :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address` rather than @@ -468,6 +489,15 @@ def _make_proto_resolve(self, addr: 'IPv4Address | IPv6Address | str | bytes', p :exc:`AttributeError`, bool or not. """ + # NOTE: checked before the ``ptype`` dispatch, for the placement reason + # ``_reject_bool`` in ``pcapkit.corekit.fields.ipaddress`` gives: the + # non-IP branches below never reach ``parse_ip_address``, so without it + # a ``bool`` falls through them unchanged and fails later as a + # ``TypeError`` from ``len()`` (#2166). + if isinstance(addr, bool): + raise FieldValueError( + f'{self.__class__.__name__}: invalid protocol address: must not be a bool, ' + f'not {addr!r} -- pass int({addr!r}) if the numeric value is what is wanted') if ptype == Enum_EtherType.Internet_Protocol_version_4 and plen in (None, 4): return cast('IPv4Address', parse_ip_address( addr, f'{self.__class__.__name__}: invalid protocol address', version=4)).packed @@ -483,7 +513,9 @@ def _make_proto_resolve(self, addr: 'IPv4Address | IPv6Address | str | bytes', p f'{addr!r}') from None if isinstance(addr, (ipaddress.IPv4Address, ipaddress.IPv6Address)): return addr.packed - return addr + if isinstance(addr, (bytes, bytearray)): + return bytes(addr) + raise ProtocolError(f'{self.__class__.__name__}: invalid protocol address: {addr!r}') class InARP(ARP, schema=Schema_ARP, data=Data_ARP): diff --git a/tests/protocols/link/test_arp_proto_addr_length_unit.py b/tests/protocols/link/test_arp_proto_addr_length_unit.py index b8971a0e89..ad5d31142d 100644 --- a/tests/protocols/link/test_arp_proto_addr_length_unit.py +++ b/tests/protocols/link/test_arp_proto_addr_length_unit.py @@ -17,7 +17,7 @@ from pcapkit.const.arp.hardware import Hardware from pcapkit.const.reg.ethertype import EtherType from pcapkit.protocols.link.arp import ARP -from pcapkit.utilities.exceptions import ProtocolError +from pcapkit.utilities.exceptions import FieldValueError, ProtocolError #: A protocol type that is neither IPv4 nor IPv6. NON_IP = EtherType.get(0x88B5) @@ -52,6 +52,84 @@ def test_from_data_round_trips(self) -> None: self.assertEqual(ARP.from_data(parsed.info).data, data) +class TestARPNonIPAddressType(unittest.TestCase): + """A non-IP ``ptype`` checks the address type too. See #2166. + + Only the IPv4 and IPv6 branches went through ``parse_ip_address``, so any + other ``ptype`` returned a :obj:`bool` or :class:`int` unchanged, and + :meth:`~pcapkit.protocols.link.arp.ARP.make` then failed with ``TypeError: + object of type 'bool' has no len()``. + """ + + def test_bool_raises_field_value_error(self) -> None: + cases = { + 'non-IP': dict(ptype=NON_IP), + 'IPv4 with plen 6': dict(ptype=EtherType.Internet_Protocol_version_4, plen=6), + } + for name, kwargs in cases.items(): + for value in (True, False): + with self.subTest(case=name, value=value): + with self.assertRaises(FieldValueError) as context: + ARP(spa=value, tpa=value, **kwargs) + self.assertIn('must not be a bool', str(context.exception)) + + def test_non_bytes_raises_protocol_error(self) -> None: + for value in (5, 1.5, None, ['a1', 'b2', 'c3']): + with self.subTest(value=value): + with self.assertRaises(ProtocolError): + ARP(ptype=NON_IP, spa=value, tpa=value) + + def test_bytes_and_bytearray_pack_as_given(self) -> None: + proto = ARP(ptype=NON_IP, spa=b'\xa1\xb2\xc3', tpa=bytearray(b'\xd4\xe5\xf6'), + sha='01:23:45:67:89:ab', tha='fe:dc:ba:98:76:54') + self.assertEqual(proto.data, ARP_NON_IP) + + +class TestARPHardwareAddressType(unittest.TestCase): + """The hardware address resolver checks the address type too. See #2166. + + A :obj:`bool` or :class:`int` ``sha``/``tha`` reached the MAC pattern + (``TypeError: expected string or bytes-like object``) for Ethernet, and fell + through unchanged (``TypeError: object of type 'bool' has no len()``) for + any other ``htype``. + """ + + HTYPES = {'Ethernet': Hardware.Ethernet, 'non-Ethernet': Hardware.Fibre_Channel} + + def test_bool_raises_field_value_error(self) -> None: + for name, htype in self.HTYPES.items(): + for value in (True, False): + with self.subTest(htype=name, value=value): + with self.assertRaises(FieldValueError) as context: + ARP(htype=htype, sha=value, tha=value) + self.assertIn('must not be a bool', str(context.exception)) + + def test_unsupported_type_raises_protocol_error(self) -> None: + for name, htype in self.HTYPES.items(): + for value in (5, None, [1, 2, 3, 4, 5, 6], memoryview(b'\x01' * 6)): + with self.subTest(htype=name, value=value): + with self.assertRaises(ProtocolError): + ARP(htype=htype, sha=value, tha=value) + + def test_supported_forms_still_pack(self) -> None: + raw = bytes.fromhex('0123456789ab') + cases = ( + (Hardware.Ethernet, '01:23:45:67:89:ab'), + (Hardware.Ethernet, '01-23-45-67-89-ab'), + (Hardware.Ethernet, b'01:23:45:67:89:ab'), + (Hardware.Ethernet, bytearray(b'01:23:45:67:89:ab')), + (Hardware.Fibre_Channel, '0123456789ab'), + (Hardware.Fibre_Channel, '01:23:45:67:89:ab'), + (Hardware.Fibre_Channel, raw), + (Hardware.Fibre_Channel, bytearray(raw)), + ) + for htype, value in cases: + with self.subTest(htype=htype.name, value=value): + proto = ARP(htype=htype, sha=value, tha=value) + self.assertEqual(proto.data[8:14], raw) + self.assertEqual(proto.data[18:24], raw) + + class TestARPLengthFields(unittest.TestCase): """``hlen``/``plen`` are derived when omitted and checked when given."""