fix(pcapng): bound the skip after a journal binary field to one octet

[JarryShaw]

Fixes #704.

Mechanism, verified against source: not quite as the issue titled it -- the value read (entry_data.read(length)) is already correctly bounded (fixed for #678/#699). The actual defect is the very next line, meant to skip the binary field's one trailing newline: entry_data.read() with no argument reads to EOF, not past that one octet. The entry loop's next readline() then returns b'' and ends the entry -- so every field behind the first binary field is discarded, silently, however well-formed.

Fix: pcapkit/protocols/schema/misc/pcapng.py, SystemdJournalExportBlock.post_process -- bound the skip to entry_data.read(1). If that octet is missing or isn't \n, end the entry with a SchemaWarning (mirrors the short-length-prefix guard just above) rather than resynchronising at the wrong offset. Skipped when the value was already clamped to the entry's own end, since that shortfall is reported there already and nothing real is left to check.

Falsification: added test_journal_fields_following_a_binary_field_are_not_discarded, a two-binary-field entry with a text field after the last one -- the shape that discriminates a bounded skip from an unbounded one (a single binary field has nothing behind it to lose). Against the unfixed code it fails:

self.assertEqual(entries[0]['SECOND'], b'two')
...
pcapkit.utilities.exceptions.MissingKeyError: 'SECOND'

Passes after the fix; BEFORE, FIRST, SECOND, AFTER all survive.

Two pre-existing fixtures relied on the old unbounded read to end cleanly without a real terminator behind the declared value; updated their expectations for the newline-bounded behaviour.

Coverage (coverage run -m pytest tests/protocols/misc/test_pcapng_unit.py, scoped to the schema module): pcapkit/protocols/schema/misc/pcapng.py -- 546 stmts / 78 branches, 100%/100% both before and after this test was added (the corrected pre-existing fixtures already reach the new lines; the new test is what actually proves the discard is gone, which line coverage alone can't show). Full file: 80 tests, 1753 subtests, all pass.

Verified pcapkit.__file__ resolves inside the worktree under test before running.

Not verified: no real-world systemd-journal-export .pcapng capture with a mid-entry binary field was available to test against; coverage is from synthetic fixtures.

Second round — newline restoration

The first cross-review found the new terminator check warned falsely on spec-legal input: self.entry.split(b'\n\n') at pcapkit/protocols/schema/misc/pcapng.py:1778 eats the last field's terminating newline along with the separator, so a binary last field made read(1) return b''.

Fixed by re-appending b'\n' to every split segment but the last. Two fixtures added:

Fixture	Guards
test_a_binary_last_field_before_a_trailing_separator_is_not_warned	trailing entry separator
test_a_binary_field_ending_the_first_of_two_entries_is_not_warned	two entries, first ends binary

Both fail at cb62a4f04 and pass here. They guard the restoration, not #704 — the original multi-field fixture is what fails on main.

tests/protocols/misc/test_pcapng_unit.py: 80 passed, 1 skipped, 1753 subtests. Coverage 546 stmts / 78 branches, 100%/100%.

[JarryShaw]

❌ NEEDS CHANGES — cross-review on Opus of Sonnet-authored work at cb62a4f04: the core fix is correct and the falsification reproduces, but the new terminator check raises a false SchemaWarning on spec-legal input because self.entry.split(b'\n\n') eats the very newline the check then demands — required change: restore that newline (re-append b'\n' to every split segment but the last) and add a fixture carrying a trailing entry separator / a second entry.

[JarryShaw]

Cross-review detail (Opus, independent of the Sonnet author)

Evidence obtained here, not taken from the PR body. pcapkit.__file__ = /local/home/jarryx/GitHub/PyPCAPKit/.claude/worktrees/agent-ab509640cefa830df/pcapkit/__init__.py, Python 3.14.7, PYTHONSAFEPATH=1. tests/protocols/misc/test_pcapng_unit.py at cb62a4f04: 79 passed, 1753 subtests (78 passed + 1 skipped before examples/generators/make_samples.py — the skip is the missing dhcp_big_endian.pcapng).

The finding: the new check fires on valid captures

self.entry.split(b'\n\n') consumes an entry's last field's terminating newline together with the blank line separating entries. For a text last field readline() does not care. For a binary last field the new entry_data.read(1) comes back b'' and warns. Measured, same fixture on origin/main vs cb62a4f04, data identical in both — only the warning is new:

fixture	main	cb62a4f04
b'MESSAGE\n' + pack('<Q',3) + b'abc\n' + b'\n' — the trailing entry separator draft-richardson-opsawg-pcapng-extras-01 says an entry may carry	MESSAGE=b'abc', 0 warnings	same data, 1 warning
b'A=1\nBIN\n' + pack('<Q',3) + b'abc\n' + b'\n' + b'B=2\n' — two entries, first ends on a binary field	both entries correct, 0 warnings	same data, 1 warning
control: same shapes with a text last field	0	0

The message is also factually wrong there — the newline was in the capture; the splitter removed it before the check ran.

Verified fix (not speculative — I ran it): re-append the newline the split took.

segments = self.entry.split(b'\n\n')
for index, entry_buffer in enumerate(segments):
    if index < len(segments) - 1:
        entry_buffer += b'\n'

Both false positives go to 0; the genuine warnings below are retained; all 6 journal tests / 23 subtests still pass unchanged. (Accepting terminator in (b'\n', b'') also silences it but throws away the real truncation warning, so it is the worse option.)

Per-claim verdict

1. Value read bounded, bug one line later — CONFIRMED. read(length) with length clamped to available; every read in the journal path is bounded (readline(), read(8), read(length), read(1)). No second unbounded path. But the PR body's "not quite as the issue titled it" is inaccurate — #704's title names entry_data.read() reading to EOF, and its body states outright that it is "not the same defect as the struct.error PR #699 fixes". Nothing needed correcting.

2. The clamped early-exit — no live variant of the defect. Two independent derivations. Arithmetic: length = available = len(entry_buffer) - entry_data.tell(), so after the clamped read the cursor is at end-of-buffer by construction and there is provably nothing to check. Measurement: with the if not clamped: guard deleted entirely, b'BINARY\n' + pack('<Q',100) + b'abc\nAFTER=one\n' yields byte-identical data (BINARY=b'abc\nAFTER=one\n\x00\x00\x00') and only a second warning — 2 instead of 1. The hypothesised "clamped value, real newline, more fields behind" is unconstructible: any bytes behind the value are inside available, so the clamp absorbs them into the value, which is what a length exceeding the buffer means and is warned. The guard is warning de-duplication, correct but redundant. Keep it.

3. break — consistent and right. The short-prefix guard 15 lines above also breaks, and data.append(entry) is outside the loop, so break keeps what was parsed: b'KEPT=yes\nBINARY\n' + pack('<Q',2) + b'abXAFTER=one\n' → [('KEPT','yes'), ('BINARY',b'ab')] + 1 warning. Only AFTER is dropped, which is correct — after an unexpected octet the offset is untrustworthy.

4. Neither modified test was weakened; both were strengthened. Both fail on origin/main at exactly the changed subtests (8 SUBFAILED), so the new expectations are specific to the new behaviour rather than fitted to it. ..._cut_short_of_its_length_is_reported went from assertEqual(schema_warnings, []) — which was asserting the bug's silence — to asserting one warning, its text, and MESSAGE == b''. For ..._declaring_more_than_its_entry_is_clamped I derived the table independently: buffer is b'BINARY\n' + <Q> + b'abc\n\x00' (19 → pad 20), terminator octet is b'abc\n\x00'[declared], so clean only at declared == 3 — matching the new assertions exactly, with the value assertion unchanged in substance.

5. Coverage claim reproduced, and the author's argument is right. 543 stmts / 76 branches, 100%/100%. Deselecting only the new test leaves it at 543/76, 100%/100% — the new test adds zero coverage yet is the only thing that fails on main. Falsification against git show origin/main:…/pcapng.py, verbatim:

self = OrderedMultiDict([('BEFORE', 'zero'), ('FIRST', b'one')]), key = 'SECOND'
pcapkit.utilities.exceptions.MissingKeyError: 'SECOND'

6. Fixtures faithfully model the format. Against systemd.io JOURNAL_EXPORT_FORMATS and draft-richardson-opsawg-pcapng-extras-01 §3.1 (draft-ietf-opsawg-pcapng-04 only registers block type 0x00000009 and refers out): NAME=value\n for text; NAME\n + 64-bit little-endian size + data + \n for binary; size covers the data only (the spec's worked example is length 7 for foo\nbar); NUL-pad to 32 bits, which _journal_block does. <Q is correct. Two shapes the fixtures miss — the second is how the finding above got through:

• No fixture has a binary value containing a newline, which is the entire reason the binary encoding exists ("fields containing newlines cannot be formatted like this"). The spec's own example is a mid-entry binary MESSAGE with text fields after it; I confirmed it now parses (MESSAGE=b'foo\nbar', _PID, __REALTIME_TIMESTAMP all survive; on main only MESSAGE does). Worth adding — it is a better Every systemd journal field after the first binary field is discarded: entry_data.read() reads to EOF where it means to skip one newline #704 fixture than one/two.
• No fixture carries a trailing entry separator or a second entry.

Adjacent, pre-existing, not this PR's job

split(b'\n\n') is unsafe against binary data, identically on main and on cb62a4f04:

• MESSAGE = b'a\n\nb' → [('MESSAGE', b'a')] plus a junk entry ('b', b'e\n\x00') declaring 7957646490146719297 octets.
• struct.pack('<Q', 2570) == b'\n\n\x00\x00\x00\x00\x00\x00', so a binary field of exactly 2570 octets has b'\n\n' inside its own length prefix — the entry splits mid-prefix and zero fields parse.

Neither is introduced here, but both mean "every field behind a binary field survives" is still not true in general. Suggest a separate issue.

Could not verify

• No real systemd-journal-export .pcapng with a mid-entry binary field — confirmed the author's own caveat: examples/captures/ has none even after make_samples.py. All evidence synthetic.
• The spec never states in words whether the declared length excludes the trailing newline; it is an inference from the wording plus the length-7 example. Code and fixtures follow that reading consistently.
• The spec neither affirms nor denies a zero-length binary field, so test A's MESSAGE == b'' expectation rests on an inference.
• Full test suite not run (scoped to tests/protocols/misc/test_pcapng_unit.py by brief), so no statement about cross-file regressions.
• Minor, no action: declared == available exactly takes the terminator branch rather than the clamp branch, so an over-declared length is reported by the less informative of the two messages.

[JarryShaw]

Reproduced on my own harness — the false positive is real. Prior GOOD-TO-GO-adjacent verdict is superseded.

New head: 1f340cb13 (still 1 commit).

• self.entry.split(b'\n\n') ate a real entry's last-field terminator along with the blank-line separator. Binary last field + trailing separator, or binary last field ending entry 1 of 2: 0 warnings on main, 1 false SchemaWarning at cb62a4f04 — confirmed both shapes, both directions.
• Fix: re-append b'\n' to every split segment but the last.
• Added 2 fixtures for these shapes; both fail without the fix (verified), pass with it.
• Genuine warnings (short prefix, over-declared length, wrong terminator) and Every systemd journal field after the first binary field is discarded: entry_data.read() reads to EOF where it means to skip one newline #704's own fixture still hold.
• tests/protocols/misc/test_pcapng_unit.py: 81 passed, 1753 subtests, 0 failed.
• Coverage: pcapng.py schema module 546 stmts / 78 branches, 100%/100%.
• Two pre-existing (not this PR's) defects filed as pcapng: journal entry split on b'\n\n' shreds binary data that contains it #723.

[JarryShaw]

✅ GOOD TO MERGE — cross-review on Opus of Sonnet-authored work at 1f340cb13: the newline restoration is correct at every boundary I could construct (8 shapes that warned falsely at the old head now warn zero times, and none warns on origin/main either), #704 is still fixed and still discriminates against origin/main, both new fixtures fail without the restore, and the short-prefix / over-declared / wrong-terminator warnings all still fire.

[JarryShaw]

✅ GOOD TO MERGE — cross-review on Opus of Sonnet-authored work at 1f340cb13.

Supersedes the ❌ NEEDS CHANGES at #722 (comment), which referred to the old head cb62a4f04.

Evidence obtained here, not from the PR body. pcapkit.__file__ = /local/home/jarryx/GitHub/PyPCAPKit/.claude/worktrees/agent-af5fd2f004dd7b8b1/pcapkit/__init__.py, Python 3.14.7, PYTHONSAFEPATH=1. tests/protocols/misc/test_pcapng_unit.py: 80 passed, 1 skipped, 1753 subtests — the skip is the ungenerated dhcp_big_endian.pcapng, which is the 81st pass reported earlier. Coverage reproduced: pcapng.py 546 stmts / 78 branches, 100%/100%. cb62a4f04..1f340cb13 in the module is exactly the restore, nothing else. CI: 24 SUCCESS, 2 SKIPPED (Docs test gate, Gate (full suite, Python 3.14)), 0 failures.

Boundary cases — no off-by-one found

bytes.split is non-overlapping, so no segment can contain \n\n and a non-last segment can never already end in \n. The restore therefore hands back exactly one octet that was in the capture and can never invent one. Measured against origin/main:

shape	result
empty payload b''	1 empty entry, 0 warnings — unchanged
single entry, no \n\n	0 warnings — unchanged
ends \n\n\n, ends \n\n\n\n	correct, 0 warnings (degenerate blank gives an empty entry on both trees)
no trailing newline at all, 1 entry and 2 entries	1 warning — new, and correct: the field's mandatory \n is genuinely absent
zero-length binary field, alone and ending entry 1 of 2	0 warnings
3 entries each ending on a binary field	all 3 parse, 0 warnings
binary value of all-NUL octets	0 warnings (read by read(length), so the NUL-line break never sees it)
over-declared by exactly 1 / by ≥2	terminator warning / clamp warning — never silent

8 shapes warned falsely at cb62a4f04 and warn 0 times now: trailing \n\n\n; 4-aligned binary last field + separator; zero-length binary ending entry 1 of 2; middle of 3 entries ending binary; 2 entries both ending binary; exact-length binary ending entry 1 of 2; and the PR's two new fixtures.

Per attack item

• Every systemd journal field after the first binary field is discarded: entry_data.read() reads to EOF where it means to skip one newline #704 not reintroduced. ..._are_not_discarded fails on origin/main ([('BEFORE','zero'),('FIRST',b'one')] — SECOND and AFTER gone) and passes here. Independent shape: the format's own worked example MESSAGE\n + <Q 7> + b'foo\nbar\n' + b'_PID=42\n' loses _PID on main, keeps it here.
• Both new fixtures discriminate against cb62a4f04, not against origin/main, where they pass. They guard the restore, not Every systemd journal field after the first binary field is discarded: entry_data.read() reads to EOF where it means to skip one newline #704. Not a defect — but worth stating so they are not later mistaken for Every systemd journal field after the first binary field is discarded: entry_data.read() reads to EOF where it means to skip one newline #704 coverage.
• clamped early-exit still sound, and now load-bearing. The arithmetic survives the restore because available is computed from the same rebound entry_buffer that backs the BytesIO, so a clamped read still lands on EOF by construction. Correcting the earlier review's "redundant": deleting the guard now fails 6 clamped subtests, AssertionError: 2 != 1 at test_pcapng_unit.py:4281 (declared 6, 1024, 2**30, 2**62, 2**63, 2**64-1). Keep it.
• Genuine warnings all fire: short prefix (of the 8 it needs), over-declared (declares 100 octet(s) with 5 left, likewise at 2**63), and a wrong terminator octet (ab + X keeps KEPT/BINARY, drops AFTER).
• pcapng: journal entry split on b'\n\n' shreds binary data that contains it #723 unchanged. A: value still shredded, 2 warnings, junk trailing entry. B (struct.pack('<Q', 2570) == b'\n\n' + NULs): byte-identical to main, [{}, {}], 1 warning — the restore is applied to that segment and makes no difference, because read(8) returns 0 octets either way.

On the "+1 byte-count artifact" — benign, and in fact a correction. available on main under-counted every non-last segment by the octet split had eaten, so main's declares 3 octet(s) with 2 left was factually wrong. On the #723-A shape BIN\n + <Q 3> + b'ab\n' + b'\n', main truncates to b'ab' while head reads the correct b'ab\n'; one warning either way, the second entry survives on both. Strictly less wrong, not a new defect.

Non-blocking

• The Note: block documents every other guard but never says split eats the terminator — the reason the check works at all lives only in an inline comment.
• A fixture with a binary value containing a newline is still absent. It is the reason the binary encoding exists, it is broken on main and fixed here (_PID above), and nothing covers it.
• declared == available exactly still reports through the terminator message rather than the clamp's more informative one.
• mergeStateStatus: BEHIND — 9 commits behind origin/main; merges into 4391dc77b cleanly, so it just needs an update.
• Still no real systemd-journal-export capture with a mid-entry binary field; all evidence here and in the PR is synthetic. The author's own caveat stands.

✅ GOOD TO MERGE — cross-review on Opus of Sonnet-authored work at 1f340cb13.