Skip to content

fix(pcapng): rebuilt blocks keep their lengths and padding (#1276, #1279, #1280, #1282, #1283, #1284) - #1373

Merged
JarryShaw merged 1 commit into
mainfrom
fix/pcapng-roundtrip-padding
Oct 8, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/pcapng-roundtrip-padding

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style — not run; isort --check-only flags these files on origin/main too
  • make test passes, and a test case covers the change — the modules below, not the full suite
  • Added a changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect

Description

Closes #1276
Closes #1279
Closes #1280
Closes #1282
Closes #1283
Closes #1284

Spec claims checked against draft-ietf-opsawg-pcapng-06 (and draft-richardson-opsawg-pcapng-extras-01 for #1280).

# Before After
1276 opt_comment é rebuilds 01000100 c3000000 01000200 c3a90000, byte-exact
1279 DSB with 3 secret octets: invalid length: 23 padded, Secrets Length stays 3
1280 journal entry: invalid length: 26 zero-padded to 32 bits
1282 NWK/APS Secrets Length rebuilds 20/24 18/22; the 2 zero octets are the block's padding_data
1283 EPB/PB 48, SPB 32, PCAP record 32 octets over snaplen 8 40/40/24 and 24
1284 ad0b0300 61626300 parses as pen=6513249 ProtocolError: [opt_custom] invalid length

Changes beyond the issue text:

The new tests are in tests/protocols/misc/test_pcapng_roundtrip2_unit.py. With origin/main's three source files written back over the fix, it reports 10 failed, 1 passed (every test fails) and the option round-trip reports 2 failed. With the fix, it reports 8 passed, 3 subtests passed.

All 165 blocks of the six examples/captures/*.pcapng fixtures rebuild byte-exactly, except the big-endian SHB (#1272, in the follow-up PR). Test results:

  • test_pcapng_unit 97 passed
  • test_pcapng_roundtrip_unit 8 passed
  • test_option_roundtrip_unit 6 passed / 362 subtests
  • test_pcapng_regression 11 passed
  • test_pcapng_end_to_end 6 passed
  • pcap frame modules all pass
  • run_unittest_leg.py protocols --exclude protocols/internet: 929 tests, 0 failures
  • tests/project: 389 passed

test_runtime_engines.py::test_pcap_run_writes_global_header… fails identically on origin/main, so this PR does not cause it.

, #1280, #1282, #1283, #1284)

- String options count UTF-8 octets, not characters, in Option Length.
- The Decryption Secrets Block pads its secrets data; Secrets Length stays
  unpadded, and the ZigBee NWK/APS secrets no longer count the block padding.
- The systemd Journal Export Block zero-pads its entry to 32 bits.
- EPB, PB, SPB and PCAP records cut a packet longer than the captured length
  (snaplen) instead of writing it whole; a snaplen of zero means no limit.
- A custom option shorter than its 4-octet PEN raises ProtocolError.
@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 labels Oct 7, 2026
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet breaking Breaks public-facing behaviour or API (apply alongside the type label) 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 68026d540: GOOD TO GO (Sonnet reviewed; author Opus).

Follow-ups filed (pre-existing or edge, not blocking): #1384 — an SPB longer than snaplen is silently truncated on parse; #1385 — a PCAP frame built with snaplen 0 writes no data. A DSB whose Secrets Length counted the padding (20) rebuilds as 18 — pcapng-06 is silent on which writers do.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.98% (unit tier, Python 3.14, 68026d540, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18777 1032 2348 843 90.27%
pcapkit/corekit 1949 84 606 20 94.91%
pcapkit/dumpkit 189 1 68 1 99.22%
pcapkit/foundation 2443 102 852 39 94.39%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16035 206 4194 177 98.07%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 0e25f9f into main Oct 8, 2026
44 checks passed
@JarryShaw
JarryShaw deleted the fix/pcapng-roundtrip-padding branch October 8, 2026 00:56
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

1 participant