Repository navigation
fix(toolkit): dpkt reads SYN, FIN and RST from the flags octet whatever the NS bit (#1618) - #1621
Merged
Merged
Conversation
…er the NS bit (#1618) `tcp_reassembly` and `tcp_traceflow` read each flag by its position in `bin(tcp.flags)` padded to eight digits, but dpkt 1.9.8 makes `TCP.flags` nine bits wide with NS as bit 8, so with NS set every flag was read one bit low (a SYN as a FIN, an RST as a SYN, a PSH as an RST). A new `_tcp_flags` masks each one out with dpkt's `TH_RST`, `TH_SYN` and `TH_FIN`.
JarryShaw
added
fix
Owner
Author
|
Verdict: GOOD TO GO at Sonnet cross-review. It confirmed the root cause in dpkt's own Its one required change was body wording: no datagrams are reassembled from these frames, so the datagram comparison is between two empty lists. I have reworded the body. Non-blocking: that |
JarryShaw
added
review: good-to-go
Contributor
|
Coverage: 89.71% (unit tier, Python 3.14,
Per-file detail: the |
JarryShaw
removed
the
review: good-to-go
3 of 13 tasks
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please follow the guide below
You will be asked some questions, please read them carefully and answer honestly
Put an
xinto 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 testpasses, and a test case covers the changeAdded a changelog entry under
docs/source/changelog/and regeneratedCHANGELOG.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) #657What is the purpose of your pull request?
Tick the commit type your subject line carries.
fix— corrects a defectfeat— adds a featureperf— changes performance, not behaviourrefactor— changes neither behaviour nor performancetest— tests onlydocs— documentation onlyci— workflows or build toolingrelease— bumps the version or rolls up a distributionchore— anything elseDescription of your pull request and other information
Closes #1618.
tcp_reassemblyandtcp_traceflowinpcapkit/toolkit/dpkt.pyread RST, SYN and FIN as positions 5–7 ofbin(tcp.flags)padded to eight digits. dpkt 1.9.8 makesTCP.flags9 bits wide, with NS as0x100and the three reserved bits in a separate_rsv. So with NS set, every flag was read one bit low: RST came from PSH, SYN from RST, and FIN from SYN.A new private
_tcp_flagsmasks each flag out with dpkt'sTH_RST,TH_SYNandTH_FIN. Those are the only flags either adapter reads (ack=is the acknowledgement number). Masking also works on older dpkt, whereflagswas a single octet.I checked every flags octet (256) against every NS and reserved-bit value (16), so 4096 frames, comparing each against the default engine. Fields that disagreed: 3072 → 0 for reassembly, and the same for flow tracing (all of them with NS set). Scapy agreed both before and after. The new tests also check that every field of each reassembly and trace input, and the flows, match the default engine. These frames carry no payload, so no datagrams are reassembled from them.