Malformed TCP SACK raises ProtocolError or FieldValueError depending on process state #525

Summary

Which exception a malformed TCP SACK option produces depends on whether the schema layer has already been exercised in the same process. The two exceptions are siblings — neither is a subclass of the other — so no single except clause catches both, and which one you get is not a property of the input.

Found while fixing #519; reported separately because it is a behavioural defect rather than a docstring one.

The two paths

• Clean process: _read_mode_sack reaches its own length check and raises ProtocolError.
• After tests/protocols/schema/ has run in the same process: the schema is unpacked before _read_mode_sack is reached, and FieldValueError comes out of pcapkit/corekit/fields/collections.py:189 instead.

Both derive from the pcapkit.utilities.exceptions hierarchy but along different branches, so except ProtocolError silently misses the second case and except FieldValueError misses the first.

Why it matters

A consumer writing defensive code around Extractor cannot write a correct handler. except ProtocolError looks right — it is what the docstring documents and what a clean run produces — and then fails to catch anything in a long-lived process that has already parsed other packets. That is the worst shape for this kind of bug: it works in tests and in short scripts, and stops working under sustained use.

The state dependence appears to be schema-field caching or lazy initialisation deciding whether the unpack happens eagerly, but the precise mechanism is not established — only the observable difference is.

Repro sketch

Parse a TCP packet carrying a SACK option whose length is not 2 + 8n (for example 11 or 14), twice:

1. in a fresh interpreter, and
2. after importing and exercising the schema tests in the same interpreter.

The raised type differs between the two.

Suggested direction

Decide which exception is correct for a malformed option and make the other path raise it — most plausibly by having the schema-layer failure wrapped into ProtocolError, since that is the documented contract and the layer a caller of Extractor is aware of. A test asserting the union of both types, as tests/protocols/transport/test_tcp_sack_length_unit.py currently does, is a workaround that records the problem rather than fixing it.

Notes

The missing length validation itself is fixed separately as part of #519 — _read_mode_sack previously accepted lengths 11, 14 and 2 with no exception at all, unlike its siblings _read_mode_sackpmt (raises on length != 2) and _read_mode_echo (raises on length != 6). This issue is only about which exception escapes once validation exists.

length == 2 remains accepted, since (2 - 2) % 8 == 0 satisfies the documented rule exactly. RFC 2018 also requires at least one block, so a stricter check is arguably warranted — deliberately not tightened past what the docstring states.