Skip to content

Option, chunk and block registries leak on lookup miss, the same way __proto__ did #425

Description

@JarryShaw

#421 covered the __proto__ next-layer registries. The __option__ / __chunk__ /
__block__ / __param__ / __cause__ family has the identical defect and was
out of scope for the fix in #426.

Reproduced, on a tree that already has #421's fix:

before = set(TCP.__option__)
hdr = bytes.fromhex('005001bb00000000000000006002ffff00000000') + bytes([156, 2, 0, 0])
TCP(hdr, len(hdr))                      # one packet, one unknown option kind

set(TCP.__option__) - before            # {<Option.Reserved_156: 156>}
register_tcp_option(Option(156), ...)   # 'option 156 already registered, overwriting'

So parsing a single TCP packet carrying an unrecognised option kind makes a later,
entirely legitimate registration report an overwrite that never happened — #421's
exact symptom, on a different registry.

Scope

Eight defaultdict registries share the shape, enumerated by inspection rather
than by grep:

TCP.__option__        MH.__option__         HOPOPT.__option__
IPv6_Opts.__option__  PCAPNG.__option__     PCAPNG.__block__
SCTP.__chunk__        SCTP.__cause__

Read sites include tcp.py:669,1948,1979, mh.py:1536,3114,3128,
hopopt.py:470,1278,1309, ipv6_opts.py:481, pcapng.py:953,1013,2043,3928,3955,
plus the SCTP chunk/parameter/cause paths.

Fix

#426 adds ProtocolBase._lookup_next_layer(registry, proto), which resolves a hit
and memoises it while returning registry.default_factory() on a miss without
recording it. These registries hold method-name pairs rather than protocol classes,
so the helper is not directly reusable, but the shape of the fix is the same and
the same helper could be generalised.

Worth a test per registry family asserting a miss leaves the key set unchanged —
that is what caught the __proto__ cases and what is missing here.

Consequences, same as #421

  • A spurious RegistryWarning pointing at a conflict that does not exist.
  • One entry accreted per distinct unknown code seen, for the process lifetime, on
    an attribute shared by every instance of the class.
  • "Is this option registered?" becomes un-answerable by inspection, since the
    answer depends on what has been parsed.

Found while fixing #421; deliberately not folded into #426 because three of the
files involved were outside that change's scope.


Correction (the reproduction above). The offset+flags word was 7002, which declares a data offset of 7 words (28 octets) against the 24 octets actually supplied — an inconsistent segment. It is now 6002: 20 octets of fixed header plus the 4-octet option is 24, i.e. 6 words. The leak reproduces identically either way (same option list, same option data, same injected key), so the malformed offset was incidental rather than what drove the parser into the option path — but the packet as written could not have existed on a wire. Caught by Copilot on #428.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions