Skip to content

Commit 5c38fc0

Browse files
committed
fix(reassembly): the deferred TCP analyser dispatches on AppType, not bare ports
Follow-up to #2418, which made `Deferred` generic over what its analyser dispatches on but left the TCP side of that constraint as `tuple[int, int]`. The owner's note on it: we are still using int tuples for transport ports, so move to AppType tuples. * `TCP.submit` resolves `bufid[1]` / `bufid[3]` through `Transport._make_port` before building the `Deferred`, in a new `TCP._make_ports` helper. A deferred analysis now dispatches on a named service rather than on a number. * `_KT`'s second constraint is `tuple[AppType_TCP, AppType_TCP]` -- the per-transport registry `pcapkit.const.reg.apptype.tcp.TCP`, which is what the resolution really answers with (`type(...) is TCP`, measured), not the base `AppType`. `Transport.analyze` keeps the base, being the one method TCP, UDP and SCTP share, and says so. * `Transport.analyze` tests the chosen port against `__proto__` exactly as it arrived and unwraps it to the registry's `int` key only when it is a member, so a bare `int` caller keeps working -- the method is in v1.4.1 -- and `(None, None)` or `(54321, '80')` still fall back to `Raw` as on mainline rather than raising or dispatching. * `_make_ports` reaches `_make_port` through `TCP` rather than through `self.protocol`: `__protocol_type__` is an extension point whose analyser need only define `analyze`, so `self.protocol._make_port` would be an `AttributeError` for one that is not a `Transport`. Going through `_make_port` rather than calling `AppType.get` directly is what keeps #2287's bool rejection: `AppType.get(True, proto=tcp)` resolves `True` as port 1 and answers `<TCP.tcpmux: 1 [tcp]>`, measured. * `DatagramID`'s ports stay bare `int`s, and its docstring now says why: it is the flow identity split out of `BufferID`, a dict key every toolkit adapter builds from a plain port number and which is dumped as part of the report. * Docs entries for `TCP._make_ports` and for `AppType.__hash__`, both now named in public prose (#1940). All 65,536 ports resolve to a member -- an assignment to its canonical service, an IANA span to that span's label, anything else to an unregistered `unknown` -- none raise and the registry does not grow, so this adds no way for reassembly to raise. For a registered port the member is the one `TCP.src` reads off a parsed header; for an unassigned one the two are equal but not identical and named apart, which the docstrings record rather than overstate. UDP and SCTP need no change: neither has a reassembler, so `Transport.analyze` has no caller but `TCP.submit`. mypy over `pcapkit/` is byte-identical to mainline (98 errors, same set); `pylint` with the Makefile flags reports the same two pre-existing `_decode_next_layer` messages as mainline over the touched files; `isort -l100 -ppcapkit` is clean. Tests: `foundation` + `protocols/transport` + `interface` with the annotation-name, tier and docstring guards, 997 tests / 4906 subtests; the full CI selection was green on the pre-review revision at 6552 tests. Reassembly over the sample captures resolves the same next layer as before, port for port.
1 parent 867616b commit 5c38fc0

7 files changed

Lines changed: 501 additions & 50 deletions

File tree

‎docs/source/pcapkit/const/reg.rst‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ registry is one enumeration per transport protocol sharing a memberless base.
109109
:members:
110110
:undoc-members:
111111
:show-inheritance:
112-
:special-members: __new__, __repr__, __str__, __int__, __eq__, __registry__, __canonical__
112+
:special-members: __new__, __repr__, __str__, __int__, __eq__, __hash__, __registry__, __canonical__
113113

114114
.. module:: pcapkit.const.reg.apptype.tcp
115115

‎docs/source/pcapkit/foundation/reassembly/tcp.rst‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ which reconstructs fragmented TCP packets back to origin.
1414

1515
.. automethod:: reassembly
1616
.. automethod:: submit
17+
.. automethod:: _make_ports
1718

1819
.. autoattribute:: __protocol_name__
1920
.. autoattribute:: __protocol_type__

‎pcapkit/foundation/reassembly/data/data.py‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,14 @@
1212
if TYPE_CHECKING:
1313
from typing import Callable, Optional
1414

15+
from pcapkit.const.reg.apptype import TCP as AppType_TCP
1516
from pcapkit.const.reg.transtype import TransType
1617
from pcapkit.foundation.reassembly.data.ip import Datagram as IP_Datagram
1718
from pcapkit.foundation.reassembly.data.tcp import Datagram as TCP_Datagram
1819
from pcapkit.protocols.protocol import ProtocolBase
1920

2021
# registry key an analyser dispatches a reassembled payload on; see ``Deferred``
21-
_KT = TypeVar('_KT', 'TransType', 'tuple[int, int]')
22+
_KT = TypeVar('_KT', 'TransType', 'tuple[AppType_TCP, AppType_TCP]')
2223

2324

2425
class Completion(EnumLookup, StrEnum):
@@ -148,23 +149,37 @@ class Deferred(Generic[_KT]):
148149
:meth:`Protocol.analyze <pcapkit.protocols.protocol.Protocol.analyze>`,
149150
which looks the next layer up by protocol registry number.
150151
* :meth:`TCP.submit <pcapkit.foundation.reassembly.tcp.TCP.submit>` builds a
151-
``Deferred[tuple[int, int]]``. It passes a ``(source, destination)`` port
152-
pair, because :meth:`Transport.analyze
152+
``Deferred[tuple[AppType_TCP, AppType_TCP]]``. It passes a
153+
``(source, destination)`` port pair, because :meth:`Transport.analyze
153154
<pcapkit.protocols.transport.transport.Transport.analyze>` overrides that
154155
parameter to one: a reassembled transport-layer payload carries no
155156
protocol number, so its application protocol is inferred from the ports.
156157
157-
Those are the two cases the type parameter is constrained to. The pairing
158-
the regression actually produced -- a port pair handed to an analyser
159-
declared over :class:`~pcapkit.const.reg.transtype.TransType` -- is
160-
rejected, which is the direction a real mismatch takes and the one
158+
Those are the two cases the type parameter is constrained to. Both are
159+
*named registry values* rather than one named value and one pair of bare
160+
port numbers: a port is a member of
161+
:class:`~pcapkit.const.reg.apptype.tcp.TCP` here -- the per-transport
162+
registry itself, not the base
163+
:class:`~pcapkit.const.reg.apptype.apptype.AppType`, since that is what
164+
:meth:`TCP._make_ports <pcapkit.foundation.reassembly.tcp.TCP._make_ports>`
165+
really answers with -- so what the deferred analysis dispatches on is of a
166+
piece with what the eager path dispatches on at
167+
:attr:`TCP.src <pcapkit.protocols.transport.tcp.TCP.src>`, whichever
168+
reassembler built it. (Of a piece, not identical: for an *unassigned* port
169+
the two paths build equal members by different routes, which
170+
``TCP.submit``'s own docstring records.)
171+
172+
The pairing the regression actually produced -- a port pair handed to an
173+
analyser declared over :class:`~pcapkit.const.reg.transtype.TransType` --
174+
is rejected, which is the direction a real mismatch takes and the one
161175
:issue:`2417` was about.
162176
163177
Rejecting the *reverse* pairing is not guaranteed by mypy alone, for a
164178
reason outside this class: |aenum|_ ships no type stubs, so under
165179
``ignore_missing_imports`` the ``aenum.IntEnum`` base of
166180
:class:`~pcapkit.const.reg.transtype.TransType` resolves to ``Any``. A
167-
``TransType`` is therefore accepted where a ``tuple[int, int]`` is
181+
``TransType`` is therefore accepted where a
182+
``tuple[AppType_TCP, AppType_TCP]`` is
168183
expected, and a ``Deferred[TransType]`` built over the port-pair analyser
169184
type-checks clean. That is a gap in the dependency's type coverage rather
170185
than in the constraint, which would bite both ways were ``TransType`` fully

‎pcapkit/foundation/reassembly/data/tcp.py‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
from typing_extensions import TypeAlias
2121

22+
from pcapkit.const.reg.apptype import TCP as AppType_TCP
2223
from pcapkit.protocols.protocol import ProtocolBase
2324

2425
_AT = TypeVar('_AT', 'IPv4Address', 'IPv6Address')
@@ -84,7 +85,26 @@ def __init__(self, bufid: 'BufferID', dsn: 'int', ack: 'int', num: 'int', syn: '
8485

8586
@info_final
8687
class DatagramID(Info, Generic[_AT]):
87-
"""Data model for :term:`TCP <reasm.tcp.datagram>` original packet identifier."""
88+
"""Data model for :term:`TCP <reasm.tcp.datagram>` original packet identifier.
89+
90+
Note:
91+
The ports here stay bare :obj:`int` port numbers, where the pair
92+
:meth:`TCP.submit <pcapkit.foundation.reassembly.tcp.TCP.submit>` hands
93+
:class:`~pcapkit.foundation.reassembly.data.data.Deferred` is one of
94+
:class:`~pcapkit.const.reg.apptype.tcp.TCP` members
95+
(:issue:`2417`). The two are not the same thing to name: this is the
96+
flow's identity, split out of :data:`BufferID`, which every
97+
:mod:`pcapkit.toolkit` adapter builds from a plain port number -- the
98+
native ones by reading :attr:`AppType.port
99+
<pcapkit.const.reg.apptype.apptype.AppType.port>` off the parsed header,
100+
the third-party ones from whatever their own engine exposes -- and which
101+
is then used as a :class:`dict` key and dumped as part of the report.
102+
The deferred pair is a *dispatch* key for
103+
:meth:`Transport.analyze
104+
<pcapkit.protocols.transport.transport.Transport.analyze>`, which is
105+
what gains from carrying the service name.
106+
107+
"""
88108

89109
#: Source address.
90110
src: 'tuple[_AT, int]'
@@ -150,7 +170,7 @@ class Datagram(DeferredPacket, Info, Generic[_AT]):
150170
# :class:`~pcapkit.foundation.reassembly.data.ip.Datagram`, which applies
151171
# here identically: ``strict=False`` reports an incomplete payload buffer as
152172
# one contiguous ``bytes`` and analyses it.
153-
def __init__(self, completed: 'Completion', id: 'DatagramID[_AT]', index: 'tuple[int, ...]', header: 'bytes', payload: 'bytes | tuple[bytes, ...]', packet: 'Optional[ProtocolBase | Deferred[tuple[int, int]]]', conflict: 'tuple[tuple[int, int], ...]') -> 'None': ... # pylint: disable=unused-argument,super-init-not-called,multiple-statements,line-too-long,redefined-builtin
173+
def __init__(self, completed: 'Completion', id: 'DatagramID[_AT]', index: 'tuple[int, ...]', header: 'bytes', payload: 'bytes | tuple[bytes, ...]', packet: 'Optional[ProtocolBase | Deferred[tuple[AppType_TCP, AppType_TCP]]]', conflict: 'tuple[tuple[int, int], ...]') -> 'None': ... # pylint: disable=unused-argument,super-init-not-called,multiple-statements,line-too-long,redefined-builtin
154174

155175

156176
@info_final

‎pcapkit/foundation/reassembly/tcp.py‎

Lines changed: 112 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import math
1313
from typing import TYPE_CHECKING
1414

15+
from pcapkit.const.reg.apptype import TransportProtocol as Enum_TransportProtocol
1516
from pcapkit.foundation.reassembly.data.data import Completion, Deferred
1617
from pcapkit.foundation.reassembly.data.tcp import (HOLE_OPEN_BOUND, Buffer, BufferID, Datagram,
1718
DatagramID, Fragment, HoleDescriptor, Packet)
@@ -21,6 +22,8 @@
2122
if TYPE_CHECKING:
2223
from typing import Type
2324

25+
from pcapkit.const.reg.apptype import TCP as AppType_TCP
26+
2427
__all__ = ['TCP']
2528

2629

@@ -653,6 +656,85 @@ def _merge_overlap(gap: 'list[tuple[int, int]]', old: 'bytes', new: 'bytes',
653656
index = stop
654657
return bytes(merged), conflicts
655658

659+
@staticmethod
660+
def _make_ports(bufid: 'BufferID') -> 'tuple[AppType_TCP, AppType_TCP]':
661+
"""The flow's ``(source, destination)`` ports, as application types.
662+
663+
Arguments:
664+
bufid: :term:`buffer <reasm.tcp.buffer>` identifier, whose positions
665+
``1`` and ``3`` are the source and destination ports.
666+
667+
Returns:
668+
The pair :meth:`submit` hands
669+
:class:`~pcapkit.foundation.reassembly.data.data.Deferred`, i.e. what
670+
:meth:`Transport.analyze
671+
<pcapkit.protocols.transport.transport.Transport.analyze>` dispatches
672+
the reassembled payload on (:issue:`2417`). Typed as the
673+
*per-transport* registry :class:`~pcapkit.const.reg.apptype.tcp.TCP`
674+
rather than as the base
675+
:class:`~pcapkit.const.reg.apptype.apptype.AppType`, because that is
676+
what it really is: ``proto=tcp`` sends
677+
:meth:`AppType.get <pcapkit.const.reg.apptype.apptype.AppType.get>`
678+
through :meth:`AppType._dispatch
679+
<pcapkit.const.reg.apptype.apptype.AppType._dispatch>` to the TCP
680+
registry, and ``type(...) is TCP`` holds of the member it answers
681+
with -- measured, for a registered port and an unassigned one alike.
682+
:meth:`Transport.analyze
683+
<pcapkit.protocols.transport.transport.Transport.analyze>` keeps the
684+
base in its own signature, since it is the one method
685+
:class:`~pcapkit.protocols.transport.udp.UDP` and
686+
:class:`~pcapkit.protocols.transport.sctp.SCTP` share, and each of
687+
them would name a different registry.
688+
689+
Important:
690+
Every port a 16-bit header field can hold resolves to a member, so
691+
this raises for nothing that can come off the wire:
692+
:meth:`AppType.get <pcapkit.const.reg.apptype.apptype.AppType.get>`
693+
answers an assignment with its canonical service, a port inside a
694+
documented IANA span with that span's label (``reserved`` for ``0``),
695+
and one in no span at all with an unregistered ``unknown`` member --
696+
never :obj:`None`, and without growing the registry (:issue:`860`).
697+
It rejects a port outside ``0``-``65535`` and a non-:obj:`int`.
698+
699+
A :obj:`bool` is rejected by :meth:`Transport._make_port
700+
<pcapkit.protocols.transport.transport.Transport._make_port>` rather
701+
than by ``get``, which is the reason to go through it here and not
702+
call ``get`` directly: :obj:`bool` is an :obj:`int` subclass, so
703+
``AppType.get(True, proto=tcp)`` resolves ``True`` as port ``1`` and
704+
answers ``<TCP.tcpmux: 1 [tcp]>`` -- measured -- where ``_make_port``
705+
raises, which is what closed :issue:`2287`. Calling ``get`` straight
706+
would reopen it. None of the three can come out of a TCP header
707+
either way; what ``_make_port`` also buys is idempotence, so a
708+
``bufid`` already holding a member passes through untouched.
709+
710+
Note:
711+
The resolution is reached through
712+
:class:`~pcapkit.protocols.transport.tcp.TCP` rather than through
713+
:attr:`self.protocol
714+
<pcapkit.foundation.reassembly.reassembly.ReassemblyBase.protocol>`,
715+
which is what :meth:`submit` reads the analyser itself off. That is
716+
deliberate: ``__protocol_type__`` is an extension point, and an
717+
analyser plugged into it need only define ``analyze`` -- it is not
718+
required to be a
719+
:class:`~pcapkit.protocols.transport.transport.Transport` subclass
720+
and so need not have ``_make_port`` at all, which reaching it
721+
through ``self.protocol`` would turn into an
722+
:exc:`AttributeError`. What a TCP port *means* belongs to the TCP
723+
protocol either way, not to whatever consumes the payload, which is
724+
also why the transport below is named outright rather than read off
725+
the analyser.
726+
727+
"""
728+
# NOTE: ``_make_port`` is protected, and reached from outside its own
729+
# class as :meth:`SCTP._decode_next_layer
730+
# <pcapkit.protocols.transport.sctp.SCTP._decode_next_layer>` reaches
731+
# ``ProtocolBase._decode_next_layer``. Duplicating it here instead would
732+
# put a second port normaliser beside the maker path's, free to drift
733+
# from it; it is idempotent, so a ``bufid`` that already holds a member
734+
# passes through unchanged.
735+
return (TCP_Protocol._make_port(bufid[1], Enum_TransportProtocol.tcp), # pylint: disable=protected-access
736+
TCP_Protocol._make_port(bufid[3], Enum_TransportProtocol.tcp)) # pylint: disable=protected-access
737+
656738
def submit(self, buf: 'Buffer', *, bufid: 'BufferID', # type: ignore[override] # pylint: disable=arguments-differ
657739
timeout: 'bool' = False) -> 'list[Datagram]':
658740
"""Submit reassembled payload.
@@ -667,6 +749,34 @@ def submit(self, buf: 'Buffer', *, bufid: 'BufferID', # type: ignore[override]
667749
Returns:
668750
Reassembled :term:`packets <reasm.tcp.datagram>`.
669751
752+
Important:
753+
The ``(source, destination)`` pair handed to
754+
:class:`~pcapkit.foundation.reassembly.data.data.Deferred` is a pair
755+
of :class:`~pcapkit.const.reg.apptype.tcp.TCP` members rather than
756+
of the bare port numbers :data:`BufferID` holds (:issue:`2417`), so
757+
a deferred analysis dispatches on the named service rather than on a
758+
number. The resolution is :meth:`Transport._make_port
759+
<pcapkit.protocols.transport.transport.Transport._make_port>`, so it
760+
is the one the maker path uses and cannot drift from it. See
761+
:meth:`_make_ports` for why it is reached through
762+
:class:`~pcapkit.protocols.transport.tcp.TCP` rather than through
763+
:attr:`self.protocol
764+
<pcapkit.foundation.reassembly.reassembly.ReassemblyBase.protocol>`.
765+
766+
For a *registered* port that is the very member
767+
:attr:`TCP.src <pcapkit.protocols.transport.tcp.TCP.src>` reads off
768+
a parsed header. For an unassigned one the two are **equal but not
769+
identical**, and not even equally named: the parse path builds its
770+
member through :meth:`EnumField._unregistered_member
771+
<pcapkit.corekit.fields.numbers.EnumField._unregistered_member>`,
772+
whose ``.name`` is ``'<unassigned>'``, where ``get`` builds its own,
773+
named ``'unknown'`` -- measured on port ``50000``. Both are real
774+
members of the same registry carrying the same :attr:`AppType.port
775+
<pcapkit.const.reg.apptype.apptype.AppType.port>`, which is what
776+
every reader of this pair goes on to use, so the distinction costs
777+
nothing here; it is recorded because "the same member the header
778+
carries" would be an overstatement of it.
779+
670780
"""
671781
datagram = [] # type: list[Datagram] # reassembled datagram
672782

@@ -751,7 +861,8 @@ def submit(self, buf: 'Buffer', *, bufid: 'BufferID', # type: ignore[override]
751861
index=tuple(buffer.ind),
752862
header=buf.hdr,
753863
payload=bytes(payload),
754-
packet=Deferred(self.protocol.analyze, (bufid[1], bufid[3]), bytes(payload)),
864+
packet=Deferred(self.protocol.analyze,
865+
self._make_ports(bufid), bytes(payload)),
755866
conflict=conflict,
756867
)
757868
datagram.append(packet)

‎pcapkit/protocols/transport/transport.py‎

Lines changed: 58 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -132,21 +132,76 @@ def register(cls, code: 'int', protocol: 'ModuleDescriptor[ProtocolBase] | Type[
132132
cls.__proto__[code] = entry
133133

134134
@classmethod
135-
def analyze(cls, ports: 'tuple[int, int]', payload: 'bytes', **kwargs: 'Any') -> 'ProtocolBase': # type: ignore[override] # pylint: disable=arguments-renamed
135+
def analyze(cls, ports: 'tuple[Enum_AppType, Enum_AppType]', payload: 'bytes', **kwargs: 'Any') -> 'ProtocolBase': # type: ignore[override] # pylint: disable=arguments-renamed
136136
"""Analyse packet payload.
137137
138138
Args:
139-
ports: Source & destination port numbers.
139+
ports: Source & destination ports, as the application types
140+
:meth:`_make_port` resolves them to -- what
141+
:attr:`TCP.src <pcapkit.protocols.transport.tcp.TCP.src>` and
142+
:attr:`TCP.dst <pcapkit.protocols.transport.tcp.TCP.dst>` read
143+
off a parsed header, resolved the same way.
140144
payload: Packet payload.
141145
**kwargs: Arbitrary keyword arguments.
142146
143147
Returns:
144148
Parsed payload as a :class:`~pcapkit.protocols.protocol.Protocol`
145149
instance.
146150
151+
Important:
152+
The annotation names the **base** registry rather than one
153+
transport's own, because this is the single method
154+
:class:`~pcapkit.protocols.transport.tcp.TCP`,
155+
:class:`~pcapkit.protocols.transport.udp.UDP` and
156+
:class:`~pcapkit.protocols.transport.sctp.SCTP` share, and each
157+
resolves its ports in a different one --
158+
:class:`~pcapkit.const.reg.apptype.tcp.TCP` for the first, and so
159+
on. A caller that knows which transport it has gets the narrower
160+
type from whatever handed it the pair, e.g.
161+
:meth:`TCP._make_ports
162+
<pcapkit.foundation.reassembly.tcp.TCP._make_ports>`.
163+
164+
A bare :obj:`int` port still resolves, so the tightened annotation
165+
is a declaration of what callers should pass rather than a new
166+
rejection -- the method predates 1.5.0, and a member and the port it
167+
carries are the same key of an :obj:`int`-keyed mapping anyway:
168+
:meth:`AppType.__hash__
169+
<pcapkit.const.reg.apptype.apptype.AppType.__hash__>` is its port's
170+
and :meth:`AppType.__eq__
171+
<pcapkit.const.reg.apptype.apptype.AppType.__eq__>` compares against
172+
it. So the port chosen below is tested against :attr:`__proto__`
173+
exactly as it arrived, and the chosen one is unwrapped to the
174+
:obj:`int` key the registry declares only when it is a member --
175+
through :meth:`AppType.__int__
176+
<pcapkit.const.reg.apptype.apptype.AppType.__int__>`, i.e.
177+
:attr:`AppType.port
178+
<pcapkit.const.reg.apptype.apptype.AppType.port>`. Reading
179+
``.port`` unconditionally would have made the :obj:`int` case an
180+
:exc:`AttributeError`, and unwrapping unconditionally would have
181+
changed what anything *else* does here: :func:`int` of :obj:`None`
182+
raises where the lookup used to fall back to
183+
:class:`~pcapkit.protocols.misc.raw.Raw`, and :func:`int` of
184+
``'80'`` would dispatch a :class:`str` nobody registered.
185+
186+
Nothing is written back on a miss either way, so a payload on an
187+
unregistered port does not grow a process-wide registry -- see
188+
:meth:`ProtocolBase._lookup_registry
189+
<pcapkit.protocols.protocol.ProtocolBase._lookup_registry>`.
190+
191+
.. versionchanged:: 1.5.0
192+
193+
``ports`` is a pair of
194+
:class:`~pcapkit.const.reg.apptype.apptype.AppType` members rather
195+
than of bare :obj:`int` port numbers, so that the one caller that
196+
reaches this method -- :meth:`TCP.submit
197+
<pcapkit.foundation.reassembly.tcp.TCP.submit>`, through
198+
:class:`~pcapkit.foundation.reassembly.data.data.Deferred` -- hands
199+
over the same named ports a parsed header carries (:issue:`2417`).
200+
147201
"""
202+
port = ports[0] if ports[0] in cls.__proto__ else ports[1]
148203
protocol = cls._lookup_next_layer(
149-
cls.__proto__, ports[0] if ports[0] in cls.__proto__ else ports[1])
204+
cls.__proto__, int(port) if isinstance(port, Enum_AppType) else port)
150205

151206
payload_io = io.BytesIO(payload)
152207
try:

0 commit comments

Comments
 (0)