Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
208 changes: 104 additions & 104 deletions pcapkit/corekit/enum.py

Large diffs are not rendered by default.

49 changes: 25 additions & 24 deletions pcapkit/corekit/fields/field.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@
#: empty, tail past a truncated area and having it decode as zero -- that is
#: how an over-long ``ihl``, or a capture cut short by the snapshot length,
#: reads as end-of-option-list or ``Pad1`` instead of wedging or raising (see
#: #431). Every such read is of a fixed-width, few-octet field, always far
#: under this ceiling, so it is untouched; only a length past it -- which no
#: fixed-width field ever legitimately is -- gets refused.
#: :issue:`431`). Every such read is of a fixed-width, few-octet field, always
#: far under this ceiling, so it is untouched; only a length past it -- which
#: no fixed-width field ever legitimately is -- gets refused.
#:
#: A ceiling on one field says nothing about how many fields a parse may pad,
#: which is what :data:`_MAX_ZERO_PAD_SHORTFALL` and
#: :data:`_ZERO_PAD_BUDGET_RATIO` below are for. See #573.
#: :data:`_ZERO_PAD_BUDGET_RATIO` below are for. See :issue:`573`.
_MAX_ZERO_PAD_LENGTH = 0x40_000

#: int: Shortfall :meth:`FieldBase.unpack` will always zero-pad for, whatever
Expand All @@ -60,17 +60,17 @@
#: declaring ``secrets_length`` of 262,142 against two supplied octets through
#: ``UnknownSecrets.data`` (``pcapkit/protocols/schema/misc/pcapng.py``) --
#: retained 50.0 MiB, an amplification of 10,922x per block. Every individual
#: field was under the ceiling, so nothing refused any of them (#573).
#: field was under the ceiling, so nothing refused any of them (:issue:`573`).
#:
#: The sum therefore wants a budget, and this is the figure that makes one
#: *safe*. A budget on its own is not: a capture cut short by its snapshot length
#: pads legitimately and must keep parsing (#431, and the reasoning that declined
#: #554), and it pads far more than it reads, so any running budget tight enough
#: to matter starts refusing real captures. Worse, it refuses them *sometimes* --
#: measured on this tree with a running budget alone, the same legitimate
#: 54-octet frame parsed to one result on 37 of 40 calls and to another on calls
#: 26, 33 and 39, because whether it fit depended on what had been parsed before
#: it. A guard whose answer moves with history is not a guard.
#: pads legitimately and must keep parsing (:issue:`431`, and the reasoning that
#: declined :issue:`554`), and it pads far more than it reads, so any running
#: budget tight enough to matter starts refusing real captures. Worse, it refuses
#: them *sometimes* -- measured on this tree with a running budget alone, the
#: same legitimate 54-octet frame parsed to one result on 37 of 40 calls and to
#: another on calls 26, 33 and 39, because whether it fit depended on what had
#: been parsed before it. A guard whose answer moves with history is not a guard.
#:
#: 65,536 is what removes that. It is the whole span of a 16-bit wire length
#: field -- which is how an IP header, an IPv6 payload, a TCP or IPv4 option and
Expand All @@ -84,10 +84,11 @@
#: else a 16-bit field can ask for.
#:
#: What is left above it is the band a *32-bit* wire length reaches --
#: PCAP-NG's own block and secrets lengths, which is where #573's amplification
#: lives -- and legitimately that is a once-per-file event, since only the last
#: block of a truncated capture is cut short. So the band gets the running budget
#: below, whose one-off term already covers any single such event outright.
#: PCAP-NG's own block and secrets lengths, which is where :issue:`573`'s
#: amplification lives -- and legitimately that is a once-per-file event, since
#: only the last block of a truncated capture is cut short. So the band gets the
#: running budget below, whose one-off term already covers any single such event
#: outright.
_MAX_ZERO_PAD_SHORTFALL = 0x10_000

#: int: Zero padding *past* :data:`_MAX_ZERO_PAD_SHORTFALL` that
Expand All @@ -105,10 +106,10 @@
#:
#: *Repeating* one is not legitimate, and that is what 16 is chosen to catch.
#: Truncation cuts the end of a file, so a capture has one short block, not two
#: hundred; #573's shape has two hundred because they are declared rather than
#: cut. 16 octets of further allowance per octet genuinely read leaves any real
#: file an allowance orders of magnitude past the one event it can want, while
#: bounding the sum for a file whose blocks all lie.
#: hundred; :issue:`573`'s shape has two hundred because they are declared
#: rather than cut. 16 octets of further allowance per octet genuinely read
#: leaves any real file an allowance orders of magnitude past the one event it
#: can want, while bounding the sum for a file whose blocks all lie.
_ZERO_PAD_BUDGET_RATIO = 0x10

#: ContextVar[Optional[list[int]]]: Running ``[octets supplied, octets
Expand Down Expand Up @@ -207,7 +208,7 @@ def _zero_pad_budget() -> 'Iterator[list[int]]':
#: sign after it (``'>Xs'``), or any other malformed template -- which is what
#: makes checking for it a reliable way to tell those cases apart from each
#: other *before* :func:`struct.calcsize` is asked to size either one. See
#: #825.
#: :issue:`825`.
_RE_NEGATIVE_LENGTH_TEMPLATE = re.compile(r'^[@=<>!]?-\d+')


Expand Down Expand Up @@ -292,7 +293,7 @@ def length(self) -> 'int':
:func:`struct.calcsize` cannot size such a template and raises
a bare :exc:`struct.error`, uncatchable as a pcapkit-specific
error; this re-raises it as the negative-length message below.
See #805.
See :issue:`805`.
ProtocolError: If :attr:`template` is otherwise malformed --
anything else :func:`struct.calcsize` cannot size, such as a
typo'd format character -- rather than the negative-length
Expand All @@ -304,7 +305,7 @@ def length(self) -> 'int':
:data:`_RE_NEGATIVE_LENGTH_TEMPLATE` against :attr:`template`
itself -- which is known already, without needing anything
:func:`struct.calcsize`'s own error says -- rather than by the
error message. See #825.
error message. See :issue:`825`.

"""
try:
Expand Down Expand Up @@ -372,7 +373,7 @@ def __copy__(self) -> 'Self':
directly is exactly what :func:`copy.copy` would have done once it
found it, so this changes nothing about *when* a field is copied or
what the copy contains -- only the redundant dispatch is removed.
See GitHub issue #730.
See GitHub issue :issue:`730`.

Returns:
A new field instance sharing this one's attribute values.
Expand Down
68 changes: 34 additions & 34 deletions pcapkit/corekit/fields/ipaddress.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,20 +90,21 @@ def _reject_bool(value: 'object', description: str) -> 'None':
warning**. On an IPv6-typed field the same conversion happens to
raise instead, because the resulting
:class:`~ipaddress.IPv4Address`'s version mismatches -- and that
asymmetry is exactly what let this slip past #469's otherwise
asymmetry is exactly what let this slip past :issue:`469`'s otherwise
equivalent guard for :meth:`MH._make_opt_mn_id
<pcapkit.protocols.internet.mh.MH._make_opt_mn_id>` (c.f. #491).
<pcapkit.protocols.internet.mh.MH._make_opt_mn_id>` (c.f.
:issue:`491`).

Every caller checks this *before* dispatching on the value's type,
for the same placement reason #469 gives: a correct check in the
wrong position does not fire, and that placement mistake has
already been made twice in this repository's history.
for the same placement reason :issue:`469` gives: a correct check
in the wrong position does not fire, and that placement mistake
has already been made twice in this repository's history.

Guarding the field classes is necessary but not sufficient, because a
``_make_*`` that must know the address family before it can build the
schema converts the argument itself and so never hands this module a
:obj:`bool` at all. :func:`parse_ip_address` is where those callers
reach this guard (c.f. #508).
reach this guard (c.f. :issue:`508`).

"""
if isinstance(value, bool):
Expand Down Expand Up @@ -142,46 +143,45 @@ def parse_ip_address(value: 'IPv4Address | IPv6Address | bytes | int | str',
This is the sanctioned way for a ``_make_*`` method to turn a
caller-supplied address into an :mod:`ipaddress` object, and it exists
because doing it with :func:`ipaddress.ip_address` directly is what
#508 turned out to be: a ``_make_*`` that has to know the address
*family* before it can build the schema -- to size an option whose
length is the only thing on the wire that carries the family -- must
convert the argument itself, and that conversion happens **before** the
schema, so it launders a :obj:`bool` into an
:class:`~ipaddress.IPv4Address` that the guard added for #491 in
:meth:`_IPAddressField.pre_process` can then only see as a legitimate
address. Seven such call sites took ``True`` / ``False`` without
complaint as ``0.0.0.1`` / ``0.0.0.0`` -- or ``::1`` / ``::`` where the
wire format fixes the family as IPv6 -- and six of them went on to pack
those octets. The seventh,
:meth:`TCP._make_mptcp_addaddr
:issue:`508` turned out to be: a ``_make_*`` that has to know the
address *family* before it can build the schema -- to size an option
whose length is the only thing on the wire that carries the family --
must convert the argument itself, and that conversion happens
**before** the schema, so it launders a :obj:`bool` into an
:class:`~ipaddress.IPv4Address` that the guard added for :issue:`491`
in :meth:`_IPAddressField.pre_process` can then only see as a
legitimate address. Seven such call sites took ``True`` / ``False``
without complaint as ``0.0.0.1`` / ``0.0.0.0`` -- or ``::1`` / ``::``
where the wire format fixes the family as IPv6 -- and six of them went
on to pack those octets. The seventh, :meth:`TCP._make_mptcp_addaddr
<pcapkit.protocols.transport.tcp.TCP._make_mptcp_addaddr>`, built an
equally corrupt schema and is only stopped from packing it by an
unrelated defect of its own.

Routing every one of them through here rather than giving each its own
:func:`isinstance` check is the whole point: #469 added exactly such a
check to :meth:`MH._make_opt_mn_id
<pcapkit.protocols.internet.mh.MH._make_opt_mn_id>`, and #491 was the
same defect surviving at every site that had not been thought of. A
guard that has to be remembered per call site is a guard that will be
forgotten at the next one.
:func:`isinstance` check is the whole point: :issue:`469` added
exactly such a check to :meth:`MH._make_opt_mn_id
<pcapkit.protocols.internet.mh.MH._make_opt_mn_id>`, and :issue:`491`
was the same defect surviving at every site that had not been thought
of. A guard that has to be remembered per call site is a guard that
will be forgotten at the next one.

The :obj:`bool` rejection is the **first** statement here, ahead of any
dispatch on the value's type, for the placement reason #469 gives and
:func:`_reject_bool` repeats.
dispatch on the value's type, for the placement reason :issue:`469`
gives and :func:`_reject_bool` repeats.

This raises :exc:`FieldValueError` and not
:exc:`~pcapkit.utilities.exceptions.ProtocolError`, which is deliberate
even though two sibling guards for the same mistake --
:meth:`MH._make_opt_mn_id
<pcapkit.protocols.internet.mh.MH._make_opt_mn_id>` from #469 and
:class:`ESP's SecurityAssociation
<pcapkit.protocols.internet.esp.SecurityAssociation>` from #491 -- raise
the latter. The layer decides: this is a field-level conversion, so it
answers with what :meth:`_IPAddressField.pre_process` answers with for
the identical value, and a caller sees one exception whether the
:obj:`bool` reached the field through the schema or through a
``_make_*``. The two protocol-level guards answer for the *option*,
<pcapkit.protocols.internet.mh.MH._make_opt_mn_id>` from :issue:`469`
and :class:`ESP's SecurityAssociation
<pcapkit.protocols.internet.esp.SecurityAssociation>` from :issue:`491`
-- raise the latter. The layer decides: this is a field-level
conversion, so it answers with what :meth:`_IPAddressField.pre_process`
answers with for the identical value, and a caller sees one exception
whether the :obj:`bool` reached the field through the schema or through
a ``_make_*``. The two protocol-level guards answer for the *option*,
alongside siblings that are not about addresses at all --
``_make_opt_mn_id`` refuses a :obj:`bool` for all eight MN-ID subtypes,
only one of which is address-typed -- so neither can route through here
Expand Down
24 changes: 12 additions & 12 deletions pcapkit/corekit/fields/misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -595,33 +595,33 @@ def nested_packet_context(packet: 'dict[str, Any]') -> 'dict[str, Any]':
:class:`dict` subclass adopted when the ``ChainMap`` was suspected of
corrupting the shared :class:`~abc.ABCMeta` cache every
:class:`Schema <pcapkit.protocols.schema.schema.Schema>` subclass used
to share on CPython <= 3.10 (issue #439), and then a
to share on CPython <= 3.10 (issue :issue:`439`), and then a
:class:`~collections.ChainMap` again once that suspicion was doubted.
A plain :class:`dict` ends the question: it satisfies every
``packet: 'dict[str, Any]'`` annotation on the rest of the field
classes natively, so no :func:`~typing.cast` is needed at the call
site, and it cannot interact with :class:`~abc.ABCMeta` at all because
:class:`dict` is not an :class:`~abc.ABCMeta`-based class.

On the #439 suspicion itself, for the record, since it drove two
rewrites: it is *probably* wrong and no longer decidable. What is
On the :issue:`439` suspicion itself, for the record, since it drove
two rewrites: it is *probably* wrong and no longer decidable. What is
directly measured is that the cache keys on the **exact type
queried**, so asking about a :class:`~collections.ChainMap` instance
caches lookups for :class:`~collections.ChainMap` and not for
:class:`dict`, and that the poisoning observed in #439 came from
ordinary code asking :func:`isinstance` about a plain :class:`dict` --
:func:`~pcapkit.corekit.infoclass.Info.__update__` does exactly that.
Against that, swapping the ``ChainMap`` for a plain literal was, at
the time and on a real CPython 3.10 venv, enough to move
``test_pcapng_remaining_constructor_branches_and_custom_dispatch``
:class:`dict`, and that the poisoning observed in :issue:`439` came
from ordinary code asking :func:`isinstance` about a plain
:class:`dict` -- :func:`~pcapkit.corekit.infoclass.Info.__update__`
does exactly that. Against that, swapping the ``ChainMap`` for a plain
literal was, at the time and on a real CPython 3.10 venv, enough to
move ``test_pcapng_remaining_constructor_branches_and_custom_dispatch``
between passing and failing, toggled both ways. The likeliest
reconciliation -- that the ``ChainMap`` was never causal but changed
which concrete types flowed through unrelated :func:`isinstance` calls
in the same run, and so changed *when* the pre-existing corruption
fired -- is plausible rather than demonstrated, and cannot now be
tested: #439 has been fixed directly, every :class:`Schema` subclass
gets its own ``_abc_impl``, and the original conditions no longer
exist. It does not affect correctness either way.
tested: :issue:`439` has been fixed directly, every :class:`Schema`
subclass gets its own ``_abc_impl``, and the original conditions no
longer exist. It does not affect correctness either way.

"""
return {**packet, '__packet__': packet}
Expand Down
Loading
Loading