Skip to content

fix(toolkit): dpkt hands IPv4 reassembly what the default engine does (#1615, #1616) - #1622

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1615-1616-dpkt-ipv4-reassembly-input
Oct 12, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1615-1616-dpkt-ipv4-reassembly-input

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 10, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

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 #1615.
Closes #1616.

Only ipv4_reassembly in pcapkit/toolkit/dpkt.py changes, plus a new private helper _first_ipv4 next to it.

tests/toolkit/test_dpkt_ipv4_reassembly_input_1615_1616_unit.py checks dpkt's inputs field by field against the default engine's over 21 built cases. It fails without the fix. Disabling the depth bound, the later-fragment check or the short-jumbogram check also turns it red.

pylint: three new C0415, from imports kept inside the helper. mypy: no new errors.

…#1615, #1616)

- #1615: ipv4_reassembly took the network layer only, so IPv4 carried in
  IPv6 gave no input. The new _first_ipv4 walks IPv6 (behind extension
  headers, or 6in6) to the first IPv4 layer, bounds its octets by each
  Payload Length above, and stops where the default engine dissects no
  IPv4: a later fragment, more than 32 extension headers, or a zero
  Payload Length with no Jumbo Payload option.
- #1616: the input was cut at the Total Length, truncating a header longer
  than it and dropping a padded fragment's trailer. It is now the IHL's
  octets and the rest, as the default engine hands them; the reassembler
  applies the Total Length itself.

New tests/toolkit/test_dpkt_ipv4_reassembly_input_1615_1616_unit.py compares
dpkt's inputs with the default engine's over 21 cases; it fails without the
fix, and each guard is load-bearing.
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 10, 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 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 89.72% (unit tier, Python 3.14, 15c1f1803, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 3 20 3 95.16%
pcapkit/const 18858 954 2366 868 91.06%
pcapkit/corekit 2366 67 760 35 96.48%
pcapkit/dumpkit 275 3 100 3 98.40%
pcapkit/foundation 2884 147 1010 36 94.07%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 17115 226 4570 191 98.06%
pcapkit/toolkit 957 134 350 6 84.85%
pcapkit/utilities 494 4 146 4 98.75%
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 15c1f1803.

Sonnet cross-review. It compared every IPv4 reassembly input field and the datagrams, default against dpkt, on 49 chain cases: all 49 match on the PR, while 28 differ on the base. Link layers and 42 truncation cut points also match, and the nested Payload-Length extent was re-derived by hand: 78, then 30, so payload 10 as measured. captured only marks an attached buffer, so ignoring it loses nothing. The TSO tests (#1547, #1555) pass. Of 17 mutants of dpkt.py, 13 are killed; the survivors are redundant or unreachable clauses. I reproduced fail-before myself: 28 passed, then 22 failed on main's dpkt.py.

Non-blocking: two of the three function-local imports could move to module top, which the review checked creates no cycle. I narrowed the body's "any one guard" claim to the three guards the mutants confirm. Out of scope and filed as #1626: dpkt and scapy drop TCP and IPv4 behind some IPv6 chains, such as a Fragment header followed by AH, or Mobility.

@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 10, 2026
@JarryShaw
JarryShaw merged commit bbcabf1 into main Oct 12, 2026
40 checks passed
@JarryShaw
JarryShaw deleted the fix/1615-1616-dpkt-ipv4-reassembly-input branch October 12, 2026 23:37
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 12, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 12, 2026
JarryShaw added a commit that referenced this pull request Oct 13, 2026
…test

- The Total Length 10 case and the accepted 4in6 case set DF so that no
  engine made an IPv4 input, because dpkt cut the header at the Total
  Length (#1616) and found no IPv4 in IPv6 (#1615). #1622 fixed both, so
  DF now only hid two inputs from the comparison.
- Both cases drop DF and expect an IPv4 input (frames 17 and 40; 15 -> 17
  inputs compared); the unused df parameter of ipv4() goes with it.

With #1622's pcapkit/ change reverted the test now fails for dpkt (frame 17
header/payload differ, frame 40 missing); before, it passed. 4 passed on
3.14 and 3.11.
JarryShaw added a commit that referenced this pull request Oct 14, 2026
Third-party engines are best effort (#1687), so the dpkt code that exists
only to match the default engine comes out:

- revert #1662, #1655, #1624, #1622, #1614 and the dpkt hunks of #1605:
  the octet walk and its link-type stash, IPv6 in IPv4 and IPv4 in IPv6,
  refusing MPLS and ISL, the tunnel and extension-header walks, the IPv4
  header checks and the layer bounds
- keep #1690's timestamps, #1594's parser choice, #1621 and #1653
- re-land as guards: a layer dpkt's own lengths misplace (#1646) is
  re-serialised, not refused with ProtocolError; a Total Length below the
  IHL keeps the header whole (#1616), and a header the capture cuts is no
  input; packet2dict and packet2chain take a str payload and a raw frame
- delete the modules #1662 and #1655 added; the 1615/1616 module keeps
  only what its #1616 tests use, as no scapy test imports it now; dpkt's
  comparisons with the default engine assert constants or the capture's
  octets, or are dropped
- drop pypcapfile's VLAN_TAGS, VLAN_TAG_LEN and IPV6_EXTENSION_HEADERS,
  which only dpkt imported, and the 1668 module's _Agreement base, which
  nothing subclasses now
- the docs drop the depth claims and name what dpkt leaves out silently

The dpkt modules, the shared modules that run dpkt, tests/foundation/engines
and the project guards pass on 3.14 and 3.11 (dpkt 1.9.8, pypcapfile 0.12.0).
JarryShaw added a commit that referenced this pull request Oct 14, 2026
Third-party engines are best effort (#1687), so the dpkt code that exists
only to match the default engine comes out:

- revert #1662, #1655, #1624, #1622, #1614 and the dpkt hunks of #1605:
  the octet walk and its link-type stash, IPv6 in IPv4 and IPv4 in IPv6,
  refusing MPLS and ISL, the tunnel and extension-header walks, the IPv4
  header checks and the layer bounds
- keep #1690's timestamps, #1594's parser choice, #1621 and #1653
- re-land as guards: a layer dpkt's own lengths misplace (#1646) is
  re-serialised, not refused with ProtocolError; a Total Length below the
  IHL keeps the header whole (#1616), and a header the capture cuts is no
  input; packet2dict and packet2chain take a str payload and a raw frame
- delete the modules #1662 and #1655 added; the 1615/1616 module keeps
  only what its #1616 tests use, as no scapy test imports it now; dpkt's
  comparisons with the default engine assert constants or the capture's
  octets, or are dropped
- drop pypcapfile's VLAN_TAGS, VLAN_TAG_LEN and IPV6_EXTENSION_HEADERS,
  which only dpkt imported, and the 1668 module's _Agreement base, which
  nothing subclasses now
- the docs drop the depth claims and name what dpkt leaves out silently

The dpkt modules, the shared modules that run dpkt, tests/foundation/engines
and the project guards pass on 3.14 and 3.11 (dpkt 1.9.8, pypcapfile 0.12.0).
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

1 participant