Skip to content

fix(toolkit): dpkt and scapy find TCP in IPv4 where the default engine does (#1606, #1617) - #1624

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1606-1617-tcp-carrier-walk
Oct 12, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1606-1617-tcp-carrier-walk

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 #1606.
Closes #1617.

I measured 45 crafted frames for TCP/trace input counts and records. Before the fix, 26 or 27 of them disagreed with default: the cross-review counts 26 on these same frames (dpkt 22, scapy 25) on 3.11 and 3.14, and the author counted 27 on 3.11 (dpkt 1.9.8, scapy 2.8.0). After the fix, 0 of the 45 disagree there or on 3.14 (scapy 2.7.0). pypcapfile still differs only on TCP over IPv6, which it declines by design. This PR drops the #1606 Gap row in test_ext_chain_limit_agreement_1604_unit.py.

@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.67% (unit tier, Python 3.14, 6c7b4d066, 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 145 1010 35 94.14%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 17115 226 4570 191 98.06%
pcapkit/toolkit 990 154 364 5 82.94%
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: NEEDS CHANGES at e0aff304a.

Sonnet cross-review. The fix holds: on 216 hand-built frames, disagreements with the default engine fall from 390 to 3, and #1617's second cause (scapy binds AH to proto 51 without frag=0) is confirmed in scapy's source. But dpkt now crashes the whole extraction on two frames that main skipped. I reproduced both on the PR tree; main reads each without raising:

  1. IPv4 proto 0, Hop-by-Hop (next header 41), IPv6 with next header 44, a Fragment header (offset 0), Hop-by-Hop, then TCP: AttributeError: 'IP6HopOptsHeader' object has no attribute 'frag_off' at dpkt's ip6.py:96. _decode_ip catches only KeyError/UnpackError, whereas the engine itself catches any Exception (foundation/engines/dpkt.py:491-493).
  2. IPv4 proto 0, Hop-by-Hop (next header 6), then TCP with source port 112, in trace mode: dpkt decodes the octets as IPv4 carrying VRRP whose data is '', and packet2dict's wrapper (toolkit/dpkt.py:692) recurses into the str.

Required: catch as the engine does in _decode_ip, and correct its comment and docstring. Skip a non-packet payload in packet2dict. Add a fail-before test for each. Minor: the review counts 26 frames disagreeing before the fix, not 27; and inet6.py:4233 is the scapy 2.7.0 line.

@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 10, 2026
@JarryShaw
JarryShaw force-pushed the fix/1606-1617-tcp-carrier-walk branch from e0aff30 to 4edf009 Compare October 11, 2026 00:05
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 11, 2026
…e does (#1606, #1617)

- dpkt and scapy step over an AH or IPv6 extension header carried directly
  in IPv4 (protocols 0, 43, 44, 51, 60, 135, 139) by the default engine's own
  rules, via pypcapfile's _past_extension_headers, bounded at
  EXTENSION_HEADER_LIMIT; the TCP segment or IP-in-IP tunnel behind them is
  decoded from the captured octets. Behind such a header, TCP is read only as
  the default TCP parser accepts it.
- dpkt keeps a tunnel it raises anything decoding undecoded, as its engine
  keeps such a frame raw, rather than ending the extraction.
- dpkt's packet2dict and packet2chain treat only a packet-like payload as a
  layer; a str payload, such as VRRP's '', is leaf data and ends the chain.
- scapy's carrier walk now runs top-down and stops at a later IPv4 fragment,
  which scapy still dissects IPv6 and AH in.
- Drop the #1606 gap row from the #1604 chain-limit agreement test.
@JarryShaw
JarryShaw force-pushed the fix/1606-1617-tcp-carrier-walk branch from 4edf009 to 6c7b4d0 Compare October 11, 2026 00:09
@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 11, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 6c7b4d066.

Sonnet cross-review, round 2. Both dpkt crashes are gone.

  • Fuzz and handcrafted frames: the 216 handcrafted frames, the 900 fuzz frames (the same 90 disagreements before and after, none added or removed) and 380 new crash-family frames (52 errors before, 0 now) all run cleanly.
  • Remaining disagreements: the 100 left on those 380 are fix(toolkit): dpkt, scapy and pyshark drop TCP and IPv4 behind some IPv6 extension-header chains #1626's shapes, which also disagree in controls that bypass this code.
  • Output unchanged: packet2chain and packet2dict give identical output on main and the PR for 2961 of 2965 frames, including all 16 fixture captures and verbose mode. The 4 that differ are the frames that raised on main.
  • Scope of except Exception: it wraps only dpkt's constructor, as the engine does.

I reproduced the three new tests' fail-before myself: 3 failed with dpkt.py at e0aff304a, 28 passed at this head. The one required change was the body's before-fix count. The two counts differ by one on the same 45 frames, and I have reworded the body to say so. Non-blocking: unlike the engine, _decode_ip drops the frame without a DPKTWarning; and one test pins dpkt 1.9.8's VRRP data == '', which will catch it if dpkt changes that.

@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 11, 2026
@JarryShaw
JarryShaw merged commit e5f3af3 into main Oct 12, 2026
39 checks passed
@JarryShaw
JarryShaw deleted the fix/1606-1617-tcp-carrier-walk 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 14, 2026
Third-party engines are best effort (#1687), so the scapy code that
exists only to match the default engine comes out:

- revert #1683, #1663, #1652 and #1631, and the scapy hunks of #1614: the
  link-type and loopback-family walk, the frame and extension-header
  bounds, the IPv6 chain walk over the octets, the stops at GRE, VXLAN,
  PPPoE and the other layers the default engine leaves raw and at ICMP
  quotes, and the IPv4 header checks
- keep #1624's walk (TCP behind AH or an IPv6 extension header in IPv4,
  and the later-fragment stop), #1605's keying by the carrying IP header
  and #1589's trailer cut; the walk calls _walk_extension_headers, which
  the pypcapfile revert keeps
- re-land as guards: a zero-length frame is no IPv4 input and has no
  packet2dict octets (#1676); IPv4 in IPv6 ends at the Payload Length
  (#1623); a packet built by hand is read off its octets (#1676);
  ipv6_reassembly takes only its own IPv6 layer's Fragment header, and
  none in a later IPv4 fragment
- delete the four scapy agreement modules and test_scapy_unit's
  default-engine checks; the guards' tests assert constants
- docs: scapy applies the extension-header bound only behind IPv4 and the
  frame-layer bound nowhere, and its silent gaps are listed
  where gaps are said to be announced

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

- revert #1683, #1663, #1652 and #1631, and the scapy hunks of #1614: the
  link-type and loopback-family walk, the frame and extension-header
  bounds, the IPv6 chain walk over the octets, the stops at GRE, VXLAN,
  PPPoE and the other layers the default engine leaves raw and at ICMP
  quotes, and the IPv4 header checks
- keep #1624's walk (TCP behind AH or an IPv6 extension header in IPv4,
  and the later-fragment stop), #1605's keying by the carrying IP header
  and #1589's trailer cut; the walk calls _walk_extension_headers, which
  the pypcapfile revert keeps
- re-land as guards: a zero-length frame is no IPv4 input and has no
  packet2dict octets (#1676); IPv4 in IPv6 ends at the Payload Length
  (#1623); a packet built by hand is read off its octets (#1676);
  ipv6_reassembly takes only its own IPv6 layer's Fragment header, and
  none in a later IPv4 fragment
- delete the four scapy agreement modules and test_scapy_unit's
  default-engine checks; the guards' tests assert constants
- docs: scapy applies the extension-header bound only behind IPv4 and the
  frame-layer bound nowhere, and its silent gaps are listed
  where gaps are said to be announced

The scapy toolkit and engine modules, the shared modules that run scapy,
and the project and tier guards pass on 3.14 (scapy 2.7.0) and 3.11
(scapy 2.8.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).
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