Skip to content

fix(pcapng): handle make(block=<bytes>) for a DSB (#2354) - #2370

Merged
JarryShaw merged 1 commit into
mainfrom
fix/2354-pcapng-dsb-raw-block-bytes
Oct 27, 2026
Merged

fix(pcapng): handle make(block=<bytes>) for a DSB (#2354)#2370
JarryShaw merged 1 commit into
mainfrom
fix/2354-pcapng-dsb-raw-block-bytes

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — bug fix

Description

Closes #2354.

  • Fix: unpack now re-unpacks when the made header's block is raw bytes, off the buffer pack has just filled — the same way a real file read would parse those octets. make itself is untouched, since an existing test pins make(block=b'raw').block == b'raw' as a deliberate verbatim escape hatch.
  • Test: the issue's own DSB repro (TLS key log, decoded correctly); a round trip for each of IDB, EPB, PB, SPB, ISB, systemd, CB and unknown via make(block=<bytes>), byte-exact against the built data and data model; and the rejection cases (too-short buffers, a length of 0).
  • Fail-without-fix: 3 failed on origin/main (AttributeError: 'bytes' object has no attribute 'length'). Passing on the branch.
  • Known gap, not closed here: a Section Header Block handed raw octets with no section context — how an SHB is normally built, since it starts its own section — still raises AttributeError: 'PCAPNG' object has no attribute '_byte' from pack, because the byte order a section header carries is the one its own maker chose and no maker runs for raw octets. With a section context present an SHB does round trip. Tracked separately.
  • Note: an external PR (fix(pcapng): parse a raw bytes block given to make() (#2354) #2360) addresses the same issue, with tests over IDB/EPB/DSB; this is our own implementation, covering the block types listed above, and it has the same SHB gap.

@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 27, 2026
@JarryShaw JarryShaw added bug Issues reporting a defect (set by the bug report template; a default, not an assessment) 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 27, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 27, 2026
@JarryShaw JarryShaw mentioned this pull request Oct 27, 2026
Closed
5 of 13 tasks
@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Review: NEEDS CHANGES at b816978dc (Sonnet). Core fix is correct; two prose/docs issues before merge.

  1. Stale _check_block_floor docstring Note. It still says the floor "runs on the parsing path only" and that a too-short constructed block is "already reported by read's own check" — both false now. make(block=b'raw') on a too-short block raises StreamEOFError from this exact floor, pinned by this PR's own test. This method is rendered in docs/source/pcapkit/protocols/misc/pcapng.rst, so it's user-facing.
  2. Overclaim on scope. The PR body says "this covers every block type" and calls fix(pcapng): parse a raw bytes block given to make() (#2354) #2360 "DSB only" — neither holds. With a section context the fix is broad, but a raw-bytes SHB built with ctx=None still raises AttributeError: 'PCAPNG' object has no attribute '_byte' in pack() — the same gap fix(pcapng): parse a raw bytes block given to make() (#2354) #2360 has, and fix(pcapng): parse a raw bytes block given to make() (#2354) #2360's own tests aren't DSB-only either (IDB/EPB covered too).

Scoping the wording rather than widening the code — this PR's own title is DSB-focused, and I don't want to expand it under review pressure. Will file a follow-up issue for the SHB/ctx=None gap separately. Fix in progress.

@JarryShaw
fix(pcapng): handle make(block=<bytes>) for a DSB (#2354)
731584c 
`block` is typed to accept raw bytes, and the header schema writes such a block
back verbatim past the Block Type -- but `read` then looked for the block's
fields on the schema `make` returned, which held the octets, so a Decryption
Secrets Block built that way raised `AttributeError: 'bytes' object has no
attribute 'length'`. So did every other block type that got as far as `read`; a
Section Header Block with no section context failed sooner, on a missing
`_byte`.

`unpack` now parses those octets off the buffer `pack` has just filled, as a
reader of the file would, instead of reusing the schema that carries them. The
block that comes back is the one the octets describe, and it rebuilds byte for
byte; octets that are no block raise `StreamEOFError` or `ProtocolError`
instead. A Section Header Block handed raw octets with no section context --
how an SHB is normally built, since it starts its own section -- still fails on
that missing `_byte` and is left for a follow-up.

Tested by a new unit module: the issue's DSB, with its TLS key log decoded and
its octets written back unchanged; one round trip per block type the module
lists; and the short and length-zero cases.
@JarryShaw
JarryShaw force-pushed the fix/2354-pcapng-dsb-raw-block-bytes branch from b816978 to 731584c Compare October 27, 2026 06:52
@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Amended b816978dc → 731584c56: fixed the stale _check_block_floor docstring Note (now describes both unpack paths, verified by a clean Sphinx render — 0 warnings against a pinned docs-venv) and scoped the PR body/test-module wording off the "every block type"/"#2360 is DSB-only" overclaims. The SHB+no-context gap is now stated plainly as a known, separate limitation rather than implied-fixed. No code changed. 3/3 tests pass.

@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Review: GOOD TO GO at 731584c56 (self-certified — prose-only fix matching the reviewer's exact two asks, no code change). Verified: tests/protocols/misc/test_pcapng_raw_block_bytes_2354_unit.py 3/3 pass; a pinned-docs-venv Sphinx render of pcapng.rst gives 0 warnings with the corrected Note rendering correctly; grepped the repo for anything pinning the removed "every block type"/"DSB only" phrasing — nothing does. CI and ordering leg re-running on the new sha.

@github-actions

github-actions Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Coverage: 91.58% (unit tier, Python 3.15, 731584c56, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19601 881 2620 867 92.12%
pcapkit/corekit 2596 74 818 37 96.51%
pcapkit/dumpkit 282 3 102 3 98.44%
pcapkit/foundation 3029 23 1050 25 98.82%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17456 218 4598 185 98.15%
pcapkit/toolkit 914 36 334 10 95.51%
pcapkit/utilities 502 7 144 5 98.14%
pcapkit/vendor 4432 1988 1018 167 51.87%

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

@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Ready to merge.

Review: GOOD TO GO (self-certified round 2, Sonnet round 1) at 731584c56.
CI: Required checks passed, 0 failures (only the non-blocking experimental GraalPy leg still pending).
Ordering leg: protocols clean — 1971 test(s), 0 failure(s), 0 error(s).
Mergeable, 0 unresolved review threads.

@JarryShaw JarryShaw added the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 27, 2026
@JarryShaw
JarryShaw merged commit 7ad0efb into main Oct 27, 2026
62 checks passed
@JarryShaw
JarryShaw deleted the fix/2354-pcapng-dsb-raw-block-bytes branch October 27, 2026 15:15
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 27, 2026
@JarryShaw JarryShaw removed review: good-to-go Cross-review at the current head says ready; CI state is separate review: running A cross-review is in flight against the current head - no verdict yet labels Oct 27, 2026
@JarryShaw JarryShaw mentioned this pull request Oct 28, 2026
Merged
4 tasks done
JarryShaw added a commit that referenced this pull request Oct 28, 2026
@JarryShaw
docs(changelog): backfill entries for the #2312-#2407 range (#2409)
7f59b3e 
Problem: of the 60 pull requests merged in the #2312-#2407 range, only
#2334, #2336, #2337, #2340 and #2348 wrote a 1.5.0 changelog entry, and
#2398 and #2403 only reworded lines that were already there. Every other
user-visible change in the range -- wire-semantics fixes, maker guards,
reassembly behaviour, public-API and error-message changes -- went
unrecorded, so the entry no longer describes the release.

Fix: one entry per user-visible pull request, in each module section's
existing style and ordered by the issue it closes. Two pairs share a
bullet, since each is the direct follow-up of the other: #2393 with
#2401, and #2405 with #2407. The diff on the entry file is purely
additive.

Internal-only work is left out deliberately: dependency and action bumps
(#2387 to #2391), test-only and docs-prose passes (#2333, #2376, #2379,
#2380, #2402), the unreachable-branch removal (#2378), and the #: comment
marker the HIP Group and Packet crawlers now write (#2381). That last one
renders nothing either way: the marker lands inside the generated
_missing_ body, conf.py's exclude-members drops _missing_ from autodoc,
and a comment in a method body is not an attribute comment to begin with.

#2392 and #2403 are recorded together, under Added rather than Fixed, for
the declarations they introduce: __proto_key__ on the seven classes owning
a __proto__ registry (#2392), __proto_dest__ on the four enum registries
and Vendor.PROTO_DEST to generate it (#2403). Both PRs added the autodoc
entries, so both are rendered public attributes. #2392's rejection-message
change is deliberately not recorded as a fix: the bare "code must be ..."
text it prefixed came from _check_code, which #2193 introduced and which
is in no release tag -- absent at v1.5.0b14 -- so no released version ever
emitted it. #2403 is no longer skipped as internal because it reworded an
existing bullet to name __proto_dest__ while nothing credited the
attribute's introduction.

Four claims are corrected against the source rather than carried over:
FTP_DATA shipped in pcapkit/protocols/application/ftp.py from 1.0.0, not
1.4.0 (class FTP_DATA(Raw) is there at v1.0.0); #2365's pre-fix fallback
read cmmd.value, so a .value-carrying object was accepted unchecked
rather than raising; #2399 left the dpkt adapter and the adapters'
ModuleNotFound raises alone, so it claims no universal prefix; and
#2397's old tail did name the version it got, so the entry quotes it and
gains the :issue:`2302` it was missing. #2370's "a Section Header Block
... still fails" sentence is dropped: #2369 settles the byte order first,
and an SHB built from raw octets with ctx=None now rebuilds byte-exact in
both byte orders.

CHANGELOG.md is regenerated with util/changelog_md.py.

Test: tests/project/ in full, 0 failures, including test_changelog_md.py's
in-step and supported-subset checks; util/changelog_md.py --check reports
the two files in step; the entry parses under docutils with no structural
warning.
JarryShaw added a commit that referenced this pull request Oct 28, 2026
@JarryShaw
docs(changelog): backfill entries for the #2312-#2407 range (#2409)
813e9f8 
Problem: of the 60 pull requests merged in the #2312-#2407 range, only
#2334, #2336, #2337, #2340 and #2348 wrote a 1.5.0 changelog entry, and
#2398 and #2403 only reworded lines that were already there. Every other
user-visible change in the range -- wire-semantics fixes, maker guards,
reassembly behaviour, public-API and error-message changes -- went
unrecorded, so the entry no longer describes the release.

Fix: one entry per user-visible pull request, in each module section's
existing style and ordered by the issue it closes. Three pairs share a
bullet, since each is the direct follow-up or counterpart of the other:
#2393 with #2401, #2405 with #2407, and #2392 with #2403. The diff on the
entry file is purely additive.

Internal-only work is left out deliberately: dependency and action bumps
(#2387 to #2391), test-only and docs-prose passes (#2333, #2376, #2379,
#2380, #2402), the unreachable-branch removal (#2378), and the #: comment
marker the HIP Group and Packet crawlers now write (#2381). That last one
renders nothing either way: the marker lands inside the generated
_missing_ body, conf.py's exclude-members drops _missing_ from autodoc,
and a comment in a method body is not an attribute comment to begin with.

#2392 and #2403 are recorded together, under Added rather than Fixed, for
the declarations they introduce: __proto_key__ on each class owning a
__proto__ registry (#2392), __proto_dest__ on the enum registries and
Vendor.PROTO_DEST to generate it (#2403). The bullet names those classes
and registries rather than counting them, per the no-counted-numbers rule
for the unreleased entry. Both PRs added the autodoc entries, so both are
rendered public attributes. #2392's rejection-message
change is deliberately not recorded as a fix: the bare "code must be ..."
text it prefixed came from _check_code, which #2193 introduced and which
is in no release tag -- absent at v1.5.0b14 -- so no released version ever
emitted it. #2403 is no longer skipped as internal because it reworded an
existing bullet to name __proto_dest__ while nothing credited the
attribute's introduction.

Four claims are corrected against the source rather than carried over:
FTP_DATA shipped in pcapkit/protocols/application/ftp.py from 1.0.0, not
1.4.0 (class FTP_DATA(Raw) is there at v1.0.0); #2365's pre-fix fallback
read cmmd.value, so a .value-carrying object was accepted unchecked
rather than raising; #2399 left the dpkt adapter and the adapters'
ModuleNotFound raises alone, so it claims no universal prefix; and
#2397's old tail did name the version it got, so the entry quotes it and
gains the :issue:`2302` it was missing. #2370's "a Section Header Block
... still fails" sentence is dropped: #2369 settles the byte order first,
and an SHB built from raw octets with ctx=None now rebuilds byte-exact in
both byte orders.

CHANGELOG.md is regenerated with util/changelog_md.py.

Test: tests/project/ in full, 0 failures, including test_changelog_md.py's
in-step and supported-subset checks; util/changelog_md.py --check reports
the two files in step; the entry parses under docutils with no structural
warning.
JarryShaw added a commit that referenced this pull request Oct 28, 2026
@JarryShaw
docs: backfill changelog entries for the #2312-#2407 range (#2412)
27c797d 
Problem: of the 60 pull requests merged in the #2312-#2407 range, only
#2334, #2336, #2337, #2340 and #2348 wrote a 1.5.0 changelog entry, and
#2398 and #2403 only reworded lines that were already there. Every other
user-visible change in the range -- wire-semantics fixes, maker guards,
reassembly behaviour, public-API and error-message changes -- went
unrecorded, so the entry no longer describes the release.

Fix: one entry per user-visible pull request, in each module section's
existing style and ordered by the issue it closes. Three pairs share a
bullet, since each is the direct follow-up or counterpart of the other:
#2393 with #2401, #2405 with #2407, and #2392 with #2403. The diff on the
entry file is purely additive.

Internal-only work is left out deliberately: dependency and action bumps
(#2387 to #2391), test-only and docs-prose passes (#2333, #2376, #2379,
#2380, #2402), the unreachable-branch removal (#2378), and the #: comment
marker the HIP Group and Packet crawlers now write (#2381). That last one
renders nothing either way: the marker lands inside the generated
_missing_ body, conf.py's exclude-members drops _missing_ from autodoc,
and a comment in a method body is not an attribute comment to begin with.

#2392 and #2403 are recorded together, under Added rather than Fixed, for
the declarations they introduce: __proto_key__ on each class owning a
__proto__ registry (#2392), __proto_dest__ on the enum registries and
Vendor.PROTO_DEST to generate it (#2403). The bullet names those classes
and registries rather than counting them, per the no-counted-numbers rule
for the unreleased entry. Both PRs added the autodoc entries, so both are
rendered public attributes. #2392's rejection-message
change is deliberately not recorded as a fix: the bare "code must be ..."
text it prefixed came from _check_code, which #2193 introduced and which
is in no release tag -- absent at v1.5.0b14 -- so no released version ever
emitted it. #2403 is no longer skipped as internal because it reworded an
existing bullet to name __proto_dest__ while nothing credited the
attribute's introduction.

Four claims are corrected against the source rather than carried over:
FTP_DATA shipped in pcapkit/protocols/application/ftp.py from 1.0.0, not
1.4.0 (class FTP_DATA(Raw) is there at v1.0.0); #2365's pre-fix fallback
read cmmd.value, so a .value-carrying object was accepted unchecked
rather than raising; #2399 left the dpkt adapter and the adapters'
ModuleNotFound raises alone, so it claims no universal prefix; and
#2397's old tail did name the version it got, so the entry quotes it and
gains the :issue:`2302` it was missing. #2370's "a Section Header Block
... still fails" sentence is dropped: #2369 settles the byte order first,
and an SHB built from raw octets with ctx=None now rebuilds byte-exact in
both byte orders.

CHANGELOG.md is regenerated with util/changelog_md.py.

Test: tests/project/ in full, 0 failures, including test_changelog_md.py's
in-step and supported-subset checks; util/changelog_md.py --check reports
the two files in step; the entry parses under docutils with no structural
warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix)

Projects

PyPCAPKit
Status: Done

Milestone

PyPCAPKit v1.5

Development

Successfully merging this pull request may close these issues.

fix(pcapng): make(block=<bytes>) for a DSB raises AttributeError, not handled

1 participant

@JarryShaw