fix(protocols)!: keep pack() from rewriting the instance it is called on (#1769)

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

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

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

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 <pcapkit.protocols.protocol.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.

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 <pcapkit.protocols.protocol.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.

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__ <Protocol.__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 <pcapkit.protocols.transport.tcp.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__ <Protocol.__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
+            <pcapkit.protocols.misc.pcap.frame.Frame.pack>` and
+            :meth:`PCAPNG.pack <pcapkit.protocols.misc.pcapng.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.

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 <pcapkit.protocols.protocol.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 <pcapkit.protocols.transport.tcp.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()

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