Skip to content

fix(sctp): accept a final parameter or cause short of its padding (#1479) - #1481

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1479-sctp-unpadded-final-param
Oct 9, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1479-sctp-unpadded-final-param

Conversation

@JarryShaw

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

nested_length sizes a chunk's parameter or cause list to include the chunk's trailing padding. When the last chunk omits that padding (RFC 9260 §3.2), the last item read past the data and raised ProtocolError. The six nested lists (INIT, INIT ACK, HEARTBEAT, HEARTBEAT ACK, ABORT, ERROR) now wrap the callback in bounded, so the span stops where the data ends. The #1477 short-padding record already rebuilds the item byte-exactly.

ASCONF has no dedicated schema. It parses as an opaque chunk and was not affected.

The issue's INIT repro now parses. Its host name abcde has no NUL terminator, so _make_param_hostname still refuses to rebuild it. That check is older and fails the same way when the parameter is padded, so it is left for a separate issue. The new tests use abcd\0.

Tests: the new test_sctp_unpadded_final_param_unit.py fails 36/36 on main and passes on this branch. test_sctp*.py, test_transport_edge_roundtrip_unit.py and tests/project pass. repr(info.to_dict()) hashes over examples/captures (23 files) are unchanged.

Closes #1479

)

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 fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 3d721832a. Cross-reviewed on Sonnet; the author ran on Opus.

  • The fix: all 6 nested_length( call sites in schema/transport/sctp.py are now bounded(...); I counted them myself. On origin/main sources the new tests give 36 failed, 4 passed; at the head, 5 passed, 35 subtests passed.
  • No input that should raise now passes. The reviewer cut five INIT, INIT ACK, ABORT, ERROR and HEARTBEAT packets at every offset, 271 cuts in all. Only the 4 full-length unpadded packets change result: ProtocolError on main, parsed byte-exact at head. 56 malformed packets with overruns, oversized chunk lengths and short fixed headers behave the same on both trees.
  • No other list was missed. The other ListFields were already bounded. ASCONF and ASCONF-ACK parse as unknown chunks and round-trip at head.
  • Regression checks: the per-capture to_dict() hashes are identical across all 23 captures. The SCTP test files give 72 passed, 292 subtests.

The Host Name rebuild failure behind #1479's literal repro predates this PR and is tracked in #1482.

@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
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Coverage: 89.57% (unit tier, Python 3.14, 3d721832a, 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 17086 239 4628 192 97.97%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 431 4 124 4 98.56%
pcapkit/vendor 4409 2342 1006 157 43.25%

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

@JarryShaw
JarryShaw merged commit 1d910ae into main Oct 9, 2026
43 checks passed
@JarryShaw
JarryShaw deleted the fix/1479-sctp-unpadded-final-param branch October 9, 2026 18:20
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 9, 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): an INIT or ABORT whose final parameter or cause omits its padding is rejected at parse

1 participant