Skip to content

fix(fields): NumberField.__init__'s bit_mask shift still leaks a bare ValueError, and #829's guard is skipped when bit_length is supplied #831

Closed

Description

@JarryShaw

NumberField.__init__ has a second eager shift that #829 does not guard, and it leaks the same bare builtins.ValueError that #828 was about.

pcapkit/corekit/fields/numbers.py:88 — self._bit_mask = (1 << bit_length) - 1. Measured on #829's head (a21a10fd9), so this survives that fix:

NumberField(length=4, bit_length=-1) -> ValueError: negative shift count   is BaseError=False
NumberField(length=4, bit_length=32) -> ok, template='>I'        (control)

is BaseError=False is the defect: a caller cannot catch it as a pcapkit error. #829 guards the shift in __call__ (reached only when bit_length was not supplied); this one is in __init__ and fires when it was.

Severity is low and deliberately stated as such. bit_length is never a callable, so it cannot carry a wire value, and all three in-tree call sites pass positive literals — pcapkit/protocols/schema/link/vlan.py:47 (bit_length=3), :49 (=1), :51 (=12). So this is reachable by a programmer typo in a new schema, not by a malformed packet. That is why it is a separate issue rather than a change to #829.

A related latent gap, same area, found in the same review: a field that supplies bit_length and a negative-resolving callable length skips #829's guard entirely — no raise at __call__ — and degrades to template='>-1s' with the error deferred to FieldBase.length's template= message. Still a ProtocolError, so not a correctness break, but one wire condition then yields two different messages depending on an unrelated constructor argument. No in-tree site combines the two today.

The fix for both is the same shape: validate before either shift, raising ProtocolError from pcapkit.utilities.exceptions and keeping the Field {name} resolved to a negative length; prefix that FieldBase.length and #829 already share, so a caller matching that substring catches every case.

Found by #829's cross-review; both measurements reproduced here independently.

Activity

  1. added
    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)
    fixPull requests that fix a defect (fix: subject prefix)
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Sep 26, 2026
  2. JarryShaw commented on Sep 26, 2026

    @JarryShaw
    OwnerAuthor

    Labelled wip and dispatching a worker — this was idle, which is the real gap rather than a missing label.

    It is not blocked: its former dependency #829 merged as a432ddec5, and I re-verified the defect is still live on the current main (5576708d4) rather than assuming:

    NumberField(length=4, bit_length=-1)               -> ValueError: negative shift count   is BaseError=False
    NumberField(length=lambda p: p['len']-4)({'len':3}) -> ProtocolError                      is BaseError=True   <- #829 fixed this one
    

    So #829 and this are genuinely different defects, and nothing else in flight touches pcapkit/corekit/fields/numbers.py — the only other open PRs are #815 (foundation/registry/protocols.py and the apptype tree) and #657 (changelog).

    Thanks for catching it. The audit it prompted found three label errors, not one: this being idle, #809 labelled blocked when #815 closes it directly, and #801 labelled wip with no covering PR.

  3. removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Sep 26, 2026
  4. added this to the 1.5 milestone on Oct 6, 2026
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)fixPull requests that fix a defect (fix: subject prefix)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions