Skip to content

fix(pcapng): blocks and options rebuild as parsed (#1272, #1273, #1277, #1278, #1281, #1286) - #1378

Merged
JarryShaw merged 2 commits into
mainfrom
fix/pcapng-roundtrip-modelling
Oct 8, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
fix/pcapng-roundtrip-modelling

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style — make isort is clean per tests/project/test_isort_clean.py; pylint and mypy not run
  • 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 #1272
Closes #1273
Closes #1277
Closes #1278
Closes #1281
Closes #1286

Spec claims are checked against draft-ietf-opsawg-pcapng-06:

  • the flags bits are LSB-0, per the NOTE under Table 5
  • epb_verdict is "multiple allowed: yes"
  • NRB names and ns_dnsname are UTF-8
# Before After
1272 dhcp_big_endian.pcapng SHB rebuilds 4d3c2b1a (LE) rebuilds big-endian; make(block={'byteorder': 'big'}) also works
1273 make(direction=INBOUND) writes 00000040; wire e5010000 → ValueError 01000000; (1, 1, 15)
1277 NRB names ('愮數慭灬攀戮數慭灬攀',) ('a.example', 'b.example'); a non-UTF-8 name rebuilds byte-exactly
1278 unknown block: invalid length: 8 byte-exact
1281 two verdicts: option must be only one, but 2 found byte-exact
1286 format: {line!r} format: 'a b c'

Breaking changes:

  • EPB_FlagsOption/PACK_FlagsOption gain a required reserved field (bits 9-23, verbatim), and the makers gain reserved=0.
  • The schema flags is a new LSB-0 FlagsField that reads the word in the section's byte order.
  • An undefined direction (0b11) or reception (5-7) raises ProtocolError.
  • A WireGuard line without exactly three fields now raises FieldValueError instead of ValueError.

Judgement call: bits 9-11 (checksum and TSO offload) and 16-23 sit inside reserved instead of getting named fields.

Existing tests that pinned old behaviour, now updated in test_pcapng_unit.py:

  • the flags dict and the two flags data models gain reserved
  • the two unknown-block bodies are now padded
  • epb_verdict is dropped from the duplicate-guard list

The 11 new tests are in TestPCAPNGModelling in tests/protocols/misc/test_pcapng_roundtrip2_unit.py. With #1373's three source files written back over this fix, the run reports 12 failed, 9 passed. All 11 new tests fail, along with the ns_dnsname subtest: Latin-1 caf\xe9 was read as café by charset guessing. With the fix, the module reports 19 passed, 7 subtests passed.

Fixture check: all 165 blocks of the six examples/captures/*.pcapng files now rebuild byte-exactly (0 mismatches).

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
  • test_docstring_contract: 7 passed
  • run_unittest_leg.py protocols --exclude protocols/internet: 940 tests, 0 failures
  • tests/project: 389 passed, 1 failed (test_isort_clean, an over-long import line). After isort fixed the line, test_isort_clean passes on its own.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on e55260826: GOOD TO GO (Sonnet reviewed; author Opus). Reviewed as a delta on #1373.

Retarget to main after #1373 merges, or its Closes lines will not fire.

Follow-ups filed, both pre-existing: #1382 — an undefined direction or reception value raises, so such a capture cannot round-trip, and bits 9–11 deserve names; #1383 — an NRB name list ending in empty names loses them.

@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
Base automatically changed from fix/pcapng-roundtrip-padding to main October 8, 2026 00:56
@JarryShaw

Copy link
Copy Markdown
Owner Author

Resolve merge conflicts

@JarryShaw

Copy link
Copy Markdown
Owner Author

Acknowledged, resolving the conflicts. #1373 merged into main, and #1378 was stacked on it, so the branch now conflicts. A worker is rebasing it onto current main and re-running the PCAP-NG legs. The new head will go back to review: pending for a delta review.

#1278, #1281, #1286)

- A Section Header Block keeps its parsed byte order; make() takes one.
- epb_flags/pack_flags number bits from the least-significant bit, in the
  section's byte order; bits 9-23 are kept as `reserved`, and an undefined
  direction or reception raises ProtocolError.
- NRB record names and ns_dnsname decode as UTF-8, and a name that is not
  UTF-8 keeps its octets through the split into names.
- An unknown block's length counts the 12 framing octets and its padding.
- epb_verdict may repeat.
- The WireGuard key log error names the offending line.
@JarryShaw
JarryShaw force-pushed the fix/pcapng-roundtrip-modelling branch from e552608 to 497a3e9 Compare October 8, 2026 01:07
@JarryShaw

Copy link
Copy Markdown
Owner Author

Conflicts resolved. The branch still carried #1373's pre-squash commit 68026d540, which duplicates the squashed 0e25f9fe1 on main. The rebase with --onto origin/main 68026d540 dropped it, and #1378's own commit replayed cleanly as 497a3e95c. Nothing had to be resolved by hand.

The verdict carries over. The patch-id is identical across the original e55260826, the rebased 497a3e95c, and the diff of your merge d05e44495 against main, so the review: good-to-go label stays. The PCAP-NG modules and tests/project pass on the rebased tree. I removed the stale "Stacked on #1373" line from the body.

JarryShaw added a commit that referenced this pull request Oct 8, 2026
…and zero-filled on rebuild

OptionField.unpack reads each option through a proxy that notes short
reads, and raises ProtocolError when an option that began with octets
present then reads past the end of its data. An option starting at the
end of the data keeps the end-of-option-list handling. Inside a parent
layer the octets are kept as captured and rebuild byte for byte.

BREAKING CHANGE: a truncated IPv4 or TCP option no longer parses with
zero-filled data; the layer raises ProtocolError, and a parent keeps it
as raw octets. The #431/#572 pins and the #1394 raw-item cases are
rewritten to match. PCAP-NG is a follow-up after #1378.

Refs #1325
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Coverage: 89.41% (unit tier, Python 3.14, d05e44495, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18791 968 2352 869 90.90%
pcapkit/corekit 2018 88 628 24 94.78%
pcapkit/dumpkit 234 2 84 2 98.74%
pcapkit/foundation 2573 99 910 44 94.63%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16304 204 4306 166 98.17%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2342 1006 157 43.25%

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

@JarryShaw
JarryShaw merged commit 4aefe94 into main Oct 8, 2026
36 checks passed
@JarryShaw
JarryShaw deleted the fix/pcapng-roundtrip-modelling branch October 8, 2026 02:34
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 8, 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

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