fix(pcapng): stop a negative length reaching a read on a truncated capture (#678)

[JarryShaw]

Fixes #678.

The defect

Every EOF-truncated PCAP-NG file raised a bare ValueError out of Extractor — the whole
extraction, not one frame. Measured over all 1,509 octet boundaries of the committed
examples/captures/dhcp.pcapng on f0999858e, with one harness before and after:

	f0999858e	this branch
levels that parse	6	1,495
ValueError: read length must be non-negative or -1	1,479	0
struct.error: bad char in struct format	10	0
in-library ProtocolError	10	2
in-library StreamEOFError	0	8
in-library FormatError	4	4

No level of that sweep raises from outside pcapkit.utilities.exceptions any more, and the frame
count degrades monotonically with the cut: 4 frames at 365 levels, then 3, 2, 1, and 0 at the 58
deepest.

None of the 14 levels that still raise costs a frame that was in the file. Twelve are cuts leaving a
file too short to hold a block at all — under twelve octets (StreamEOFError), under four not even
identifiable as PCAP-NG (FormatError). The other two, at cuts 1462 and 1463, land inside the
Interface Description Block's if_tsresol option (ProtocolError), which is before the first
packet block either way.

That claim is about this sweep, not about every possible input — see What this does not fix
below, which names the two foreign-exception families a fuzz still reaches and why neither is in
scope.

Where it came from, and the four changes

#678's line numbers are stale after #676 and #683; these are the re-located sites.

1. PCAPNG.read, pcapkit/protocols/misc/pcapng.py:1060-1097 — the root. Block Total Length is
cross-checked against its own trailing copy and never against the file, so seek_cur = _seek_set + block.length seeked past the real end on a truncated last block — legal and silent. The next
block read then measured a negative remainder, since prepare derives it as the end of the
stream less the current position. That is why nearly every level failed rather than only the one
holding the cut. _read_fileng had already stopped at the end of the file, so the octets it returned
are the authority on where the block really finishes; the seek is clamped to them and a
ProtocolWarning names the overrun.

2. PCAPNG._check_block_floor, new, pcapkit/protocols/misc/pcapng.py:1241. A block is twelve
octets at its smallest, which is what __length_hint__ already reports. A shorter tail is not a
block, so parsing one out of it can only invent fields from FieldBase.unpack's zero padding — and
once PCAPNG.type's four octets are padded out of nothing, __length__ is negative. Reported as the
quiet StreamEOFError that Extractor.record_frames already catches.

3. nonnegative(), new, pcapkit/protocols/schema/misc/pcapng.py:210. Every span in the module
is a subtraction of wire fields, and _TextField.__call__ builds its template as f'{length}s'
unconditionally — so -8 becomes the format '-8s' and struct.calcsize raises. Applied to the
eight unwrapped arithmetic sites (length - 12/- 16/- 20/- 24/- 28, and the DSB's
- 20 - secrets_length - padding), the eight __option_padding__-sized padding fields, and composed
into bounded_option and bounded_area. Measured: 28 of the module's 74 length callbacks
returned a negative on the parent commit, from -1 on an epb_hash declaring no payload to
-16777248 on an EPB option area; 0 do now.

4. SystemdJournalExportBlock.post_process, pcapkit/protocols/schema/misc/pcapng.py:1711 —
three more of the same family, all found by the cross-review.

• struct.unpack('<Q', entry_data.read(8)) refuses a short buffer with a bare struct.error, and it
  was reachable from valid input, not only truncated input: the block body is padded to a 32-bit
  boundary with NULs, bytes.strip() takes only ASCII whitespace, so the padding survived it and was
  read as the name of a binary field whose 64-bit length prefix then had nothing behind it.
  Measured: MESSAGE=hello\n — 14 octets, so two NULs of padding — raised. A NUL-only line now ends
  the entry, and a genuinely short prefix ends it with a SchemaWarning.
• A binary field's declared length is the widest in the format and nothing bounded it against the
  entry: at 2**63 and above BytesIO.read refuses it with a bare OverflowError (cannot fit 'int' into an index-sized integer), and below that it silently returned whatever was there — the same
  malformed prefix fatal or invisible by magnitude alone. Clamped to what the entry has left, and
  reported.
• A field name, key or value that is not UTF-8 raised a bare UnicodeDecodeError — a ValueError, so
  foreign on both counts, and fatal to the whole extraction over one octet in one field. Decoded with
  errors='replace' and reported, which is the option this module's own StringField already takes.

Worth stating plainly: the first of these is not a truncation defect at all. It made
SystemdJournalExportBlock raise on any journal entry whose length is not a multiple of four, which
is most of them.

5. Option.register, pcapkit/protocols/schema/misc/pcapng.py:694 — the registrar guard. It
wrote into its namespace dictionaries with no check at all, one of five in the package. Now warns
RegistryWarning, #681's pattern.

Relation to #676's clamp-and-warn

Deliberately consistent with it, and the one departure is argued. nonnegative clamps and warns for
exactly bounded_area's reason: there is no catch point above FieldBase.unpack, so a refusal
inside a block read aborts the whole extraction instead of one block, which is what the #431
accommodation exists to prevent.

The end of the file is the case that is not a clamp, because there no block is being read at all.
StreamEOFError is an EOFError, which the frame loop catches by design, so raising it costs no
frame that was actually in the file — the argument against refusing does not apply to it. Clamping
there instead would have fabricated a whole block out of zero padding, which is worse than reporting
the truncation.

#676's note that the five non-packet option areas keep the framing assumption is updated rather than
closed: they now go through nonnegative, which is the part of #678 that stops a declared length
reaching a read, but the per-block equality against __length__ is still open. #593's 32-bit band for
block-level payloads is untouched.

The struct.error manifestation

Fixed, both ways in. The issue's comment reports a huge captured_len driving bounded_area's
span negative; measured, that was because nominal <= available is true for a negative nominal, so
it returned it unclamped. The 200-block / 8,048-octet vector now parses to 200 frames. The truncation
sweep reached the same struct.error by a second route — __option_padding__ at -32 on
EnhancedPacketBlock.padding_opts, at cuts 372 and 373 — which the same floor covers.

Behaviour change, and why breaking

Labelled breaking alongside fix and test. Well-formed captures are unaffected — verified by
regenerating examples/captures/pcapng.txt and diffing: byte-identical to the same file
regenerated on f0999858e. But for a whole class of inputs the output changes:

• any truncated PCAP-NG now yields the frames before the cut where it previously raised, so a caller
  that read "extraction raised" as "this file is unusable" now gets a partial result, and the last
  frame may carry zero-padded octets;
• three exception classes change at the margins: eight short-file depths that raised
  ProtocolError: unknown byteorder magic now raise StreamEOFError, and PCAPNG(b'...') with
  under twelve octets raises StreamEOFError rather than ValueError.

That the old behaviour was a defect makes the change justified, not invisible, which is the same call
#683 made for a change of the same shape.

Tests

26 new tests in tests/protocols/misc/test_pcapng_unit.py, appended; all 26 pass here, over 1,552
subtests. 11 of them fail on f0999858e with the sources reverted and the tests kept (1,519
subtest failures). The cross-review checked the other side of that and named eight it judged not
load-bearing — the monotonicity guard, the twelve-octet boundary's accept side, the unseekable-stream
and construction-path pins, the __length__-whole exemption, and three of the five registry-guard
tests — which is a fair reading: they are guards against the fix's own footguns rather than
regression tests for the defect, and they are labelled as such in their docstrings.

The sweep tests walk every octet boundary rather than picking one — the levels that behave
differently are not ones anybody would have chosen: 372 and 373 held the struct.error, and 376 is
where the cut lands on a block boundary. test_every_length_callback_in_the_module_is_floored_at_zero
walks every schema in the module and drives all 74 length callbacks with a hostile packet, so a newly
added unfloored subtraction is caught without anyone updating a list. The crafted captured_len
vector runs in a subprocess under RLIMIT_AS, since #594 is about amplification and an
in-process regression would take the test host rather than fail.

EXPECTED_FAILURES was imported rather than grepped: 44 entries, 35 PCAP-NG, none moved. One
intermediate attempt did move two of them — flooring the three decryption-secrets payloads that read
__length__ whole packs nothing, which emptied both payloads and turned
pcapng-secrets/TLS_Key_Log and .../WireGuard_Key_Log from MISMATCH to OK because an empty
payload compares equal to an empty payload. A regression that reads as a fix; reverted, and pinned by
test_a_field_sized_by_the_remaining_length_whole_is_left_alone.

What this does not fix

A 4,000-round bounded mutation fuzz over dhcp.pcapng, run with the same seed and harness either side
of the change under a 2 GiB RLIMIT_AS, takes the parse rate from 2,118 to 3,438 and removes both of
#678's families entirely (ValueError: read length must be non-negative or -1, and struct.error: bad char in struct format at 131 → 0). Two foreign-exception families remain, both pre-existing and
measured unchanged:

• ValueError: N is not a valid BlockType. An unassigned Block Type raises from aenum inside
  EnumField.post_process, so the UnknownBlock default the schema registry declares is unreachable
  and the format's skip-unknown-blocks requirement is not met. Reachable from a single byte flip in the
  committed capture. Filed as An unassigned PCAP-NG Block Type raises a bare ValueError from aenum, so UnknownBlock is unreachable and one unknown block costs the whole extraction #701; the site is pcapkit/corekit/fields/numbers.py, shared by
  every protocol, so fixing it here would move every enum-typed wire field in the package at once.
• MemoryError. Exactly 30 of 4,000 in both trees under the same cap — fix(corekit): bound the total zero padding a parse may synthesise (#573) #593's 32-bit band for
  block-level payload fields, which fix(pcapng): bound an option's payload to the area its block declares (#594) #676's note already records as a known gap.

Also out of scope, and noted rather than fixed: SystemdJournalExportBlock.post_process's
entry_data.read() after a binary field reads to the end of the entry rather than past one newline, so
any field after the first binary one is discarded. Filed as #704.

Coverage

Measured and reported inside each tree, over the same three test files:

	f0999858e	this branch
pcapkit/protocols/misc/pcapng.py	1545 stmts, 1 miss → 99.91%	1557 stmts, 1 miss → 99.91%
pcapkit/protocols/schema/misc/pcapng.py	510 stmts, 0 miss → 100.00%	523 stmts, 0 miss → 100.00%
total	2055 stmts, 736 branches → 99.93%	2080 stmts, 746 branches → 99.93%

25 new statements, all covered; the single remaining miss is the pre-existing
return datetime.timezone.utc. An earlier draft did drop the total to 99.89% — an explicit _read
guard in unpack whose false branch can never be taken, since the construction path sets
__header__ from make before it gets there. Removed rather than pragma'd.

Also

examples/captures/pcapng.txt does not move further under this change (#685 unaffected) — the
116-line diff against the committed fixture is entirely the pre-existing #683 drift, byte-identical
before and after this branch.

Cross-review

Reviewed by a subagent on Sonnet (this change was authored on Opus), briefed to falsify rather than
to bless, running read-only. It came back NEEDS CHANGES twice, and both rounds were right:

• Round one found the journal block's bare struct.error — item 4 of the list above, which I had
  not looked at — and that the description's claim "nothing raises from outside
  pcapkit.utilities.exceptions" was unscoped and therefore false. Both addressed: the exception is
  fixed, and the claim is now scoped to the sweep it was measured on with the two remaining families
  named above. It also caught two miscounts (20 vs 21 tests, 13 vs 14 residual levels) and one wrong
  attribution — I had written that all 14 residual levels cut into the Section Header Block, where two
  of them cut into the Interface Description Block.
• Round two confirmed those closed, independently reproducing both the defect and the repair
  against a separately reconstructed pre-fix tree, and then found the OverflowError and
  UnicodeDecodeError above in the same function — pre-existing, but, as it put it, "silently leaving
  them out after specifically fixing this function's other foreign-exception path is the same
  unscoped-claim shape as the original NEEDS CHANGES". Fair, and fixed rather than deferred.

Two of its observations I accepted without changing the code, and both are recorded above rather than
folded away: that eight of the new tests are guards against the fix's own footguns rather than
regression tests for the defect, and that the MemoryError family is scoped out on the strength of its
being measured identical either side rather than on argument.

It could not verify the "11 of the new tests fail on the parent commit" claim without reverting the
tree, which it correctly declined to do; it reasoned about each test instead.

CI has not run yet; no claim is made about it.

[JarryShaw]

Two things measured after the PR description was written, both worth having on the record here.

A bounded fuzz, run either side of the change

4,000 rounds of word-aligned mutation over the committed examples/captures/dhcp.pcapng — huge
lengths, lengths under every block's fixed-field floor, random type words, single-octet flips, and a
random truncation in 40% of cases — each case capped at the original 1,508 octets and run under a 2 GiB
RLIMIT_AS. Same seed, same harness, once on f0999858e and once on this branch:

	f0999858e	this branch
parsed	2,118	3,438
ValueError: read length must be non-negative or -1	present, within the 1,285	0
struct.error: bad char in struct format	131	0
ValueError: N is not a valid BlockType	present	46
MemoryError (at the 2 GiB cap)	30	30
in-library (FormatError / ProtocolError / StreamEOFError / FieldValueError)	436	486

Both of #678's families are gone under fuzz as well as under the systematic sweep. The two foreign
families that remain are pre-existing and unchanged in kind:

• MemoryError at exactly 30 in both trees — fix(corekit): bound the total zero padding a parse may synthesise (#573) #593's 32-bit band for block-level payload fields,
  which fix(pcapng): bound an option's payload to the area its block declares (#594) #676's note already records as a known gap and which this change does not touch.
• ValueError: N is not a valid BlockType — in both trees' findings; the count differs only because
  more inputs now get far enough to reach it. Filed as An unassigned PCAP-NG Block Type raises a bare ValueError from aenum, so UnknownBlock is unreachable and one unknown block costs the whole extraction #701, with its own repro: an unassigned
  Block Type raises from aenum inside EnumField.post_process, so the UnknownBlock default the
  schema registry declares is unreachable and the format's skip-unknown-blocks requirement is not met.
  The site is pcapkit/corekit/fields/numbers.py, shared by every protocol, so it is deliberately not
  in this change.

Amended since the description

715b96801 → dbad35c32, one test assertion only. The subprocess in
test_the_captured_len_vector_parses_under_a_memory_cap compared its pcapkit.__file__ against the
parent process's, which can differ in path form rather than in target — this machine reaches the same
checkout through both /home/jarryx/... and /local/home/jarryx/..., so the comparison was
environmentally fragile. Both sides now assert against the repository root they were pointed at
instead, which keeps the guarantee (the subprocess measured this tree, not an installed copy) without
the cross-process path comparison. The count in the description is corrected with it: 21 new tests,
not 20.

[JarryShaw]

Cross-review verdict: GOOD TO GO

Model: Sonnet, against this change authored on Opus — a different model by construction, as the
standing rule requires. No substitution was needed; no model was unavailable. It ran read-only and
wrote nothing itself; this comment and every change it prompted are mine.

It took three rounds, and the first two were NEEDS CHANGES. Both were right, and what it
disputed is worth recording because none of it was cosmetic:

Round one — NEEDS CHANGES.

1. A bare struct.error in SystemdJournalExportBlock.post_process, which I had not looked at.
   Reproducing it led somewhere broader than reported: it is reachable from valid input, because the
   block's own 32-bit NUL padding was read as a binary field's name. Fixed.
2. The description's claim that "nothing raises from outside pcapkit.utilities.exceptions any more"
   was unscoped and therefore false. Narrowed to the truncation sweep it was measured on, with the
   two remaining families named in What this does not fix and filed as An unassigned PCAP-NG Block Type raises a bare ValueError from aenum, so UnknownBlock is unreachable and one unknown block costs the whole extraction #701.
3. Three miscounts and one wrong attribution: 20 vs 21 tests, 13 vs 14 residual levels, and my claim
   that all 14 residual levels cut into the Section Header Block when two of them cut into the
   Interface Description Block. All corrected.

Round two — NEEDS CHANGES. It confirmed the above closed, reproducing both the defect and the repair
against a pre-fix tree it reconstructed itself, then found two more foreign exceptions in the same
function: a bare OverflowError from a binary field's 64-bit length at or above 2**63 reaching
BytesIO.read, and a bare UnicodeDecodeError from a field name, key or value that is not UTF-8. Both
pre-existing. Its argument for not deferring them was the decisive one and I accepted it verbatim:
"silently leaving them out after specifically fixing this function's other foreign-exception path is
the same unscoped-claim shape as the original NEEDS CHANGES." Fixed rather than filed.

Round three — GOOD TO GO. It re-ran its own repros unmodified, added the 2**63 boundary, and then
ran a fresh 6,000-trial fuzz aimed only at this function — declared lengths spanning 0 to 2**64-1, NUL
runs, raw garbage — and got zero exceptions of any kind, not merely zero foreign ones. It also
independently re-derived every number in the description (26 new tests, the coverage table, the
statement counts) and matched all of them.

What it disputed that I did not change, both recorded rather than folded away:

• Eight of the new tests are not load-bearing. It checked the other side of the "11 fail on the
  parent commit" claim and named them: the monotonicity guard, the twelve-octet boundary's accept side,
  the unseekable-stream and construction-path pins, the __length__-whole exemption, and three of the
  five registry-guard tests. That is a fair reading — they are guards against this fix's own footguns
  rather than regression tests for the defect, and they say so in their docstrings. Kept deliberately.
• The MemoryError family is scoped out on a measurement, not an argument. It said so, and it is
  right: 30 of 4,000 fuzz trials in both trees under the same cap and seed, which establishes unchanged
  rather than harmless. It remains fix(corekit): bound the total zero padding a parse may synthesise (#573) #593's 32-bit band.

What it could not verify: that 11 of the new tests fail on f0999858e. Checking it needs the source
files reverted, which it correctly declined to do in a live worktree; it reasoned test by test instead
and confirmed the eight above from a separately reconstructed pre-fix tree. That claim rests on my
measurement, not on an independent one.

Two design judgements it was asked to second-guess and agreed with, with its own reasoning rather than
by assent: that a clamped binary-field read may reach into the block's NUL padding (drawing the line
without #704's restructuring would be guessing, and the well-formed path never clamps), and that
errors='replace' beats keeping raw bytes (the OrderedMultiDict[str, str | bytes] key must be str
regardless, so a per-site rule would be the inconsistency).

AutoSDE has no equivalent here; CI is still queued at the time of writing and no claim is made about
it. This PR is unpublished in the sense that matters: it is not merged, and merging is not mine to do.