Skip to content

Commit 36b82bb

Browse files
authored
fix(protocols)!: keep pack() from rewriting the instance it is called on (#1769) (#1791)
- protocol.py: pack() assigned the header it made to the instance, so UDP(srcport=1234).pack() left schema.srcport at 0 while info kept 1234. Once the instance has been parsed or constructed, the new _packing_target() hands pack() a shallow copy, so neither the header nor what make() sets on the way (TCP's connection flags) reaches it. The construction call in __post_init__ still keeps its header for unpack(). - frame.py, pcapng.py: their pack overrides make on the same target; PCAPNG's copy gets its own option counter, so re-packing a block with an only-one option no longer raises. - httpv2.py: the header is dropped only while constructing. - Docs: pack() states the contract with a versionchanged; _packing_target and Frame.pack are documented. - Tests: new unit module over Ethernet, IPv4, UDP, TCP, Frame, PCAPNG and HTTP/2; the MPTCP stale-flags case now calls make() directly. Closes #1769
1 parent 23eaa8d commit 36b82bb

8 files changed

Lines changed: 260 additions & 24 deletions

File tree

‎docs/source/pcapkit/protocols/misc/pcap.rst‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,7 @@ whose structure is described as below:
131131
.. automethod:: index
132132

133133
.. automethod:: unpack
134+
.. automethod:: pack
134135

135136
.. automethod:: read
136137
.. automethod:: make

‎docs/source/pcapkit/protocols/protocol.rst‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ Internal Definitions below.
7878
.. automethod:: _make_data
7979
.. automethod:: _make_index
8080
.. automethod:: _make_payload
81+
.. automethod:: _packing_target
8182

8283
.. automethod:: _lookup_registry
8384
.. automethod:: _lookup_next_layer

‎pcapkit/protocols/application/httpv2.py‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -412,13 +412,17 @@ def pack(self, **kwargs: 'Any') -> 'bytes':
412412
Notes:
413413
:meth:`make` takes the frame payload as raw octets or as a built
414414
frame schema as well, and neither carries what the frame readers
415-
need, e.g. ``__flags__``. So the header :meth:`make` returns is
416-
dropped once packed, and :meth:`unpack` parses the packed octets
417-
back with the frame type's own schema.
415+
need, e.g. ``__flags__``. So while the instance is being
416+
constructed, the header :meth:`make` returns is dropped once packed,
417+
and :meth:`unpack` parses the packed octets back with the frame
418+
type's own schema. Any other call leaves the instance's header as it
419+
was (:issue:`1769`).
418420
419421
"""
420422
data = super().pack(**kwargs)
421-
self.__header__ = None # type: ignore[assignment]
423+
# NOTE: Constructing, by the test ``ProtocolBase._packing_target`` makes.
424+
if '_info' not in vars(self):
425+
self.__header__ = None # type: ignore[assignment]
422426
return data
423427

424428
@staticmethod

‎pcapkit/protocols/misc/pcap/frame.py‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -205,11 +205,17 @@ def pack(self, **kwargs: 'Any') -> 'bytes':
205205
global packet data to underlying methods. This is useful when
206206
the packet data is not available in the current instance.
207207
208+
As in :meth:`Protocol.pack <pcapkit.protocols.protocol.Protocol.pack>`,
209+
the header is made on a copy outside construction, so the instance
210+
keeps its own (:issue:`1769`).
211+
208212
"""
209-
self.__header__ = self.make(**kwargs)
213+
# pylint: disable=protected-access
214+
target = self._packing_target()
215+
target.__header__ = target.make(**kwargs)
210216
packet = kwargs.get('__packet__', {}) # packet data
211-
packet['byteorder'] = self._ghdr.magic_number.byteorder
212-
return self.__header__.pack(packet)
217+
packet['byteorder'] = target._ghdr.magic_number.byteorder
218+
return target.__header__.pack(packet)
213219

214220
def unpack(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_Frame':
215221
r"""Unpack (parse) packet data.

‎pcapkit/protocols/misc/pcapng.py‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1190,18 +1190,29 @@ def pack(self, **kwargs: 'Any') -> 'bytes':
11901190
global packet data to underlying methods. This is useful when
11911191
the packet data is not available in the current instance.
11921192
1193-
"""
1194-
self.__header__ = self.make(**kwargs)
1193+
As in :meth:`Protocol.pack <pcapkit.protocols.protocol.Protocol.pack>`,
1194+
the header is made on a copy outside construction, so the instance
1195+
keeps its own header, block type, byte order and option counts
1196+
(:issue:`1769`).
1197+
1198+
"""
1199+
# pylint: disable=protected-access
1200+
target = self._packing_target()
1201+
if target is not self:
1202+
# NOTE: The copy would share the instance's counter, which
1203+
# :meth:`_make_pcapng_options` increments in place.
1204+
target._opt = collections.Counter()
1205+
target.__header__ = target.make(**kwargs)
11951206
packet = kwargs.get('__packet__', {}) # packet data
11961207

1197-
if self._ctx is not None:
1198-
self._byte = self._ctx.section.byteorder
1199-
packet['byteorder'] = self._byte
1200-
elif self._type == Enum_BlockType.Section_Header_Block:
1208+
if target._ctx is not None:
1209+
target._byte = target._ctx.section.byteorder
1210+
packet['byteorder'] = target._byte
1211+
elif target._type == Enum_BlockType.Section_Header_Block:
12011212
# NOTE: A section header starts its own section, so the byte order
12021213
# is the one :meth:`_make_block_shb` chose rather than the host's.
1203-
packet.setdefault('byteorder', self._byte)
1204-
return self.__header__.pack(packet)
1214+
packet.setdefault('byteorder', target._byte)
1215+
return target.__header__.pack(packet)
12051216

12061217
def unpack(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_PCAPNG':
12071218
"""Unpack (parse) packet data.

‎pcapkit/protocols/protocol.py‎

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import collections.abc
2727
import contextlib
2828
import contextvars
29+
import copy
2930
import difflib
3031
import enum
3132
import functools
@@ -485,6 +486,8 @@ class ProtocolBase(Generic[_PT, _ST], metaclass=ProtocolMeta):
485486
_next: 'ProtocolBase'
486487
#: Protocol chain instance.
487488
_protos: 'ProtoChain'
489+
#: If a header set before :meth:`unpack` may hold raw items.
490+
_make_raw: 'bool'
488491

489492
# Internal data storage for cached properties.
490493
__cached__: 'dict[str, Any]'
@@ -772,17 +775,56 @@ def pack(self, **kwargs: 'Any') -> 'bytes':
772775
The octets are built from ``kwargs`` alone, through :meth:`make`;
773776
the instance's own parsed or constructed state is not read. A bare
774777
``SomeProtocol(...).pack()`` therefore returns the octets of a
775-
default header, not the instance's: :attr:`data` holds those. The
776-
schema made here also replaces the instance's
777-
:attr:`__header__ <Protocol.__header__>`.
778+
default header, not the instance's: :attr:`data` holds those.
779+
780+
Nor is that state written. Once the instance has been parsed or
781+
constructed, the header is made on a shallow copy of it (see
782+
:meth:`_packing_target`), so :attr:`schema`, :attr:`info` and
783+
anything :meth:`make` sets along the way, such as
784+
:attr:`TCP.connection <pcapkit.protocols.transport.tcp.TCP.connection>`,
785+
still describe that packet. Only the call that
786+
:meth:`__post_init__` makes to construct the instance keeps the
787+
header made here, as the one :meth:`unpack` reads back.
788+
789+
.. versionchanged:: 1.5.0
790+
791+
Calling it no longer replaces the instance's
792+
:attr:`__header__ <Protocol.__header__>`, which left :attr:`schema`
793+
disagreeing with :attr:`info` (:issue:`1769`).
778794
779795
"""
780-
self.__header__ = self.make(**kwargs)
796+
target = self._packing_target()
797+
header = target.__header__ = target.make(**kwargs)
781798
#: bool: If the made header may hold raw items (see :meth:`unpack`).
782-
self._make_raw = _passes_raw(kwargs) or any(
783-
isinstance(self.__header__.get(name), bytes) for name in _schema_slots(type(self.__header__)))
799+
target._make_raw = _passes_raw(kwargs) or any( # pylint: disable=protected-access
800+
isinstance(header.get(name), bytes) for name in _schema_slots(type(header)))
784801
packet = kwargs.get('__packet__', {}) # packet data
785-
return self.__header__.pack(packet)
802+
return header.pack(packet)
803+
804+
def _packing_target(self) -> 'Self':
805+
"""Return the instance that :meth:`pack` makes its header on.
806+
807+
Returns:
808+
The instance itself while it is being constructed, i.e. before
809+
:meth:`__post_init__` has set its :attr:`info`, since its
810+
:meth:`unpack` reads the made header back; a shallow copy of it
811+
otherwise, so that neither that header nor an attribute that
812+
:meth:`make` sets is left on the instance (:issue:`1769`).
813+
814+
Note:
815+
A :meth:`pack` override that does not call this one has to build
816+
on this target too, as :meth:`Frame.pack
817+
<pcapkit.protocols.misc.pcap.frame.Frame.pack>` and
818+
:meth:`PCAPNG.pack <pcapkit.protocols.misc.pcapng.PCAPNG.pack>` do.
819+
The copy is shallow, so a :meth:`make` that mutates a container
820+
in place still reaches the instance's.
821+
822+
"""
823+
# NOTE: ``_info`` is the last thing ``__post_init__`` sets, from
824+
# ``unpack``; until then the instance holds no packet to keep.
825+
if '_info' not in vars(self):
826+
return self
827+
return copy.copy(self)
786828

787829
def unpack(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> '_PT':
788830
"""Unpack (parse) packet data.
Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
# -*- coding: utf-8 -*-
2+
"""Calling ``pack()`` leaves the instance it is called on as it was. C.f. #1769.
3+
4+
:meth:`ProtocolBase.pack <pcapkit.protocols.protocol.ProtocolBase.pack>` builds
5+
a fresh header from its keywords alone. It used to assign that header to the
6+
instance as well, so a bare ``UDP(srcport=1234, dstport=53).pack()`` left
7+
``schema.srcport`` at ``0`` while ``info.srcport`` was still ``1234``, and
8+
:meth:`TCP.make <pcapkit.protocols.transport.tcp.TCP.make>` overwrote
9+
:attr:`~pcapkit.protocols.transport.tcp.TCP.connection` on the way. The
10+
``pack`` overrides of ``Frame`` and ``PCAPNG`` did the same, and HTTP/2's
11+
dropped the header altogether.
12+
13+
Every value is built here, so the module is in the unit tier.
14+
15+
"""
16+
from __future__ import annotations
17+
18+
import importlib
19+
import importlib.util
20+
import unittest
21+
import warnings
22+
from typing import TYPE_CHECKING
23+
24+
from tests._support import reimport_once_per_class
25+
26+
if TYPE_CHECKING:
27+
from typing import Any
28+
29+
from pcapkit.protocols.protocol import ProtocolBase
30+
31+
RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper')
32+
HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS)
33+
34+
35+
class _KeepsState(unittest.TestCase):
36+
"""Shared assertions."""
37+
38+
def setUp(self) -> 'None':
39+
reimport_once_per_class(self)
40+
41+
def assertPackKeepsState(self, proto: 'ProtocolBase', **kwargs: 'Any') -> 'bytes': # noqa: N802
42+
"""Call ``proto.pack(**kwargs)`` and check nothing on ``proto`` moved.
43+
44+
``schema`` is a cached property, so it is read only *after* the call:
45+
read before, it would hide a replaced header behind its cache.
46+
47+
Returns:
48+
What ``pack`` returned.
49+
50+
"""
51+
header, info, data = proto.__header__, proto.info, proto.data
52+
with warnings.catch_warnings():
53+
warnings.simplefilter('ignore')
54+
packed = proto.pack(**kwargs)
55+
self.assertIs(proto.__header__, header)
56+
self.assertIs(proto.schema, header)
57+
self.assertIs(proto.info, info)
58+
self.assertEqual(proto.data, data)
59+
return packed
60+
61+
62+
@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
63+
class TestLayers(_KeepsState):
64+
"""A link, an internet and two transport protocols, constructed and parsed."""
65+
66+
CASES = (
67+
('pcapkit.protocols.link.ethernet', 'Ethernet',
68+
{'src': 'aa:bb:cc:dd:ee:ff', 'dst': '11:22:33:44:55:66'}),
69+
('pcapkit.protocols.internet.ipv4', 'IPv4', {'src': '1.2.3.4', 'dst': '5.6.7.8', 'ttl': 7}),
70+
('pcapkit.protocols.transport.udp', 'UDP', {'srcport': 1234, 'dstport': 53}),
71+
('pcapkit.protocols.transport.tcp', 'TCP', {'srcport': 80, 'dstport': 443, 'syn': True, 'ack': True}),
72+
)
73+
74+
def test_bare_pack_returns_a_default_header(self) -> 'None':
75+
for module, name, kwargs in self.CASES:
76+
with self.subTest(protocol=name):
77+
cls = getattr(importlib.import_module(module), name)
78+
proto = cls(**kwargs)
79+
self.assertEqual(self.assertPackKeepsState(proto), cls().data)
80+
# NOTE: the schema still describes the octets the instance holds.
81+
self.assertEqual(bytes(proto.schema), proto.data)
82+
83+
def test_pack_with_keywords_keeps_state(self) -> 'None':
84+
for module, name, kwargs in self.CASES:
85+
with self.subTest(protocol=name):
86+
cls = getattr(importlib.import_module(module), name)
87+
proto = cls()
88+
self.assertEqual(self.assertPackKeepsState(proto, **kwargs), cls(**kwargs).data)
89+
self.assertEqual(bytes(proto.schema), cls().data)
90+
91+
def test_parsed_instance_keeps_state(self) -> 'None':
92+
for module, name, kwargs in self.CASES:
93+
with self.subTest(protocol=name):
94+
cls = getattr(importlib.import_module(module), name)
95+
octets = cls(**kwargs).data
96+
proto = cls(octets, len(octets))
97+
self.assertEqual(self.assertPackKeepsState(proto), cls().data)
98+
self.assertEqual(bytes(proto.schema), octets)
99+
100+
def test_issue_example(self) -> 'None':
101+
from pcapkit.protocols.transport.udp import UDP
102+
103+
udp = UDP(srcport=1234, dstport=53)
104+
self.assertEqual(udp.data, bytes.fromhex('04d2003500080000'))
105+
self.assertEqual(self.assertPackKeepsState(udp), bytes.fromhex('0000000000080000'))
106+
self.assertEqual(udp.schema.srcport, 1234)
107+
self.assertEqual(udp.info.srcport, 1234)
108+
109+
def test_tcp_connection_flags(self) -> 'None':
110+
from pcapkit.const.tcp.flags import Flags
111+
from pcapkit.protocols.transport.tcp import TCP
112+
113+
tcp = TCP(srcport=80, dstport=443, syn=True, ack=True)
114+
self.assertEqual(tcp.connection, Flags.SYN | Flags.ACK)
115+
self.assertPackKeepsState(tcp)
116+
self.assertEqual(tcp.connection, Flags.SYN | Flags.ACK)
117+
118+
119+
@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
120+
class TestOverrides(_KeepsState):
121+
"""The ``pack`` overrides that built their own header."""
122+
123+
def test_frame(self) -> 'None':
124+
from pcapkit.protocols.misc.pcap.frame import Frame
125+
from pcapkit.protocols.misc.pcap.header import Header
126+
127+
header = Header().info
128+
frame = Frame(num=1, header=header, ts_sec=1, ts_usec=2, packet=b'abcd')
129+
packed = self.assertPackKeepsState(frame, num=1, header=header, ts_sec=3, ts_usec=4, packet=b'xy')
130+
self.assertEqual(packed, Frame(num=1, header=header, ts_sec=3, ts_usec=4, packet=b'xy').data)
131+
self.assertEqual(frame.schema.ts_sec, 1)
132+
133+
def test_pcapng(self) -> 'None':
134+
from pcapkit.const.pcapng.block_type import BlockType
135+
from pcapkit.const.pcapng.option_type import OptionType
136+
from pcapkit.const.reg.linktype import LinkType
137+
from pcapkit.foundation.engines.pcapng import Context
138+
from pcapkit.protocols.misc.pcapng import PCAPNG
139+
140+
section = PCAPNG(num=0, sct=1, ctx=None, type=BlockType.Section_Header_Block, block={'byteorder': 'big'})
141+
self.assertEqual(section.byteorder, 'big')
142+
self.assertPackKeepsState(section, num=0, sct=1, ctx=None,
143+
type=BlockType.Section_Header_Block, block={'byteorder': 'little'})
144+
self.assertEqual(section.byteorder, 'big')
145+
146+
context = Context(section.info)
147+
kwargs = {'num': 1, 'sct': 1, 'ctx': context, 'type': BlockType.Interface_Description_Block,
148+
'block': {'linktype': LinkType.ETHERNET, 'snaplen': 0x40000,
149+
'options': [(OptionType.if_name, {'name': 'eth0'})]}}
150+
interface = PCAPNG(**kwargs)
151+
counts = dict(interface._opt) # pylint: disable=protected-access
152+
self.assertEqual(self.assertPackKeepsState(interface, **kwargs), interface.data)
153+
self.assertEqual(dict(interface._opt), counts) # pylint: disable=protected-access
154+
# NOTE: the bare make defaults to another block type.
155+
packed = self.assertPackKeepsState(interface, num=1, sct=1, ctx=context)
156+
self.assertNotEqual(packed, interface.data)
157+
self.assertEqual(interface.block, BlockType.Interface_Description_Block)
158+
159+
def test_httpv2(self) -> 'None':
160+
from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2
161+
162+
kwargs = {'type': 0x00, 'sid': 1, 'frame': {'data': b'abc'}}
163+
frame = HTTPv2(**kwargs)
164+
self.assertIsNotNone(frame.__header__)
165+
self.assertEqual(self.assertPackKeepsState(frame, **kwargs), frame.data)
166+
167+
168+
if __name__ == '__main__':
169+
unittest.main()

‎tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -565,14 +565,16 @@ def test_make_re_derives_the_flags_on_every_call(self) -> None:
565565
parsed = self._parsed_join_syn()
566566
self.assertEqual(parsed._flags, Enum_Flags.SYN) # type: ignore[attr-defined] # pylint: disable=protected-access
567567

568-
parsed.pack(syn=False, ack=True, **TCP_HEADER) # type: ignore[attr-defined]
568+
# NOTE: Through ``make`` itself. ``pack`` makes its header on a copy, so it
569+
# leaves the parsed ``_flags`` on the instance (#1769).
570+
parsed.make(syn=False, ack=True, **TCP_HEADER) # type: ignore[attr-defined]
569571

570572
self.assertEqual(parsed._flags, Enum_Flags.ACK) # type: ignore[attr-defined] # pylint: disable=protected-access
571573
self.assertNotIn(Enum_Flags.SYN, parsed._flags) # type: ignore[attr-defined] # pylint: disable=protected-access
572574

573575
# And again, to a third combination, so the assertion above cannot be satisfied
574576
# by a one-off rather than by a per-call derivation.
575-
parsed.pack(syn=True, ack=True, **TCP_HEADER) # type: ignore[attr-defined]
577+
parsed.make(syn=True, ack=True, **TCP_HEADER) # type: ignore[attr-defined]
576578

577579
self.assertEqual(parsed._flags, Enum_Flags.SYN | Enum_Flags.ACK) # type: ignore[attr-defined] # pylint: disable=protected-access
578580

0 commit comments

Comments
 (0)