Skip to content

fix(arp): reject bool and non-bytes addresses for non-IP ptypes (#2166) - #2182

Merged
JarryShaw merged 1 commit into
mainfrom
fix/2166-arp-non-ip-address
Oct 25, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/2166-arp-non-ip-address

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 25, 2026 •

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description

Closes #2166.

  • Defect: ARP._make_proto_resolve rejected a bool only on the IPv4/IPv6 branches. For any other ptype, a bool or int came back unchanged, and make() failed with TypeError: object of type 'bool' has no len().
  • Defect, hardware side: ARP._make_addr_resolve has the same gap. On Ethernet a bool or int dies in the MAC regex (TypeError: expected string or bytes-like object). On any other htype it dies with TypeError: … has no len(), and a list dies with a raw struct.error.
  • Fix: in both resolvers, a bool raises FieldValueError before the type dispatch, bytes/bytearray pack as bytes, and anything else raises ProtocolError. Every input that worked on main still packs the same octets. The Raises: sections match.
  • Tests: TestARPNonIPAddressType and TestARPHardwareAddressType in tests/protocols/link/test_arp_proto_addr_length_unit.py. The latter includes test_supported_forms_still_pack, which pins the forms that worked on main. Without the fix, 19 fail.
  • Tests run: tests/protocols/link/ plus the ARP pcapng, short-read round-trip and docstring-contract tests pass (131), as do the project tests that read ARP (38).
  • Overlap: docs(protocols): fifth whole-repo pass over link and transport protocols (#1103) #2152 rewrites the same Raises: block. Whichever merges second needs a docstring-only rebase.

@JarryShaw JarryShaw added the bug Issues reporting a defect (set by the bug report template; a default, not an assessment) label Oct 25, 2026
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 25, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 25, 2026
@JarryShaw
JarryShaw force-pushed the fix/2166-arp-non-ip-address branch from b22f339 to 0f7af9c Compare October 25, 2026 07:38
@JarryShaw

Copy link
Copy Markdown
Owner Author

Review: GOOD TO GO at 0f7af9c4d (Sonnet): delta on the hardware resolver: of 28 inputs, the 12 that worked on main give the same output, only crashing inputs changed (bool → FieldValueError, others → ProtocolError), and 19 fail on main.

@JarryShaw JarryShaw added 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 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 91.55% (unit tier, Python 3.15, 0f7af9c4d, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19599 880 2618 866 92.12%
pcapkit/corekit 2589 75 816 37 96.48%
pcapkit/dumpkit 282 3 102 3 98.44%
pcapkit/foundation 2971 23 1036 25 98.80%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17428 219 4584 185 98.15%
pcapkit/toolkit 869 36 298 10 95.20%
pcapkit/utilities 504 7 146 5 98.15%
pcapkit/vendor 4432 1988 1018 167 51.87%

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

@JarryShaw

Copy link
Copy Markdown
Owner Author

Ready to merge at 0f7af9c4d. Merging closes #2166.

  • Review: Sonnet GOOD TO GO, including the delta covering the hardware resolver.
  • CI: every check has completed, Required checks passed SUCCESS, 0 failed.
  • KTLO: 12 legs, no warnings.
  • Ordering: the protocols/link leg, merged into main: 337 tests, 0 failures.
  • Overlap: docs(protocols): fifth whole-repo pass over link and transport protocols (#1103) #2152 rewrites the same Raises: block in arp.py. Whichever lands second gets a docstring-only rebase.

Problem: ARP._make_proto_resolve rejected a bool only on the IPv4/IPv6
branches; for any other ptype a bool or int was returned unchanged and
make() failed with "TypeError: object of type 'bool' has no len()".
ARP._make_addr_resolve had the same gap: a bool or int sha/tha hit the
MAC regex as a TypeError for Ethernet, and fell through to len() or a raw
struct error for any other htype.

Fix: in both resolvers, reject a bool with FieldValueError before the
dispatch, keep every str/bytes/bytearray form that already worked, return
bytes/bytearray as bytes, and raise ProtocolError for anything else.

Test: TestARPNonIPAddressType and TestARPHardwareAddressType in
test_arp_proto_addr_length_unit.
@JarryShaw

Copy link
Copy Markdown
Owner Author

Rebased onto main after #2152 and #2153 merged: 0f7af9c4d → f7c0c7921.

  • Conflict: one hunk, in ARP._make_proto_resolve's Raises: FieldValueError entry, where both PRs rewrote the same lines. I kept both: a bool is rejected whatever the ptype (this PR), and an IP-packed address that isn't valid or is the wrong version is rejected too (docs(protocols): fifth whole-repo pass over link and transport protocols (#1103) #2152).
  • Everything else is unchanged. git range-diff shows only that docstring hunk moving.
  • Tests: tests/protocols/link plus the docstring contract pass on the new head (116).
  • Old head: Required checks passed SUCCESS, 12 KTLO legs with no warnings.

Because the patch changed in that docstring, CI has to run again. A short delta review is running.

@JarryShaw
JarryShaw force-pushed the fix/2166-arp-non-ip-address branch from 0f7af9c to f7c0c79 Compare October 25, 2026 13:57
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 25, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Review: GOOD TO GO at f7c0c7921 (Sonnet rebase check): range-diff differs only in the merged Raises: hunk, every input I probed behaves as the combined docstring says, and the link tests and the docstring contract pass. The ready note follows once CI completes on this head.

@JarryShaw JarryShaw added 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 25, 2026
@JarryShaw
JarryShaw merged commit deec032 into main Oct 25, 2026
64 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 25, 2026
@JarryShaw
JarryShaw deleted the fix/2166-arp-non-ip-address branch October 25, 2026 14:43
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 25, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(arp): a bool or int protocol address with a non-IP ptype raises TypeError, not FieldValueError

1 participant