Skip to content

fix(udp)!: keep the octets past the UDP Length field as captured (#1461) - #1467

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1461-udp-trailer
Oct 9, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1461-udp-trailer

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly

  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])

  • Use Preview tab to see how your pull request will actually look like

  • Searched for similar pull requests

  • Followed the coding style (make pylint, make mypy, make isort) — make pylint not run; mypy is clean on the three UDP files; isort flags two TransportProtocol import lines that were already on main and that this PR leaves as is

  • make test passes, and a test case covers the change — the whole suite was not run (see below); a new test module covers the change

  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A — centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • 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 #1461

UDP's payload now stops at its Length field, and the octets after it are info.trailer (present only when non-empty), as IPv4/IPv6 (#1431), IPX (#1435) and L2TPv2/OSPF (#1460) do. make(trailer=) writes them outside the declared length. !: on a datagram with excess octets, the payload no longer includes them.

Repro 0035003500080000 + pp: before, from_data(info) gave 8 of 10 octets; now 10/10, through info and through info.to_dict(). A Length below 8, of 0, or past the data still rebuilds exactly with no trailer. pcapkit does not compute the UDP checksum, so that path is unchanged. examples/captures: per-frame info is identical on all 1616 frames, and none gains a trailer.

Tests: tests/protocols/transport/test_udp_trailer_kept_unit.py fails 5 cases before the fix. On 94ae9664c + this commit: protocols --exclude protocols/internet leg 1156 tests, 0 failures, 0 errors; test_capture_roundtrip_runtime.py + tests/project + the new module 427 passed, 1 skipped.

@JarryShaw JarryShaw added the fix Pull requests that fix a defect (fix: subject prefix) label Oct 9, 2026
@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
- The payload now stops at Length rather than at the end of the capture,
  and the octets after it are info.trailer, present only when non-empty,
  as IPv4/IPv6 (#1209) and IPX (#1433) do; make() accepts trailer= and
  writes it outside the length it declares. A Length below 8 still lets
  the payload take the rest, so it rebuilds exactly.
- tests/protocols/transport/test_udp_trailer_kept_unit.py pins the
  rebuild through from_data(info) and from_data(info.to_dict()).
@JarryShaw
JarryShaw force-pushed the fix/1461-udp-trailer branch from 81c2aee to 71b7a21 Compare October 9, 2026 03:49
@JarryShaw JarryShaw added 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
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

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

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 957 2356 870 91.00%
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 16898 222 4564 187 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: GOOD TO GO at 71b7a219c (Sonnet cross-review; the author ran on Opus).

  • 60 hand-built datagrams rebuild byte-exact through info and to_dict(). They cover Length 0, 1, 7–12, 20 and 65535, payloads of 0, 1 and 8 octets, with and without a trailer. A trailer appears only when there are octets past Length.
  • make(trailer=) writes outside the declared length.
  • 16 Ethernet/IPv4/UDP frames are exact. They cover ports 53, 67, 1701 and 9999, each with and without a trailer on both the UDP and IPv4 layers. The upper layer gets exactly Length − 8 octets.
  • I re-ran the issue repro myself: 10 of 10 octets, trailer=b'pp', in both forms.
  • The captures are unchanged. Per-frame info hashes match main for all 23 captures (1616 frames, 103 UDP). Nothing in foundation/ or application/ reads the UDP payload.
  • The tests fail without the fix: 5 failures with the UDP files taken from main.
  • Legs: rest 1156, internet 617, and capture/project 427, all passing. The transport edge module passes.
  • The ! is justified, and nothing builds Data_UDP directly.

@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
JarryShaw merged commit beb6c90 into main Oct 9, 2026
40 checks passed
@JarryShaw
JarryShaw deleted the fix/1461-udp-trailer branch October 9, 2026 13:17
@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 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(udp): octets past the UDP Length field are dropped on rebuild

1 participant