diff --git a/docs/source/pcapkit/protocols/misc/pcap.rst b/docs/source/pcapkit/protocols/misc/pcap.rst index 2d5aafa28..e4c0a82d3 100644 --- a/docs/source/pcapkit/protocols/misc/pcap.rst +++ b/docs/source/pcapkit/protocols/misc/pcap.rst @@ -131,6 +131,7 @@ whose structure is described as below: .. automethod:: index .. automethod:: unpack + .. automethod:: pack .. automethod:: read .. automethod:: make diff --git a/docs/source/pcapkit/protocols/protocol.rst b/docs/source/pcapkit/protocols/protocol.rst index caba2553b..5c36e31de 100644 --- a/docs/source/pcapkit/protocols/protocol.rst +++ b/docs/source/pcapkit/protocols/protocol.rst @@ -78,6 +78,7 @@ Internal Definitions below. .. automethod:: _make_data .. automethod:: _make_index .. automethod:: _make_payload + .. automethod:: _packing_target .. automethod:: _lookup_registry .. automethod:: _lookup_next_layer diff --git a/pcapkit/protocols/application/httpv2.py b/pcapkit/protocols/application/httpv2.py index beee8b6ac..f8aaafc4c 100644 --- a/pcapkit/protocols/application/httpv2.py +++ b/pcapkit/protocols/application/httpv2.py @@ -412,13 +412,17 @@ def pack(self, **kwargs: 'Any') -> 'bytes': Notes: :meth:`make` takes the frame payload as raw octets or as a built frame schema as well, and neither carries what the frame readers - need, e.g. ``__flags__``. So the header :meth:`make` returns is - dropped once packed, and :meth:`unpack` parses the packed octets - back with the frame type's own schema. + need, e.g. ``__flags__``. So while the instance is being + constructed, the header :meth:`make` returns is dropped once packed, + and :meth:`unpack` parses the packed octets back with the frame + type's own schema. Any other call leaves the instance's header as it + was (:issue:`1769`). """ data = super().pack(**kwargs) - self.__header__ = None # type: ignore[assignment] + # NOTE: Constructing, by the test ``ProtocolBase._packing_target`` makes. + if '_info' not in vars(self): + self.__header__ = None # type: ignore[assignment] return data @staticmethod diff --git a/pcapkit/protocols/misc/pcap/frame.py b/pcapkit/protocols/misc/pcap/frame.py index 95bc5d5f3..98b05103c 100644 --- a/pcapkit/protocols/misc/pcap/frame.py +++ b/pcapkit/protocols/misc/pcap/frame.py @@ -205,11 +205,17 @@ def pack(self, **kwargs: 'Any') -> 'bytes': global packet data to underlying methods. This is useful when the packet data is not available in the current instance. + As in :meth:`Protocol.pack `, + the header is made on a copy outside construction, so the instance + keeps its own (:issue:`1769`). + """ - self.__header__ = self.make(**kwargs) + # pylint: disable=protected-access + target = self._packing_target() + target.__header__ = target.make(**kwargs) packet = kwargs.get('__packet__', {}) # packet data - packet['byteorder'] = self._ghdr.magic_number.byteorder - return self.__header__.pack(packet) + packet['byteorder'] = target._ghdr.magic_number.byteorder + return target.__header__.pack(packet) def unpack(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_Frame': r"""Unpack (parse) packet data. diff --git a/pcapkit/protocols/misc/pcapng.py b/pcapkit/protocols/misc/pcapng.py index 2fa85936f..64eb932fb 100644 --- a/pcapkit/protocols/misc/pcapng.py +++ b/pcapkit/protocols/misc/pcapng.py @@ -1190,18 +1190,29 @@ def pack(self, **kwargs: 'Any') -> 'bytes': global packet data to underlying methods. This is useful when the packet data is not available in the current instance. - """ - self.__header__ = self.make(**kwargs) + As in :meth:`Protocol.pack `, + the header is made on a copy outside construction, so the instance + keeps its own header, block type, byte order and option counts + (:issue:`1769`). + + """ + # pylint: disable=protected-access + target = self._packing_target() + if target is not self: + # NOTE: The copy would share the instance's counter, which + # :meth:`_make_pcapng_options` increments in place. + target._opt = collections.Counter() + target.__header__ = target.make(**kwargs) packet = kwargs.get('__packet__', {}) # packet data - if self._ctx is not None: - self._byte = self._ctx.section.byteorder - packet['byteorder'] = self._byte - elif self._type == Enum_BlockType.Section_Header_Block: + if target._ctx is not None: + target._byte = target._ctx.section.byteorder + packet['byteorder'] = target._byte + elif target._type == Enum_BlockType.Section_Header_Block: # NOTE: A section header starts its own section, so the byte order # is the one :meth:`_make_block_shb` chose rather than the host's. - packet.setdefault('byteorder', self._byte) - return self.__header__.pack(packet) + packet.setdefault('byteorder', target._byte) + return target.__header__.pack(packet) def unpack(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_PCAPNG': """Unpack (parse) packet data. diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index a5a92c9d4..33a50eda1 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -26,6 +26,7 @@ import collections.abc import contextlib import contextvars +import copy import difflib import enum import functools @@ -485,6 +486,8 @@ class ProtocolBase(Generic[_PT, _ST], metaclass=ProtocolMeta): _next: 'ProtocolBase' #: Protocol chain instance. _protos: 'ProtoChain' + #: If a header set before :meth:`unpack` may hold raw items. + _make_raw: 'bool' # Internal data storage for cached properties. __cached__: 'dict[str, Any]' @@ -772,17 +775,56 @@ def pack(self, **kwargs: 'Any') -> 'bytes': The octets are built from ``kwargs`` alone, through :meth:`make`; the instance's own parsed or constructed state is not read. A bare ``SomeProtocol(...).pack()`` therefore returns the octets of a - default header, not the instance's: :attr:`data` holds those. The - schema made here also replaces the instance's - :attr:`__header__ `. + default header, not the instance's: :attr:`data` holds those. + + Nor is that state written. Once the instance has been parsed or + constructed, the header is made on a shallow copy of it (see + :meth:`_packing_target`), so :attr:`schema`, :attr:`info` and + anything :meth:`make` sets along the way, such as + :attr:`TCP.connection `, + still describe that packet. Only the call that + :meth:`__post_init__` makes to construct the instance keeps the + header made here, as the one :meth:`unpack` reads back. + + .. versionchanged:: 1.5.0 + + Calling it no longer replaces the instance's + :attr:`__header__ `, which left :attr:`schema` + disagreeing with :attr:`info` (:issue:`1769`). """ - self.__header__ = self.make(**kwargs) + target = self._packing_target() + header = target.__header__ = target.make(**kwargs) #: bool: If the made header may hold raw items (see :meth:`unpack`). - self._make_raw = _passes_raw(kwargs) or any( - isinstance(self.__header__.get(name), bytes) for name in _schema_slots(type(self.__header__))) + target._make_raw = _passes_raw(kwargs) or any( # pylint: disable=protected-access + isinstance(header.get(name), bytes) for name in _schema_slots(type(header))) packet = kwargs.get('__packet__', {}) # packet data - return self.__header__.pack(packet) + return header.pack(packet) + + def _packing_target(self) -> 'Self': + """Return the instance that :meth:`pack` makes its header on. + + Returns: + The instance itself while it is being constructed, i.e. before + :meth:`__post_init__` has set its :attr:`info`, since its + :meth:`unpack` reads the made header back; a shallow copy of it + otherwise, so that neither that header nor an attribute that + :meth:`make` sets is left on the instance (:issue:`1769`). + + Note: + A :meth:`pack` override that does not call this one has to build + on this target too, as :meth:`Frame.pack + ` and + :meth:`PCAPNG.pack ` do. + The copy is shallow, so a :meth:`make` that mutates a container + in place still reaches the instance's. + + """ + # NOTE: ``_info`` is the last thing ``__post_init__`` sets, from + # ``unpack``; until then the instance holds no packet to keep. + if '_info' not in vars(self): + return self + return copy.copy(self) def unpack(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> '_PT': """Unpack (parse) packet data. diff --git a/tests/protocols/test_pack_keeps_instance_state_1769_unit.py b/tests/protocols/test_pack_keeps_instance_state_1769_unit.py new file mode 100644 index 000000000..4943d7672 --- /dev/null +++ b/tests/protocols/test_pack_keeps_instance_state_1769_unit.py @@ -0,0 +1,169 @@ +# -*- coding: utf-8 -*- +"""Calling ``pack()`` leaves the instance it is called on as it was. C.f. #1769. + +:meth:`ProtocolBase.pack ` builds +a fresh header from its keywords alone. It used to assign that header to the +instance as well, so a bare ``UDP(srcport=1234, dstport=53).pack()`` left +``schema.srcport`` at ``0`` while ``info.srcport`` was still ``1234``, and +:meth:`TCP.make ` overwrote +:attr:`~pcapkit.protocols.transport.tcp.TCP.connection` on the way. The +``pack`` overrides of ``Frame`` and ``PCAPNG`` did the same, and HTTP/2's +dropped the header altogether. + +Every value is built here, so the module is in the unit tier. + +""" +from __future__ import annotations + +import importlib +import importlib.util +import unittest +import warnings +from typing import TYPE_CHECKING + +from tests._support import reimport_once_per_class + +if TYPE_CHECKING: + from typing import Any + + from pcapkit.protocols.protocol import ProtocolBase + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + + +class _KeepsState(unittest.TestCase): + """Shared assertions.""" + + def setUp(self) -> 'None': + reimport_once_per_class(self) + + def assertPackKeepsState(self, proto: 'ProtocolBase', **kwargs: 'Any') -> 'bytes': # noqa: N802 + """Call ``proto.pack(**kwargs)`` and check nothing on ``proto`` moved. + + ``schema`` is a cached property, so it is read only *after* the call: + read before, it would hide a replaced header behind its cache. + + Returns: + What ``pack`` returned. + + """ + header, info, data = proto.__header__, proto.info, proto.data + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + packed = proto.pack(**kwargs) + self.assertIs(proto.__header__, header) + self.assertIs(proto.schema, header) + self.assertIs(proto.info, info) + self.assertEqual(proto.data, data) + return packed + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class TestLayers(_KeepsState): + """A link, an internet and two transport protocols, constructed and parsed.""" + + CASES = ( + ('pcapkit.protocols.link.ethernet', 'Ethernet', + {'src': 'aa:bb:cc:dd:ee:ff', 'dst': '11:22:33:44:55:66'}), + ('pcapkit.protocols.internet.ipv4', 'IPv4', {'src': '1.2.3.4', 'dst': '5.6.7.8', 'ttl': 7}), + ('pcapkit.protocols.transport.udp', 'UDP', {'srcport': 1234, 'dstport': 53}), + ('pcapkit.protocols.transport.tcp', 'TCP', {'srcport': 80, 'dstport': 443, 'syn': True, 'ack': True}), + ) + + def test_bare_pack_returns_a_default_header(self) -> 'None': + for module, name, kwargs in self.CASES: + with self.subTest(protocol=name): + cls = getattr(importlib.import_module(module), name) + proto = cls(**kwargs) + self.assertEqual(self.assertPackKeepsState(proto), cls().data) + # NOTE: the schema still describes the octets the instance holds. + self.assertEqual(bytes(proto.schema), proto.data) + + def test_pack_with_keywords_keeps_state(self) -> 'None': + for module, name, kwargs in self.CASES: + with self.subTest(protocol=name): + cls = getattr(importlib.import_module(module), name) + proto = cls() + self.assertEqual(self.assertPackKeepsState(proto, **kwargs), cls(**kwargs).data) + self.assertEqual(bytes(proto.schema), cls().data) + + def test_parsed_instance_keeps_state(self) -> 'None': + for module, name, kwargs in self.CASES: + with self.subTest(protocol=name): + cls = getattr(importlib.import_module(module), name) + octets = cls(**kwargs).data + proto = cls(octets, len(octets)) + self.assertEqual(self.assertPackKeepsState(proto), cls().data) + self.assertEqual(bytes(proto.schema), octets) + + def test_issue_example(self) -> 'None': + from pcapkit.protocols.transport.udp import UDP + + udp = UDP(srcport=1234, dstport=53) + self.assertEqual(udp.data, bytes.fromhex('04d2003500080000')) + self.assertEqual(self.assertPackKeepsState(udp), bytes.fromhex('0000000000080000')) + self.assertEqual(udp.schema.srcport, 1234) + self.assertEqual(udp.info.srcport, 1234) + + def test_tcp_connection_flags(self) -> 'None': + from pcapkit.const.tcp.flags import Flags + from pcapkit.protocols.transport.tcp import TCP + + tcp = TCP(srcport=80, dstport=443, syn=True, ack=True) + self.assertEqual(tcp.connection, Flags.SYN | Flags.ACK) + self.assertPackKeepsState(tcp) + self.assertEqual(tcp.connection, Flags.SYN | Flags.ACK) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class TestOverrides(_KeepsState): + """The ``pack`` overrides that built their own header.""" + + def test_frame(self) -> 'None': + from pcapkit.protocols.misc.pcap.frame import Frame + from pcapkit.protocols.misc.pcap.header import Header + + header = Header().info + frame = Frame(num=1, header=header, ts_sec=1, ts_usec=2, packet=b'abcd') + packed = self.assertPackKeepsState(frame, num=1, header=header, ts_sec=3, ts_usec=4, packet=b'xy') + self.assertEqual(packed, Frame(num=1, header=header, ts_sec=3, ts_usec=4, packet=b'xy').data) + self.assertEqual(frame.schema.ts_sec, 1) + + def test_pcapng(self) -> 'None': + from pcapkit.const.pcapng.block_type import BlockType + from pcapkit.const.pcapng.option_type import OptionType + from pcapkit.const.reg.linktype import LinkType + from pcapkit.foundation.engines.pcapng import Context + from pcapkit.protocols.misc.pcapng import PCAPNG + + section = PCAPNG(num=0, sct=1, ctx=None, type=BlockType.Section_Header_Block, block={'byteorder': 'big'}) + self.assertEqual(section.byteorder, 'big') + self.assertPackKeepsState(section, num=0, sct=1, ctx=None, + type=BlockType.Section_Header_Block, block={'byteorder': 'little'}) + self.assertEqual(section.byteorder, 'big') + + context = Context(section.info) + kwargs = {'num': 1, 'sct': 1, 'ctx': context, 'type': BlockType.Interface_Description_Block, + 'block': {'linktype': LinkType.ETHERNET, 'snaplen': 0x40000, + 'options': [(OptionType.if_name, {'name': 'eth0'})]}} + interface = PCAPNG(**kwargs) + counts = dict(interface._opt) # pylint: disable=protected-access + self.assertEqual(self.assertPackKeepsState(interface, **kwargs), interface.data) + self.assertEqual(dict(interface._opt), counts) # pylint: disable=protected-access + # NOTE: the bare make defaults to another block type. + packed = self.assertPackKeepsState(interface, num=1, sct=1, ctx=context) + self.assertNotEqual(packed, interface.data) + self.assertEqual(interface.block, BlockType.Interface_Description_Block) + + def test_httpv2(self) -> 'None': + from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 + + kwargs = {'type': 0x00, 'sid': 1, 'frame': {'data': b'abc'}} + frame = HTTPv2(**kwargs) + self.assertIsNotNone(frame.__header__) + self.assertEqual(self.assertPackKeepsState(frame, **kwargs), frame.data) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py b/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py index 762425807..2990cbbe3 100644 --- a/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py @@ -565,14 +565,16 @@ def test_make_re_derives_the_flags_on_every_call(self) -> None: parsed = self._parsed_join_syn() self.assertEqual(parsed._flags, Enum_Flags.SYN) # type: ignore[attr-defined] # pylint: disable=protected-access - parsed.pack(syn=False, ack=True, **TCP_HEADER) # type: ignore[attr-defined] + # NOTE: Through ``make`` itself. ``pack`` makes its header on a copy, so it + # leaves the parsed ``_flags`` on the instance (#1769). + parsed.make(syn=False, ack=True, **TCP_HEADER) # type: ignore[attr-defined] self.assertEqual(parsed._flags, Enum_Flags.ACK) # type: ignore[attr-defined] # pylint: disable=protected-access self.assertNotIn(Enum_Flags.SYN, parsed._flags) # type: ignore[attr-defined] # pylint: disable=protected-access # And again, to a third combination, so the assertion above cannot be satisfied # by a one-off rather than by a per-call derivation. - parsed.pack(syn=True, ack=True, **TCP_HEADER) # type: ignore[attr-defined] + parsed.make(syn=True, ack=True, **TCP_HEADER) # type: ignore[attr-defined] self.assertEqual(parsed._flags, Enum_Flags.SYN | Enum_Flags.ACK) # type: ignore[attr-defined] # pylint: disable=protected-access