Skip to content

fix(pcapng): constructed and rebuilt blocks round-trip (#1267, #1268, #1269, #1270, #1271) - #1339

Merged
JarryShaw merged 1 commit into
mainfrom
fix/pcapng-roundtrip-k1-k5
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/pcapng-roundtrip-k1-k5

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — ran pylint, mypy and isort with the Makefile flags on the two pcapkit/ files only. No new findings; mypy's 4 errors were already there.
  • make test passes, and a test case covers the change — the modules run are listed below.
  • Added a changelog entry — N/A, added centrally after the wave

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1267
Closes #1268
Closes #1269
Closes #1270
Closes #1271

Before → after. The repro in each issue now builds and rebuilds byte-exactly. Every block of examples/captures/*.pcapng goes through from_data byte-exactly: 120 EPB, 19 IDB, 14 ISB, 2 NRB, 1 SPB, 1 DSB, 1 systemd and 6 of 7 SHB. The one exception is the big-endian SHB, which is #1272.

EXPECTED_FAILURES. 33 of the 35 pcapng-* entries are removed. pcapng-secrets/{TLS,WireGuard}_Key_Log now fail with CONSTRUCT invalid length, recorded against #1279 (_make_block_dsb never pads). The newline-terminated logs are not 4-aligned, so the #1271 fix unmasked it.

Tests. The new tests/protocols/misc/test_pcapng_roundtrip_unit.py has 8 tests. Without the fix, 6 of them fail. The captured_len=0 test guards the #1270 fix itself: it passes on main, and fails without the cut (both subtests, original_len 0 and 60). The remaining one is a control and passes both ways: it checks that the "only one" guard still rejects a repeated option. test_pcapng_unit.py pinned if_IPv6addr length 8, so two of its assertions now expect 17.

Results:

  • round-trip suite: 6 passed, 362 subtests
  • test_pcapng_unit: 97 passed
  • new module: 8 passed
  • test_pcapng_regression: 11 passed
  • test_pcapng_engine: 14 passed
  • plist/json escaping regressions: 4 and 1 passed
  • tests/project: 389 passed, 1 skipped
  • run_unittest_leg.py protocols --exclude protocols/internet: 858 tests, 0 failures

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 97c74a524: NEEDS CHANGES — one small fix (Sonnet reviewed; author Opus). The reviewer said GOOD TO GO and called one regression non-blocking. I re-derived it, and it blocks on its own.

A zero-length EPB regresses. With captured_len=0, the parsed packet field holds 20000000, which is the block's own trailing Total Length swallowed by the zero-length payload read (#1275). The new _make_packet_data then writes those octets back:

tree rebuilt exact
11ae68ebf 32 B yes
this PR 36 B no (malformed)
this PR merged with #1331 32 B yes

So it is only correct if #1331 lands first. Slice the packet bytes to captured_len in _make_packet_data, so the PR stands alone in any merge order.

Everything else held:

  • EXPECTED_FAILURES: imported on both sides, it goes from 35 to 2 entries. 33 were removed, none were added, and the two secrets entries now fail with [Block 10] invalid length: 230/126 (both 2 mod 4, which is fix(pcapng): Decryption Secrets Block never pads its secrets data #1279). They are stricter than before, not looser.
  • K1: a genuinely repeated if_name is still rejected, both when built and when parsed.
  • K2: 17 matches the pcapng draft and the library's own reader.
  • K4: EPB, SPB and PB are byte-exact with and without packet across all 121 fixture blocks, including truncated EPBs.
  • Fixtures: all 6 .pcapng captures rebuild block for block. The protocols leg reports 857 tests and 0 failures.

Worth a line each, not blocking:

  • On a parsed secrets block, editing entries is silently ignored because data wins.
  • The no-packet K4 fallback drops Ethernet pad octets and emits a mis-sized block.
  • The secrets entries' expected length depends on __version__'s length, so a release could flip them into a false "delete the entry" alarm.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
…1269, #1270, #1271)

- #1267: reset the option counter between the make pass and the re-parse in
  PCAPNG.__post_init__, so an option with an "only one" guard no longer trips
  it on itself.
- #1268: if_IPv6addr declares its 17-octet length, not 8.
- #1269: _make_block_isb sets _isb_interface_id before building the
  isb_starttime / isb_endtime options.
- #1270: EPB, SPB and PB rebuilt from their data model keep the packet data,
  taken from the parsed info's packet octets cut to captured_len.
- #1271: the TLS and WireGuard key-log writers end lines with a newline and
  no longer stamp the current time; a parsed log keeps its text and is
  written back verbatim, comments and line order included.

Removes the 33 EXPECTED_FAILURES entries these close. The two key-log entries
now record #1279 (the DSB is never padded) instead.
@JarryShaw
JarryShaw force-pushed the fix/pcapng-roundtrip-k1-k5 branch from 97c74a5 to 901e123 Compare October 7, 2026 19:48
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round-2 verdict on 901e123d4: GOOD TO GO. I verified the delta from 97c74a524 myself: 3 files, +36/−4.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 7, 2026
@JarryShaw
JarryShaw merged commit 13aa677 into main Oct 7, 2026
34 of 36 checks passed
@JarryShaw
JarryShaw deleted the fix/pcapng-roundtrip-k1-k5 branch October 7, 2026 20:42
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 7, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment