From 69eb8bfcfc2d393ad584bcbf09ecbf1c56aba6be Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 5 Oct 2026 13:24:26 -0400 Subject: [PATCH] docs(protocols): tighten the transport-layer docstrings and cut timed context (#719) - Drop issue/PR citations and "used to"/"until" history from the TCP, UDP and base-class notes and comments; keep the reasons the code is shaped as it is (Enum_Flags(0) vs cast, flags resolved before options, MPTCP option lengths, parse_ip_address, per-protocol registries). - Correct the Transport._decode_next_layer lookup description (the higher port is used when only it is registered) and the TCP NOP option title. - Docstrings and comments only; AST identical to main. --- pcapkit/protocols/transport/sctp.py | 35 ++-- pcapkit/protocols/transport/tcp.py | 217 ++++++++--------------- pcapkit/protocols/transport/transport.py | 49 +++-- pcapkit/protocols/transport/udp.py | 25 ++- 4 files changed, 121 insertions(+), 205 deletions(-) diff --git a/pcapkit/protocols/transport/sctp.py b/pcapkit/protocols/transport/sctp.py index c337c74a7..fb0fea6db 100644 --- a/pcapkit/protocols/transport/sctp.py +++ b/pcapkit/protocols/transport/sctp.py @@ -225,13 +225,11 @@ class SCTP(Transport[Data_SCTP, Schema_SCTP], (``NG_Application_Protocol``) and 66 (``NGAP_over_DTLS_over_SCTP``). Every other PPID resolves to :class:`~pcapkit.protocols.misc.raw.Raw`. - Only PPID 60 actually decodes, though. A PPID 66 payload is an NGAP PDU - wrapped in a DTLS record, and :mod:`pcapkit` has no DTLS implementation, so - those bytes are not aligned PER and the parse degrades to - :class:`~pcapkit.protocols.misc.raw.Raw` -- every time, not only when - ``pycrate`` is absent. It is registered so that the PPID is *named* in the - protochain rather than reported as an unassigned number, which is strictly - more than leaving it out would give. + Only PPID 60 actually decodes. A PPID 66 payload is an NGAP PDU wrapped in + a DTLS record, and :mod:`pcapkit` has no DTLS implementation, so the bytes + are not aligned PER and the parse always degrades to + :class:`~pcapkit.protocols.misc.raw.Raw`. It is registered so that the PPID + is *named* in the protochain rather than reported as an unassigned number. This class currently supports parsing of the following SCTP chunks, which are directly mapped to the :class:`pcapkit.const.sctp.chunk.Chunk` @@ -356,10 +354,9 @@ class SCTP(Transport[Data_SCTP, Schema_SCTP], lambda: ModuleDescriptor('pcapkit.protocols.misc.raw', 'Raw'), { # PPID 66 is NGAP wrapped in a DTLS record rather than a bare - # NGAP-PDU, and pcapkit implements no DTLS. It is registered anyway - # so that the PPID is *named*: the payload then fails in NGAP's own - # decoder and `beholder` degrades it to Raw, which is where an - # unregistered PPID would have left it regardless. + # NGAP-PDU, and pcapkit implements no DTLS. It is registered so + # that the PPID is *named*: the payload fails in NGAP's own decoder + # and `beholder` degrades it to Raw, as an unregistered PPID would. Enum_PayloadProtocolIdentifier.PayloadProtocolIdentifier_3GPP_NG_Application_Protocol: ModuleDescriptor('pcapkit.protocols.application.ngap', 'NGAP'), # NGAP Enum_PayloadProtocolIdentifier.PayloadProtocolIdentifier_3GPP_NGAP_over_DTLS_over_SCTP: ModuleDescriptor('pcapkit.protocols.application.ngap', 'NGAP'), # NGAP over DTLS }, @@ -615,12 +612,13 @@ def register(cls, code: 'Enum_PayloadProtocolIdentifier | int', protocol: 'Modul Warns: pcapkit.utilities.warnings.RegistryWarning: If this PPID is already - registered, naming the displaced entry and its replacement so a - caller can tell *what* was lost. Fires only when the incumbent - differs from the replacement, as the port-keyed + registered and the incumbent differs from the replacement; the + message names both. As with the port-keyed :meth:`Transport.register ` it - overrides does. + overrides, an unresolved + :class:`~pcapkit.corekit.module.ModuleDescriptor` incumbent + counts as different from the class it names. """ if isinstance(protocol, ModuleDescriptor): @@ -824,15 +822,14 @@ def _decode_next_layer(self, dict_: 'Data_SCTP', proto: 'Optional[int]' = None, The PPID is passed through **unchanged**, registered or not, so that an unregistered payload is still labelled with the identifier it - arrived with -- as :meth:`Internet._import_next_layer + arrived with, as :meth:`Internet._import_next_layer ` does for an unregistered transport type. Resolving it to :class:`~pcapkit.protocols.misc.raw.Raw` is :meth:`ProtocolBase._import_next_layer `'s job, - which looks the PPID up through - :meth:`ProtocolBase._lookup_next_layer - ` and so + through :meth:`ProtocolBase._lookup_next_layer + `, which leaves :attr:`self.__proto__ ` untouched. """ diff --git a/pcapkit/protocols/transport/tcp.py b/pcapkit/protocols/transport/tcp.py index d448067ac..fdbf8e1a0 100644 --- a/pcapkit/protocols/transport/tcp.py +++ b/pcapkit/protocols/transport/tcp.py @@ -333,9 +333,7 @@ class TCP(Transport[Data_TCP, Schema_TCP], # 80 -- so the port cannot decide the version and the payload has to. # That dispatch is a positive identification (the RFC 9113 ยง3.4 # connection preface, then an HTTP/1 start line) rather than a trial - # parse; see #682 for the repoint and #800 for the identification it - # waited on. UDP's table already bound the proxy for the same ports, - # so this is also what removes the asymmetry between the two. + # parse. UDP's table binds the proxy for the same ports. 20: ModuleDescriptor('pcapkit.protocols.application.ftp', 'FTP_DATA'), 21: ModuleDescriptor('pcapkit.protocols.application.ftp', 'FTP'), 80: ModuleDescriptor('pcapkit.protocols.application.http', 'HTTP'), @@ -491,16 +489,14 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_TCP': # connection control flags # # NOTE: ``Enum_Flags(0)``, not ``cast('Enum_Flags', 0)``. :func:`typing.cast` is a - # runtime no-op -- it returns its second argument unchanged -- so the accumulator - # used to stay the plain :class:`int` ``0`` for a segment whose flags octet is all - # zero. Nothing then promoted it, because the ``|=`` below is the only promotion and - # it never runs; ``Enum_Flags.SYN in self._flags`` raised ``TypeError: argument of - # type 'int' is not a container or iterable`` instead of answering. A segment with - # any flag set masked the defect entirely. :class:`Enum_Flags` is an - # :class:`aenum.IntFlag` and declares no ``_missing_`` of its own, so - # ``Enum_Flags(0)`` is a valid flagless member that still compares equal to ``0`` - # and still ORs as before; only the type, and hence the ``repr``, differs. That is - # what :attr:`connection` already advertises it returns. C.f. #616. + # runtime no-op, so the accumulator would stay the plain :class:`int` ``0`` for a + # segment whose flags octet is all zero: the ``|=`` below is the only promotion and + # never runs, and ``Enum_Flags.SYN in self._flags`` would raise ``TypeError: + # argument of type 'int' is not a container or iterable``. A segment with any flag + # set would mask that. :class:`Enum_Flags` is an :class:`aenum.IntFlag` with no + # ``_missing_`` of its own, so ``Enum_Flags(0)`` is a valid flagless member that + # compares equal to ``0`` and ORs as usual. This is the type :attr:`connection` + # advertises. _flag = Enum_Flags(0) for key, val in schema.flags.items(): if val == 1: @@ -570,18 +566,14 @@ def make(self, # built, because option makers reached from ``_make_tcp_options`` read # :attr:`self._flags` -- ``_make_mptcp_join`` branches on it to pick between # the three MP_JOIN layouts of :rfc:`8684` section 3.2 (figure 5 for SYN, - # figure 6 for SYN/ACK, figure 7 for ACK). This block used to sit *after* the - # ``_make_tcp_options`` call below, so on a fresh instance MP_JOIN construction - # died with ``AttributeError: 'TCP' object has no attribute '_flags'``, and on - # an instance that had already parsed a segment it silently built the option - # for *that* segment's flags instead: measured pre-fix, a parsed MP_JOIN-SYN - # instance asked to ``pack`` an MP_JOIN-ACK segment emitted an ACK header - # carrying the 12-octet SYN option, dropping the caller's 20-octet HMAC. That - # second outcome is why initialising ``_flags`` to zero is not the fix -- it - # would leave the stale read intact and turn the fresh case into a spurious - # ``invalid flags combination``. Nothing between here and the old assignment - # site reads ``self._flags`` or depends on the option build, so hoisting the - # whole block is behaviour-preserving for every other option. C.f. #587. + # figure 6 for SYN/ACK, figure 7 for ACK). Resolving them afterwards would + # raise ``AttributeError: 'TCP' object has no attribute '_flags'`` on a fresh + # instance, and on an instance that had already parsed a segment would build + # the option from *that* segment's flags: packing an MP_JOIN-ACK segment from + # a parsed MP_JOIN-SYN instance would emit the 12-octet SYN option and drop + # the caller's 20-octet HMAC. Initialising ``_flags`` to zero would not fix + # that: it leaves the stale read and turns the fresh case into a spurious + # ``invalid flags combination``. flags = { 'cwr': int(cwr), 'ece': int(ece), @@ -594,21 +586,16 @@ def make(self, } # type: Schema_Flags # NOTE: ``Enum_Flags(0)``, not ``cast('Enum_Flags', 0)``. - # :func:`typing.cast` is a runtime no-op, so the accumulator used to stay the - # plain :class:`int` ``0`` whenever no flag was set, and ``Enum_Flags.SYN in - # self._flags`` then raised ``TypeError: argument of type 'int' is not a + # :func:`typing.cast` is a runtime no-op, so the accumulator would stay the + # plain :class:`int` ``0`` whenever no flag is set, and ``Enum_Flags.SYN in + # self._flags`` would raise ``TypeError: argument of type 'int' is not a # container or iterable`` instead of reaching ``_make_mptcp_join``'s own - # ``ProtocolError: ... invalid flags combination``. That branch was unreachable - # before the hoist above -- construction died on the missing attribute first -- - # so this keeps the newly reachable no-SYN-no-ACK case raising the library's - # documented error rather than a bare Python one. :class:`Enum_Flags` is an + # ``ProtocolError: ... invalid flags combination``. :class:`Enum_Flags` is an # :class:`aenum.IntFlag`, so ``Enum_Flags(0)`` is a valid flagless member that - # still compares equal to ``0`` and still ORs as before. :meth:`read` seeds itself - # the same way; it kept the ``cast`` until #616, on the grounds that - # ``mptcp_data_selector`` rejects a flagless MP_JOIN before ``_read_mptcp_join`` - # runs, so no caller could reach the ``TypeError``. That made the read path latent - # rather than sound -- latent by virtue of a guard in another file -- which is a - # fragile reason for a ``TypeError`` not to happen. C.f. #587, #616. + # compares equal to ``0`` and ORs as usual. :meth:`read` seeds itself the same + # way, rather than relying on ``mptcp_data_selector`` rejecting a flagless + # MP_JOIN before ``_read_mptcp_join`` runs, which would leave the ``TypeError`` + # avoided only by a guard in another file. _flag = Enum_Flags(0) for key, val in flags.items(): if val == 1: @@ -808,7 +795,7 @@ def _read_mode_eool(self, schema: 'Schema_EndOfOptionList', *, options: 'Option' def _read_mode_nop(self, schema: 'Schema_NoOperation', *, options: 'Option') -> 'Data_NoOperation': # pylint: disable=unused-argument """Read TCP No Operation option. - Structure of TCP maximum segment size option [:rfc:`793`]: + Structure of TCP no-operation option [:rfc:`793`]: .. code-block:: text @@ -1543,10 +1530,8 @@ def _read_mptcp_capable(self, schema: 'Schema_MPTCPCapable', *, options: 'Option """ # NOTE: :rfc:`8684` section 3.1 gives MP_CAPABLE as 12 octets without - # the receiver's key and 20 octets with it -- this guard, and the - # ``rkey=`` below, read ``(20, 32)``/``32`` until #567, which is what - # made a spec-correct 12-octet MP_CAPABLE unparseable and read a - # spec-correct 20-octet one (with the key) as having none. + # the receiver's key and 20 octets with it, hence this guard and the + # ``rkey=`` test below. if schema.length not in (12, 20): raise ProtocolError(f'{self.alias}: [OptNo {schema.kind}] invalid format') @@ -1660,19 +1645,11 @@ def _read_join_synack(self, schema: 'Schema_MPTCPJoinSYNACK', options: 'Option') ProtocolError: If length is **NOT** ``16``. Note: - The accepted length is ``16``, which is what the figure above -- and - :rfc:`8684` section 3.2 figure 6, which it reproduces -- states, and - what :class:`~pcapkit.protocols.schema.transport.tcp.MPTCPJoinSYNACK` - actually packs and unpacks: ``Kind`` (1) + ``Length`` (1) + - subtype/flags (1) + ``Address ID`` (1) + the truncated HMAC (8) + the - random number (4). - - This guard required ``20`` until :issue:`576` -- a value that appears in - neither the figure nor the schema, and that contradicted this method's - own docstring. Together with ``_make_join_synack``'s ``length=12`` it - made the SYN/ACK form unusable in both directions at once: the maker - could not produce a length this guard accepted, and a spec-correct - 16-octet option off the wire was rejected as an invalid format. + The accepted length is ``16``, as in the figure above (:rfc:`8684` + section 3.2 figure 6) and in what + :class:`~pcapkit.protocols.schema.transport.tcp.MPTCPJoinSYNACK` + packs and unpacks: ``Kind`` (1) + ``Length`` (1) + subtype/flags (1) + + ``Address ID`` (1) + the truncated HMAC (8) + the random number (4). """ if schema.length != 16: @@ -1849,16 +1826,13 @@ def _read_mptcp_remove(self, schema: 'Schema_MPTCPRemoveAddress', *, options: 'O ``Length`` (1) + subtype-and-reserved (1), and each of the *n* Address IDs is one further octet. :attr:`~pcapkit.protocols.schema.transport.tcp.MPTCPRemoveAddress.addr_id` - sizes its list as ``pkt['length'] - 3`` from exactly this, which is why - ``_make_mptcp_remove``'s constant ``length=4`` (fixed in :issue:`576`) also - mis-sized the parse rather than only the pack. + sizes its list as ``pkt['length'] - 3`` from exactly this, so the + declared length governs the parse as well as the pack. The guard permits ``3``, i.e. ``n = 0``, which the figure does not describe -- it shows one Address ID plus "n-1 Address IDs, if - required". Left as it stands: tightening it to reject an empty list is - a behaviour change beyond :issue:`576`'s scope, and a zero-ID REMOVE_ADDR now - at least round-trips honestly instead of declaring an octet it never - packed. + required". It is left permissive: a zero-ID REMOVE_ADDR round-trips + honestly, and rejecting an empty list would be a behaviour change. """ if schema.length < 3: @@ -1907,12 +1881,10 @@ def _read_mptcp_prio(self, schema: 'Schema_MPTCPPriority', *, options: 'Option') 4-octet :rfc:`6824` form is therefore legacy, and the guard stays permissive so that traffic carrying it still parses. - ``_make_mptcp_prio`` declared a constant ``length=4`` until :issue:`576`, - which meant the construction side could only ever emit the legacy - form -- and emitted it with an all-zero phantom Address ID when the - caller supplied none, because + On the construction side, :class:`~pcapkit.protocols.schema.transport.tcp.MPTCPPriority`'s - ``addr_id`` is conditional on that very length being 4. + ``addr_id`` is conditional on ``length`` being 4, so ``length`` has + to follow whether the caller supplied an Address ID. """ if schema.length not in (3, 4): @@ -1998,18 +1970,9 @@ def _read_mptcp_fastclose(self, schema: 'Schema_MPTCPFastclose', options: 'Optio The figure above is :rfc:`8684` section 3.5 figure 14, and the option it draws is **12** octets: ``Kind`` (1) + ``Length`` (1) + subtype-and-reserved (2, being 4 subtype bits and 12 reserved) + the - option receiver's key (64 bits, 8). Note that section 3.5 is Fast - Close; section 3.7 is Fallback (MP_FAIL), which :issue:`576`'s own text cited - here by mistake. - - Three sites disagreed on this number before :issue:`576`, all three now - reading 12: this guard required ``16``, an octet count nothing in the - RFC produces for MP_FASTCLOSE; ``_make_mptcp_fastclose`` declared the - correct 12 but the schema packed only **11**, missing the reserved - octet entirely. The net effect was that constructing an MP_FASTCLOSE - through :class:`TCP` raised ``ProtocolError`` from this very guard -- - the maker's *correct* length failing the parser's wrong check -- which - is why ``tcp-mptcp/MP_FASTCLOSE`` sat in ``EXPECTED_FAILURES``. + option receiver's key (64 bits, 8). The guard, + ``_make_mptcp_fastclose`` and the schema must all agree on 12, the + schema accounting for the reserved octet with a padding field. """ if schema.length != 12: @@ -2729,13 +2692,13 @@ def _make_mode_mp(self, code: 'Enum_Option', opt: 'Optional[Data_MPTCP]' = None, schema = meth(subtype_val, opt, **kwargs) # NOTE: ``Schema_MPTCP.subtype`` is not a packable field (see the - # comment on :class:`~pcapkit.protocols.schema.transport.tcp.MPTCP`), - # so nothing above set it -- ``subtype`` only ever went into ``test``, - # the bitfield each concrete maker actually packs. Real unpacking gets - # it from :meth:`~pcapkit.protocols.schema.transport.tcp._MPTCP.post_process`; - # this is that same assignment for the construction path, so a schema + # comment on :class:`~pcapkit.protocols.schema.transport.tcp.MPTCP`), so + # the makers above only put ``subtype`` into ``test``, the bitfield they + # actually pack. Unpacking sets it from + # :meth:`~pcapkit.protocols.schema.transport.tcp._MPTCP.post_process`; + # this is the same assignment for the construction path, so a schema # built via ``TCP(options=[(Enum_Option.Multipath_TCP, ...)])`` has - # ``.subtype`` set exactly as one built by parsing bytes does. C.f. #566. + # ``.subtype`` set just like one built by parsing bytes. schema.subtype = subtype_val return schema @@ -2803,10 +2766,7 @@ def _make_mptcp_capable(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_M return Schema_MPTCPCapable( kind=cast('Enum_Option', Enum_Option.Multipath_TCP), # NOTE: :rfc:`8684` section 3.1 gives MP_CAPABLE as 12 octets - # without the receiver's key and 20 octets with it. This read - # ``20 if rkey is None else 32`` until #567 -- both branches - # wrong, and the no-key branch writing the value that RFC 8684 - # assigns to the *other* case. + # without the receiver's key and 20 octets with it. length=12 if rkey is None else 20, test={ 'subtype': subtype.value, @@ -2904,12 +2864,9 @@ def _make_join_synack(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPT if opt is not None: backup = opt.backup addr_id = opt.addr_id - # NOTE: ``hmac`` used to be missing from this branch entirely, while - # ``nonce = opt.nonce`` appeared on two consecutive lines -- so - # reconstructing a parsed MP_JOIN-SYN/ACK silently substituted the - # ``bytes(8)`` default for the truncated HMAC that was actually on - # the wire, and the HMAC is the whole point of this form of the - # option. C.f. #576. + # NOTE: ``hmac`` must be carried over here, or reconstructing a + # parsed MP_JOIN-SYN/ACK would substitute the ``bytes(8)`` default + # for the truncated HMAC that was on the wire. hmac = opt.hmac nonce = opt.nonce @@ -2918,10 +2875,8 @@ def _make_join_synack(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPT # NOTE: :rfc:`8684` section 3.2 figure 6 gives ``Length = 16`` for the # SYN/ACK form: ``Kind`` (1) + ``Length`` (1) + subtype/flags (1) + # ``Address ID`` (1) + the truncated HMAC (64 bits, 8) + the random - # number (32 bits, 4). This read ``12`` -- ``_make_join_syn``'s own - # correct length for the *SYN* form of figure 5, which carries a - # 4-octet token where this one carries an 8-octet HMAC -- copied - # across without recomputing. C.f. #576. + # number (32 bits, 4). The SYN form of figure 5 is 12, since it + # carries a 4-octet token where this one carries an 8-octet HMAC. length=16, test={ 'subtype': subtype.value, @@ -2955,9 +2910,7 @@ def _make_join_ack(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPTCPJ # NOTE: :rfc:`8684` section 3.2 figure 7 gives ``Length = 24`` for the # ACK form: ``Kind`` (1) + ``Length`` (1) + subtype-and-reserved (2, # being 4 subtype bits and 12 reserved) + the full HMAC (160 bits, - # 20). This read ``8``, a third of the truth -- and - # ``_read_join_ack``'s guard already required 24, so nothing this - # maker produced could be parsed back. C.f. #576. + # 20). ``_read_join_ack``'s guard requires the same 24. length=24, test={ 'subtype': subtype.value, @@ -3011,29 +2964,19 @@ def _make_mptcp_dss(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPTCP return Schema_MPTCPDSS( kind=cast('Enum_Option', Enum_Option.Multipath_TCP), - # NOTE: this arithmetic is correct against :rfc:`8684` section 3.3 - # figure 9 and is deliberately left as it stands -- #576 filed it as - # one of six wrong lengths, and re-deriving it from the figure found - # it right. Read it as a base plus a widening increment rather than + # NOTE: this arithmetic follows :rfc:`8684` section 3.3 figure 9. + # Read it as a base plus a widening increment rather than # as one term per field: 4 for ``Kind``/``Length``/subtype/flags, then # ``A`` contributes the 4-octet Data ACK and ``a`` a further 4 to make # it 8; ``M`` contributes 12 (a 4-octet DSN, the 4-octet Subflow # Sequence Number, the 2-octet Data-Level Length and the 2-octet # Checksum) and ``m`` a further 4 to widen the DSN to 8. All flags set # gives 4 + 4 + 4 + 12 + 4 = 28, which is the maximum the section - # states in prose. What was wrong was the *schema* it describes: - # ``MPTCPDSS.ack`` and ``.dsn`` packed 0 octets rather than 4 in the - # unextended case, so the option came out 4 or 8 octets short of this - # length and produced ``packet length < 0`` on the way back in. + # states in prose. length=4 + (4 if flag_A else 0) + (4 if flag_a else 0) + (12 if flag_M else 0) + (4 if flag_m else 0), test={ 'subtype': subtype.value, }, - # NOTE: ``'A': flag_A`` used to appear twice in this literal, once at - # the top and once at the bottom. Harmless -- the same value under the - # same key, so the second simply won -- but it made the set of flags - # being written hard to read against figure 9's ``F|m|M|a|A``. C.f. - # #576. flags={ 'F': data_fin, 'A': flag_A, @@ -3076,18 +3019,8 @@ def _make_mptcp_addaddr(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_M # and the option length below are both derived from the family here, # ahead of the schema, so a bare ``ipaddress.ip_address`` would launder # a ``bool`` into an ``IPv4Address`` that the schema's own guard can no - # longer tell from a real address -- ``addr=True`` reached - # ``mptcp_add_address_selector`` as ``0.0.0.1`` with ``version=4`` - # (c.f. #508). Until #541, this option could not be constructed end to - # end at all for an unrelated reason -- ``KeyError: 'length'`` from the - # ``port`` field's condition at - # pcapkit/protocols/schema/transport/tcp.py:819, since - # ``Schema_MPTCPAddAddress`` (like every ``MPTCP`` subtype schema) - # declared no ``kind``/``length`` fields of its own for ``kind=``/ - # ``length=`` below to land in -- which is why the corruption here was - # only ever visible on the schema the maker returns. #541 gave - # ``MPTCP`` real ``kind``/``length`` fields, so both now land and this - # constructs and packs correctly. + # longer tell from a real address: ``addr=True`` would reach + # ``mptcp_add_address_selector`` as ``0.0.0.1`` with ``version=4``. addr_val = parse_ip_address( addr, f'{self.alias}: [OptNo {Enum_Option.Multipath_TCP}] invalid address') version = addr_val.version @@ -3128,15 +3061,10 @@ def _make_mptcp_remove(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MP kind=cast('Enum_Option', Enum_Option.Multipath_TCP), # NOTE: :rfc:`8684` section 3.4.2 figure 13 gives ``Length = 3 + n``, # where the 3 is ``Kind`` (1) + ``Length`` (1) + subtype-and-reserved - # (1) and each of the *n* Address IDs is one further octet. This read - # a constant ``4``, which is right for exactly one list length -- and - # ``examples.generators.options``' own fixture passes ``addr_id=[1]``, - # so the round-trip suite exercised only that one. Measured before the - # fix: ``addr_id=[1, 2]`` packed 5 octets declaring 4, and - # ``addr_id=[]`` packed 3 declaring 4. This is the field + # (1) and each of the *n* Address IDs is one further octet. A constant + # length would be right for only one list length. This is the field # ``MPTCPRemoveAddress.addr_id`` sizes itself from, as - # ``pkt['length'] - 3``, so the constant also mis-sized the parse. - # C.f. #576. + # ``pkt['length'] - 3``, so it governs the parse as well as the pack. length=3 + len(addr_id_list), test={ 'subtype': subtype.value, @@ -3162,11 +3090,9 @@ def _make_mptcp_prio(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPTC """ if opt is not None: - # NOTE: ``backup`` used to be missing from this branch, so - # reconstructing a parsed MP_PRIO always wrote ``B=0`` regardless of - # what was on the wire -- and the ``B`` flag is the entire payload of - # this option. The same shape as ``_make_join_synack``'s dropped - # ``hmac`` above. C.f. #576. + # NOTE: ``backup`` must be carried over, or reconstructing a parsed + # MP_PRIO would always write ``B=0``; the ``B`` flag is the entire + # payload of this option. backup = opt.backup addr_id = opt.addr_id @@ -3179,11 +3105,10 @@ def _make_mptcp_prio(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPTC # option". The 4-octet form is :rfc:`6824`'s, which this schema and # ``_read_mptcp_prio`` both still accept, so the length has to follow # whether an Address ID was actually given rather than being a - # constant. It read a constant ``4``, and because - # ``MPTCPPriority.addr_id`` is conditional on ``pkt['length'] == 4``, - # that constant *satisfied its own predicate*: with ``addr_id=None`` - # the option packed ``1e045000``, a phantom all-zero Address ID octet - # that no caller asked for. C.f. #576. + # constant. ``MPTCPPriority.addr_id`` is conditional on + # ``pkt['length'] == 4``, so a constant ``4`` would satisfy its own + # predicate and pack a phantom all-zero Address ID octet when + # ``addr_id=None``. length=3 if addr_id is None else 4, test={ 'subtype': subtype.value, diff --git a/pcapkit/protocols/transport/transport.py b/pcapkit/protocols/transport/transport.py index d94d69efd..8399813a8 100644 --- a/pcapkit/protocols/transport/transport.py +++ b/pcapkit/protocols/transport/transport.py @@ -92,18 +92,20 @@ def register(cls, code: 'int', protocol: 'ModuleDescriptor[ProtocolBase] | Type[ Warns: pcapkit.utilities.warnings.RegistryWarning: If this port is already - registered, naming the displaced entry and its replacement so a - caller can tell *what* was lost. Fires only when the - incumbent differs from the replacement -- see + registered and the incumbent differs from the replacement; the + message names both. An unresolved + :class:`~pcapkit.corekit.module.ModuleDescriptor` counts as + different from the class it names. See :meth:`ProtocolBase.register ` for the guard this shares with ``register_protocol``. Note: - ``cls.__proto__`` belongs to the concrete protocol, not to - :class:`Transport`, so ``register_apptype`` reaching this method - twice for one call -- once as ``TCP``, once as ``UDP`` -- inspects - two different registries and cannot warn spuriously. + :class:`~pcapkit.protocols.transport.tcp.TCP` and + :class:`~pcapkit.protocols.transport.udp.UDP` each define their own + ``__proto__`` rather than sharing one on :class:`Transport`. + ``register_apptype`` reaches this method once per protocol and + inspects two different registries, so it cannot warn spuriously. """ if cls is Transport: @@ -173,15 +175,13 @@ def _make_port(port: 'Enum_AppType | int', Important: :meth:`self.make ` accepts a bare :obj:`int` for a - port, and the schema field only converts one on the way *out* (in + port, but the schema field converts it only on the way *out* (in :meth:`PortEnumField.pre_process `), - leaving the schema attribute holding whatever it was handed. A - constructed packet therefore reached :meth:`self.read - ` with an :obj:`int` where a parsed one carries an - :class:`~pcapkit.const.reg.apptype.AppType`, and reading ``.port`` - off it raised :exc:`AttributeError`. Normalising here keeps the two - paths agreeing on the type the schema declares. + so the schema attribute keeps whatever it was handed. Normalising + here makes a constructed packet carry the same + :class:`~pcapkit.const.reg.apptype.AppType` as a parsed one, so + reading ``.port`` off it works on both paths. """ if isinstance(port, Enum_AppType): @@ -192,9 +192,9 @@ def _decode_next_layer(self, dict_: '_PT', ports: 'tuple[int, int]', length: 'Op packet: 'Optional[dict[str, Any]]' = None) -> '_PT': # pylint: disable=arguments-renamed """Decode next layer protocol. - The method will check if the next layer protocol is supported based on - the source and destination port numbers. We will use the lower port - number from both ports as the primary key to lookup the next layer. + The next layer is looked up by port number. The lower of the two ports + is the primary key; the higher one is used only when it alone is + registered. Arguments: dict_: info buffer @@ -206,21 +206,20 @@ def _decode_next_layer(self, dict_: '_PT', ports: 'tuple[int, int]', length: 'Op Current protocol with next layer extracted. Important: - The port is forwarded **whether or not it is registered**, since + The port is forwarded **whether or not it is registered**: :meth:`ProtocolBase._import_next_layer ` passes it on as ``alias`` and :class:`~pcapkit.protocols.misc.raw.Raw` records it as :attr:`Data_Raw.protocol - `. Dropping it -- as - this used to, by falling back to :obj:`None` -- anonymised the very - case the field is most useful for: a payload on a port we do not - decode is then indistinguishable from one on port 22. The lower port - is the one carried, for the same reason it is the primary lookup - key. :meth:`SCTP._decode_next_layer + `. Dropping it would + make a payload on a port we do not decode indistinguishable from + one on port 22. The lower port is the one carried, for the same + reason it is the primary lookup key. + :meth:`SCTP._decode_next_layer ` and :meth:`Internet._import_next_layer ` - already behave this way for an unregistered PPID and transport type. + behave the same way for an unregistered PPID and transport type. """ sort_port = sorted(ports) diff --git a/pcapkit/protocols/transport/udp.py b/pcapkit/protocols/transport/udp.py index 01d667424..f0e06e394 100644 --- a/pcapkit/protocols/transport/udp.py +++ b/pcapkit/protocols/transport/udp.py @@ -65,14 +65,11 @@ class UDP(Transport[Data_UDP, Schema_UDP], - :class:`pcapkit.protocols.application.http.HTTP` Note: - Both HTTP ports here resolve to + Both HTTP ports resolve to :class:`pcapkit.protocols.application.http.HTTP`, which identifies the version from the payload and delegates. :attr:`TCP.__proto__ ` - bound :class:`pcapkit.protocols.application.httpv1.HTTP` directly for the - same ports until :issue:`682`, which repointed it here and so removed an - asymmetry that had predated the 8080 entries -- port 80 was already split - that way. Both tables now agree. + binds the same class for its HTTP ports. """ @@ -94,19 +91,17 @@ class UDP(Transport[Data_UDP, Schema_UDP], # 1701 l2tp l2tp # 8080 http-alt HTTP Alternate (see port 80) # - # Both HTTP entries keep pointing at the version-dispatching - # :class:`pcapkit.protocols.application.http.HTTP`, which is what - # port 80 already used here. TCP bound HTTP/1 directly for the same - # ports until #682 repointed it at the proxy too, so the two tables - # no longer disagree. c.f. the note in the class docstring. + # Both HTTP entries bind the version-dispatching + # :class:`pcapkit.protocols.application.http.HTTP`, as TCP's table + # does. c.f. the note in the class docstring. 80: ModuleDescriptor('pcapkit.protocols.application.http', 'HTTP'), 8080: ModuleDescriptor('pcapkit.protocols.application.http', 'HTTP'), - # L2TPv2 (RFC 2661) is UDP-borne, and v2 is the only version the - # package implements, so the concrete class is bound rather than the - # abstract L2TP base. IANA protocol number 115 stays unbound because - # it is L2TPv3 (RFC 3931), which has no class yet -- when it does, it - # takes 115 and this entry becomes a version switch on the Ver + # L2TPv2 (RFC 2661) is UDP-borne, and v2 is the only version + # implemented, so the concrete class is bound rather than the + # abstract L2TP base. IANA protocol number 115 (L2TPv3, RFC 3931) + # stays unbound because that version has no class; once it has, + # it takes 115 and this entry becomes a version switch on the Ver # nibble. c.f. pcapkit.protocols.link.l2tp. 1701: ModuleDescriptor('pcapkit.protocols.link.l2tpv2', 'L2TPv2'), },