Skip to content

fix(pcapng): keep captured padding octets on rebuild (#1437) - #1441

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1437-ipv4-l2tp-pcapng-padding
Oct 9, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1437-ipv4-l2tp-pcapng-padding

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Refs #1437 (part B). Part A, the MH sites, is a separate PR.

Measured on main first:

  • PCAP-NG zero-filled. SPB, EPB and Packet Block data padding, DSB secrets padding, IDB reserved octets, and the padding of every option and NRB record all rebuilt as zeros. For example, SPB …61eeeeee… came back as …61000000…. Octets after opt_endofopt were dropped, so SHB 24000000 … 00000000 dddddddd rebuilt at length 0x20. The reader now keeps each one on the data model through the shared _read_padding/_make_padding helpers: padding_data, reserved and padding only when not all zeros, and padding_opts whenever present, written back after the options. make() writes them back, and fresh builds still write zeros. The new keys are optional, so no data model gains a required field.
  • IPv4 and L2TP already round-trip. These are the Quick-Start report Not Used octet, octets after EOOL, and the L2TPv2 offset pad. There is no code change, only guard tests.

Tests: tests/protocols/misc/test_pcapng_padding_kept_1437_unit.py (22 failures and 1 error on main) and tests/protocols/test_ipv4_l2tp_padding_guard_1437_unit.py (a guard that passes on main). Per-frame info across the 23 make_samples.py captures is identical before and after.

- PCAP-NG padding rebuilt as zeros: SPB/EPB/Packet Block packet data
  padding, DSB secrets padding, IDB reserved octets, and the padding of
  every option and NRB record. Each is now kept on the data model when it
  is not all zeros (`padding_data`, `reserved`, `padding`) and written
  back by make(); fresh builds still write zeros.
- Octets after `opt_endofopt` were dropped, shrinking the block. They are
  kept as `padding_opts` and written back after the options.
- IPv4 Quick-Start/EOOL padding and the L2TPv2 offset pad already
  round-trip on main; guard tests added, no code change.

Refs #1437 (part B).
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 8, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 8dac30e9c (Sonnet cross-review; the author ran on Opus).

  • IPv4/L2TP already round-trip on main. The worker's claim is confirmed. Quick-Start Not Used, octets after EOOL (1, 3, 7) and L2TPv2 offset pad (1, 3) all rebuild exactly on 16c87f901. The LSR/RR/SSR remainder pads are unreachable, because a length that leaves a remainder raises invalid format.
  • PCAP-NG. 66 crafted cases per byte order cover SHB/IDB/EPB/SPB/PB/NRB/ISB/DSB/systemd/custom/unknown blocks. They exercise data, option, record and reserved padding plus post-opt_endofopt octets. main rebuilds 24/132 exactly and the PR 128/132; the other 4 are harness cases that fail on both.
  • Fresh builds still write zeros. Wrong-width option padding follows fix(corekit)!: non-zero padding octets are zeroed on rebuild (#1223) #1436's PaddingField.pack.
  • Real fixtures. Every block's info in all 6 examples/captures/*.pcapng is byte-identical to main, so no new key appears on real captures. Block-by-block rebuild is exact on both.
  • Tests, which I re-ran: the new modules give 6 passed with 27 subtests. With the two pcapng.py files from main the padding module gives 23 failed, 2 passed. The IPv4/L2TP guard module passes on main, as a guard should.
  • Merge and engines. It merges into 7c6203892 with no conflicts, and the misc leg is 385 OK. The engine and dumpers only call to_dict(), and the json/plist/tree/xml outputs all extract.

Nits, which are the owner's call:

  • PCAPNG(type=…, block={…}) honours option-level padding but silently drops block-level padding_data/reserved/padding_opts. That is consistent with "fresh builds write zeros", but the two levels differ.
  • A padding_opts length that is not a multiple of 4 raises a ProtocolError naming the block length, not the key.

@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 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Coverage: 89.49% (unit tier, Python 3.14, 8dac30e9c, 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 2043 89 636 25 94.77%
pcapkit/dumpkit 234 2 84 2 98.74%
pcapkit/foundation 2648 98 930 48 94.80%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16712 218 4498 182 98.08%
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 8fcee46 into main Oct 9, 2026
41 checks passed
@JarryShaw
JarryShaw deleted the fix/1437-ipv4-l2tp-pcapng-padding branch October 9, 2026 00:17
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 9, 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

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant