diff --git a/pcapkit/protocols/application/__init__.py b/pcapkit/protocols/application/__init__.py index deeff803d..c1a1a3adc 100644 --- a/pcapkit/protocols/application/__init__.py +++ b/pcapkit/protocols/application/__init__.py @@ -28,7 +28,7 @@ from pcapkit.protocols.application.ospf import OSPF from pcapkit.protocols.application.rarp import RARP, DRARP -# Deprecated / Base Classes +# Base Classes from pcapkit.protocols.application.http import HTTP # Transport Layer Protocol Numbers diff --git a/pcapkit/protocols/application/ftp.py b/pcapkit/protocols/application/ftp.py index 62e605550..b9832bbff 100644 --- a/pcapkit/protocols/application/ftp.py +++ b/pcapkit/protocols/application/ftp.py @@ -41,10 +41,9 @@ class Type(EnumLookup, StrEnum): """FTP packet type. - Re-parented onto :class:`~pcapkit.corekit.enum.EnumLookup` per GitHub - issue :issue:`877`'s ruling that every non-registry enumeration shares that - lookup contract -- pure re-parenting, since this class defines neither - ``get`` nor ``_missing_`` of its own to reconcile with the base. + Built on :class:`~pcapkit.corekit.enum.EnumLookup`, the lookup contract + shared by every non-registry enumeration (:issue:`877`). The class defines + neither ``get`` nor ``_missing_`` of its own. """ diff --git a/pcapkit/protocols/application/http.py b/pcapkit/protocols/application/http.py index 231bdc0ac..027eeef92 100644 --- a/pcapkit/protocols/application/http.py +++ b/pcapkit/protocols/application/http.py @@ -35,9 +35,8 @@ #: identifiable without parsing: it is a well-formed HTTP/1.1 request line whose #: method ``PRI`` is reserved and permanently unregistered, so no valid HTTP/1 #: message can begin with it and a prefix compare cannot false-positive on one. -#: That is what makes it a positive identification rather than a heuristic, and -#: it is why :meth:`HTTP._guess_version` tests it before attempting any parse -#: (:issue:`800`). +#: That makes it a positive identification rather than a heuristic, which is why +#: :meth:`HTTP._guess_version` tests it before attempting any parse. _HTTP2_PREFACE = b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n' @@ -57,10 +56,10 @@ class HTTP(Application[_PT, _ST], Generic[_PT, _ST]): #: the 24-octet HTTP/2 connection preface, when :meth:`_guess_version` #: identified one and parsed the frame that follows it. They belong to this #: packet's header rather than to its payload, so :meth:`read` adds them to - #: :attr:`length`; without that, ``ProtocolBase.__init__``'s - #: ``self._info.__update__(packet=self.packet.payload)`` slices the payload - #: from octet 9 of a buffer whose frame starts at octet 24 and reports the - #: tail of the preface as packet payload. See :issue:`800`. + #: :attr:`length`; otherwise ``ProtocolBase.__init__``'s + #: ``self._info.__update__(packet=self.packet.payload)`` would slice the + #: payload from octet 9 of a buffer whose frame starts at octet 24 and report + #: the tail of the preface as packet payload. _preface_length = 0 #: This class is a version dispatcher rather than a protocol with a header of @@ -68,11 +67,11 @@ class HTTP(Application[_PT, _ST], Generic[_PT, _ST]): #: declares only ``version`` and forwards everything else to #: :meth:`HTTPv1.make ` or #: :meth:`HTTPv2.make ` - #: according to that value -- so the set of names that is correct here depends - #: on an argument. :obj:`None` therefore opts out of the construction keyword - #: check that :meth:`ProtocolBase.__init__ - #: ` performs (:issue:`617`); the - #: two versioned classes are checked normally when constructed directly. + #: according to that value, so the correct set of names depends on an + #: argument. :obj:`None` therefore opts out of the construction keyword check + #: that :meth:`ProtocolBase.__init__ + #: ` performs; the two + #: versioned classes are checked normally when constructed directly. __keywords__ = None ########################################################################## @@ -140,12 +139,9 @@ def read(self, length: 'Optional[int]' = None, *, raise # NOTE: :exc:`struct.error` alongside :exc:`ValueError` because the # two are disjoint -- it derives straight from :exc:`Exception` -- - # and a payload too short for a versioned parser's fixed header - # raises the former from deep inside the schema machinery - # (``FieldBase.length`` calls :func:`struct.calcsize` on a template - # built from a negative length). A caller of this method cannot - # catch that as a protocol error, which is the whole point of the - # conversion the next line performs, so it is converted too. + # and a payload too short for a versioned parser's fixed header can + # raise the former from inside the schema machinery. A caller cannot + # catch that as a protocol error, so it is converted too. except (ValueError, struct.error) as error: raise ProtocolError(f'HTTP/{version}: invalid format') from error @@ -177,18 +173,15 @@ def make(self, # NOTE: ``protocol.make`` is an ordinary instance method (the abstract # declaration at ``ProtocolBase.make`` takes ``self``, and the # versioned overrides use instance-bound helpers such as - # ``self._make_index``/``self.__frame__``), so calling it on the - # class itself -- as this used to -- left ``self`` unfilled and raised - # ``TypeError`` for every real call; see GH-452. There is no ``file`` - # or ``length`` to construct with here, since building a packet from - # keyword arguments is the inverse of parsing one, so a bare instance - # via ``protocol.__new__`` -- bypassing ``__init__``'s parse/pack - # machinery entirely -- is what the versioned ``make`` needs to be - # called on. This is safe only as long as the versioned ``make`` never - # reads state that ``__init__``/``__post_init__`` would otherwise have - # established (neither ``HTTPv1.make`` nor ``HTTPv2.make`` does today); - # a future ``make`` override that reaches for such state would need a - # different dispatch here. + # ``self._make_index``/``self.__frame__``), so it cannot be called on the + # class itself. There is no ``file`` or ``length`` to construct with here, + # since building a packet from keyword arguments is the inverse of + # parsing one, so a bare instance via ``protocol.__new__`` -- bypassing + # ``__init__``'s parse/pack machinery entirely -- is what the versioned + # ``make`` is called on. This is safe only while the versioned ``make`` + # reads no state that ``__init__``/``__post_init__`` would otherwise have + # established (neither ``HTTPv1.make`` nor ``HTTPv2.make`` does); a + # ``make`` override that did would need a different dispatch here. return protocol.__new__(protocol).make(**kwargs) # type: ignore[return-value] ########################################################################## @@ -219,17 +212,11 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP': """Identify the HTTP version of the payload, and parse it with that version. The payload is *identified* first and trial-parsed only as a last resort. - Until :issue:`800` there was no identification step at all: both versions were - tried in turn and whichever parser did not object was taken as the - answer, which answers "did a parser accept this?" where the question is - "what is this?" -- and got both directions wrong. The HTTP/2 connection - preface came back ``version='2'`` only because ``httpv2.HTTP`` read its - leading ``b'PRI'`` as a 24-bit declared frame length of 5,265,993, and - ``b'foo bar baz\\r\\nX: y\\r\\n\\r\\n'`` -- not HTTP at all -- came back - ``version='2'`` the same way. :issue:`799` closed the second of those by - requiring a frame's declared length to be backed by its buffer, but that - left the preface *unidentifiable*: a real HTTP/2 connection opening is - refused by both arms and reported as not-HTTP. + Trying each version in turn and taking whichever parser does not object + answers "did a parser accept this?" where the question is "what is + this?": the HTTP/2 preface reads as a frame whose declared length is + 5,265,993 (``b'PRI'`` as a 24-bit integer), and text that is not HTTP at + all can be accepted the same way. Args: length: Length of packet data. @@ -282,9 +269,8 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP': # false-positive on HTTP/1, and it needs no parse to reach. # # ``length`` bounds the compare as well as ``self._data``, because a - # caller may hand this method fewer octets than the buffer holds, and - # claiming a preface out of octets that were not part of this payload - # would be the same kind of accident this change removes. + # caller may hand this method fewer octets than the buffer holds, and a + # preface must not be claimed out of octets outside this payload. preface_len = len(_HTTP2_PREFACE) if length >= preface_len and self._data[:preface_len] == _HTTP2_PREFACE: from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 # isort: skip # pylint: disable=line-too-long,import-outside-toplevel @@ -292,15 +278,15 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP': # The preface is not a frame -- the frames begin after it # (:rfc:`9113#section-3.4` requires a ``SETTINGS`` frame immediately # following), so it is skipped rather than fed to ``httpv2.HTTP``, - # which is how it used to be misread as framing. + # which would misread it as framing. if length == preface_len: # A preface with nothing after it *is* HTTP/2, but this library's # HTTP/2 data model is one frame per packet and has no # representation for a frameless segment, so there is nothing to - # return. Refused with a message that says which of the two it - # was -- an HTTP/2 connection opening truncated at the preface, - # not an unrecognised payload -- because that distinction is the - # whole point of identifying before parsing. + # return. Refused with a message that says so -- an HTTP/2 + # connection opening truncated at the preface, not an + # unrecognised payload -- because that distinction is the point + # of identifying before parsing. raise ProtocolError('HTTP/2: connection preface with no frame') try: @@ -308,19 +294,15 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP': except ProtocolError: raise # NOTE: Converted, unlike the HTTP/1 commit below, because this route - # is new and has no escaping-error contract to keep: the old arm 2 - # suppressed :exc:`struct.error` and fell through to ``unknown HTTP - # version``, so a preface followed by a frame that used to trip the - # #805 residual (an inner field shortfall, e.g. a 16-octet - # ``GOAWAY``) had to reach the caller as something it could catch, - # not as a bare stdlib error. That residual has since been closed at - # ``FieldBase.length``, so the same ``GOAWAY`` now raises - # ``ProtocolError`` on its own and is caught by the ``except - # ProtocolError: raise`` above, never reaching this clause -- but - # the conversion stays for whichever :exc:`ValueError` or - # :exc:`struct.error` the schema machinery has not been shown never - # to raise here again. Same normalisation, and the same reasoning, - # as ``read``'s explicit ``version=`` path above. + # has no escaping-error contract to keep: a preface followed by a + # malformed frame must reach the caller as something it can catch, + # not as a bare stdlib error. The schema machinery raises + # ``ProtocolError`` itself for a short inner field (e.g. a 16-octet + # ``GOAWAY``, via ``FieldBase.length``), which the ``except + # ProtocolError: raise`` above passes through, so this clause is a + # backstop for any :exc:`ValueError` or :exc:`struct.error` it has + # not been shown never to raise. Same normalisation as ``read``'s + # explicit ``version=`` path above. except (ValueError, struct.error) as error: raise ProtocolError('HTTP/2: invalid format') from error @@ -339,13 +321,10 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP': # disagree about what HTTP/1 looks like. # # Identified means *committed*: a malformed HTTP/1 message is reported as - # the malformed HTTP/1 message it is, instead of being handed to the - # HTTP/2 arm, which accepts any self-consistent nine-octet-or-longer - # buffer. Re-trying an identified HTTP/1 payload as HTTP/2 is exactly how - # HTTP/1 traffic acquires a confident HTTP/2 mislabel -- the failure #787 - # exists to stop -- and #682 now routes 231 real HTTP/1 frames from the - # fixture corpus through here, TCP:80/8080 having been repointed at this - # class. + # such instead of being handed to the HTTP/2 arm, which accepts any + # self-consistent buffer of nine octets or more. Retrying an identified + # HTTP/1 payload as HTTP/2 is how HTTP/1 traffic acquires a confident + # HTTP/2 mislabel. # # Nothing is suppressed on this arm, deliberately: it is not a candidate # to be declined, so there is nothing to decline *to*, and @@ -359,74 +338,32 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP': # NOTE: Neither identification matched, so this is the fall-through: a # trial parse, kept because a *mid-stream* payload carries no start line # and no preface, and a self-consistent HTTP/2 frame is still the best - # answer available for one. It is the last resort rather than the whole - # method, which is the #800 change. + # answer available for one. It is the last resort, not the whole method. # - # The two arms suppress different sets, and the asymmetry is - # deliberate. Only the *last* arm additionally suppresses - # :exc:`struct.error`, because a payload too short to hold HTTP/2's - # nine-octet frame header, or one whose frame-specific fields exceed - # what a slightly-longer buffer holds, used to fail inside the schema - # machinery with that stdlib exception rather than with a protocol - # error -- ``FieldBase.length`` called :func:`struct.calcsize` on a - # template built from a negative length -- and it was neither a - # ``ProtocolError`` nor a :exc:`ValueError`, so it used to leave this - # method uncatchable by any caller: ``HTTP(io.BytesIO(b'\x00' * 8), - # 8)`` used to raise a bare :exc:`struct.error` instead of reaching - # the closing ``raise`` below. #799's nine-octet guard has since - # closed that particular route -- the same call now raises - # ``ProtocolError: unknown HTTP version``, the documented answer. - # Suppressing :exc:`struct.error` on the last arm stays regardless -- - # kept deliberately, as defence in depth, rather than retired now - # that the case it was added for is closed. Whether anything - # can still reach it, and whether it should therefore go, is #825's - # open question, not settled here. + # The two arms suppress different sets, deliberately. Only the *last* + # arm also suppresses :exc:`struct.error`, as defence in depth: a payload + # too short for HTTP/2's nine-octet frame header, or whose frame-specific + # fields exceed what the buffer holds, must not escape as a stdlib + # exception that no caller of this method can catch. Such payloads raise + # ``ProtocolError`` at the source -- ``httpv2.HTTP.unpack`` rejects a + # buffer under nine octets, and :attr:`FieldBase.length + # ` re-raises a + # negative-length template as + # :exc:`~pcapkit.utilities.exceptions.ProtocolError` -- so the + # suppression only guards whatever path is not yet known to do likewise. # - # #799 closed the *outer*-header slice of this: ``httpv2.HTTP.unpack`` - # now rejects a buffer under nine octets before the schema layer runs - # at all, and ``read`` requires the declared length, the available - # buffer, and their consistency (``schema.length <= length``) all to - # hold. That did *not* retire this suppression, only shrink what it - # has to catch: a buffer that clears nine octets can still carry a - # frame type whose own fixed-width fields exceed what is left after - # the header -- a ``GOAWAY`` at 9-16 octets (``stream`` and ``error`` - # alone are eight), a ``PUSH_PROMISE`` at 9-12, or any ``PADDED`` - # ``DATA``/``HEADERS``/``PUSH_PROMISE`` whose ``pad_len`` exceeds the - # remainder -- and those still drive ``pkt['__length__']`` negative one - # field further in, past this guard's reach. Measured: a 16-octet - # ``GOAWAY`` (``b'\x00\x00\x15\x07\x00\x00\x00\x00\x00' + b'\xff' * 7``) - # used to raise a bare :exc:`struct.error` through ``httpv2.HTTP`` - # directly. That class was closed at its actual root -- - # :attr:`FieldBase.length - # ` now catches - # :func:`struct.calcsize`'s failure on a negative-length template and - # re-raises :exc:`~pcapkit.utilities.exceptions.ProtocolError`, - # generic across every schema in the tree rather than special-cased - # here. :meth:`Schema.unpack - # `'s own - # running-counter warning was left alone on purpose: converting it too - # would reject a ``SETTINGS`` frame with a short trailing entry that - # parses successfully today while only warning, so that - # :class:`~pcapkit.utilities.warnings.SchemaWarning` still fires. The - # same ``GOAWAY`` now raises ``ProtocolError: Field debug resolved to - # a negative length; template='-1s'``. - # - # Widening the *first* arm the same way was measured and reverted, as it - # buys nothing and costs a great deal. Nothing reaches a - # :exc:`struct.error` through ``httpv1.HTTP``: nine byte patterns over - # lengths 0-24, on this route and on ``read(version=1)``, answered - # ``ProtocolError`` 450 times out of 450. And + # Suppressing it on the *first* arm is wrong: nine byte patterns over + # lengths 0-24 raised only ``ProtocolError`` through ``httpv1.HTTP``, so + # it buys nothing, and # :class:`~pcapkit.utilities.exceptions.StructError` *subclasses* # :exc:`struct.error`, so suppressing it on a non-final arm swallows - # pcapkit's own signal and hands the payload to the arm below -- which + # pcapkit's own signal and hands the payload to the arm below, which # accepts anything of at least nine octets. With a fault injected at arm # 1, a *valid* HTTP/1.1 request came back ``version='2'``, and over UDP # port 80 its ``protochain`` read ``UDP:HTTP/2``. A confident HTTP/2 - # mislabel of HTTP/1 traffic is the exact failure #787 exists to stop, and - # it is worse than letting the error escape to ``beholder``, which turns - # it into ``Raw``; it would also erase ``StructError.eof``, which - # ``NoPayload`` handling reads. ``unknown HTTP version`` is only the best - # case, needing arm 2 to decline as well. + # mislabel of HTTP/1 traffic is worse than letting the error escape to + # ``beholder``, which turns it into ``Raw``; it would also erase + # ``StructError.eof``, which ``NoPayload`` handling reads. with contextlib.suppress(ProtocolError): return HTTPv1(self._data, length, **kwargs) diff --git a/pcapkit/protocols/application/httpv1.py b/pcapkit/protocols/application/httpv1.py index 00486fc50..e18b2eace 100644 --- a/pcapkit/protocols/application/httpv1.py +++ b/pcapkit/protocols/application/httpv1.py @@ -78,10 +78,9 @@ def _test_start_line(data: 'bytes') -> 'bool': This is a *classification* predicate and parses nothing: it answers "is this HTTP/1?" for :meth:`HTTP._guess_version - `, which until :issue:`800` - answered that question by trial-parsing every version in the family and - keeping whichever one did not object -- so a payload that is not HTTP at all - was classified by which parser happened to fail less loudly. + `. Classifying by + trial-parsing every version in the family would decide a payload that is + not HTTP at all by which parser happened to fail less loudly. Args: data: Payload to classify. @@ -111,9 +110,9 @@ def _test_start_line(data: 'bytes') -> 'bool': tested only the first line would answer :data:`True` for it. Split at the header/body separator first, as :meth:`HTTP.read ` does, and the preface's - header is ``PRI * HTTP/2.0`` with no CRLF left in it -- which is exactly - why the parser refuses it, and now why this does. Measured: without the - separator split this returned :data:`True` for the preface. + header is ``PRI * HTTP/2.0`` with no CRLF left in it -- which is why the + parser refuses it, and why this does. Without the separator split this + returns :data:`True` for the preface. An HTTP/0.9 request line carries only two tokens and so is not recognised here either, matching the parser, which raises on fewer than three. @@ -141,10 +140,9 @@ def _test_start_line(data: 'bytes') -> 'bool': class Type(EnumLookup, StrEnum): """HTTP packet type. - Re-parented onto :class:`~pcapkit.corekit.enum.EnumLookup` per GitHub - issue :issue:`877`'s ruling that every non-registry enumeration shares that - lookup contract -- pure re-parenting, since this class defines neither - ``get`` nor ``_missing_`` of its own to reconcile with the base. + Built on :class:`~pcapkit.corekit.enum.EnumLookup`, the lookup contract + shared by every non-registry enumeration (:issue:`877`). The class defines + neither ``get`` nor ``_missing_`` of its own. """ @@ -214,14 +212,14 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_HTTP': packet = schema.data # NOTE: A payload carrying no header/body separator at all unpacks short - # here, and the bare ``ValueError`` that used to escape is what made - # ``HTTP._guess_version``'s HTTP/2 arm unreachable: that dispatcher falls - # through on ``ProtocolError`` alone, so an HTTP/1 attempt on HTTP/2 wire - # bytes aborted the guess rather than failing it, and the HTTP/2 attempt - # never ran (#787). ``ProtocolError`` is what the ``Raises:`` section - # above already promises for a malformed packet, and the same conversion - # the explicit ``version=`` path performs at ``http.py:119``; chained, so - # the underlying unpacking error stays reachable as ``__cause__``. + # here. It must surface as ``ProtocolError``, not a bare ``ValueError``: + # ``HTTP._guess_version`` falls through to its HTTP/2 arm on + # ``ProtocolError`` alone, so any other exception from an HTTP/1 attempt + # on HTTP/2 wire bytes would abort the guess instead of failing it. + # ``ProtocolError`` is also what the ``Raises:`` section above promises, + # and the same conversion the explicit ``version=`` path of + # ``HTTP.read`` performs; chained, so the unpacking error stays + # reachable as ``__cause__``. try: header, body = packet.split(b'\r\n\r\n', maxsplit=1) except ValueError as error: @@ -375,9 +373,9 @@ def _read_http_header(self, header: 'bytes') -> 'tuple[Data_Header, OrderedMulti # message: a header of one line with no CRLF -- the HTTP/2 connection # preface, ``PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n``, splits to exactly that # -- and a start line of fewer than three whitespace-separated tokens. - # Raised as ``ProtocolError`` for the reason ``read`` gives above, and - # to the same message this method already uses below for a start line it - # cannot recognise (#787). + # Raised as ``ProtocolError`` for the reason ``read`` gives above, with + # the same message this method uses below for a start line it cannot + # recognise. try: startline, headerfield = header.split(b'\r\n', 1) para1, para2, para3 = re.split(rb'\s+', startline, maxsplit=2) @@ -387,17 +385,14 @@ def _read_http_header(self, header: 'bytes') -> 'tuple[Data_Header, OrderedMulti # NOTE: A field line beginning with SP or HTAB is an ``obs-fold`` # continuation of the line before it (:rfc:`9112#section-5.2`), and is # unfolded here -- the RFC's own remedy -- rather than treated as a field - # line of its own. Deprecated, but present in real captures, and the two - # ways it used to come out were both wrong: a continuation carrying no - # colon left the split below one element long and ``item[1]`` raised + # line of its own. Deprecated, but present in real captures. Treating a + # continuation as a field line of its own goes wrong twice: one carrying + # no colon leaves the split below one element long, so ``item[1]`` raises # :exc:`IndexError`, which is neither a :exc:`ValueError` nor a - # ``ProtocolError`` and so escaped ``HTTP._guess_version``'s suppression - # exactly as the bare :exc:`ValueError` of #787 did; a continuation that - # happened to contain one was worse, parsing silently into a spurious - # extra field (``X-Long: a`` plus ``b: c``, for a folded ``X-Long: a b``) - # with nothing raised at all. Unfolded, a folded message parses to the - # field it actually carries, so this input class stops reaching the - # HTTP/2 arm by accident instead of merely failing more politely. + # ``ProtocolError`` and escapes ``HTTP._guess_version``'s suppression; + # one that happens to contain a colon parses silently into a spurious + # extra field (``X-Long: a`` plus ``b: c``, for a folded ``X-Long: a b``). + # Unfolded, a folded message parses to the field it actually carries. fields = [] # type: list[bytes] for line in headerfield.split(b'\r\n'): if line.startswith((b' ', b'\t')): diff --git a/pcapkit/protocols/application/httpv2.py b/pcapkit/protocols/application/httpv2.py index 4206a3ae3..fbaa9a28e 100644 --- a/pcapkit/protocols/application/httpv2.py +++ b/pcapkit/protocols/application/httpv2.py @@ -83,10 +83,10 @@ Flags = Schema_FrameType.Flags FrameParser = Callable[[Schema_FrameType, NamedArg(Schema_HTTP, 'header')], Data_HTTP] - # NB: ``Flags`` is bound just above, so it goes in unquoted. Quoting it left a - # bare ``ForwardRef('Flags')`` inside the alias, which every *other* module - # that spells ``FrameConstructor`` in an annotation then had to resolve in its - # own namespace -- where the name does not exist. + # NB: ``Flags`` is bound just above, so it goes in unquoted. Quoted, it would + # leave a bare ``ForwardRef('Flags')`` inside the alias, which every *other* + # module that spells ``FrameConstructor`` in an annotation would have to + # resolve in its own namespace -- where the name does not exist. FrameConstructor = Callable[[DefaultArg(Optional[Data_HTTP]), KwArg(Any)], Tuple[Schema_FrameType, Flags]] @@ -97,7 +97,7 @@ class HTTP(HTTPBase[Data_HTTP, Schema_HTTP], schema=Schema_HTTP, data=Data_HTTP): """This class implements Hypertext Transfer Protocol (HTTP/2). - This class currently supports parsing of the following HTTP/2 frames, + This class supports parsing of the following HTTP/2 frames, which are directly mapped to the :class:`pcapkit.const.http.frame.Frame` enumeration: @@ -204,49 +204,40 @@ def unpack(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_HTTP' Notes: This guards ahead of :meth:`Schema.unpack ` rather than - leaving the same check to :meth:`read`, because a buffer shorter - than the fixed 9-octet header is not merely invalid -- it can - crash the schema layer outright. ``pkt['__length__']`` there is - decremented by each field's *nominal* width regardless of how - many octets the buffer actually had, so it goes negative, and the - frame payload's length-derived fields (e.g. an unpadded ``DATA`` - frame's ``data: BytesField(length=lambda pkt: pkt['__length__'])``) - resolve to that negative number. A negative field length becomes - a struct template such as ``'-5s'``, and :func:`struct.calcsize` - raises :exc:`struct.error` for it -- uncaught, since nothing - downstream expects a :exc:`ProtocolError` to be spelled that way. - Rejecting here, before :meth:`Schema.unpack` ever runs, keeps a - frame too short to hold a header from reaching that arithmetic at - all. This closes the *outer*-header class only -- an inner payload - field can still drive ``pkt['__length__']`` negative on a buffer - that clears nine (e.g. a ``GOAWAY`` frame at 9-16 octets, whose - fixed ``stream`` and ``error`` fields alone consume eight); that - residual is why :meth:`HTTP._guess_version - ` still - suppresses :exc:`struct.error` on its last arm. See :issue:`799`. + leaving the check to :meth:`read`, because a buffer shorter than the + fixed 9-octet header cannot be parsed meaningfully. + ``pkt['__length__']`` there is decremented by each field's + *nominal* width regardless of how many octets the buffer had, so it + goes negative and the frame payload's length-derived fields (e.g. an + unpadded ``DATA`` frame's ``data: BytesField(length=lambda pkt: + pkt['__length__'])``) resolve to a negative width. Rejecting here + keeps such a frame from reaching that arithmetic. An inner field can + still drive it negative on a buffer that clears nine (e.g. a + ``GOAWAY`` frame at 9-16 octets, whose fixed ``stream`` and + ``error`` fields alone consume eight); the schema layer raises + :exc:`ProtocolError` for that itself. ``length`` is resolved against :func:`len` only for *this* method's own check, and the *original* argument -- ``None`` - included -- is what is actually forwarded to :meth:`Protocol.unpack + included -- is what is forwarded to :meth:`Protocol.unpack `. Collapsing - ``None`` to a concrete ``0`` before forwarding would change what - :meth:`Schema.unpack`'s own ``prepare`` decorator does with a - now-exhausted stream: it raises + ``None`` to a concrete ``0`` would change what + :meth:`Schema.unpack`'s own ``prepare`` decorator does with an + exhausted stream: it raises :exc:`~pcapkit.utilities.exceptions.StreamEOFError` (an :exc:`EOFError`, the documented "no more packets" signal) only when - the *caller* left ``length`` unspecified, and forwarding a resolved - ``0`` instead would report an exhausted stream as a malformed - packet. A *non-zero* short buffer is unambiguously malformed either - way, so it is rejected here regardless of whether ``length`` was - given explicitly. + the *caller* left ``length`` unspecified, and a resolved ``0`` + would report an exhausted stream as a malformed packet. A + *non-zero* short buffer is malformed either way, so it is rejected + here whether or not ``length`` was given. """ if length is None: # ``0 < ... < 9`` rather than ``... < 9``: an exhausted stream - # (``len(self) == 0``) is left alone here, so ``length`` reaches + # (``len(self) == 0``) is left alone, so ``length`` reaches # :meth:`Protocol.unpack ` # still ``None`` and its ``prepare`` decorator raises - # :exc:`StreamEOFError` as documented above -- only a *non-empty* + # :exc:`StreamEOFError` as documented above; only a *non-empty* # short stream is rejected as malformed here. if 0 < len(self) < 9: raise ProtocolError(f'HTTP/2: invalid format, packet ({len(self)} octet(s)) ' @@ -293,21 +284,16 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_HTTP': schema = self.__header__ # NOTE: ``schema.length`` is the *declared* length off the wire -- the - # 24-bit field a hostile or truncated capture controls outright -- and - # checking only it lets a frame whose buffer holds far fewer octets than - # it claims sail through this guard and report a length nothing backs. - # ``length`` is the actual number of octets available for this frame - # (``Protocol.__len__`` returns ``len(self._data)``, and a caller that - # supplies ``length`` explicitly means the same thing by it), so both - # have to clear the minimum header size for the frame to be viable at - # all, *and* the declared value must not exceed what is actually - # available -- this library's convention (see ``_make_http_length``) - # is that ``length`` counts the whole frame, header included, so the - # two are directly comparable. Without the third clause a frame that - # declares far more than its buffer holds -- the headline case in - # #799, e.g. a nine-octet buffer declaring 16777215 -- still passed - # this guard and reported the declared, attacker-controlled length as - # if the capture actually contained it. See #799. + # 24-bit field a hostile or truncated capture controls outright -- so + # checking only it would let a frame whose buffer holds far fewer octets + # than it claims report a length nothing backs (e.g. a nine-octet buffer + # declaring 16777215). ``length`` is the number of octets actually + # available for this frame (``Protocol.__len__`` returns + # ``len(self._data)``, and an explicit ``length`` means the same), so + # both have to clear the minimum header size *and* the declared value + # must not exceed what is available. This library's convention (see + # ``_make_http_length``) is that ``length`` counts the whole frame, + # header included, so the two are directly comparable. if schema.length < 9 or length < 9 or schema.length > length: raise ProtocolError(f'HTTP/2: [Type {schema.type}] invalid format') if schema.type in (Enum_Frame.SETTINGS, Enum_Frame.PING) and schema.stream['sid'] != 0: diff --git a/pcapkit/protocols/application/ngap.py b/pcapkit/protocols/application/ngap.py index 876db65c0..e0f45b337 100644 --- a/pcapkit/protocols/application/ngap.py +++ b/pcapkit/protocols/application/ngap.py @@ -173,21 +173,19 @@ def load_pycrate() -> 'Optional[Any]': # # PDUKind and Criticality are closed ASN.1 ENUMERATED types with no extension # marker, so a lookup for a value neither declares is a bug, not a version -# skew -- they stay hand-written here rather than generated, matching #877's -# ruling for the closed mh.py helper enums. +# skew -- they stay hand-written here rather than generated, like the closed +# mh.py helper enums (#877). # # ProcedureCode and ProtocolIE are the other shape: 3GPP TS 38.413 keeps # assigning new elementary procedures and IEs to both, but neither is an IANA # registry with a crawlable page -- it is a PDF, with no CSV or HTML kept in -# step with it. GitHub issue #880's owner ruling sources them from |pycrate|_'s -# own compiled specification instead (pycrate_asn1dir.NGAP's NGAP_Constants), -# which is the same optional dependency this module already needs installed to -# decode NGAP at all. They therefore live under pcapkit.const.ngap, generated -# by pcapkit.vendor.ngap.procedure_code and pcapkit.vendor.ngap.protocol_ie, -# and are merely re-exported here under their historical names for backward -# compatibility. Nothing at import time needs pycrate to define them -- only -# the vendor crawler that generates their const module does, not this module -# nor the generated one. +# step with it. They are sourced from |pycrate|_'s own compiled specification +# instead (pycrate_asn1dir.NGAP's NGAP_Constants, #880), the same optional +# dependency this module needs to decode NGAP at all. They therefore live under +# pcapkit.const.ngap, generated by pcapkit.vendor.ngap.procedure_code and +# pcapkit.vendor.ngap.protocol_ie, and are re-exported here under their +# original names. Only the vendor crawler that generates the const module +# needs pycrate; neither this module nor the generated one does at import. # # The SCTP payload protocol identifiers *are* IANA's, and are used from # pcapkit.const.sctp.payload_protocol_identifier rather than duplicated here. @@ -200,10 +198,9 @@ class PDUKind(EnumLookup, StrEnum): The values are spelled as the ASN.1 identifiers, so that a name decoded by |pycrate|_ resolves by value. - Re-parented onto :class:`~pcapkit.corekit.enum.EnumLookup` per GitHub - issue :issue:`877`'s ruling that every non-registry enumeration shares that - lookup contract -- pure re-parenting, since this class defines neither - ``get`` nor ``_missing_`` of its own to reconcile with the base. + Built on :class:`~pcapkit.corekit.enum.EnumLookup`, the lookup contract + shared by every non-registry enumeration (:issue:`877`). The class defines + neither ``get`` nor ``_missing_`` of its own. """ @@ -223,26 +220,18 @@ class Criticality(EnumLookup, IntEnum): by |pycrate|_ through the standard member map. The values are the ``ENUMERATED`` indices, which is what goes on the wire. - Carries no ``get`` of its own. GitHub issue :issue:`877` re-parented this class onto - :class:`~pcapkit.corekit.enum.EnumLookup` and kept a delegating override for - one reason only: it converted the base's name-miss :exc:`KeyError` into a - :exc:`ValueError`, so that an unknown *name* and an unknown *value* reported - identically. GitHub issue :issue:`923`'s ruling retired that conversion -- a name - miss is :exc:`KeyError`-shaped, exactly as ``E['nosuch']`` is on a stdlib - :class:`~enum.Enum` -- which left the override a pure pass-through, so it - went with it. - - The one thing that could have made the deletion unsafe is ``default`` - handling, and there the inherited base is identical to what the override - forwarded: ``get('nosuch', Criticality.reject)`` answers ``reject`` on both, - and ``get('nosuch', 99)`` raises on both, since a ``default`` naming no - registered value is not honoured. That is also what keeps the deletion safe - on a closed ASN.1 ``ENUMERATED`` with no extension marker -- a member-valued - ``default`` resolves through ``_value2member_map_`` and never through the - constructor, so no path here can mint a fourth value; see :meth:`_missing_`, - which refuses one outright. The only loss is the narrower - ``int | str | Criticality`` key annotation, a typing nicety rather than - behaviour. + Carries no ``get`` of its own: the inherited + :meth:`~pcapkit.corekit.enum.EnumLookup.get` is used as is, so an unknown + *name* is :exc:`KeyError`-shaped, exactly as ``E['nosuch']`` is on a stdlib + :class:`~enum.Enum` (:issue:`923`), and an unknown *value* raises + :exc:`ValueError`. + + A ``default`` is honoured only when it names a registered value: + ``get('nosuch', Criticality.reject)`` answers ``reject``, and + ``get('nosuch', 99)`` raises. A member-valued ``default`` resolves through + ``_value2member_map_`` and never through the constructor, so on this closed + ASN.1 ``ENUMERATED`` with no extension marker no path can mint a fourth + value; see :meth:`_missing_`, which refuses one outright. """ @@ -271,13 +260,13 @@ def _missing_(cls, value: 'int') -> 'NoReturn': #: Re-exported from :class:`pcapkit.const.ngap.procedure_code.ProcedureCode` -#: under this module's historical name -- see the ``Enumerations`` comment -#: above. A plain alias, not a subclass: :mod:`aenum` refuses to extend an -#: enumeration that already has members, and this one carries 81 of them. +#: under its original name -- see the ``Enumerations`` comment above. A plain +#: alias, not a subclass: :mod:`aenum` refuses to extend an enumeration that +#: already has members, and this one carries 81 of them. ProcedureCode = Enum_ProcedureCode #: Re-exported from :class:`pcapkit.const.ngap.protocol_ie.ProtocolIE` under -#: this module's historical name -- see :data:`ProcedureCode` above for why +#: its original name -- see :data:`ProcedureCode` above for why #: this is an alias rather than a subclass. #: #: An IE's ID name and the name of the open type its value is keyed under @@ -457,7 +446,7 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_NGAP': # NOTE: a ProtocolError raised inside this block is already # specific about what went wrong, so it must not be caught below # and relabelled "malformed NGAP-PDU". Nothing here raises one - # today; this is what keeps that true of the next edit. + # now; this keeps that true of the next edit. raise except Exception as exc: # NOTE: `Exception` rather than something narrower on purpose. diff --git a/pcapkit/protocols/application/ospf.py b/pcapkit/protocols/application/ospf.py index abea78aaa..1906b6833 100644 --- a/pcapkit/protocols/application/ospf.py +++ b/pcapkit/protocols/application/ospf.py @@ -83,7 +83,7 @@ class OSPF(Application[Data_OSPF, Schema_OSPF], :doc:`/contributing/conventions/protocol-layer-placement`. Note: - The subpackage does not track the dispatch tier. OSPF is still dispatched + The subpackage does not track the dispatch tier. OSPF is dispatched from :attr:`Internet.__proto__ ` at :attr:`~pcapkit.const.reg.transtype.TransType.OSPFIGP` (IANA protocol @@ -93,13 +93,11 @@ class OSPF(Application[Data_OSPF, Schema_OSPF], #: Version number of corresponding protocol, as read off the header. Held on #: the instance rather than read back out of :attr:`self._info #: ` because :attr:`name` and - #: :attr:`alias` are both needed *during* :meth:`read` -- it is - #: :meth:`self._decode_next_layer - #: ` that builds - #: the protocol chain out of :attr:`alias` -- and ``_info`` is not assigned - #: until :meth:`read` has returned. c.f. ``ARP._acnm`` on - #: :class:`~pcapkit.protocols.link.arp.ARP`, which carries the same - #: constraint. + #: :attr:`alias` are needed *during* :meth:`read` -- :meth:`self._decode_next_layer + #: ` builds the + #: protocol chain out of :attr:`alias` -- and ``_info`` is not assigned until + #: :meth:`read` has returned. c.f. ``ARP._acnm`` on + #: :class:`~pcapkit.protocols.link.arp.ARP`, which has the same constraint. _version: 'int' ########################################################################## @@ -332,13 +330,12 @@ def _make_id_numbers(self, id: 'IPv4Address | str | bytes | bytearray') -> 'byte :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address`). Notes: - Latent rather than live: nothing in this module calls this - method today (:meth:`make` builds ``router_id``/``area_id`` - straight from its own arguments), so the only caller is a unit - test. It is routed through :func:`parse_ip_address` anyway, so - that it does not resurface the defect the moment a caller - reaches it -- the same kind of omission is how :issue:`469`'s single-site - fix survived to become :issue:`491` and then :issue:`508` (c.f. :issue:`540`). + Latent rather than live: nothing in this module calls this method + (:meth:`make` builds ``router_id``/``area_id`` straight from its + own arguments), so the only caller is a unit test. It is routed + through :func:`parse_ip_address` anyway, so that a future caller passing a + :obj:`bool` gets :exc:`~pcapkit.utilities.exceptions.FieldValueError` + rather than a silent ``0.0.0.1``. The description below uses :attr:`self.__class__.__name__ ` rather than :attr:`self.alias diff --git a/pcapkit/protocols/application/rarp.py b/pcapkit/protocols/application/rarp.py index d864967d7..3d1ccc0e2 100644 --- a/pcapkit/protocols/application/rarp.py +++ b/pcapkit/protocols/application/rarp.py @@ -59,7 +59,7 @@ class RARP(Application, ARP, schema=Schema_ARP, data=Data_ARP): # pylint: disab :class:`~pcapkit.protocols.link.link.Link`, which *owns* ``__layer__``, so ``class RARP(ARP, Application)`` would report ``'Link'``. - The subpackage does not track the dispatch tier either: RARP is still + The subpackage does not track the dispatch tier either: RARP is dispatched from :attr:`Link.__proto__ ` at :attr:`~pcapkit.const.reg.ethertype.EtherType.Reverse_Address_Resolution_Protocol`, diff --git a/pcapkit/protocols/link/arp.py b/pcapkit/protocols/link/arp.py index c7869a4c7..b8ca28c47 100644 --- a/pcapkit/protocols/link/arp.py +++ b/pcapkit/protocols/link/arp.py @@ -351,11 +351,10 @@ def _read_proto_resolve(self, addr: 'bytes', ptype: 'int') -> 'str | IPv4Address ptype: Protocol type. Returns: - Protocol address. If ``ptype`` is ``0x0800``, i.e. IPv4 adddress, - returns an :class:`~ipaddress.IPv4Address` object; if ``ptype`` is - ``0x86dd``, i.e. IPv6 address, returns an :class:`~ipaddress.IPv6Address` - object; otherwise, returns a raw :data:`str` representing the - protocol address. + Protocol address. If ``ptype`` is ``0x0800`` (IPv4), an + :class:`~ipaddress.IPv4Address`; if ``0x86dd`` (IPv6), an + :class:`~ipaddress.IPv6Address`; otherwise, the address as a hex + :data:`str`. """ if ptype == Enum_EtherType.Internet_Protocol_version_4: # IPv4 @@ -371,8 +370,13 @@ def _make_addr_resolve(self, addr: 'str | bytes', htype: 'int') -> 'bytes': addr: Hardware address. Returns: - Hardware address. If ``htype`` is ``1``, i.e. MAC address, - returns ``:`` separated *hex* encoded MAC address. + Hardware address as :obj:`bytes`. If ``htype`` is ``1``, i.e. MAC + address, the ``:``- or ``-``-separated hex string is validated and + returned with the separators removed (still hex encoded). + + Raises: + ProtocolError: If ``htype`` is ``1`` and ``addr`` is not a + well-formed MAC address. """ _addr = addr.encode() if isinstance(addr, str) else addr @@ -390,11 +394,11 @@ def _make_proto_resolve(self, addr: 'IPv4Address | IPv6Address | str | bytes', p addr: Protocol address. Returns: - Protocol address. If ``ptype`` is ``0x0800``, i.e. IPv4 adddress, - returns an :class:`~ipaddress.IPv4Address` object; if ``ptype`` is - ``0x86dd``, i.e. IPv6 address, returns an :class:`~ipaddress.IPv6Address` - object; otherwise, returns a raw :data:`str` representing the - protocol address. + Packed protocol address. If ``ptype`` is ``0x0800`` (IPv4) or + ``0x86dd`` (IPv6), the address is validated and packed to 4 or 16 + octets; otherwise, a :data:`str` is encoded, an + :class:`~ipaddress.IPv4Address`/:class:`~ipaddress.IPv6Address` is + packed, and anything else is returned as given. Raises: FieldValueError: If ``addr`` is a :obj:`bool` (c.f. @@ -404,9 +408,9 @@ def _make_proto_resolve(self, addr: 'IPv4Address | IPv6Address | str | bytes', p Through :func:`parse_ip_address` rather than :class:`~ipaddress.IPv4Address`/:class:`~ipaddress.IPv6Address` directly, because :obj:`bool` is an :class:`int` subclass that - either constructor accepts without complaint. Before this, - ``addr=True`` packed as ``00000001`` (IPv4) or ``::1`` (IPv6) with - no exception and no warning at all (c.f. :issue:`508`, :issue:`540`). + either constructor accepts without complaint: ``addr=True`` would + pack as ``00000001`` (IPv4) or ``::1`` (IPv6) with no exception and + no warning. The description below uses :attr:`self.__class__.__name__ ` rather than :attr:`self.alias diff --git a/pcapkit/protocols/link/l2tp.py b/pcapkit/protocols/link/l2tp.py index 84ff18ed1..d1e308787 100644 --- a/pcapkit/protocols/link/l2tp.py +++ b/pcapkit/protocols/link/l2tp.py @@ -38,18 +38,17 @@ IP (both versions) utilizes the IANA-assigned IP protocol ID 115"*). That second route is why :attr:`Internet.__proto__ ` -leaves 115 unbound today: the binding waits on an ``L2TPv3`` class, not on a +leaves 115 unbound: the binding waits on an ``L2TPv3`` class, not on a different framing decision. It also means v3 is the first member of this family to have a real :meth:`~pcapkit.protocols.protocol.Protocol.__index__`. -GitHub issue :issue:`548` proposed closing that gap by binding -:class:`~pcapkit.protocols.link.l2tpv2.L2TPv2` at 115 instead, which does not -work and is worth recording so it is not proposed again. Over IP the v3 session -header is, in :rfc:`3931` §4.1.1's own words, *"free of any restrictions imposed -by coexistence with L2TPv2 and L2F"* -- a data message opens with the raw 32-bit -Session ID and carries **no version nibble at all**, so there is nothing a v2 -parser could even test to recognise that the datagram is not its own. Measured, -that binding reported ``version=4``, ``tunnelid=0x5678`` and ``sessionid=0xff03`` +Binding :class:`~pcapkit.protocols.link.l2tpv2.L2TPv2` at 115 instead does not +work (:issue:`548`), and is recorded so it is not proposed again. Over IP the v3 +session header is, in :rfc:`3931` §4.1.1's own words, *"free of any restrictions +imposed by coexistence with L2TPv2 and L2F"* -- a data message opens with the raw +32-bit Session ID and carries **no version nibble at all**, so there is nothing a +v2 parser could test to recognise that the datagram is not its own. Measured, +that binding reports ``version=4``, ``tunnelid=0x5678`` and ``sessionid=0xff03`` for a v3-over-IP datagram: a complete header assembled out of the top half of a Session ID and the first two octets of the PPP frame behind it. 115 is a missing *class*, not a missing registration, and until that class exists an undissected @@ -69,7 +68,7 @@ Selecting a version ------------------- -Nothing *dispatches* on the version nibble yet, because only one version exists +Nothing *dispatches* on the version nibble, because only one version exists -- but :meth:`L2TPv2.read ` does **check** it, and refuses anything other than ``2``. That is the half of the mechanism which is useful with one version implemented: it keeps v3 traffic on diff --git a/pcapkit/protocols/link/l2tpv2.py b/pcapkit/protocols/link/l2tpv2.py index b667aa65f..cdb867641 100644 --- a/pcapkit/protocols/link/l2tpv2.py +++ b/pcapkit/protocols/link/l2tpv2.py @@ -97,10 +97,10 @@ class L2TPv2(L2TP[Data_L2TP, Schema_L2TP], IANA protocol number 115 (``L2TP``) is deliberately left unbound. It references :rfc:`3931`, i.e. **L2TPv3**, whose session and control message headers are a different shape -- so the binding waits on an - ``L2TPv3`` class rather than on this one. Binding *this* class there was - proposed in GitHub issue :issue:`548` and does not work: over IP the v3 session - header carries no version nibble at all, so this class cannot recognise - that the datagram is not its own. See + ``L2TPv3`` class rather than on this one. Binding *this* class there does + not work (:issue:`548`): over IP the v3 session header carries no + version nibble at all, so this class cannot recognise that the datagram + is not its own. See :class:`~pcapkit.protocols.link.l2tp.L2TP` for the measurement. The class subclasses :class:`~pcapkit.protocols.link.link.Link` and so @@ -192,12 +192,12 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_L2TP': # Refuse it rather than parse it: every field after this word has a # different meaning (or no meaning) in another version, so continuing # reports a tunnel and session ID assembled out of octets that are - # neither. Reported as GitHub issue #548, where an :rfc:`3931` §4.1.1 - # L2TPv3-over-IP datagram yielded ``version=4``, ``tunnelid=0x5678`` and - # ``sessionid=0xff03`` -- read out of the top half of a Session ID and - # the first two octets of the PPP frame behind it. This is also what - # makes the hard-coded ``Literal[2]`` of ``version`` true, instead of - # disagreeing with ``info.version`` on the same datagram. + # neither: an :rfc:`3931` §4.1.1 L2TPv3-over-IP datagram would otherwise + # yield ``version=4``, ``tunnelid=0x5678`` and ``sessionid=0xff03``, read + # out of the top half of a Session ID and the first two octets of the PPP + # frame behind it (:issue:`548`). This is also what keeps the hard-coded + # ``Literal[2]`` of ``version`` true, instead of disagreeing with + # ``info.version`` on the same datagram. if _flag['version'] != 2: raise ProtocolError(f'{self.alias}: invalid version: {_flag["version"]}') diff --git a/pcapkit/protocols/link/link.py b/pcapkit/protocols/link/link.py index f4377a1a4..682dcb7eb 100644 --- a/pcapkit/protocols/link/link.py +++ b/pcapkit/protocols/link/link.py @@ -35,7 +35,7 @@ class Link(ProtocolBase[_PT, _ST], Generic[_PT, _ST]): # pylint: disable=abstract-method """Abstract base class for link layer protocol family. - This class currently supports parsing of the following protocols, which are + This class supports parsing of the following protocols, which are registered in the :attr:`self.__proto__ ` attribute: diff --git a/pcapkit/protocols/link/s_tag.py b/pcapkit/protocols/link/s_tag.py index efe0f48c2..febd8e173 100644 --- a/pcapkit/protocols/link/s_tag.py +++ b/pcapkit/protocols/link/s_tag.py @@ -40,7 +40,7 @@ class S_Tag(VLAN, schema=Schema_VLAN, data=Data_VLAN): Note: 802.1ad was incorporated into IEEE 802.1Q-2011, so the service tag is - specified by 802.1Q today. The ``802.1ad`` name is kept because it is + specified by 802.1Q. The ``802.1ad`` name is kept because it is what the provider-bridging tag is universally called, and because it is the only thing distinguishing this class from :class:`~pcapkit.protocols.link.c_tag.C_Tag` by name. diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index 890b7a31a..494584319 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -69,41 +69,39 @@ #: Keywords that configure the construction rather than naming a field, and are #: therefore consumed by :meth:`ProtocolBase.__init__ #: ` or by the schema layer -#: instead of by a :meth:`make `. They -#: are declared by no signature, so :func:`_declared_keywords` cannot find them -#: and they are listed here instead. +#: instead of by a :meth:`make `. No +#: signature declares them, so :func:`_declared_keywords` cannot find them and +#: they are listed here. #: -#: ``packet`` is here because the library puts it there itself, rather than -#: because a caller might: :meth:`ProtocolBase.__init__ -#: ` injects -#: ``packet=self.packet.payload`` into every parsed ``_info``, so the default -#: :meth:`ProtocolBase._make_data -#: ` -- which is -#: ``data.to_dict()`` -- carries it into the keywords that -#: :meth:`ProtocolBase.from_data ` -#: reconstructs from. Refusing it would make ``from_data`` fail on any protocol -#: whose ``make`` does not happen to declare a ``packet``, starting with -#: :class:`~pcapkit.protocols.misc.null.NoPayload`, which is reached for the -#: innermost layer of every packet. It is a field name for some protocols all the -#: same -- :meth:`HIP.make ` takes the -#: HIP packet *type* under that name -- and listing it here does not change how it -#: binds, only that it is never refused. +#: ``packet`` is here because the library puts it there itself: +#: :meth:`ProtocolBase.__init__ ` +#: injects ``packet=self.packet.payload`` into every parsed ``_info``, so the +#: default :meth:`ProtocolBase._make_data +#: ` (``data.to_dict()``) +#: carries it into the keywords that :meth:`ProtocolBase.from_data +#: ` reconstructs from. +#: Refusing it would make ``from_data`` fail on any protocol whose ``make`` does +#: not declare a ``packet``, starting with +#: :class:`~pcapkit.protocols.misc.null.NoPayload`, the innermost layer of every +#: packet. It is also a real field name for some protocols -- +#: :meth:`HIP.make ` takes the HIP +#: packet *type* under it -- and listing it here does not change how it binds, +#: only that it is never refused. OUT_OF_BAND_KEYWORDS = frozenset({'_layer', '_protocol', '__context__', '__packet__', 'packet'}) -#: Cache for :func:`_declared_keywords`, keyed by protocol class. A protocol's -#: signatures do not change after the class is created, and the walk below is -#: :math:`O(\\text{MRO} \\times \\text{methods})`, so it is done once per class +#: Cache for :func:`_declared_keywords`, keyed by protocol class. Signatures do +#: not change after the class is created and the walk below is +#: :math:`O(\\text{MRO} \\times \\text{methods})`, so it runs once per class #: rather than once per constructed packet. _DECLARED_KEYWORDS = {} # type: dict[type, Optional[frozenset[str]]] -#: Methods that a construction keyword may legitimately be destined for. The -#: keywords handed to :class:`Protocol` are forwarded to all of them -- see -#: :meth:`ProtocolBase.__post_init__ -#: `, which passes the -#: same ``**kwargs`` to :meth:`pack ` -#: (and through it to ``make``) *and* to :meth:`unpack +#: Methods a construction keyword may be destined for. The keywords handed to +#: :class:`Protocol` reach all of them: :meth:`ProtocolBase.__post_init__ +#: ` passes the same +#: ``**kwargs`` to :meth:`pack ` (and +#: through it to ``make``) *and* to :meth:`unpack #: ` (and through it to ``read``). _KEYWORD_CONSUMERS = ('make', 'read', 'pack', 'unpack', '__post_init__', '__init__') @@ -124,23 +122,21 @@ def _declared_keywords(cls: 'type') -> 'Optional[frozenset[str]]': and are not to be checked -- inherited :obj:`None` does not count, for the reason given at the read below. - The union is deliberately wider than the signature of ``cls.make`` alone, - because a keyword reaching ``make`` is not necessarily *for* ``make``: + The union is deliberately wider than the signature of ``cls.make``, because + a keyword reaching ``make`` is not necessarily *for* ``make``: :meth:`ProtocolBase.__post_init__ ` hands one ``**kwargs`` to both the construction and the parse of the packet it has just - constructed, so a keyword declared by ``read`` travels through ``make`` as - well. :class:`~pcapkit.protocols.internet.hip.HIP` is the live example -- + constructed, so a keyword declared by ``read`` travels through ``make`` too. + :class:`~pcapkit.protocols.internet.hip.HIP` is the live example: :meth:`HIP.read ` declares - ``extension`` and :meth:`HIP.make ` - does not, yet :meth:`HIP.__post_init__ + ``extension``, :meth:`HIP.make ` + does not, and :meth:`HIP.__post_init__ ` forwards it to both. - Rejecting on ``make`` alone would reject that, which is correct code. The walk covers the whole MRO rather than the most derived override of each - method, for the same reason: a subclass that declares its own keyword and - forwards the rest to its parent must not make the parent's keywords - unreachable. + method, so that a subclass declaring its own keyword and forwarding the rest + to its parent does not make the parent's keywords unreachable. """ try: @@ -151,22 +147,22 @@ def _declared_keywords(cls: 'type') -> 'Optional[frozenset[str]]': unchecked = False names = set(OUT_OF_BAND_KEYWORDS) for klass in cls.__mro__: - # NOTE: A keyword read out of ``**kwargs`` by name rather than declared - # as a parameter is invisible to :func:`inspect.signature`, so the class - # says so itself. Read per class in the MRO, for the same reason the - # methods are: a subclass should not have to repeat its parents'. + # NOTE: A keyword read out of ``**kwargs`` by name is invisible to + # :func:`inspect.signature`, so the class lists it in ``__keywords__``. + # Read per class in the MRO, like the methods, so a subclass need not + # repeat its parents' entries. keywords = klass.__dict__.get('__keywords__', ABSENT) if keywords is None: # NOTE: The :obj:`None` opt-out is *not* inherited, unlike a set, # which is unioned down the MRO. It describes how the class that - # declares it dispatches, which is not a property its subclasses - # share: :class:`~pcapkit.protocols.application.http.HTTP` cannot - # enumerate its keywords because it forwards them to whichever of + # declares it dispatches, not a property its subclasses share: + # :class:`~pcapkit.protocols.application.http.HTTP` cannot enumerate + # its keywords because it forwards them to whichever of # :class:`HTTPv1 ` and # :class:`HTTPv2 ` the - # ``version`` names -- but those two declare theirs in full, and - # inheriting the opt-out would silently exempt the very classes that - # can be checked. A subclass that dispatches in turn says so itself. + # ``version`` names, but those two declare theirs in full, and + # inheriting the opt-out would exempt the very classes that can be + # checked. A subclass that dispatches in turn says so itself. if klass is cls: unchecked = True elif keywords is not ABSENT: @@ -175,9 +171,9 @@ def _declared_keywords(cls: 'type') -> 'Optional[frozenset[str]]': for method in _KEYWORD_CONSUMERS: # NOTE: Read from ``__dict__`` rather than with :func:`getattr`, so # that each class in the MRO contributes its *own* definition instead - # of the most derived one over and over. An ``@overload``-decorated - # stub is overwritten by the implementation that follows it, which is - # what lands here. + # of the most derived one every time. An ``@overload`` stub is + # overwritten by the implementation that follows it, which is what + # lands here. func = klass.__dict__.get(method) if func is None: continue @@ -185,9 +181,9 @@ def _declared_keywords(cls: 'type') -> 'Optional[frozenset[str]]': try: signature = inspect.signature(func) except (TypeError, ValueError): # pragma: no cover - # NOTE: A C-implemented or otherwise unintrospectable callable is - # skipped rather than fatal: failing to widen the accepted set is - # a false rejection, so the safe move is to keep walking. + # NOTE: An unintrospectable callable is skipped rather than fatal: + # failing to widen the accepted set is a false rejection, so keep + # walking. continue for name, param in signature.parameters.items(): @@ -234,10 +230,10 @@ def _check_construction_keywords(cls: 'type', kwargs: 'dict[str, Any]', if not unexpected: return - # NOTE: The whole point of the check is a misspelling, so name the neighbour - # that was probably meant: ``seq`` for ``seq_no`` and ``ack_flag`` for - # ``ack`` are both a :func:`difflib.get_close_matches` hit, and the message - # is the only place the caller looks before reading the signature. + # NOTE: The check exists to catch misspellings, so name the neighbour that + # was probably meant (``seq`` for ``seq_no``, ``ack_flag`` for ``ack``, both + # a :func:`difflib.get_close_matches` hit); the message is the only place the + # caller looks before reading the signature. report = [] # type: list[str] for key in unexpected: suggestions = difflib.get_close_matches(key, declared, n=1) @@ -250,19 +246,17 @@ def _check_construction_keywords(cls: 'type', kwargs: 'dict[str, Any]', # NOTE: A warning rather than an error, because nobody typed these: they are # whatever ``_make_data`` returned, so the defect is a key of that mapping # disagreeing with the signature it is spread into, and the person who meets - # it is not the person who can fix it. Raising would also turn three latent - # defects of exactly that shape into a broken ``from_data`` -- ``Frame`` - # returns ``ts_src`` for ``ts_sec``, ``Header`` an undeclared - # ``magic_number``, ``L2TPv2`` ``prio`` for ``priority`` -- each of which has - # been losing that field in silence and each of which belongs to its own - # change. This is what makes them audible meanwhile. + # it cannot fix it. Raising would turn every such mismatch -- e.g. ``Frame`` + # returns ``ts_src`` for ``ts_sec`` and ``L2TPv2`` ``prio`` for ``priority`` + # -- into a broken ``from_data``, where the field is merely dropped; the + # warning makes that audible instead. # # No explicit ``stacklevel``: the default blames the innermost frame outside # :mod:`pcapkit`, which is the ``from_data`` call the reader wants to be - # pointed at, and it stays right if the frames between here and there ever - # change, where a hardcoded count would not. It is also what - # :meth:`Schema.__update__ ` - # passes for the warning this one is the counterpart of. + # pointed at, and it stays right if the frames between change, where a + # hardcoded count would not. It is also what :meth:`Schema.__update__ + # ` passes for the + # warning this one is the counterpart of. warn(f'{cls.__name__}._make_data returned keyword(s) that no signature of ' f'{cls.__name__} declares, so they are discarded: {listed}', UnknownFieldWarning) @@ -331,20 +325,20 @@ class ProtocolBase(Generic[_PT, _ST], metaclass=ProtocolMeta): #: Construction keywords this protocol consumes out of ``**kwargs`` instead #: of declaring as a parameter, e.g. with ``kwargs.get('spam')`` in #: :meth:`read` -- as :meth:`ESP.read ` - #: does with ``packet``. :func:`~pcapkit.protocols.protocol._declared_keywords` - #: finds a protocol's keywords by reading its signatures, which cannot see - #: such a name, so a protocol that consumes one names it here and the - #: construction check of :meth:`__init__` accepts it. The union over the MRO - #: is used, so a subclass need not repeat its parents' entries. + #: does with ``packet``. + #: :func:`~pcapkit.protocols.protocol._declared_keywords` finds keywords by + #: reading signatures, which cannot see such a name, so a protocol that + #: consumes one names it here and the construction check of :meth:`__init__` + #: accepts it. The union over the MRO is used, so a subclass need not repeat + #: its parents' entries. #: - #: Declaring the parameter is preferable where it is possible, since that is - #: also what documents the keyword to the caller and to :mod:`inspect`. This - #: is for the cases where it is not -- a keyword handled uniformly for a whole - #: family of names, say -- and *not* a way to reopen the silence :issue:`617` closed: - #: it is opt-in per class, so it can only ever exempt a name whose author - #: wrote it down. + #: Declaring the parameter is preferable where possible, since that also + #: documents the keyword to the caller and to :mod:`inspect`. This is for the + #: cases where it is not -- a keyword handled uniformly for a whole family of + #: names, say. It is opt-in per class, so it can only exempt a name whose + #: author wrote it down (:issue:`617`). #: - #: :obj:`None` means the keywords cannot be enumerated at all and the check is + #: :obj:`None` means the keywords cannot be enumerated and the check is #: skipped for this protocol. That is for a *dispatcher*, whose real signature #: belongs to a class chosen at call time: #: :meth:`HTTP.make ` declares @@ -352,28 +346,27 @@ class ProtocolBase(Generic[_PT, _ST], metaclass=ProtocolMeta): #: :meth:`HTTPv1.make ` or #: :meth:`HTTPv2.make ` #: depending on that value, so no set of names is right for it. Use it only - #: for that shape; a protocol that forgoes the check gets the pre-:issue:`617` - #: behaviour back, and with it the silence. Unlike a set, the :obj:`None` is - #: **not** inherited: a subclass of a dispatcher is checked normally unless it - #: dispatches too and says so, because ``HTTPv1`` and ``HTTPv2`` declare their - #: keywords in full and exempting them along with their base would forgo the - #: check on the only two classes here that can have it. + #: for that shape, since a protocol that skips the check silently discards an + #: unknown keyword. Unlike a set, the :obj:`None` is **not** inherited: a + #: subclass of a dispatcher is checked normally unless it dispatches too and + #: says so, because ``HTTPv1`` and ``HTTPv2`` declare their keywords in full + #: and exempting them along with their base would forgo the check on the only + #: two classes here that can have it. __keywords__: 'Optional[frozenset[str]]' = frozenset() #: Whether this instance is being rebuilt by :meth:`from_data` from a parsed #: data model, as against constructed from keywords somebody wrote. It governs #: only whether the construction keyword check of :meth:`__init__` raises or - #: warns (:issue:`617`), and is set for the duration of that call alone -- the class - #: level :data:`False` is what every other code path sees, including an - #: instance built without going through ``__init__`` at all. + #: warns, and is set for the duration of that call alone; the class-level + #: :data:`False` is what every other code path sees, including an instance + #: built without ``__init__``. __reconstructing__: 'bool' = False #: Caller supplied parsing context, c.f. :mod:`pcapkit.corekit.context`. #: :meth:`self.__init__ ` replaces this with a real - #: :class:`~pcapkit.corekit.context.ContextRegistry`; the class level - #: :data:`None` is what an instance built without going through - #: ``__init__`` -- e.g. ``object.__new__(SomeProtocol)`` -- sees, so that - #: reading it is always safe. + #: :class:`~pcapkit.corekit.context.ContextRegistry`; the class-level + #: :data:`None` is what an instance built without ``__init__`` (e.g. + #: ``object.__new__(SomeProtocol)``) sees, so reading it is always safe. _exctx: 'Optional[ContextRegistry]' = None ########################################################################## @@ -447,17 +440,15 @@ def packet(self) -> 'Data_Packet': there to the end of the buffer. Both hold for a protocol laid out as a header followed by its payload, which is nearly all of them. - A protocol that is not laid out that way has to override this: one - whose :attr:`~length` counts something else, or one carrying a - *trailer* after the payload, gets a header that eats the payload and - a payload of ``b''``. That is what - :class:`~pcapkit.protocols.misc.pcapng.PCAPNG` did to every packet - block -- its :attr:`~pcapkit.protocols.misc.pcapng.PCAPNG.length` is - the wire's Block Total Length and the captured octets sit ahead of - the option list and the trailing length field -- and - :meth:`ProtocolBase.__init__` injects this payload into every parsed - ``_info``, so the empty value reached the dumpers and corrupted the - files they wrote. See :issue:`646`. + A protocol laid out otherwise has to override this: one whose + :attr:`~length` counts something else, or one carrying a *trailer* + after the payload, gets a header that eats the payload and a payload + of ``b''``. :class:`~pcapkit.protocols.misc.pcapng.PCAPNG` is the + example -- its :attr:`~pcapkit.protocols.misc.pcapng.PCAPNG.length` + is the wire's Block Total Length and the captured octets sit ahead of + the option list and the trailing length field. The payload matters + because :meth:`ProtocolBase.__init__` injects it into every parsed + ``_info``, which the dumpers write out. """ try: @@ -534,12 +525,11 @@ def make(self, **kwargs: 'Any') -> '_ST': ` hands to the parse as well as to the construction, so an implementation is not expected to declare every keyword it is called with. It is *not* a - place for a caller to put a keyword no signature declares: since - :issue:`617`, building a protocol *through its constructor* with such a - keyword raises :exc:`~pcapkit.utilities.exceptions.UnsupportedCall` - from :meth:`ProtocolBase.__init__ - ` rather than - discarding it. + place for a caller to put a keyword no signature declares: building a + protocol *through its constructor* with such a keyword raises + :exc:`~pcapkit.utilities.exceptions.UnsupportedCall` from + :meth:`ProtocolBase.__init__ + ` (:issue:`617`). Warning: **Calling this method directly is not checked**, and still discards an @@ -552,9 +542,9 @@ def make(self, **kwargs: 'Any') -> '_ST': package's own tests and by :meth:`HTTP.make ` to reach its versioned implementation. Covering it would mean interposing on every ``make`` - in the tree rather than on the one place their keywords converge, which - is a larger change than :issue:`617` and deliberately not made here. Construct - through the constructor to get the check. + in the tree rather than on the one place their keywords converge, so + it is deliberately not done. Construct through the constructor to get + the check. """ @@ -742,25 +732,24 @@ def register(cls, code: 'int', protocol: 'ModuleDescriptor | Type[ProtocolBase]' Warns: pcapkit.utilities.warnings.RegistryWarning: If ``code`` is already - registered. The warning names the displaced entry and its - replacement, so a caller can tell *what* was lost rather than - only that something was. + registered, naming the displaced entry and its replacement so a + caller can tell *what* was lost rather than only that something + was. Fires only when the incumbent differs from the + replacement; an unresolved + :class:`~pcapkit.corekit.module.ModuleDescriptor` incumbent + counts as different from the class it names. Note: - The guard now matches :func:`register_protocol + The guard matches :func:`register_protocol `'s: it fires only when the incumbent differs from the replacement, so re-registering the exact same class object under the same ``code`` is a silent no-op rather than a warning about nothing displaced. - GitHub issue :issue:`718` corrected the previous presence-only guard here, - which read every repeat registration as a caller mistake even when - the value was unchanged. The identity check does not reintroduce - the concern that guard was written to avoid: it is a plain ``is`` - comparison, so an incumbent left as an unresolved - :class:`~pcapkit.corekit.module.ModuleDescriptor` is never equal to - the resolved replacement without the descriptor being resolved -- - the comparison itself resolves nothing, so the deferred import - stays deferred and such an incumbent still reports as different. + The comparison is a plain ``is`` that resolves nothing, so an + incumbent left as an unresolved + :class:`~pcapkit.corekit.module.ModuleDescriptor` still reports as + different from the class it names, and the deferred import stays + deferred. """ if isinstance(protocol, ModuleDescriptor): @@ -815,12 +804,12 @@ def from_data(cls, data: '_PT | dict[str, Any]') -> 'Self': kwargs = self._make_data(data) # NOTE: These keywords came out of ``_make_data``, not out of a caller, so - # the construction keyword check of ``__init__`` (#617) warns here instead - # of raising: a key of that mapping which disagrees with the signature it - # is spread into is a defect in this protocol, and the caller of + # the construction keyword check of ``__init__`` warns here instead of + # raising: a key of that mapping which disagrees with the signature it is + # spread into is a defect in this protocol, and the caller of # ``from_data`` can do nothing about it. Set for the duration of the call - # and removed afterwards, so an instance built this way is afterwards - # indistinguishable from one built directly. + # and removed afterwards, so the instance is indistinguishable from one + # built directly. self.__reconstructing__ = True try: # initialize protocol instance @@ -882,31 +871,28 @@ def __init__(self, file: 'Optional[IO[bytes] | bytes]' = None, length: 'Optional keyword names no parameter of this protocol's :meth:`make`, :meth:`read`, :meth:`pack`, :meth:`unpack`, :meth:`__post_init__` or :meth:`__init__`, anywhere in the MRO, - and is not listed in :attr:`__keywords__`. See :issue:`617`; until then - such a keyword was silently discarded. Parsing (``file`` is + and is not listed in :attr:`__keywords__`. Parsing (``file`` is given) is unaffected. Note: Three of the keywords above are *out-of-band*: they configure the parse rather than describing the packet, and every one of them is consumed here, at the one point each of a protocol's producers passes - through. That is deliberate, and it is what the normalisation below - relies on -- fixing a spelling here fixes it for the engines, for all - four :meth:`_import_next_layer ` + through. That is deliberate, and the normalisation below relies on it: fixing + a spelling here fixes it for the engines, for all four + :meth:`_import_next_layer ` implementations, and for any third party protocol that copied their shape, rather than one call site at a time. """ #logger.debug('%s(file, %s, **%s)', type(self).__name__, length, kwargs) - # Whether this instantiation parses an existing packet, as opposed to - # constructing a new one. ``file`` is the discriminator the rest of this - # method already turns on: ``__post_init__`` reads the stream when there - # is one and calls ``self.pack(**kwargs)`` when there is not. It matters - # below because ``layer``, ``protocol`` and ``packet`` are out-of-band - # only while parsing -- on the construction path they are ordinary - # ``make()`` arguments, and consuming them there would silently drop the - # value being constructed. + # Whether this instantiation parses an existing packet rather than + # constructing a new one; ``__post_init__`` turns on the same ``file``. + # ``layer``, ``protocol`` and ``packet`` are out-of-band only while + # parsing -- on the construction path they are ordinary ``make()`` + # arguments, and consuming them there would silently drop the value + # being constructed. parsing = file is not None #: int: File pointer. @@ -916,19 +902,17 @@ def __init__(self, file: 'Optional[IO[bytes] | bytes]' = None, length: 'Optional #: str: Parse packet until such protocol. self._exproto = kwargs.pop('_protocol', None) # type: Optional[str | ProtocolBase | Type[ProtocolBase]] - # NOTE: The parse limits are documented here as ``_layer`` and - # ``_protocol``, but no producer in the tree spells them that way. The - # engines build the outermost protocol with ``layer=``/``protocol=`` + # NOTE: The parse limits are documented as ``_layer`` and ``_protocol``, + # but no producer in the tree spells them that way. The engines build the + # outermost protocol with ``layer=``/``protocol=`` # (``pcapkit.foundation.engines.pcap.PCAP.read_frame`` and # ``pcapkit.foundation.engines.pcapng.PCAPNG.read_frame``), every # ``_import_next_layer`` recurses into the next one the same way, and the - # un-prefixed pair is also the public spelling that - # ``pcapkit.extract(layer=..., protocol=...)`` and the CLI's ``-L``/``-P`` - # use. Both were therefore dropped into ``**kwargs`` and ignored, so - # neither option did anything at all; see GH-356. Accepting both - # spellings is what makes them work, and the prefixed one still wins so - # that a caller which reads this docstring is not overridden by a limit - # its parent happened to be forwarding. + # un-prefixed pair is the public spelling of + # ``pcapkit.extract(layer=..., protocol=...)`` and the CLI's ``-L``/``-P``. + # Left in ``**kwargs`` they would be ignored, so both spellings are + # accepted (GH-356); the prefixed one wins, so a caller following this + # docstring is not overridden by a limit its parent happened to forward. if parsing: layer = kwargs.pop('layer', None) protocol = kwargs.pop('protocol', None) @@ -963,13 +947,12 @@ def __init__(self, file: 'Optional[IO[bytes] | bytes]' = None, length: 'Optional # NOTE: The enclosing layer's packet context arrives as ``packet=`` -- the # spelling ``_import_next_layer`` uses -- but the schema layer reads it # from ``__packet__`` (``self.unpack`` below, and the ``pack``/``unpack`` - # overrides of ``Frame`` and ``PCAPNG``). Nothing bridged the two, so a - # schema's ``unpack``/``post_process`` always saw an empty dict however - # much the outer layer had put in it: an ``IPv6`` source address never - # reached the HOPOPT MPL option that RFC 7731 elides from the wire, and a - # destination address never reached the RPL source route header that - # RFC 6554 needs it to decompress. Republish it here, for the same reason - # the limits above are normalised here. See GH-382. + # overrides of ``Frame`` and ``PCAPNG``). Republishing it here, for the + # same reason the limits above are normalised here, is what lets a + # schema's ``unpack``/``post_process`` see what the outer layer put in it: + # the ``IPv6`` source address that the HOPOPT MPL option elides from the + # wire (RFC 7731), and the destination address that the RPL source route + # header needs to decompress (RFC 6554). See GH-382. # # A copy rather than the dict itself: ``Schema.unpack`` writes every field # it reads into the context it is given, plus its own ``__length__`` and @@ -977,20 +960,18 @@ def __init__(self, file: 'Optional[IO[bytes] | bytes]' = None, length: 'Optional # hands one dict to each header in turn. Sharing it would leave one # header's fields visible to the next, where a ``ConditionalField`` test # or a length callback could read a sibling's stale value instead of - # failing. ``Schema.unpack`` already isolates its own per-field contexts - # the same way. + # failing. ``Schema.unpack`` isolates its own per-field contexts the same + # way. if parsing and '__packet__' not in kwargs and isinstance(kwargs.get('packet'), dict): kwargs['__packet__'] = dict(kwargs['packet']) # NOTE: Construction only. A keyword that names no parameter of this - # protocol is a mistake rather than a value, and until #617 it was - # silently discarded: every ``make`` in the tree ends its signature with - # ``**kwargs`` and never reads it, so the keyword reached the schema as - # nothing at all and the field kept its default. The cost was measured on - # #602, where ``TCP_BASE`` asked for ``seq=1`` -- which ``TCP.make`` - # spells ``seq_no`` -- and 25 generated fixture frames carried ``seq = 0`` - # with an empty ``warnings`` list to show for it. The schema layer has - # never been that permissive: :meth:`Schema.__update__ + # protocol is a mistake rather than a value. Every ``make`` in the tree + # ends its signature with ``**kwargs`` and never reads it, so an unknown + # keyword would reach the schema as nothing at all and the field would + # keep its default -- ``TCP_BASE`` asking for ``seq=1``, which + # ``TCP.make`` spells ``seq_no``, would yield ``seq = 0`` with no warning + # (:issue:`617`). The schema layer is not that permissive: :meth:`Schema.__update__ # ` warns # :exc:`~pcapkit.utilities.warnings.UnknownFieldWarning` for a field it # does not know, and this closes the asymmetry from the other end. @@ -999,9 +980,9 @@ def __init__(self, file: 'Optional[IO[bytes] | bytes]' = None, length: 'Optional # whatever the engines and the four ``_import_next_layer`` # implementations forward -- ``alias``, ``packet``, and the limits # normalised above -- and a protocol has no way to know which of its - # ancestors' keywords its parent chose to pass on. Nothing was ever lost - # that way either: a dropped parse keyword changes how a packet is read, - # not what the octets say. + # ancestors' keywords its parent chose to pass on. Nothing is lost that + # way either: a dropped parse keyword changes how a packet is read, not + # what the octets say. if not parsing: _check_construction_keywords( type(self), kwargs, strict=not self.__reconstructing__) @@ -1075,19 +1056,18 @@ def __init_subclass__(cls, /, schema: 'Optional[Type[_ST]]' = None, :class:`~pcapkit.protocols.schema.misc.raw.Raw` and :class:`~pcapkit.protocols.data.misc.raw.Raw` classes will be used. - Dispatch registration is **opt-in**, exactly like the ``name=``/``protocol=``/ + Dispatch registration is **opt-in**, like the ``name=``/``protocol=``/ ``fmt=`` keywords of :class:`~pcapkit.foundation.engines.engine.Engine`, :class:`~pcapkit.foundation.reassembly.reassembly.Reassembly`, :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlow` and :class:`~pcapkit.dumpkit.common.Dumper`. Omitting ``code`` is a - deliberate, documented way for a subclass to decline registration, not - an oversight -- it is exactly what every built-in protocol class does - today, since the built-in dispatch tables (e.g. ``Link.__proto__``) - are populated by literal assignment in each layer module, not by this - hook, so leaving ``code`` unset here changes nothing about them. A - subclass that declines can still be registered later, on demand, via - the owning class's :meth:`register` classmethod or the matching - ``register_*`` helper in :mod:`pcapkit.foundation.registry.protocols`. + deliberate way to decline registration, and what every built-in + protocol class does: the built-in dispatch tables (e.g. + ``Link.__proto__``) are populated by literal assignment in each layer + module, not by this hook. A subclass that declines can still be + registered later via the owning class's :meth:`register` classmethod or + the matching ``register_*`` helper in + :mod:`pcapkit.foundation.registry.protocols`. ``code`` accepts: @@ -1114,8 +1094,8 @@ def __init_subclass__(cls, /, schema: 'Optional[Type[_ST]]' = None, Inference refuses rather than guesses: an enum member whose type names no known destination raises - :exc:`~pcapkit.utilities.exceptions.RegistryError` instead of - silently doing nothing or picking an arbitrary registry. + :exc:`~pcapkit.utilities.exceptions.RegistryError` rather than doing + nothing or picking an arbitrary registry. See Also: :func:`pcapkit.foundation.registry.protocols.register_protocol_code` @@ -1611,11 +1591,10 @@ def _make_index(cls, name: 'str | int | StdlibEnum | AenumEnum', default: 'Optio namespace = cast('dict[int, str]', namespace) index = {v: k for k, v in namespace.items()}[name] else: - # Caught by the handler immediately below and converted, so - # this never escapes -- it is a jump to the shared "name is - # not in namespace" path, not a stdlib exception leaking out - # of the library. A pcapkit exception here would log at - # CRITICAL for something that is handled two lines later. + # Caught by the handler below, so this never escapes: it jumps + # to the shared "name is not in namespace" path. A pcapkit + # exception here would log at CRITICAL for something handled + # two lines later. raise KeyError(name) except KeyError as error: if default is None: @@ -1690,15 +1669,14 @@ def _lookup_registry(registry: 'DefaultDict[Any, _VT]', code: 'Any') -> '_VT': Every one of these registries is a :class:`collections.defaultdict` held on a *class* attribute, shared by every instance of the class in the process. So ``registry[code]`` inserts each code it misses, and - parsing one packet carrying an unrecognised code is enough to grow - the registry permanently. - - The inserted value is whatever the default factory would have - produced anyway, so the entry buys nothing. It costs a spurious - "already registered" warning from the next genuine ``register`` call - for that code, and it makes "is this code registered?" - unanswerable by inspection, since the answer depends on what has - been parsed. The fallback is therefore read from the default factory + parsing one packet carrying an unrecognised code grows the registry + permanently. + + The inserted value is what the default factory would have produced + anyway, so the entry buys nothing. It costs a spurious "already + registered" warning from the next genuine ``register`` call for that + code, and makes "is this code registered?" depend on what has been + parsed. The fallback is therefore read from the default factory directly rather than through a lookup that records it. """ @@ -1740,13 +1718,11 @@ def _lookup_next_layer(registry: 'DefaultDict[int, ModuleDescriptor[ProtocolBase what keeps that affordable is :attr:`ModuleDescriptor.klass ` reading :data:`sys.modules` instead of re-entering - :func:`importlib.import_module` -- see :issue:`574`. Memoising the resolved - class here instead, whether under ``proto``, in ``registry``'s + :func:`importlib.import_module` (:issue:`574`). Memoising the + resolved class instead, whether under ``proto``, in ``registry``'s default factory, or in a cache beside the registry, would retain a - class that :func:`importlib.reload` then makes stale; :issue:`421` at - this layer, :issue:`425` for the option, chunk and block registries - beside it, and :issue:`555` at the schema layer are the same retention - defect, of a lookup miss rather than of a resolved class. + class that :func:`importlib.reload` then makes stale (:issue:`421`, + :issue:`425`, :issue:`555`). """ protocol = ProtocolBase._lookup_registry(registry, proto)