Skip to content

Address fields let a bare ValueError escape for a malformed address, one line before FieldValueError is raised for a version mismatch #465

Description

@JarryShaw

Summary

pcapkit/corekit/fields/ipaddress.py lets a bare ValueError from the
stdlib ipaddress module escape when an address value is malformed, even
though the very next statement in the same method raises the library's own
FieldValueError for a value that is merely the wrong IP version. So a caller
cannot rely on pcapkit.utilities.exceptions.BaseError to catch a bad field
value: whether the exception is in-library depends on how the value is wrong.

Reproduction

Measured on 0283a6d59 (current main):

import ipaddress
from pcapkit.corekit.fields.ipaddress import IPv6AddressField
from pcapkit.utilities.exceptions import BaseError

f = IPv6AddressField()

try:
    f.pre_process('not-an-address', {})
except Exception as e:
    print(type(e).__module__, type(e).__name__, isinstance(e, BaseError))
    # builtins ValueError False

try:
    f.pre_process(ipaddress.IPv4Address('1.2.3.4'), {})
except Exception as e:
    print(type(e).__module__, type(e).__name__, isinstance(e, BaseError))
    # pcapkit.utilities.exceptions FieldValueError True

All four public field classes behave the same way for a malformed string —
IPv4AddressField, IPv6AddressField, IPv4InterfaceField and
IPv6InterfaceField each give builtins.ValueError, in-library=False.

Where

The two halves of the inconsistency are adjacent lines of the same method,
_IPAddressField.pre_process:

        if isinstance(value, (ipaddress.IPv4Address, ipaddress.IPv6Address)):
            ip = value  # type: IPv4Address | IPv6Address
        else:
            ip = ipaddress.ip_address(value)          # :71  -- bare ValueError escapes

        if ip.version != self.version:
            raise FieldValueError(...)               # :74  -- in-library

FieldValueError(BaseError, ValueError) is defined at
pcapkit/utilities/exceptions.py:372, and the module already imports it at
ipaddress.py:9 — so the intent to raise an in-library error is present, the
unguarded conversion just predates or bypasses it.

The same unguarded-conversion pattern appears at these sites in that module:

  • :71 — _IPAddressField.pre_process, demonstrated reachable above
  • :186, :254 — _IPInterfaceField.pre_process (also caller-supplied,
    demonstrated above via IPv4InterfaceField / IPv6InterfaceField)
  • :88 — _IPAddressField.post_process
  • :210, :211, :282 — interface post_process

I have not demonstrated a reachable failure for the post_process sites:
those take wire bytes whose length is fixed by the field, so a 4- or 16-octet
slice always converts. They are listed as the same pattern, not as verified
defects.

Why it matters

pcapkit documents its own exception hierarchy and raises from it everywhere
else in this module, so except BaseError is the natural way to handle a bad
field value. Here it silently misses the malformed-address case, and the
distinction is invisible from the call site.

Suggested direction

Wrap the conversions and re-raise as FieldValueError, preserving the original
message. Note that FieldValueError subclasses ValueError, so any
surrounding except ValueError must put except FieldValueError: raise (or the
BaseError equivalent) first, or it will re-wrap the library's own
exception and lose the original detail — the same ordering trap that
ProtocolError (exceptions.py:356) already carries.

Provenance

Surfaced while verifying #464 (the fix for #448). Not a regression from that
PR — I measured both trees. On main an invalid MN-ID identifier builds a
schema with a wrong declared length and then dies at pack() with
builtins.ValueError; on #464 it raises ipaddress.AddressValueError (also a
ValueError subclass, also not in-library) earlier and more clearly. #464 makes
the failure better, not worse, and this issue is filed separately rather than
folded into it.

Same family as #438 (bare struct.error, closed) and #458 (bare EOFError,
open).

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

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

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions