Skip to content

fix(sctp)!: keep the octets after the last whole chunk as captured (#1468) - #1472

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1468-sctp-stray-octets
Oct 9, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1468-sctp-stray-octets

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Closes #1468

SCTP has no packet length, so octets after the last whole chunk can't be told apart from a cut chunk. Per #1458, the packet now parses and keeps them as trailer, as #1431/#1460/#1467 do. A raw last element was considered but not used: chunks is keyed by chunk type, so bytes would need a fake key, and from_data could not rebuild from it. A ChunkListField stops at the last chunk whose declared length is present (≥ 4). make(trailer=) writes the trailer under the checksum. A raw chunk passed to make() that is not whole is still rejected.

Before: COOKIE ACK + b'x' (and 3, 4, 5 octets, or a chunk declaring 40 with 8 present) raise ProtocolError. After: each parses and rebuilds exactly, through from_data(info) and from_data(info.to_dict()), alone and under IPv4.

tests/protocols/transport/test_sctp_stray_octets_unit.py: 7 tests, 36 subtests; before the fix, 6 tests and 33 subtests fail. Two existing tests pinned the old behaviour, a chunk running past the packet raising: I updated three test_sctp_unit cases so the declared octets are present, and dropped the SCTP case from test_declared_length_overrun_unit. Not in this PR: the last-chunk padding cut left over from #1465 (sctp/last-chunk-unpadded) is a different shape, octets missing rather than extra, and stays a known failure.

…1468)

SCTP has no packet length field, so octets after the last whole chunk -- under
a chunk header, a chunk whose length runs past the data, or one declaring less
than its own header -- cannot be told apart from a cut chunk, and they made
the whole packet raise "Field chunks has an option that runs past the end of
the data". The chunk list (`ChunkListField`) now stops at the last whole chunk
and the rest is a length-less `trailer` PayloadField; read() keeps it as
info.trailer when non-empty, make(trailer=) writes it under the computed
checksum, and _make_data round-trips it, as IPv4/IPv6 (#1209) and
L2TPv2/OSPF (#1455) do. A raw chunk given to make() that is not whole is
still rejected.

Breaking: such packets now parse instead of raising, and info gains a
`trailer` key when they carry those octets. Two tests whose malformed cases
relied on a chunk running past the packet are updated.
@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 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at ad90caeea. The cross-review ran on Sonnet.

  • Byte-exact under both from_data(info) and from_data(info.to_dict()) for 1–7 stray octets, a cut INIT, declared lengths 0/1/2/65535 and mixed chunk runs, both bare and inside IPv4. The CRC32c of the make(trailer=…) output matches an independent computation.
  • The tests bite. I re-ran this myself: with main's three SCTP sources the new module plus test_sctp_unit.py give 36 failed; with the PR's they all pass (7 and 37).
  • The edits to existing tests are legitimate. The three malformed-length cases still pin the exactly-4 check. The dropped INIT-parameter overrun case now round-trips through trailer rather than raising, which is what fix(protocol): main is red, IPv4 make() with an unparsable raw item no longer raises the wrapped error #1394 is after.
  • The per-frame to_dict() hashes over the 1590 frames of examples/captures are unchanged.
  • Trailer over short read is the right model here: SCTP carries no packet length, and __short_read__ (fix(schema): a header cut inside a fixed-width field rebuilds as captured (#1458, #1451) #1465) only records cuts in the top-level header.

Not blockers, and both identical on main:

@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.54% (unit tier, Python 3.14, ad90caeea, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 960 2356 873 90.97%
pcapkit/corekit 2043 89 636 25 94.77%
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 16924 227 4572 190 98.02%
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 6323852 into main Oct 9, 2026
42 checks passed
@JarryShaw
JarryShaw deleted the fix/1468-sctp-stray-octets branch October 9, 2026 13:18
@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

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(sctp): octets after the last whole chunk make the packet fail to parse

1 participant