Repository navigation
fix(reassembly): the deferred TCP analyser dispatches on AppType, not bare ports - #2421
Conversation
JarryShaw
added
fix
JarryShaw
added
review: running
|
Can we actually use TCP's AppType for TCP's Reassembly? |
|
Yes — it already does. |
|
Coverage: 94.22% (unit tier, Python 3.15,
Per-file detail: the |
|
Review: NEEDS CHANGES at Blocking: the pickle regression is not an edge case. Every sample capture with TCP flows ( Non-blocking prose/doc fixes: My read on your three judgment calls: keep Minor: |
JarryShaw
added
review: needs-changes
|
why and I was referring to the typing annotation. why dont we directly use TCP enum, rather the base AppType? |
|
Both good questions, and both point at a real improvement. Why Why the annotation uses the base |
d323fee to
5c38fc0
Compare
JarryShaw
added
review: pending
5c38fc0 to
2365640
Compare
JarryShaw
added
review: running
|
Live CI break found on this head ( Fix dispatched: moving that class (and its capture-reading helpers) into a new Also found: |
JarryShaw
added
review: needs-changes
|
Review: NEEDS CHANGES at Blocking: Also before merge: the PR body still says the pickle regression is "NOT fixed here, deliberate call" — stale, contradicts the current head. Minor, non-blocking: the full 65,536-port sweep sits in an Everything else verified independently: commit layering intact (other agent's work not lost), pickle mechanism sound across Py3.8–3.15/protocols 0–5/multiprocessing, full 262,144-member sweep 0 failures, registry non-growth confirmed, ripple-check across all 127 |
… bare ports Follow-up to #2418, which made `Deferred` generic over what its analyser dispatches on but left the TCP side of that constraint as `tuple[int, int]`. The owner's note on it: we are still using int tuples for transport ports, so move to AppType tuples. * `TCP.submit` resolves `bufid[1]` / `bufid[3]` through `Transport._make_port` before building the `Deferred`, in a new `TCP._make_ports` helper. A deferred analysis now dispatches on a named service rather than on a number. * `_KT`'s second constraint is `tuple[AppType_TCP, AppType_TCP]` -- the per-transport registry `pcapkit.const.reg.apptype.tcp.TCP`, which is what the resolution really answers with (`type(...) is TCP`, measured), not the base `AppType`. `Transport.analyze` keeps the base, being the one method TCP, UDP and SCTP share, and says so. * `Transport.analyze` tests the chosen port against `__proto__` exactly as it arrived and unwraps it to the registry's `int` key only when it is a member, so a bare `int` caller keeps working -- the method is in v1.4.1 -- and `(None, None)` or `(54321, '80')` still fall back to `Raw` as on mainline rather than raising or dispatching. * `_make_ports` reaches `_make_port` through `TCP` rather than through `self.protocol`: `__protocol_type__` is an extension point whose analyser need only define `analyze`, so `self.protocol._make_port` would be an `AttributeError` for one that is not a `Transport`. Going through `_make_port` rather than calling `AppType.get` directly is what keeps #2287's bool rejection: `AppType.get(True, proto=tcp)` resolves `True` as port 1 and answers `<TCP.tcpmux: 1 [tcp]>`, measured. * `DatagramID`'s ports stay bare `int`s, and its docstring now says why: it is the flow identity split out of `BufferID`, a dict key every toolkit adapter builds from a plain port number and which is dumped as part of the report. * Docs entries for `TCP._make_ports` and for `AppType.__hash__`, both now named in public prose (#1940). All 65,536 ports resolve to a member -- an assignment to its canonical service, an IANA span to that span's label, anything else to an unregistered `unknown` -- none raise and the registry does not grow, so this adds no way for reassembly to raise. For a registered port the member is the one `TCP.src` reads off a parsed header; for an unassigned one the two are equal but not identical and named apart, which the docstrings record rather than overstate. UDP and SCTP need no change: neither has a reassembler, so `Transport.analyze` has no caller but `TCP.submit`. Make an unregistered member survive pickling -------------------------------------------- Dispatching on members rather than numbers put them inside the `Deferred` that `TCP.submit` stores as a `Datagram`'s `packet`, and that made an unresolved datagram unpicklable for most ports. `AppType.get` answers a port it holds no assignment for through `AppType._unregistered_member`, which builds a member deliberately absent from every one of the registry's lookup tables so the lookup does not grow it (#860). `Enum.__reduce_ex__` reduces a member to `(cls, (value,))` -- a value-keyed lookup, i.e. the one table such a member is missing from -- so it pickled fine and then raised on read-back: ValueError: 'unknown [50000 - tcp]' is not a valid TCP Measured before the fix: 59,413 of the 65,536 possible TCP ports failed to round-trip, 90.7% of the space, and not merely the truly unregistered `unknown` ones but every IANA-span label too. All four sample captures with TCP flows failed, every datagram, because a client socket is ephemeral. * `EnumRegistry._make_reducible` installs a per-instance `__reduce_ex__` that rebuilds through `cls._unregistered_member(*args)` instead of through a value lookup, with module-level `_rebuild_unregistered_member` / `_reduce_unregistered_member` so that pickle protocols below 4 can name the callable. This is the technique `EnumField._unregistered_member` already uses for the same class of problem on the *parse* path (#575, #1774). * `AppType._unregistered_member` opts in, reducing to its own `(value, name, proto)` rather than to the crafted `'svc [port - proto]'` string it hands its base -- reducing to the latter would rebuild a member whose value had been formatted twice. It is opt-in per registry for exactly that reason; a registry whose `_missing_` calls the base method directly is unchanged. * Rebuilding re-enters `_unregistered_member` and never `cls.__new__`, so an unpickled member is unregistered just as the original was and no table grows -- `__members__`, `_value2member_map_` and `__registry__` all measured unchanged before and after, on every transport. A restored member is itself reducible, so a datagram read back from a dump can be written out again. After: all 65,536 ports round-trip on every transport (262,144 members), on every pickle protocol 0-5, under `copy.copy`/`copy.deepcopy`, and all four named captures load back -- 231 deferred datagrams across `http.pcap`, `stream.pcap`, `tcp.pcap` and `http6.cap`. A registered member still unpickles to the identical shared singleton, which pins that the reducer went on the unregistered path only. * `tests/const/test_const_apptype_unregistered_pickle_2417_unit.py` pins the member-level round trip over the whole port space, every protocol, and the no-growth property. * `tests/foundation/reassembly/test_datagram_deferred_pickle_2417_unit.py` pins the symptom on a synthesised datagram, unassigned and assigned alike. `test_datagram_deferred_pickle_2417_runtime.py` pins the same thing end to end over the four sample captures, split into its own fixture-dependent module because CI's unit-tests leg never generates them -- the unit file reading a generated capture directly is what broke that leg. Both scope to pickle protocols 2-5: `Deferred` declares `__slots__` and no `__getstate__`, which protocols 0 and 1 cannot serialise at all. That is pre-existing and independent of these members -- it reproduces for an assigned flow and for a bare `Deferred(..., 42, b'x')` holding a plain `int` -- so it is recorded there rather than worked around, and wants its own issue. * The #1774 canary asserted that `get`'s miss member was *not* picklable, which was a defect rather than a distinction. It now asserts the round trip works and that the member's `name` is the only remaining difference from what a parse returns -- which is what still justifies the peek. `AppType.__hash__` gained the docstring its new `#1940` entry requires. mypy over `pcapkit/` is byte-identical to mainline (98 errors in 32 files, same set); `pylint` with the Makefile flags reports the same message set and counts over the touched files as the pre-review revision; `isort -l100 -ppcapkit` is clean. Tests: 876 tests / 25,888 subtests over `foundation/reassembly`, `protocols/transport`, the `const` registries, the field-pickling suites, the `#1940` docs guard and the docstring contract, with 4 skips that predate this branch (the local venv has sphinx-autodoc-typehints 3.13.6, not the 3.17.2 `docs/requirements.txt` pins). Reassembly over the sample captures resolves the same next layer as before, port for port.
2365640 to
4466f52
Compare
JarryShaw
added
review: running
JarryShaw
added
review: good-to-go
|
Review: GOOD TO GO at Everything else independently re-confirmed at this head: diff vs the prior head is only the two test files with an unchanged parent; CI-break reproduced and fixed (old |
…evel tuple The AST audit in tests/_tiers.py only recognised a generated-capture read as a literal argument to tests._support.sample_path(...). It missed the indirection PR #2421's first revision hit: a module-level CAPTURES tuple read in a for loop whose body joined SAMPLE_ROOT / name directly, bypassing sample_path(). The guard's own static pass never flagged it, which is the defect this fixes -- CI's unit-tests leg did fail on that revision, on every affected Python interpreter, because the capture genuinely was not on disk; it only passed locally where make_samples.py had already been run. - tests/_tiers.py: add sample_root_join_calls(), which follows a SAMPLE_ROOT join back to a module-level name bound to a single string, or to a for loop's target over a module-level (or inline) tuple/list of strings. audit_module() now merges these findings with the existing sample_path_calls() ones, in source order. Deliberately narrow, per the module docstring's existing rationale for rejecting a blanket ".pcap literal" rule: no new false positive across the real suite (verified). - tests/test_tier_guard.py: new AuditTests cases pin the loop and direct-join shapes being flagged, and that a committed capture, a try/except-handled read, and a for loop over sorted(CAPTURES) (a module-level tuple wrapped in a call, rather than read bare) stay unflagged. Verified: reverting the tests/_tiers.py change makes the three new "is flagged" tests fail (0 findings where 1-2 are expected, plus an AttributeError for the new function); restoring it makes them pass. Full tests/test_tier_guard.py + test_tier_guard_xdist.py + test_capture_tracking.py suite green (156 passed, 3 skipped), and a whole-suite audit_module() sweep over every unit-tier module on disk still reports zero findings. mypy and pylint report no new findings against either file. Closes #2426
JarryShaw
removed
the
review: good-to-go
…evel tuple (#2427) The AST audit in tests/_tiers.py only recognised a generated-capture read as a literal argument to tests._support.sample_path(...). It missed the indirection PR #2421's first revision hit: a module-level CAPTURES tuple read in a for loop whose body joined SAMPLE_ROOT / name directly, bypassing sample_path(). The guard's own static pass never flagged it, which is the defect this fixes -- CI's unit-tests leg did fail on that revision, on every affected Python interpreter, because the capture genuinely was not on disk; it only passed locally where make_samples.py had already been run. - tests/_tiers.py: add sample_root_join_calls(), which follows a SAMPLE_ROOT join back to a module-level name bound to a single string, or to a for loop's target over a module-level (or inline) tuple/list of strings. audit_module() now merges these findings with the existing sample_path_calls() ones, in source order. Deliberately narrow, per the module docstring's existing rationale for rejecting a blanket ".pcap literal" rule: no new false positive across the real suite (verified). - tests/test_tier_guard.py: new AuditTests cases pin the loop and direct-join shapes being flagged, and that a committed capture, a try/except-handled read, and a for loop over sorted(CAPTURES) (a module-level tuple wrapped in a call, rather than read bare) stay unflagged. Verified: reverting the tests/_tiers.py change makes the three new "is flagged" tests fail (0 findings where 1-2 are expected, plus an AttributeError for the new function); restoring it makes them pass. Full tests/test_tier_guard.py + test_tier_guard_xdist.py + test_capture_tracking.py suite green (156 passed, 3 skipped), and a whole-suite audit_module() sweep over every unit-tier module on disk still reports zero findings. mypy and pylint report no new findings against either file. Closes #2426
…nst as a commit-type * pcapkit/protocols/protocol.py: flip the 11 remaining unqualified `ProtocolBase.<member>` cross-references to `Protocol.<member>`, display and target both, matching the mechanical flip #2420/#2422/#2428 already did for 37+ other sites. The `__init_subclass__` sites and `register`'s `Raises:` clause are untouched, per the existing #2316/#1016 carve-out. * pcapkit/protocols/transport/transport.py: same flip for the `ProtocolBase._lookup_registry` reference #2421 added after #2422's sweep had already run, so it was left behind. * CONTRIBUTING.md, .github/PULL_REQUEST_TEMPLATE.md: add `const` to the valid commit-type list and PR template checkbox. process.rst already documented `const` as a Type label; CONTRIBUTING.md and the template had not caught up. `tests/project/test_pr_template_commit_types_1102_unit.py` derives its expected set from CONTRIBUTING.md, so no test change was needed. * tests/project/test_conventions_doc_claims.py: drop the now-false "const has no tickbox" example from test_the_page_names_every_commit_type_the_template_ticks's docstring -- the template gained one above -- and restate why the check is one-way without relying on that example. * docs/source/conf.py's two "Sphinx 9.1" comments were re-verified against the pinned toolchain (Sphinx 9.2.0, per docs/requirements.txt) by instrumenting each code path directly: autodoc still hands `is_enum_member` the plain int for an enum member, and still prefers a pass-through `__new__` over `__init__` for a class signature. Both comments are accurate at 9.2.0 and left as is. Prose-only: an AST-structure diff (docstrings stripped) against origin/main is identical for both touched .py files. `make -C docs html` under the pinned toolchain is warning-for-warning identical to a main build (1 pre-existing, unrelated `TransType` ambiguity). `tests/project/` is failure-for-failure identical to main (234 failed/11 errors, all pre-existing and environmental, unrelated to this change; 2010 passed either way).
…nst as a commit-type (#2431) * pcapkit/protocols/protocol.py: flip the 11 remaining unqualified `ProtocolBase.<member>` cross-references to `Protocol.<member>`, display and target both, matching the mechanical flip #2420/#2422/#2428 already did for 37+ other sites. The `__init_subclass__` sites and `register`'s `Raises:` clause are untouched, per the existing #2316/#1016 carve-out. * pcapkit/protocols/transport/transport.py: same flip for the `ProtocolBase._lookup_registry` reference #2421 added after #2422's sweep had already run, so it was left behind. * CONTRIBUTING.md, .github/PULL_REQUEST_TEMPLATE.md: add `const` to the valid commit-type list and PR template checkbox. process.rst already documented `const` as a Type label; CONTRIBUTING.md and the template had not caught up. `tests/project/test_pr_template_commit_types_1102_unit.py` derives its expected set from CONTRIBUTING.md, so no test change was needed. * tests/project/test_conventions_doc_claims.py: drop the now-false "const has no tickbox" example from test_the_page_names_every_commit_type_the_template_ticks's docstring -- the template gained one above -- and restate why the check is one-way without relying on that example. * docs/source/conf.py's two "Sphinx 9.1" comments were re-verified against the pinned toolchain (Sphinx 9.2.0, per docs/requirements.txt) by instrumenting each code path directly: autodoc still hands `is_enum_member` the plain int for an enum member, and still prefers a pass-through `__new__` over `__init__` for a class signature. Both comments are accurate at 9.2.0 and left as is. Prose-only: an AST-structure diff (docstrings stripped) against origin/main is identical for both touched .py files. `make -C docs html` under the pinned toolchain is warning-for-warning identical to a main build (1 pre-existing, unrelated `TransType` ambiguity). `tests/project/` is failure-for-failure identical to main (234 failed/11 errors, all pre-existing and environmental, unrelated to this change; 2010 passed either way).
Please follow the guide below
make pylint,make mypy,make isort)make testpasses, and a test case covers the changedocs/source/changelog/and regeneratedCHANGELOG.md, if the change is user-visible — N/A, centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657What is the purpose of your pull request?
fix— corrects a defectDescription of your pull request and other information
Follow-up to #2418 (#2417), which made
Deferredgeneric but left the TCP constraint astuple[int, int]. Your note on it — we are still using int tuples for transport ports, let's move toAppTypetuples — is what this does.TCP.submitresolvesbufid[1]/bufid[3]throughTransport._make_portbefore building theDeferred(newTCP._make_ports), so a deferred analysis dispatches on the sameAppTypememberTCP.src/TCP.dstalready read off a parsed header._KT's second constraint andTransport.analyzeboth becometuple[AppType, AppType].Unregistered ports. Measured:
AppType.getanswers every port a 16-bit field can hold — an assignment with its canonical service (443→https), an IANA span with that span's label (0→reserved), anything else with an unregisteredunknown(50000,65535) — neverNone, without growing the registry (#860). It rejects only a port outside0-65535, a non-int, or abool(#2287), none of which an adapter can read out of a TCP header. So this adds no way for reassembly to raise.Three judgement calls for your eye.
Transport.analyzeis in v1.4.1, so the annotation is tightened without a new rejection: the chosen port is tested against__proto__as it arrived and only that one goes throughint(), so the registry is read with theintkey it declares and a bare-intcaller keeps working (test_transport_unit.pyneeded no edit). Labelledbreakingfor the annotation and because a custom__protocol_type__now receives members — drop it if you read this as a typing refinement._make_portsreaches_make_portthroughTCP, not throughself.protocolwhere the analyser comes from:__protocol_type__is an extension point whose analyser need only defineanalyze(the reassembly tests' own stubs do exactly that), soself.protocol._make_portwould be anAttributeError.DatagramID's ports stay bareints, with a docstring saying why — it is the flow identity split out ofBufferID, a dict key every toolkit adapter builds from a plain port number and which is dumped as part of the report, where the deferred pair is a dispatch key. Say the word if you want it moved too.UDP and SCTP need no change — neither has a reassembler, so
Transport.analyzehas no caller butTCP.submit. One pre-existing oddity left alone:SCTPinheritsTransport.analyzebut keys__proto__by PPID, so a directSCTP.analyze((src, dst), ...)searches the wrong registry. Nothing calls it; say if you want an issue.Verification.
mypyoverpcapkit/byte-identical to mainline (98 errors, diffed line for line).pylintover the four touched files reports the same two pre-existing_decode_next_layermessages as mainline, measured in a scratch worktree at867616b8;isort -l100 -ppcapkitclean. Tests: reassembly + transport 412/1408 subtests,interface const dumpkit cli vendor702/27798, the annotation-name/tier/docstring/public-API guards 204/3371. The full local CI selection (pytest --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py') is green on the pushed commit: 6552 passed, 112 skipped, 69451 subtests. Reassembly over the sample captures resolves the same next layer as before, port for port. Revertingsubmitto bare ints fails four of the new cases plus thetcpsubtest oftest_declared_type_admits_what_each_reassembler_passes; reverting only_KTfails that subtest alone.Review round at
d323feef→5c38fc0ba. Applied, with thanks: the ports are now typed as the per-transport registrypcapkit.const.reg.apptype.tcp.TCPrather than the baseAppType(confirmedtype(...) is TCPfor a registered and an unassigned port alike) —Transport.analyzekeeps the base, being the one method TCP/UDP/SCTP share, and now says so; the bool rejection is correctly attributed to_make_port, not toAppType.get, which really does answerAppType.get(True, proto=tcp)with<TCP.tcpmux: 1 [tcp]>(that is why_make_portsgoes through_make_port); the "same memberTCP.srcreads" claim is qualified — for an unassigned port the two paths build equal, differently-named members ('<unassigned>'vs'unknown'), now pinned by a test; docs entries added forTCP._make_portsandAppType.__hash__. Also fixed, unprompted from the same note:analyze((None, None))and(54321, '80')fall back toRawas on mainline again, instead of raising / dispatching.The pickle regression IS now fixed, in this PR. The earlier round deliberately deferred it as a pre-existing registry hole out of scope here; a cross-review pushed back with the exhaustive measurement (90.7% of ports, not just the
unknownsubset) and it landed instead:EnumRegistry._make_reducible(pcapkit/corekit/enum.py) installs a per-instance pickle reducer for dynamically-created unregistered members, mirrored into the vendor template somake vendorwon't revert it, opted into byAppType._unregistered_memberwith its own constructor args. Verified: 0 pickle failures across all 65,536 ports × 4 transports (TCP/UDP/SCTP/DCCP), registry provably unchanged, no ripple into the other 122EnumRegistrysubclasses that don't opt in.CI-break fix. The sample-capture round trip test (
SampleCaptureDatagramPickleTests) read generated/gitignored captures directly from a_unit.pymodule; CI's unit-tests leg never runsmake_samples.pyfor that tier, so it failed on every Python leg though it passed locally. Moved totest_datagram_deferred_pickle_2417_runtime.py(ALLtier, matching the precedent attest_ipv6_cut_non_final_fragment_1567_runtime.py) — verified failing without captures present and passing with them, both before and after this move.Verification after both rounds. mypy over
pcapkit/still byte-identical to mainline (98 errors, same set); pylint unchanged; isort clean.tests/foundation/reassembly157 passed with captures generated (151 under CI's selection without them); base is 145, delta is the 12 test functions this PR adds (4 pickle round-trip, 8 proto-type). Full pickle sweep 262,144 members / 1,572,864 round trips, 0 failures. The full CI selection is green on the pushed commit.