Skip to content

fix(corekit)!: non-zero padding octets are zeroed on rebuild (#1223) - #1436

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1223-padding-kept-as-captured
Oct 8, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1223-padding-kept-as-captured

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 8, 2026 •

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

Closes #1223. Closes #1329 (its PadN remainder; the rest merged in #1345).

PaddingField now packs the octets it was given (zeros when absent; a value of the wrong width is truncated or zero-filled, as before, so fresh builds are unchanged); Schema.pack delegates to it. Each site carries the octets through reader, data model and make(): HOPOPT/IPv6-Opts PadN pad and QS report unused, IPv6-Frag reserved_octet, IPv6-Route SR0/Type 2 reserved and RPL padding, MH PadN option/sub-option data, HTTP/2 padding ×3, OSPF crypto reserved, and reserved on the 12 HIP parameters listed here. HIP and SCTP trailing alignment padding is kept as a padding attribute only when non-zero.

Breaking: the data models above gain required fields; five existing test modules that build them by hand were updated.

Before → after, e.g. HOPOPT PadN 3b000104aabbccdd → 3b00010400000000 → exact; SCTP DATA …61ffffff → …61000000 → exact. New tests: tests/protocols/test_padding_kept_as_captured_unit.py, tests/protocols/internet/test_mh_padn_contents_unit.py (both fail on main: 32 + 3 failures). TCP 020405b4 00aabbcc already round-trips on main; kept as a guard.

Left for follow-up (other owners): IPv4 Quick-Start report and option padding (ipv4.py), L2TP offset pad (link/), PCAP-NG SPB and option padding (misc/pcapng.py reader/maker needed), and ~37 MH reserved PaddingField sites not listed on these issues.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) breaking Breaks public-facing behaviour or API (apply alongside the type label) 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.47% (unit tier, Python 3.14, 03265271b, 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 2637 98 928 48 94.78%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16657 219 4484 183 98.06%
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

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 3edd77fab (Sonnet cross-review; the author ran on Opus). One blocking regression, which I reproduced:

  • L2TPv2 wrong-width padding is now zero-filled. L2TPv2(offset=3, padding=X, payload=b'xx').data:

    X main 3edd77fab
    aa …0003aa00007878 (zero-padded) …0003000000 7878
    aabbccddee …0003aabbcc7878 (truncated) …0003000000 7878

    ConditionalField-wrapped PaddingFields used to pack through BytesField (struct Ns). The new PaddingField.pack returns zeros for any wrong width, which contradicts link/l2tpv2.py:283 ("zero-filled or truncated to offset octets"). The fix is to keep the truncate/zero-pad semantics for a non-empty wrong-width value, plus a pinning test.

  • Non-blocking:

    • Consider warning on a wrong-width value.
    • The new data-model fields are required, so PadOption(type=…, length=4) and IPv6_Frag(...) without them now raise TypeError. The makers also read them inconsistently: some use getattr(..., b''), some read strictly. Defaults or tolerant reads, chosen consistently.
  • Everything else held:

    • Byte-exact: HOPOPT PadN, IPv6-Frag, MH PadN, HTTP/2 DATA, SCTP DATA and HIP reserved/trailing were all lossy on main and are exact here.
    • Captures: 120 of 1614 frames gain only all-zero keys (additive), and all 4858 layers rebuild exactly.
    • Tests: the five edited test modules only supply the new fields.
    • Legs: protocols/internet 579, the rest of protocols 1082, corekit 633 and project 591, all with 0 failures.

The worker is addressing the regression; the label is now review: needs-changes.

@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 8, 2026
PaddingField now packs the octets it was given, truncated or zero-filled
to its width as struct's `s` format does, and zeros when it has none, so
a freshly built packet is unchanged. Schema.pack delegates its padding
branch to the field.

Each site carries the octets through its reader, data model and make();
every maker reads the new field tolerantly, defaulting to zeros:

- HOPOPT/IPv6-Opts PadN `pad`, QS report `unused`
- IPv6-Frag `reserved_octet`; IPv6-Route SR0/Type 2 `reserved`, RPL `padding`
- MH PadN option and Flow-ID PadN sub-option `data` (#1329)
- HTTP/2 DATA/HEADERS/PUSH_PROMISE `padding`; OSPF crypto auth `reserved`
- HIP: `reserved` on the 12 parameters listed on #1223
- HIP and SCTP trailing alignment padding: kept on the data model as
  `padding` only when not all zeros, and written back on make

Existing tests that build these data models by hand now supply the new
fields.
@JarryShaw
JarryShaw force-pushed the fix/1223-padding-kept-as-captured branch from 3edd77f to 0326527 Compare October 8, 2026 19:57
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 8, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 03265271b (round 2, Sonnet cross-review; the author ran on Opus).

  • L2TPv2 regression is fixed, re-derived by me: L2TPv2(offset=3, padding=X, payload=b'xx').data matches main for aa, aabbcc and aabbccddee. The reviewer also matched b'', None and the HTTP/2 wrong-width cases. The new pin test fails on round 1's pack.
  • Tolerant reads: all 21 maker reads use getattr(..., <zero default>), and the HIP defaults are per width. A stand-in that lacks the new fields rebuilds with zeros. The data-model constructors still require the new fields; Info (corekit/infoclass.py:150-201) generates __init__ without defaults, so this is consistent with the !.
  • No warning on wrong width: accepted. Captured padding always has the field's width, so only make() callers reach that branch, and L2TP documents silent truncate/zero-fill there.
  • Regression re-check:
    • The byte-exact rows (HOPOPT PadN, IPv6-Frag, MH PadN, HTTP/2, SCTP, HIP) are still exact.
    • Across 23 captures, 1614 frames gain only additive zero-valued keys, and make_samples md5s are identical to main.
    • Legs: protocols/internet 579, the rest of protocols 1084, corekit 633 and project 591, all with 0 failures. pytest on tests/project gives 404, which is the same suite counted differently.
  • Leftover PaddingField sites that are not in fix(corekit): non-zero padding octets are zeroed on rebuild #1223/fix(mh): reserved fields, BU/BA flag bits and PadN contents are written back as zero #1329 are tracked in fix(corekit): PaddingField sites outside #1223 still zero-fill their padding on rebuild #1437.

@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
@JarryShaw
JarryShaw merged commit 3fdcf53 into main Oct 8, 2026
62 of 64 checks passed
@JarryShaw
JarryShaw deleted the fix/1223-padding-kept-as-captured branch October 8, 2026 22:35
@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

Development

Successfully merging this pull request may close these issues.

fix(mh): reserved fields, BU/BA flag bits and PadN contents are written back as zero fix(corekit): non-zero padding octets are zeroed on rebuild

1 participant