Skip to content

fix(sctp): rebuild a last chunk short of its padding as captured (#1474) - #1477

Merged
JarryShaw merged 2 commits into
mainfrom
fix/1474-sctp-unpadded-last-chunk
Oct 9, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
fix/1474-sctp-unpadded-last-chunk

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

Closes #1474. A last chunk with fewer pad octets than its length calls for, or none, now rebuilds as captured through both from_data(info) and from_data(info.to_dict()). Before, padding_length (pcapkit/protocols/schema/transport/sctp.py:84) always packed the full width, so 17 octets in gave 20 out.

Why the existing padding key. #1223 already keeps a chunk's, parameter's or cause's padding in info when a fresh build would not reproduce it (non-zero). A short pad is the same situation, so _keep_padding now also keeps it when it is short, possibly as b''. It is the same key, holding the octets as captured, and it already round-trips through to_dict(). A __short_read__-style dunder would duplicate that record. trailer cannot carry it either, because it follows the chunk, which would still pad itself. No new key, so no !.

On rebuild, _restore_padding gives the schema a __padding__ width that padding_length packs to. The pre_pack of Chunk, Parameter and ErrorCause resets the key for each schema, because Schema.pack shares one context across siblings. Only the last item of a list keeps the record: an earlier chunk, and anything nested in it, packs to the full aligned width even if an edited info says otherwise. make takes no padding keyword, as the data-model docstrings now say.

The five #1474 Gap rows are deleted from the transport edge table. New module tests/protocols/transport/test_sctp_unpadded_last_chunk_unit.py: on origin/main sources, 14 failed and 6 passed; with this fix, 8 passed (12 subtests). The SCTP, transport-edge, capture-runtime and tests/project legs pass. The per-frame to_dict() hashes over examples/captures (1616 frames) match origin/main.

Out of scope: an INIT or ABORT whose final parameter or cause is also unpadded is still rejected at parse ("option that runs past the end of the data"), not mis-rebuilt.

@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 9, 2026
@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 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at e583895e4. Cross-reviewed on Sonnet; the author ran on Opus.

Required: honour the short-padding record only on the last item in the packet (chunk, parameter or cause), or document and test the alternative.

  • Today a non-last chunk whose padding is shortened in its info is packed verbatim, and the output is malformed.
  • Repro, which I re-ran myself: parse COMMON + chunk(0x3f, b'a') + chunk(0xbf, b'b'), set the first chunk's padding to b'' via __update__, then rebuild with SCTP.from_data(d).
  • The result is 3f00000561 bf00000562 000000: the second chunk starts unaligned, and re-parsing it swallows that chunk's header as padding.
  • main zero-fills in this case.

Confirmed fine:

  • Root cause and mechanism are as described.
  • The per-schema pre_pack reset works for interleaved short and full chunks.
  • No padding key appears on ordinary traffic: the 34 SCTP frames in examples/captures and synthetic full-width cases carry none, and per-capture to_dict() hashes match main.
  • All five fix(sctp): a last chunk sent without its padding rebuilds with the padding zero-filled #1474 Gap rows close.
  • 8 threads × 200 rebuilds produced no cross-packet leak.
  • The new test fails on main's sources (14) and passes here; I confirmed this myself.

Minor: make(chunks=[...]) ignores a padding keyword, while the from_data path honours it. Please state this in the docstrings.

Separate defect, filed as its own issue: an INIT or ABORT whose final parameter or cause is unpadded at the end of the packet is rejected at parse time. This happens on main too.

@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 9, 2026
@JarryShaw
JarryShaw force-pushed the fix/1474-sctp-unpadded-last-chunk branch from e583895 to 3c064da Compare October 9, 2026 13:53
@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 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 3c064da75. This is the round-2 cross-review, run on Sonnet; the author ran on Opus. The source change is correct. What is missing is a test.

  • The gap. Nothing tests the nested half of _last_item_only: the for name in ('parameters', 'error') loop at pcapkit/protocols/transport/sctp.py:305. I disabled that loop (for name in ()) and test_sctp_unpadded_last_chunk_unit.py still passed, 8 of 8. So the loop is untested.
  • Why it matters. The reviewer probed a non-last INIT whose last parameter has padding=b'', followed by a SHUTDOWN_ACK. With the loop it rebuilds aligned at 44 octets; without it, misaligned at 41. A non-last ABORT with a short last cause gives 28 octets with the loop and 25 without.
  • Fix. Add one test for each of those two cases, and show that both fail when the loop is disabled.

Everything else checked out:

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment review: running A cross-review is in flight against the current head - no verdict yet and removed review: running A cross-review is in flight against the current head - no verdict yet review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 9, 2026
A final SCTP chunk may arrive with fewer pad octets than its length calls
for, or none. Parsing read what was there, but the rebuild zero-filled the
rest, so 17 octets in gave 20 out.

- `_keep_padding` now keeps a chunk's, parameter's or cause's `padding` in
  the info when it is short as well as when it is non-zero, so the record
  is the octets as captured, possibly `b''`.
- `_restore_padding` hands a short record's width to the schema as
  `__padding__`, which `padding_length` packs to. The `pre_pack` of the
  chunk, parameter and cause bases resets the key per schema, so one
  chunk's record cannot shorten the next through the shared context.
- Only the last item of a list keeps that record; an earlier chunk, and
  the items nested in it, pack to the full aligned width.
- The five #1474 rows leave the transport edge-case known-failure table.

New tests/protocols/transport/test_sctp_unpadded_last_chunk_unit.py.
@JarryShaw
JarryShaw force-pushed the fix/1474-sctp-unpadded-last-chunk branch from 25f82aa to efa8793 Compare October 9, 2026 14:08
@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 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at efa879309. This is the round-3 delta review, run on Sonnet; the author ran on Opus. The commit adds 34 lines, to test_sctp_unpadded_last_chunk_unit.py only. I then rebased it onto main at bbfd4bf64: range-diff shows both commits as =, and the patch-id is unchanged.

  • The round-2 gap is closed. There are two new tests: a non-last INIT whose last parameter is short (44 octets), and a non-last ABORT whose last cause is short (28 octets).

  • Each test pins its own half of the loop at sctp.py:305. I re-ran this:

    Loop at sctp.py:305 Result
    for name in () 2 failed
    ('parameters',) only the ABORT test fails
    ('error',) only the INIT test fails
    unchanged 10 passed
  • Packet layout: the chunk length excludes the last item's padding, but the pad octets are still on the wire, so the next chunk stays aligned.

  • SCTP and transport edge files: 67 passed, 257 subtests.

Non-blocking: test_short_record_on_a_non_last_parameter_rebuilds_aligned iterates .values() over two parameters of the same type, so it only strips the first one. It still checks the case it is named for.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Coverage: 89.56% (unit tier, Python 3.14, efa879309, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 958 2356 871 90.99%
pcapkit/corekit 2107 93 672 27 94.60%
pcapkit/dumpkit 242 2 88 2 98.79%
pcapkit/foundation 2648 98 930 48 94.80%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 17079 240 4626 193 97.96%
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 ed3064c into main Oct 9, 2026
38 checks passed
@JarryShaw
JarryShaw deleted the fix/1474-sctp-unpadded-last-chunk branch October 9, 2026 17:40
@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 added a commit that referenced this pull request Oct 9, 2026
) (#1481)

The span of a chunk's nested parameter or error cause list counts the
chunk's trailing padding, which the last chunk of a packet may omit
(RFC 9260 Sec. 3.2). The last item then read past the data and parsing
raised ProtocolError for INIT, INIT ACK, HEARTBEAT, HEARTBEAT ACK, ABORT
and ERROR.

- Clamp the span to the octets present by wrapping nested_length in
  bounded, so the final item reads only the padding there is; the #1477
  short-padding record then rebuilds it byte-exactly.

New tests/protocols/transport/test_sctp_unpadded_final_param_unit.py.
@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.

fix(sctp): a last chunk sent without its padding rebuilds with the padding zero-filled

1 participant