Skip to content

fix(toolkit): accept tshark's True/False TCP flags in the pyshark trace (#1514) - #1527

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1514-pyshark-tcp-flags
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1514-pyshark-tcp-flags

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 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

Fixes pcapkit.toolkit.pyshark so its flow-trace input matches the default engine's. The #1514 ValueError had been hiding the later bugs:

  • Flags: tshark 4.x reports tcp.flags_* as 'True'/'False', so bool(int(...)) raised. _parse_flag accepts '1'/'0' and 'True'/'False' and raises ProtocolError otherwise.
  • Timestamp: frame.time_epoch was handed over as text. It is now float(Decimal(...)), bit-identical to the default engine. A Simple Packet Block has no time_epoch, so its sniff_timestamp is used instead.
  • seq/ack: these were tshark's relative numbers. The fix reads tcp.seq_raw/tcp.ack_raw, which exist from Wireshark 3.2.0 (checked in packet-tcp.c for 3.0.0, 3.2.0 and CI's 4.2.2), and falls back to seq/ack without them.
  • Frame layer: packet2dict, encap_type and time_epoch read frame_info, which is pkt_comment on a commented packet; a commented TCP packet raised MissingKeyError: 'frame'. The frame layer is now looked up by name.
  • Index: this now takes the engine's count, as dpkt and scapy do, instead of tshark's packet.number. This aligns with fix(engines): the pyshark engine skips non-packet PCAP-NG blocks (#1515) #1531; it needs a one-line change at the tcp_traceflow call in foundation/engines/pyshark.py.

Engine agreement: deletes the #1514 Gap row. Per the ruling on #1507, pyshark has no frame bytes, so two pyshark comparisons are narrowed:

  • trace skips header/payload (NO_TCP_OCTETS, citing extraction.py:1446-1455).
  • flows compares labels and indices only (JSON_TRACE, citing extraction.py:1440-1444).

Tests: 19 new or updated in tests/toolkit/test_pyshark_unit.py; 16 fail on origin/main. With pyshark and tshark 4.6.9 the engine module passes (625 subtests), and so does a merge with #1531. That merge conflicts only on the adjacent #1514/#1515 rows; resolve it by deleting both. On a journal block followed by two TCP packets, the merge gives indices 1 and 2, as the default engine does.

Closes #1514

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) 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: 90.11% (unit tier, Python 3.14, b1c6cc248, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 843 2356 756 92.08%
pcapkit/corekit 2161 66 700 33 96.26%
pcapkit/dumpkit 242 2 88 2 98.79%
pcapkit/foundation 2665 94 934 40 95.28%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 17086 238 4628 190 97.98%
pcapkit/toolkit 575 74 188 3 85.71%
pcapkit/utilities 431 4 124 4 98.56%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

…ce (#1514)

- Flags: tcp_traceflow parsed tcp.flags_syn/fin/reset with bool(int(...)),
  but tshark 4.x reports them as 'True'/'False', so the first traced TCP
  frame raised ValueError. _parse_flag accepts both spellings and raises
  ProtocolError otherwise.
- Timestamp: frame.time_epoch was handed over as text; it is now
  float(Decimal(...)), the default engine's float.
- Sequence numbers: tcp.seq/tcp.ack are tshark's relative numbers; read
  tcp.seq_raw/tcp.ack_raw, falling back where they are absent.
- Frame layer: packet2dict, encap_type and time_epoch read frame_info,
  which is pkt_comment on a commented packet; look the frame layer up by
  name.
- Index: take the engine's frame count, as dpkt and scapy do, not tshark's
  frame number.
- Engine agreement: delete the #1514 Gap row; compare pyshark trace inputs
  without header/payload and pyshark flows by label and index only, since
  pyshark has no frame bytes (ruling on #1507).

Unit tests cover each fix; tests/toolkit, the engine module, tests/project,
the docstring contract and the tier guard pass.

Closes #1514
@JarryShaw
JarryShaw force-pushed the fix/1514-pyshark-tcp-flags branch from 80ecc34 to b1c6cc2 Compare October 9, 2026 23:24
@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
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at b1c6cc248. Cross-reviewed on Sonnet; the author ran Opus.

  • Timestamps: float(Decimal(time_epoch)) is bit-identical to the default engine's float (frame.py:253-257, pcapng.py:1810, .hex() compared) on all 1,616 frames of the 23 captures, down to tshark's nanosecond output.
  • seq/ack: tcp.seq_raw/ack_raw are absent in Wireshark 3.0.0 packet-tcp.c, present in 3.2.0 (:6909/:6921) and in 4.2.2 (:8785/:8797). They match the default engine on every traced frame.
  • Skips: the only skipped trace fields are header and payload. Mutating seq, ack, flags, timestamp, ports, addresses or index is caught by trace.
  • Merged with fix(engines): the pyshark engine skips non-packet PCAP-NG blocks (#1515) #1531: after deleting both adjacent Gap rows, a journal + commented-TCP + SPB capture matches the default engine exactly.
  • Tests: fail-before is 16 of 19 tests (35 counting subtests). The engine module has 81 failures on main (int('True')).

Non-blocking:

@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 50cd95f into main Oct 10, 2026
40 checks passed
@JarryShaw
JarryShaw deleted the fix/1514-pyshark-tcp-flags branch October 10, 2026 01:51
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 10, 2026
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

Development

Successfully merging this pull request may close these issues.

fix(toolkit): pyshark TCP trace crashes because tshark 4.x reports flags as 'True'/'False'

1 participant