Skip to content

protocols: real routing type in IPv6-Route diagnostics, and fix HTTP's drained explicit version= buffer - #451

Merged
JarryShaw merged 2 commits into
mainfrom
fix/ipv6-route-typeno-and-http-explicit-version
Sep 18, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
fix/ipv6-route-typeno-and-http-explicit-version

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Summary

Two small, independent, well-diagnosed defects.

#442 -- IPv6-Route diagnostics print <class 'type'>

pcapkit/protocols/internet/ipv6_route.py's three _read_data_type_* methods
(_read_data_type_src:462, _read_data_type_2:506, _read_data_type_rpl:546)
raised ProtocolError(f'... [TypeNo {type}] ...'), and none of the three
signatures binds a type parameter, so {type} resolved to the builtin
type and every message read [TypeNo <class 'type'>] instead of the routing
type number.

Before (measured on this branch before the fix):

IPv6-Route [TypeNo <class 'type'>]: invalid format
IPv6-Route: [TypeNo <class 'type'>] invalid format
IPv6-Route: [TypeNo <class 'type'>] invalid format

Fix: use header.type, which is already in scope with the value wanted
(the read dispatch at :211 looks the handler up by schema.type and calls
meth(schema.data, header=schema) -- no signature change needed). Also
normalises _read_data_type_src's punctuation from alias [TypeNo x]: ... to
the colon-before-bracket form the other two sites, and mh.py's ten
_make_opt_* sites, already use.

After:

IPv6-Route: [TypeNo 250] invalid format
IPv6-Route: [TypeNo 250] invalid format
IPv6-Route: [TypeNo 250] invalid format

Cross-PR compatibility: #440 (merged) records these two cases in
tests/protocols/test_option_roundtrip_unit.py keyed on the substring tuple
('IPv6-Route', '[TypeNo', 'invalid format'). All three substrings are
preserved by this fix, and tests/protocols/test_option_roundtrip_unit.py was
run against this branch directly: 7 passed / 299 subtests passed, unaffected.

#447 -- HTTP.read's explicit version= path passes a drained buffer

pcapkit/protocols/application/http.py's HTTP.read's explicit version=
path built the sub-protocol from self._file, while the auto-guessing
_guess_version path (a few lines below) builds it from self._data. Once the
outer read has already consumed self._file, the explicit path gets a short
or empty buffer.

Before:

HTTP(io.BytesIO(raw), len(raw))            -> OK, alias=HTTP/1.1 length=43
HTTP(io.BytesIO(raw), len(raw), version=1) -> ValueError: not enough values to unpack (expected 2, got 1)

Fix: pass self._data on the explicit path too, matching _guess_version.
Also wraps a still-malformed payload's escaping bare ValueError in a chained
ProtocolError, so a caller can catch it as a library exception (matching
_guess_version's own raise ProtocolError("unknown HTTP version") at the end
of its fallback chain) rather than losing the original error.

After:

HTTP(io.BytesIO(raw), len(raw), version=1) -> OK, alias=HTTP/1.1 length=43  (matches the auto-guess result)
HTTP(io.BytesIO(bad), len(bad), version=1) -> ProtocolError: HTTP/1: invalid format  (chained from the original ValueError)

HTTP.make() was checked for the same class of mistake (a few lines
below, as suggested) and does not have it -- it never touches
self._file/self._data. It does have a separate, pre-existing, unrelated
bug: protocol.make(**kwargs) calls HTTPv1.make/HTTPv2.make -- both
ordinary instance methods -- unbound on the class, with no self. This raises
TypeError: HTTP.make() missing 1 required positional argument: 'self' for any
real (non-test-double) call; the existing test suite doesn't catch it because
its fakes define make as @staticmethod. Left unfixed here as out of scope
for #447, which is specifically about read().

Tests

  • tests/protocols/internet/test_ipv6_extension_unit.py:
    test_ipv6_route_read_data_type_errors_report_real_routing_type asserts the
    exact rendered message text (including the real type number 250), not
    merely that ProtocolError is raised -- a bare assertRaises(ProtocolError)
    would not have caught this defect.
  • tests/protocols/application/test_http_unit.py:
    • test_http_read_explicit_version_uses_same_buffer_as_guess pins which
      buffer object (self._data, not self._file) reaches the sub-protocol
      constructor, for both version=1 and version=2.
    • test_http_read_explicit_version_1_matches_guess_on_real_bytes is the
      literal issue reproduction: the explicit and auto-guessed paths now parse
      identically.
    • test_http_read_explicit_version_wraps_malformed_payload asserts the
      chained ProtocolError.

All four new tests were verified to fail before their respective fix and pass
after, by toggling each fix in isolation.

Test plan

  • PYTHONSAFEPATH=1 + repo-root PYTHONPATH + pcapkit.__file__ printed
    to confirm the tree under test, per this repo's measurement convention.
  • Reproduced both issues verbatim before the fix, on this branch.
  • Full suite: 927 passed / 20 skipped / 1248 subtests passed on
    Python 3.14.7, all optional runtime deps present, sample captures
    regenerated via python examples/generators/make_samples.py.
  • mypy --config-file mypy.ini clean on both changed modules.
  • tests/protocols/test_option_roundtrip_unit.py (from merged tests: round-trip 258 option codes from the registries, recording the 86 that cannot close the cycle #440) run
    directly against this branch: 7 passed / 299 subtests passed.

Closes #442
Closes #447

…top draining HTTP's explicit version= buffer

* ipv6_route.py's three `_read_data_type_*` diagnostics interpolated a bare
  `{type}`, which resolves to the builtin `type` since none of the enclosing
  methods bind a `type` parameter, so every message read
  `[TypeNo <class 'type'>]` instead of the routing type number. `header.type`
  is already in scope with the value wanted, so no signature change is
  needed. Also normalises `_read_data_type_src`'s punctuation to the
  colon-before-bracket form the other two sites and `mh.py`'s `_make_opt_*`
  family already use.
* http.py's `HTTP.read`'s explicit `version=` path passed `self._file` --
  already drained by the outer read -- to the sub-protocol constructor,
  instead of `self._data` as `_guess_version` does, so it built the
  sub-protocol from a short or empty buffer. Passes `self._data` on both
  paths, and wraps a malformed payload's bare `ValueError` escaping that path
  in a chained `ProtocolError`, matching `_guess_version`'s
  `"unknown HTTP version"` convention.
* Checked `HTTP.make()` for the same file/data mistake: it has none, but a
  separate, pre-existing bug (`protocol.make(**kwargs)` calls an instance
  method unbound on the class) is out of scope here and left unfixed.

Adds a regression test per fix: one pinning the real routing-type-number
message text (a bare `ProtocolError` assertion would not have caught this),
and two covering the explicit `version=1`/`version=2` HTTP paths against
real bytes and against a malformed payload.

Suite 927 passed / 20 skipped / 1248 subtests passed on this branch. mypy
clean on both changed modules.

Closes #442
Closes #447
Comment thread pcapkit/protocols/internet/ipv6_route.py
Comment thread pcapkit/protocols/application/http.py
@JarryShaw

Copy link
Copy Markdown
Owner Author

Review of e2a1134ff (standing in for Copilot review)

Reviewed head sha e2a1134ff47461f4e9391baabbec2d4ee0b5554e on fix/ipv6-route-typeno-and-http-explicit-version. All code was read via git show <ref>:<path> (never the ambient working tree). Fetched refs/pull/451/head and confirmed it resolves to exactly e2a1134ff... before reading anything.

CI

gh pr view 451 --json statusCheckRollup was 20 pending / 2 skipped at dispatch. Waited for it to settle: final state is 21 checks COMPLETED/SUCCESS (including CodeQL "Analyze"), 2 SKIPPED (Docs test gate, Gate full suite Python 3.14 — both conditional gates, not failures), and 1 SUCCESS status context (pyup.io/safety-ci). Zero FAILURE/ERROR/CANCELLED/TIMED_OUT checks among the 24 total.

Commits behind main

git rev-list --count e2a1134ff..origin/main = 1; git rev-list --count origin/main..e2a1134ff = 1. The PR is exactly one commit behind main, missing e2d8ed6d1 (#435, "foundation: give IP reassembly the RFC timeout, and trace TCP flows bidirectionally"). Checked that commit's diff (git show e2d8ed6d1 --stat): it touches only docs/, pcapkit/foundation/{reassembly,traceflow}/*, and pcapkit/interface/core.py — zero overlap with pcapkit/protocols/internet/ipv6_route.py or pcapkit/protocols/application/http.py. Being behind does not matter here: no merge conflict risk, no logical interaction with either fix.

#442 — IPv6-Route diagnostics

Full-file diff against origin/main confirmed only the three claimed lines changed (:462, :506, :546), each {type}{header.type}, with :462's punctuation normalised to the colon-before-bracket form the other two already use. header.type is in scope with the right value — the read dispatch (:204/:211 on main) looks the handler up by schema.type and calls meth(schema.data, header=schema), so no signature change was needed, matching the issue's diagnosis exactly.

Cross-cutting compatibility, checked directly rather than taken on faith. Ran tests/protocols/test_option_roundtrip_unit.py against e2a1134ff:

7 passed, 1 warning, 299 subtests passed in 0.79s

Exact match to the PR's claim. That file's EXPECTED_FAILURES entries for ipv6-route-type/Source_Route and ipv6-route-type/Type_2_Routing_Header record the fragment tuple ('IPv6-Route', '[TypeNo', 'invalid format'), matched via per-substring assertIn (confirmed by reading test_option_roundtrip_unit.py:847-857) rather than a literal message match — deliberately, per that file's own comment, so it survives #442 landing. All three substrings are present in the new message f'{self.alias}: [TypeNo {header.type}] invalid format', and the passing run proves it rather than just the text of the message implying it.

Fail-before/pass-after, verified by hand. Swapped origin/main's pre-fix versions of both ipv6_route.py and http.py into the worktree (leaving the new tests as the PR wrote them) and reran the four new tests: all four failed — the ipv6 one with a clean AssertionError diffing "...[TypeNo <class 'type'>]..." against "...[TypeNo 250]...", the three HTTP ones with the pre-fix ValueError/buffer-identity failures #447 describes. Restored the PR's files and reran: all four pass. These are genuine regression tests, not vacuous assertRaises(ProtocolError) checks — confirmed rather than assumed.

#447 — HTTP.read's explicit version= path

Full-file diff against origin/main confirmed only 6 lines changed in http.py, exactly the claimed self._fileself._data plus the new try/except. make() is untouched (0 diff lines in that method).

Scrutinised the new wrapper (except ProtocolError: raise / except ValueError as error: raise ProtocolError(...) from error) per the task's ask — does it catch too broadly, preserve the original, or risk converting an error a caller relies on catching as ValueError:

  • ProtocolError is defined as class ProtocolError(BaseError, ValueError) (pcapkit/utilities/exceptions.py:356) — it is already a ValueError subclass. That makes the except ProtocolError: raise clause load-bearing, not merely tidy: without it ahead of the ValueError branch, a genuine ProtocolError from deeper in the sub-protocol constructor would fall into the ValueError handler and get double-wrapped into a second, less-specific ProtocolError, losing the original message. The PR has the order right.
  • raise ProtocolError(...) from error correctly chains — reproduced by hand that ctx.exception.__cause__ is the original ValueError from httpv1.py:131's packet.split(b'\r\n\r\n', maxsplit=1), matching what test_http_read_explicit_version_wraps_malformed_payload asserts.
  • It does not catch too broadly: only ValueError, not Exception, and ProtocolError (itself a ValueError) is peeled off first and re-raised untouched.
  • On "could this now convert an error some caller relies on catching as ValueError": yes, narrowly — a caller catching a bare ValueError from the explicit-version path specifically would now need to catch ProtocolError instead (which remains a ValueError, so except ValueError still works transparently). Not a behavior break.
  • Left an inline note (non-blocking) about a pre-existing asymmetry: _guess_version only suppresses ProtocolError per attempt, not bare ValueError, so a payload that made both HTTPv1/HTTPv2 raise plain ValueError would still escape the guess path unwrapped. This diff doesn't introduce or worsen that — it makes the explicit path strictly more defensive than the guess path was before.

#452 (HTTP.make unbound-call bug)

Confirmed issue #452 is open and its description matches the PR's characterization exactly. Confirmed via the same full-file diff that this PR's changes touch only read()make() has zero diff lines, so #452 is neither fixed nor made worse here, as claimed.

Measurements

Interpreter: /local/home/jarryx/GitHub/PyPCAPKit/.venv/bin/python (3.14.7), run with PYTHONSAFEPATH=1 and PYTHONPATH set to the worktree under test. Confirmed tree: pcapkit.__file__/local/home/jarryx/GitHub/PyPCAPKit/.claude/worktrees/agent-afb6007cc9edfab40/pcapkit/__init__.py, checked out at e2a1134ff. Sample captures regenerated via examples/generators/make_samples.py before measuring (fresh worktree had none).

  • mypy --config-file mypy.ini pcapkit/protocols/internet/ipv6_route.py pcapkit/protocols/application/http.pySuccess: no issues found in 2 source files. Matches the claim.
  • Full suite, my own measurement, this sha: 930 passed / 17 skipped / 1268 subtests passed (634s non-verbose, confirmed again at 600s with -v to get a comparable subtests count). PR claims 927 passed / 20 skipped / 1248 subtests passed.
  • These numbers differ from the PR's claim, but the total collected item count matches exactly both ways (927+20 = 947 = 930+17). All 17 of my skips are optional-runtime-dependency gates (pypcap engine, pypcapfile engine, PCAP_CT engine, a plistlib-based report-format test, a pcapng decryption-secrets test) — visible by name in the verbose log. The 3-test/20-subtest gap is consistent with the PR author's environment being missing a couple of optional packages that mine has installed, not with any test actually failing anywhere in either run. Zero failures in my run. Per this repo's own convention, I'm not trusting the PR's number or my own as a cross-machine constant — reporting mine, on this sha, with the interpreter and tree pinned above.
  • Targeted runs of both modified test files (test_ipv6_extension_unit.py + test_http_unit.py): 63 passed, 71 subtests passed, 0 failed.

One claim from the task brief that does not apply

The review brief mentioned the PR body might contain an inaccurate reconciliation of an earlier "949 passed / 17 skipped" figure ("the subtests count, not the passed count"). Checked the current PR body and comments directly (gh pr view 451 --json body,comments) and grepped for "949", "subtests count", "reconcil" — none of that text is present in the PR as it currently stands, and there are zero PR comments. Nothing to flag here.

Verdict

GOOD TO MERGE at e2a1134ff47461f4e9391baabbec2d4ee0b5554e.

Both fixes match their issues' diagnoses exactly, the diffs are minimal and touch only what's described, the four new tests are genuine (independently confirmed fail-before/pass-after), the cross-cutting #440 round-trip table still passes against the new message text, mypy is clean, and CI is fully green with no failing checks. Being one commit behind main doesn't matter — the missing commit touches unrelated files. The two non-blocking observations left inline (exception-ordering note on http.py, and the _guess_version asymmetry) are informational, not blockers.

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

Labels

None yet

Projects

None yet

1 participant