From 2c2dcab70e7c92830d34846a8bdb17e1cd12a603 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 24 Sep 2026 20:09:23 -0400 Subject: [PATCH] fix(reg)!: reject an out-of-range port in AppType.get, and claim each span by transport * `get`'s `except ValueError` caught the range rejection `_missing_` raises and minted regardless, so `AppType.get(-1, proto='tcp')` returned `PORT_-1_tcp` -- a `__members__` key no attribute access can reach -- while `TCP(-1)` raised. It now tests `_missing_` for `None`, which is its "no row for this port" answer, and lets the rejection through, so both entry points raise the same `ValueError`. A valid but unassigned port still mints, which is what `get` is for. Caller-visible: `Transport._make_port` passes an unvalidated `int`, so `TCP.make(srcport=99999)` now raises where it minted junk. * the crawler rendered one `_missing_` branch per registry row, so the two spans IANA registers on more than one transport emitted the same condition twice and the second copy was unreachable. Every registry answered with the first: `AppType.get(6010, proto='udp')` gave a UDP member whose `proto` read `tcp`, `6666/udp` answered `ircu` rather than `reserved`, and `proto='sctp'` minted `x11` where IANA assigns nothing. A branch naming a transport now tests `cls.__transport__`; the 762 naming none answer every registry as before. * regenerated `pcapkit/const/reg/apptype/` from the crawler: 4 conditions changed in `apptype.py`, and `tcp.py`, `udp.py`, `sctp.py`, `dccp.py` byte-identical. 3 pins added, each red on 932cb48d1 (12, 3 and 4 failures) and green here. tests/const 55 passed, transport 66, vendor + tier guard 143, registry callers 36, 2,519 subtests; pylint 9.90/10 unchanged, mypy clean, isort clean. --- pcapkit/const/reg/apptype/apptype.py | 37 ++++--- pcapkit/vendor/reg/apptype/apptype.py | 50 +++++++-- tests/const/test_const_apptype_split_unit.py | 102 +++++++++++++++++++ 3 files changed, 167 insertions(+), 22 deletions(-) diff --git a/pcapkit/const/reg/apptype/apptype.py b/pcapkit/const/reg/apptype/apptype.py index 90966670e..42210679d 100644 --- a/pcapkit/const/reg/apptype/apptype.py +++ b/pcapkit/const/reg/apptype/apptype.py @@ -2390,7 +2390,10 @@ def get(cls, key: 'int', *, ValueError: If called on a class that holds no members, i.e. on :class:`AppType` itself, with a ``proto`` naming no registry to delegate to. Also for a ``key`` that is not a port number, since - this registry resolves ports and not service names. + this registry resolves ports and not service names -- including + one outside ``0..65535``, whose rejection by :meth:`_missing_` this + method propagates rather than minting over, so that ``get`` is + never more permissive than ``AppType(...)``. :meta private: """ @@ -2409,11 +2412,15 @@ def get(cls, key: 'int', *, # answered with. return matched[0] - try: - ret = owner._missing_(key) - if ret is None: - raise ValueError - except ValueError: + # NOTE: :meth:`_missing_` answers :obj:`None` for a port it holds no row + # for, which is what minting is for, and *raises* for a value that is not a + # port at all. Catching that rejection was GitHub issue #758's defect: it + # minted ``PORT_999999_tcp`` and ``PORT_-1_tcp``, the latter a name no + # attribute access can reach, and left ``get`` more permissive than + # ``AppType(...)``, which has always raised here. The rejection now + # propagates, so both entry points answer an out-of-range port identically. + ret = owner._missing_(key) + if ret is None: ret = extend_enum(owner, 'PORT_%d_%s' % (key, owner.__transport__.name), key, 'unknown', owner.__transport__) return ret @@ -2460,10 +2467,16 @@ def _missing_(cls, value: 'int') -> 'Optional[AppType]': raise ValueError('%r is not a valid %s' % (value, cls.__name__)) # NOTE: extending this class would give it a member, and aenum then # refuses to subclass it -- permanently, for every registry not yet - # imported. The spans below are IANA's unassigned ranges, which belong to - # whichever registry was asked, never to this one. + # imported. The spans below belong to whichever registry was asked, never + # to this one. if cls.__registry__ is None: raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + # NOTE: most spans are IANA's unassigned and reserved markers, which name + # no transport protocol and so answer every registry. A span that does name + # one tests ``cls.__transport__`` and answers that registry alone -- + # GitHub issue #760, where source order decided instead and a UDP lookup in + # 6000-6063 came back carrying ``tcp``. A registry a named span excludes + # falls through to a mint, which is what IANA assigning it nothing means. if 225 <= value <= 241: #: [N/A] Reserved [:rfc:`1060`] return extend_enum(cls, 'reserved_%d' % value, value, 'reserved', TransportProtocol.get('undefined')) @@ -2887,10 +2900,10 @@ def _missing_(cls, value: 'int') -> 'Optional[AppType]': if 5995 <= value <= 5998: #: [N/A] Unassigned return extend_enum(cls, 'unassigned_%d' % value, value, 'unassigned', TransportProtocol.get('undefined')) - if 6000 <= value <= 6063: + if 6000 <= value <= 6063 and cls.__transport__ is TransportProtocol.get('tcp'): #: [TCP] X Window System return extend_enum(cls, 'x11_%d' % value, value, 'x11', TransportProtocol.get('tcp')) - if 6000 <= value <= 6063: + if 6000 <= value <= 6063 and cls.__transport__ is TransportProtocol.get('udp'): #: [UDP] X Window System return extend_enum(cls, 'x11_%d' % value, value, 'x11', TransportProtocol.get('udp')) if 6078 <= value <= 6079: @@ -3037,10 +3050,10 @@ def _missing_(cls, value: 'int') -> 'Optional[AppType]': if 6658 <= value <= 6664: #: [N/A] Unassigned return extend_enum(cls, 'unassigned_%d' % value, value, 'unassigned', TransportProtocol.get('undefined')) - if 6665 <= value <= 6669: + if 6665 <= value <= 6669 and cls.__transport__ is TransportProtocol.get('tcp'): #: [TCP] IRCU return extend_enum(cls, 'ircu_%d' % value, value, 'ircu', TransportProtocol.get('tcp')) - if 6665 <= value <= 6669: + if 6665 <= value <= 6669 and cls.__transport__ is TransportProtocol.get('udp'): #: [UDP] Reserved return extend_enum(cls, 'reserved_%d' % value, value, 'reserved', TransportProtocol.get('udp')) if 6674 <= value <= 6677: diff --git a/pcapkit/vendor/reg/apptype/apptype.py b/pcapkit/vendor/reg/apptype/apptype.py index 45819b215..788bb3719 100644 --- a/pcapkit/vendor/reg/apptype/apptype.py +++ b/pcapkit/vendor/reg/apptype/apptype.py @@ -345,7 +345,10 @@ def get(cls, key: 'int', *, ValueError: If called on a class that holds no members, i.e. on :class:`{NAME}` itself, with a ``proto`` naming no registry to delegate to. Also for a ``key`` that is not a port number, since - this registry resolves ports and not service names. + this registry resolves ports and not service names -- including + one outside ``0..65535``, whose rejection by :meth:`_missing_` this + method propagates rather than minting over, so that ``get`` is + never more permissive than ``{NAME}(...)``. :meta private: """ @@ -364,11 +367,15 @@ def get(cls, key: 'int', *, # answered with. return matched[0] - try: - ret = owner._missing_(key) - if ret is None: - raise ValueError - except ValueError: + # NOTE: :meth:`_missing_` answers :obj:`None` for a port it holds no row + # for, which is what minting is for, and *raises* for a value that is not a + # port at all. Catching that rejection was GitHub issue #758's defect: it + # minted ``PORT_999999_tcp`` and ``PORT_-1_tcp``, the latter a name no + # attribute access can reach, and left ``get`` more permissive than + # ``{NAME}(...)``, which has always raised here. The rejection now + # propagates, so both entry points answer an out-of-range port identically. + ret = owner._missing_(key) + if ret is None: ret = extend_enum(owner, 'PORT_%d_%s' % (key, owner.__transport__.name), key, 'unknown', owner.__transport__) return ret @@ -415,10 +422,16 @@ def _missing_(cls, value: 'int') -> 'Optional[{NAME}]': raise ValueError('%r is not a valid %s' % (value, cls.__name__)) # NOTE: extending this class would give it a member, and aenum then # refuses to subclass it -- permanently, for every registry not yet - # imported. The spans below are IANA's unassigned ranges, which belong to - # whichever registry was asked, never to this one. + # imported. The spans below belong to whichever registry was asked, never + # to this one. if cls.__registry__ is None: raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + # NOTE: most spans are IANA's unassigned and reserved markers, which name + # no transport protocol and so answer every registry. A span that does name + # one tests ``cls.__transport__`` and answers that registry alone -- + # GitHub issue #760, where source order decided instead and a UDP lookup in + # 6000-6063 came back carrying ``tcp``. A registry a named span excludes + # falls through to a mint, which is what IANA assigning it nothing means. {MISS} {'' if ''.join(MISS.splitlines()[-1:]).startswith('return') else 'return super()._missing_(value)'} '''.strip() # type: Callable[[str, str, str, str, str, str], str] @@ -684,10 +697,27 @@ def records(self, data: 'list[str]') -> 'tuple[OrderedDict[str, Record], list[st except ValueError: start, stop = port.split('-') - miss.append(f'if {start} <= value <= {stop}:') + # NOTE: a span IANA assigns to a transport protocol is claimed by + # that registry alone, through a test on ``cls.__transport__``. + # Source order decided instead until GitHub issue #760: the only + # two spans registered on more than one transport -- 6000-6063 and + # 6665-6669 -- rendered the same condition twice, leaving the + # second copy unreachable, so *every* registry answered with the + # first row's. That is not merely a duplicate to collapse: the + # 6665-6669 rows are two different services, ``ircu`` on TCP and + # IANA's ``reserved`` marker on UDP, so one merged branch carrying + # ``tcp | udp`` would have to discard one of them. + # + # A span naming no transport protocol is an unassigned or reserved + # marker belonging to whichever registry was asked, so it carries + # no test and keeps answering all four. + flag = self.flag([proto]) + claim = '' if proto == 'undefined' else f' and cls.__transport__ is {flag}' + + miss.append(f'if {start} <= value <= {stop}{claim}:') miss.append(f' #: {cmmt}') miss.append(f" return extend_enum(cls, '{self.safe_name(svc)}_%d' % value, " - f"value, {svc!r}, {self.flag([proto])})") + f"value, {svc!r}, {flag})") return line, miss diff --git a/tests/const/test_const_apptype_split_unit.py b/tests/const/test_const_apptype_split_unit.py index 7684ec32d..28896b545 100644 --- a/tests/const/test_const_apptype_split_unit.py +++ b/tests/const/test_const_apptype_split_unit.py @@ -274,6 +274,108 @@ def test_a_real_port_with_no_transport_no_longer_resolves(self) -> None: # It is still documented, just not resolvable. self.assertIn('``reserved``', AppType.__doc__ or '') + def test_get_rejects_an_out_of_range_port_as_the_constructor_does(self) -> None: + """GitHub issue #758: ``get`` swallowed ``_missing_``'s own rejection. + + ``test_every_registry_rejects_a_negative_value`` above asserts ``cls(-1)`` + and ``cls(65536)`` and **never** ``.get``, which is how CI stayed green + while ``AppType.get(-1, proto='tcp')`` minted ``PORT_-1_tcp`` -- a key no + attribute access can reach, since it is not an identifier. ``_missing_`` + did raise; ``get``'s ``except ValueError`` caught that very rejection and + minted regardless, leaving ``get`` more permissive than ``cls(...)``. So + the assertion has to be on ``.get``, and on the registry not having grown: + the defect was visible as ``len(TCP)`` going 6147 to 6148. + """ + from pcapkit.const.reg.apptype import DCCP, SCTP, TCP, UDP, AppType, TransportProtocol + + for cls in (TCP, UDP, SCTP, DCCP): + for port in (-1, 65536, 999999): + with self.subTest(registry=cls.__name__, port=port): + before = len(cls) + with self.assertRaises(ValueError): + cls.get(port) + with self.assertRaises(ValueError): + cls.get_all(port) + with self.assertRaises(ValueError): + AppType.get(port, proto=cls.__transport__) + self.assertEqual(len(cls), before) + self.assertNotIn('PORT_%d_%s' % (port, cls.__transport__.name), + cls.__members__) + + # Paid for by narrowing nothing: a valid but unassigned port is what the + # mint is *for*, and it still answers with one. + minted = AppType.get(59000, proto=TransportProtocol.tcp) + self.addCleanup(self._purge_member, TCP, 'PORT_59000_tcp', 59000) + self.assertEqual(minted.svc, 'unknown') + self.assertEqual(minted.port, 59000) + + def test_a_span_on_two_transports_answers_each_with_its_own_row(self) -> None: + """GitHub issue #760: source order decided, so the TCP row answered all four. + + IANA registers exactly two port *spans* on more than one transport + protocol, and the crawler rendered one ``_missing_`` branch per registry + row -- two branches with an identical condition, the second unreachable. + Every registry therefore answered with the first: ``AppType.get(6010, + proto='udp')`` returned a **UDP** member whose ``proto`` read ``tcp``. + + 6665-6669 is why the duplicate is not simply collapsed into one branch + carrying ``tcp | udp``: its two rows are different services -- ``ircu`` on + TCP, IANA's ``reserved`` marker on UDP -- so a merged branch would have to + discard one of them, and #732's ``TransportProtocol`` retype would then + want a *named* combination for it. Each branch tests ``cls.__transport__`` + instead, which also stays right for a span naming one transport only. + """ + from pcapkit.const.reg.apptype import SCTP, TCP, UDP, AppType + + for cls, port, svc in ((TCP, 6010, 'x11'), (UDP, 6010, 'x11'), + (TCP, 6666, 'ircu'), (UDP, 6666, 'reserved')): + with self.subTest(registry=cls.__name__, port=port): + resolved = AppType.get(port, proto=cls.__transport__) + self.addCleanup(self._purge_member, cls, '%s_%d' % (svc, port), port) + self.assertIs(type(resolved), cls) + self.assertEqual(resolved.svc, svc) + self.assertIs(resolved.proto, cls.__transport__) + + # A span IANA assigns to TCP and UDP assigns nothing to SCTP, so the third + # registry mints rather than inheriting the TCP row -- which it did, as + # ````. + resolved = AppType.get(6010, proto='sctp') + self.addCleanup(self._purge_member, SCTP, 'PORT_6010_sctp', 6010) + self.assertEqual(resolved.svc, 'unknown') + self.assertIs(resolved.proto, SCTP.__transport__) + + def test_every_transport_named_span_is_claimed_by_one_registry(self) -> None: + """The structural half of #760, which is what survives the next crawl. + + :meth:`~pcapkit.const.reg.apptype.apptype.AppType._missing_` is generated, + so the defect returns the moment + :mod:`pcapkit.vendor.reg.apptype.apptype` stops emitting the test -- and + it returns silently, because a shadowed branch is unreachable rather than + wrong. This asserts the invariant over every branch instead of over the two + spans that happen to collide today: a branch minting a member for a named + transport protocol is claimed by that registry, and one minting for + ``undefined`` claims nothing, since IANA's unassigned and reserved markers + belong to whichever registry was asked. + """ + import inspect + import re + + from pcapkit.const.reg.apptype import AppType + + source = inspect.getsource(AppType._missing_.__func__) # type: ignore[attr-defined] + branches = re.findall(r'\n if (.+?):\n #:.*?\n ' + r'return extend_enum\(.+?TransportProtocol\.get\((.+?)\)\)', + source) + self.assertEqual(len(branches), 766) + + for condition, proto in branches: + with self.subTest(condition=condition): + claim = 'cls.__transport__ is TransportProtocol.get(%s)' % proto + if proto == "'undefined'": + self.assertNotIn('cls.__transport__', condition) + else: + self.assertIn(claim, condition) + @staticmethod def _purge_member(cls: type, name: str, port: int) -> None: """Undo an :func:`~aenum.extend_enum` so the registry is left as found.