…d IP field value
_IPAddressField.pre_process let a bare ValueError from ipaddress.ip_address()
escape when a value was malformed, one line above where it already raises
the library's own FieldValueError for a value that is merely the wrong IP
version -- so `except BaseError` could not reliably catch a bad field value;
whether the exception was in-library depended on how the value was wrong.
The same unguarded-conversion pattern was in every pre_process/post_process
of this module. Fixes #465.
- pcapkit/corekit/fields/ipaddress.py: adds a _reraise_as_field_value_error
context manager and wraps every ipaddress.* conversion in it, preserving
the original message via `from error`. Fixes two independently-confirmed
defects: the malformed-value escape in all four public classes'
pre_process (as reported), and a second one found while checking
reachability of the post_process sites -- IPv4InterfaceField.post_process
raised the same bare ValueError when the wire's trailing four "netmask"
octets are not a contiguous netmask (e.g. 0.255.0.255), reachable through
unpack() alone. The remaining post_process sites are not reachable --
wire bytes there are always exactly the field's fixed length, and any
such octet string converts cleanly, or the value is already range-checked
before use -- but are wrapped too for consistency. Raises: sections
added/extended to match.
- tests/corekit/test_fields_ipaddress.py: new cases pin FieldValueError
(and BaseError) for a malformed value on all four public field classes,
a case for the newly found post_process netmask defect, and a case
pinning that the pre-existing wrong-version FieldValueError message is
not relabelled as a malformed-value message.
Full suite: 1009 passed, 17 skipped, 1557 subtests, at PYTHONSAFEPATH=1,
interpreter 3.14.7. Baseline at fa12895 (origin/main): 1006 passed, 17
skipped, 1553 subtests, before the 3 new tests existed. mypy (Makefile's
invocation) reports the same 125 errors in 40 files before and after --
the only ipaddress.py lines it flags are two pre-existing return-value
errors, unrelated to this change and merely shifted by the new lines.
Closes #465.
pcapkit/corekit/fields/ipaddress.pylet a bare stdlibValueErrorescape whenan address value was malformed, while the very next statement in the same method
raised the library's own
FieldValueErrorfor a value that was merely the wrongIP version. So
except BaseErrorcould not catch a bad field value: whether theexception was in-library depended on how the value was wrong.
Before / after
All four public classes, malformed value into
pre_process:The original stdlib message is preserved rather than replaced. The wrong-version
path is unchanged and still reports
IP version mismatch: 4 != 6, so the fix didnot broaden into relabelling one failure as the other — there is a test pinning
exactly that.
One site reachable from the wire, which #465 did not describe
The issue listed the
post_processsites as "same pattern, reachability notdemonstrated". One of them is reachable, and from wire bytes alone rather
than from a malformed caller argument:
IPv4InterfaceField.post_processbuildsip_interface(f'{ip}/{mask}')from thetrailing four octets as a netmask. If those octets are not a contiguous netmask
—
0.255.0.255here —ip_interface()raisesNetmaskValueError, aValueErrorsubclass. The input is a well-formed eight octets, so this needs no malformed
length and no caller error: a capture containing such a field raises a bare
stdlib exception out of the parse path. That makes this issue more serious than
filed.
Reachability, judged per site
Reachable and demonstrated:
_IPAddressField.pre_process; both interfacepre_processmethods; andIPv4InterfaceField.post_process'sip_interfacecall above.
Judged not reachable, with reasoning: the
ip_address/IPv4Address/IPv6Addressconversions in thepost_processmethods, becausevaluethere isalways exactly 4 or 16 octets — fixed by
struct.unpack's'4s'/'16s'template, which pads or truncates before this code runs — and every such octet
string converts cleanly (checked with all-zero, all-
0xffand sequential bytepatterns). And
IPv6InterfaceField.post_process'sip_interfacecall, whichonly runs after the existing
prefixlen > 128guard, and every prefix length in0..128is valid.All sites are wrapped regardless, reachable or not — uniform treatment, on the
grounds that the asymmetry between adjacent lines is precisely what caused this
bug.
How it is wrapped
A module-level
@contextmanagerhelper,_reraise_as_field_value_error, ratherthan eight inline
try/exceptblocks. Eachwithblock spans only the rawipaddress.*conversion, never a followingraise FieldValueError(...), so there-wrapping trap cannot arise by construction; the helper additionally puts
except FieldValueError: raiseahead ofexcept ValueErroras a guard against afuture edit widening a block, with an inline note pointing at the same trap
ProtocolErrorcarries.Raises:sections were added or extended on all six touched methods, includingfor pre-existing
FieldValueErrorraises that were previously undocumented. TheSphinx page uses
autoclass :members:, so no.rstchange is needed.Verification
tests/corekit/test_fields_ipaddress.py, each confirmed tofail against the reverted source and pass against the fix.
tests/protocols/test_option_roundtrip_unit.pyuntouched.after 1009 / 17 / 1557 — exactly the three new tests. Zero failures either side.
directly. The only movement is two pre-existing
[return-value]errors shiftingline number because of added lines.
Related, deliberately not fixed
Both interface
post_processmethods return the result ofipaddress.ip_interface(...),typed
IPv4Interface | IPv6Interface, against declared return types ofIPv4InterfaceandIPv6Interfacerespectively — the two pre-existing mypy[return-value]errors mentioned above. Pre-existing onmainand unrelated tothis issue, so filed separately rather than folded in.
Corrections to this description
Three things above are wrong or understated. The review found the first two; I
verified all three. Leaving the original text in place because the review cites
it.
1. The padding is not
struct.unpack's doing. The reachability sectioncredits "
struct.unpack's'4s'/'16s'template, which pads or truncates". Itdoes not — the padding happens one level out, in plain Python, at
pcapkit/corekit/fields/field.py:244:buffer[:length]truncates and.rjust(length, b'\x00')left-pads, both beforestruct.unpacksees anything. The conclusion is unaffected, and the reviewestablished it more strongly than this description claimed: address fields were
fed buffers of length 0, 1, N-1, N, N+1 and N+10 through
.unpack()and nonefailed — so the
post_processconversions are unreachable for any bufferlength, not merely for well-formed wire bytes.
2. "Each was confirmed to fail against reverted source" is true for two of the
three tests, not all three.
test_pre_process_malformed_value_raises_in_library_errorand
test_ipv4_interface_post_process_rejects_a_non_contiguous_netmaskdo failbefore and pass after.
test_wrong_version_message_is_not_relabelled_as_a_malformed_valuepasses unchanged against the reverted source — it pins pre-existing,
untouched behaviour, which is a legitimate thing for it to do, but it is a
guard rather than a regression test and this description should not have implied
otherwise.
3. The non-contiguous-netmask case is near-universal, not a corner. Described
above only as "a non-contiguous netmask", which undersells it: of 2000 random
four-byte masks, 0 were contiguous and 2000 raised. Only the 33 canonical
contiguous masks out of 2^32 possible byte patterns avoid it, so essentially any
mask field that is not a real netmask reaches the defect this PR fixes.