Skip to content

fix(arp): non-IP protocol addresses pack as ASCII text, and hlen/plen are never checked - #1145

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1139-arp-proto-addr
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1139-arp-proto-addr

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — ran pylint/isort/mypy on arp.py only: no new findings vs main; not the full make targets
  • make test passes, and a test case covers the change — modules listed below only, not the full suite
  • Added a changelog entry — N/A, added centrally after the wave

What is the purpose of your pull request?

  • fix — corrects a defect

Closes #1139. A non-IP str protocol address now packs via bytes.fromhex, matching #1137's hardware-address fix, and non-hex input raises ProtocolError. make() derives hlen/plen from the packed sha/spa when they are omitted. It raises ProtocolError when any address disagrees with its length field. test_link_unit.py pinned 'raw' → b'raw'; it now pins hex → octets.

Probe (before → after): ARP(ptype=0x88B5, plen=2, spa='c000') reads back 6330 → c000. ARP(hlen=4) truncated silently to 24 bytes → ProtocolError. ARP(ptype=IPv6, spa='::1') wrote plen=4 with a truncated address → plen=16, 52 bytes.

Tests: in the new tests/protocols/link/test_arp_proto_addr_length_unit.py, 9 fail without the fix and all pass with it. The 3 that pass either way are the IPv4/IPv6 round-trip and explicit-length guards. All of these pass: test_link_unit 24, test_arp_pcapng_hwaddr_octets_unit 6, test_layer_placement_unit 17, test_rarp_unit 1, test_option_roundtrip_unit 6, test_generated_pcap_runtime 2, test_dispatch_reachability_unit 3, tests/project 385 (1 skipped).

… are never checked

- _make_proto_resolve takes a str protocol address of a non-IP ptype as the
  hex string _read_proto_resolve returns, packs it with bytes.fromhex (as
  #1137 does for hardware addresses), and raises ProtocolError on non-hex.
- make() derives hlen/plen from the packed sha/spa when omitted, and raises
  ProtocolError when any packed address disagrees with its length field,
  instead of letting the schema truncate it.
- test_link_unit pinned 'raw' packing to b'raw'; it now pins hex to octets.

Closes #1139
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on c2acac1e0: GOOD TO GO (ran on Sonnet; author Opus)

  • Default output is unchanged: ARP(), ARP().pack(oper=1) and RARP().pack(oper=3) are byte-identical to main. The only in-repo callers, examples/generators/dispatch.py:283 and :291, are unaffected. make() is keyword-only, so the default changing from 6/4 to derived cannot break a positional caller.

  • Bugs reproduced on main, fixed here:

    Input main This PR
    non-IP spa='c000' packs ASCII 6330 packs c000
    hlen=4 silently truncated ProtocolError
    IPv6 28 bytes 52 bytes
  • Mismatch checks: a wrong length on each of sha, tha, spa and tpa raises its own ProtocolError.

  • Round trips: IPv4, IPv6, non-IP 0x88B5 and Frame Relay hardware all round-trip.

  • Tests: with the fix reverted, the new module fails 9.

  • mypy: the PR adds no errors; the same 4 arg-type errors are on main.

Labelled breaking: pack-path output changes. Non-IP str addresses now pack as octets, non-hex input raises, mismatched hlen/plen raises, and IPv6 output grows from 28 to 52 bytes.

@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.75% (unit tier, Python 3.14, c2acac1e0, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1035 2342 846 90.23%
pcapkit/corekit 1873 91 578 22 94.33%
pcapkit/dumpkit 136 0 40 0 100.00%
pcapkit/foundation 2425 145 842 34 92.56%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15671 188 3948 163 98.18%
pcapkit/toolkit 487 65 144 1 85.42%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 3e4507b into main Oct 6, 2026
42 checks passed
@JarryShaw
JarryShaw deleted the fix/1139-arp-proto-addr branch October 6, 2026 22:35
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 2026
JarryShaw added a commit that referenced this pull request Oct 6, 2026
- 22 new 1.5.0 entries for the 30 PRs merged since b10e795
  (#1106-#1158), with related fixes grouped per the #918 ruling;
  three marked breaking to match their labels (#1116, #1138, #1145)
- regenerate CHANGELOG.md with util/changelog_md.py
- process.rst: entry-count pin re-measured, 167 -> 189
- pep.rst: note the plist round-trip test skipped on the upstream
  dictdumper fractional-second <date> defect (DictDumper#122)

tests/project: conventions_doc_claims, changelog_md and
documentation_claims pass; changelog_md.py --check is in step.
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(arp): non-IP protocol addresses pack as ASCII text, and hlen/plen are never checked

1 participant