Follow-up to #508. PR #539 adds parse_ip_address at pcapkit/corekit/fields/ipaddress.py:115 and routes seven make-path call sites in hip.py, mh.py and tcp.py through it. Four more sites have the identical defect and were deliberately left out of that PR, because they live in files another change owned at the time.
Filing them together rather than one at a time on purpose: #481 fixed a single site, which is how this survived to become #491 and then #508. Piecemeal is the failure mode here.
All measurements below are on main at 122d32795, repo venv, PYTHONSAFEPATH=1, with pcapkit.__file__ asserted to be the repository tree.
1. ARP._make_proto_resolve — pcapkit/protocols/link/arp.py:400,402
if ptype == Enum_EtherType.Internet_Protocol_version_4:
return ipaddress.IPv4Address(addr).packed
if ptype == Enum_EtherType.Internet_Protocol_version_6:
return ipaddress.IPv6Address(addr).packed
bool is a subclass of int, so True converts to 0.0.0.1 and packs silently:
ARP._make_proto_resolve(True, IPv4) -> 00000001
ARP._make_proto_resolve('1.2.3.4', IPv4) -> 01020304
2. IPv6_Route._make_data_type_rpl — pcapkit/protocols/internet/ipv6_route.py:737,745,753,764,768
Worse than the ARP case, because the compression metadata is derived from the laundered value, so a bool corrupts cmpr_e and the address list together:
_make_data_type_rpl(dst=2001:db8::1, ip=[True]) -> cmpr_i=16 cmpr_e=0 addresses=['00000001']
_make_data_type_rpl(dst=2001:db8::1, ip=['2001:db8::2']) -> cmpr_i=16 cmpr_e=15 addresses=['02']
3. IPv6_Route.make parameter dst — pcapkit/protocols/internet/ipv6_route.py:314
dst_val = cast('IPv6Address', ipaddress.ip_address(dst)) if dst is not None else None
Same shape, on the public make entry point rather than a private helper, so it is the most reachable of the four.
4. OSPF._make_id_numbers — pcapkit/protocols/link/ospf.py:328
return ipaddress.ip_address(id).packed
Latent rather than live: grep -rn '_make_id_numbers' finds the definition plus exactly one reference, a test at tests/protocols/link/test_link_unit.py:856. No production caller, so nothing reaches it today. Worth fixing with the others so it does not become live later. Note ospf.py:316 is a second bare conversion in the same file and should be triaged in the same pass.
Why the read path is not affected
Only the make path is exposed. A read-path conversion receives bytes straight off the wire and cannot be handed a bool by a caller, so the ~30 other bare ipaddress.* calls under pcapkit/protocols/ are not in scope here. A sweep did turn up further make-path candidates that want triage rather than assumption — pcapkit/protocols/internet/esp.py:575, pcapkit/protocols/schema/internet/ipv4.py:278,293,308, and pcapkit/protocols/internet/hip.py:3072,3077 — and I have not confirmed any of those reproduce. They should be checked as part of this change rather than taken on faith.
Suggested fix
Route all four through the parse_ip_address helper that #539 introduces, which calls the existing _reject_bool. The helper is already the established pattern by the time this is picked up, so this is mechanical rather than a design question.
Coverage
Each site needs a test asserting a bool is rejected, and each test must be shown to fail without its fix. For site 2 the assertion should cover the derived cmpr_e and addresses as well as the exception, since that is where the corruption is worst and an exception-only test would not have caught the metadata damage.
Depends on #539 landing first, since it introduces the helper.
Follow-up to #508. PR #539 adds
parse_ip_addressatpcapkit/corekit/fields/ipaddress.py:115and routes seven make-path call sites inhip.py,mh.pyandtcp.pythrough it. Four more sites have the identical defect and were deliberately left out of that PR, because they live in files another change owned at the time.Filing them together rather than one at a time on purpose: #481 fixed a single site, which is how this survived to become #491 and then #508. Piecemeal is the failure mode here.
All measurements below are on
mainat122d32795, repo venv,PYTHONSAFEPATH=1, withpcapkit.__file__asserted to be the repository tree.1.
ARP._make_proto_resolve—pcapkit/protocols/link/arp.py:400,402boolis a subclass ofint, soTrueconverts to0.0.0.1and packs silently:2.
IPv6_Route._make_data_type_rpl—pcapkit/protocols/internet/ipv6_route.py:737,745,753,764,768Worse than the ARP case, because the compression metadata is derived from the laundered value, so a bool corrupts
cmpr_eand the address list together:3.
IPv6_Route.makeparameterdst—pcapkit/protocols/internet/ipv6_route.py:314Same shape, on the public
makeentry point rather than a private helper, so it is the most reachable of the four.4.
OSPF._make_id_numbers—pcapkit/protocols/link/ospf.py:328Latent rather than live:
grep -rn '_make_id_numbers'finds the definition plus exactly one reference, a test attests/protocols/link/test_link_unit.py:856. No production caller, so nothing reaches it today. Worth fixing with the others so it does not become live later. Noteospf.py:316is a second bare conversion in the same file and should be triaged in the same pass.Why the read path is not affected
Only the make path is exposed. A read-path conversion receives
bytesstraight off the wire and cannot be handed aboolby a caller, so the ~30 other bareipaddress.*calls underpcapkit/protocols/are not in scope here. A sweep did turn up further make-path candidates that want triage rather than assumption —pcapkit/protocols/internet/esp.py:575,pcapkit/protocols/schema/internet/ipv4.py:278,293,308, andpcapkit/protocols/internet/hip.py:3072,3077— and I have not confirmed any of those reproduce. They should be checked as part of this change rather than taken on faith.Suggested fix
Route all four through the
parse_ip_addresshelper that #539 introduces, which calls the existing_reject_bool. The helper is already the established pattern by the time this is picked up, so this is mechanical rather than a design question.Coverage
Each site needs a test asserting a
boolis rejected, and each test must be shown to fail without its fix. For site 2 the assertion should cover the derivedcmpr_eandaddressesas well as the exception, since that is where the corruption is worst and an exception-only test would not have caught the metadata damage.Depends on #539 landing first, since it introduces the helper.