Skip to content

fix(toolkit): key pyshark link-type resolution on frame.encap_type - #850

Merged
JarryShaw merged 1 commit into
mainfrom
fix/843-encap-type-keying
Sep 27, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/843-encap-type-keying

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #843.

Link-type resolution was keyed on packet.layers[0].layer_name, a Wireshark display-filter name,
with a blanket Enum_LinkType.get(name.upper()) fallback. One filter name serves several
encapsulations, so that fallback returned wrong DLTs silently — null for a DLT_LOOP capture,
101 for rawip6. It is now keyed on frame.encap_type, which is distinct per encapsulation, and
the fallback is gone: an unknown encapsulation or name raises MissingKeyError rather than
substituting a plausible DLT, per the standing ruling on this file.

Per your ruling — "register only when PyShark is used, so still local to toolkit" — nothing is
aliased onto LinkType; both tables live in pcapkit/toolkit/pyshark.py.

The tables were measured, not transcribed — and that is deliberate

Wireshark's source is not on this host, so transcribing wiretap/pcap-common.c would have meant
copying a table nobody here could read. Instead each entry is one round trip through upstream's own
table
: editcap -F pcap -T <encap> writes a real capture, the DLT is read from that file's pcap
header (network, offset 20), and the key is read back with tshark -r … -T pdml. Every entry
carries a trailing comment naming its editcap -T source, per your "comments point to the source of
truth"
.

  • ENCAP_TYPE_TO_LINKTYPE — 152 entries, no DLT serving two keys, no key serving two DLTs
  • FILTER_NAME_TO_LINKTYPE — 2 → 58 entries, every root filter name measured single-DLT (including
    ones that already spell their member, since there is no upper-case fallback left to catch them)

Verification, re-run by me on this rebased branch

  • Sweep re-derived independently: editcap -T accepts 226, 157 writable from
    examples/captures/in.pcap, 69 refused. This corrects a figure I had been quoting as 158/68 —
    the worker's count is the right one.
  • Nine published keys confirmed: 15→NULL, 174→LOOP, 25→LINUX_SLL, 210→LINUX_SLL2, 7→RAW,
    129→IPV4, 130→IPV6, 1→ETHERNET, 2→IEEE802_5.
  • Three full round trips re-run from scratch: loop file-DLT 108, linux-sll2 276,
    rawip6 229 — all match the table.
  • New tests against unfixed main → 26 failed. On this branch, tests/toolkit →
    69 passed, 11 skipped, 257 subtests. Plain unittest re-derivation → Ran 14 tests … OK,
    so no pytest-subtests undercount.
  • pcapkit/toolkit/pyshark.py → 100%, 42 stmts / 0 miss, 14 branch / 0 partial.

Caveats, stated rather than buried

  • Runtime pyshark parsing is unexercised. pyshark.FileCapture is broken on Python 3.14
    (asyncio has no attribute 'set_child_watcher'). The tests use a fake whose semantics were derived
    from pyshark's source (LayerFieldsContainer(str, Pickleable); BaseLayer.__getattr__ raising
    AttributeError; XmlLayer._sanitize_field_name stripping the frame. prefix so
    frame_info.encap_type resolves frame.encap_type). That the field is present on every packet is
    verified from tshark PDML across all 157 captures — not from a live pyshark run.
  • "Unambiguous" means within the 157 swept. ether-nettl/tr-nettl are unswept, so eth and
    tr being unique is unproven beyond the sweep. The 69 refused encapsulations have no evidence
    either way and raise.
  • Five filter names root at a payload dissector, not a link-layer one — llc, sctp, pppoes,
    fpp, irlap. Single-DLT as measured, but weaker evidence; named in the table's comment.
  • editcap -T hhdlc → DLT 121 has no LinkType member (verified). Left out rather than invented;
    a candidate follow-up for const/reg/linktype.py, which this PR does not touch.
  • No Sphinx build was run, so the new .. autodata:: rendering is unverified. No hyperlink
    reference was added, so the Unknown target name failure mode is avoided by construction.

* `packet.layers[0].layer_name` is a Wireshark display-filter name, and one
  name serves several DLTs, so resolving from it answered a valid but wrong
  DLT with no error: 101 for `rawip4`/`rawip6`, 0 for a `DLT_LOOP` capture,
  9 for `ppp-with-direction`, 203 for `linux-lapd`.
* Resolve from `frame.encap_type` instead, through a new
  `ENCAP_TYPE_TO_LINKTYPE` of 152 entries; the filter name is now only a
  fallback for a frame layer that carries no such field. A `WTAP_ENCAP_*`
  number is not a DLT, hence the table rather than a cast.
* Expand `FILTER_NAME_TO_LINKTYPE` from 2 to 58 entries, per the ruling on
  #842: every filter name measured as unambiguous, and no upper-case
  fallback onto a like-named `LinkType` member -- that fallback is what
  returned the wrong DLTs above.
* Both tables were generated by measurement, not transcribed: each entry is
  one `editcap -F pcap -T <encap>` rewrite, its DLT read from the resulting
  pcap file header and its key read back with `tshark -T pdml` 4.6.9. The
  `editcap` name sits in a comment on every line so an entry can be
  re-checked when upstream moves.
* An encapsulation or name outside the tables still raises `MissingKeyError`
  rather than standing in a plausible DLT.

tests/toolkit: 69 passed, 11 skipped, 257 subtests; pyshark.py at 100%
statement and branch coverage.
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) 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 Sep 27, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO on 4230e49b0 — cross-review (sonnet, a different model from the opus author), no
substitution needed. Every figure re-derived independently in its own detached worktrees, then cleaned up.

  • 26 of the 152 table entries re-derived from scratch — every 6th, spanning keys 1–223, each via a
    fresh editcap -F pcap -T <name>, the DLT read at pcap-header offset 20, and frame.encap_type from
    tshark -T pdml. 26/26 matched, zero mismatches.
  • No duplicate keys in either table, checked at source level with grep -oP | uniq -d rather than
    on the loaded dict, which would have silently collapsed a duplicate — 152 → 152 distinct values,
    58 → 58.
  • Sweep 226 / 157 / 69 confirmed exactly, by sweeping all 226 itself.
  • The no-silent-substitution claim is the strongest part. It read pyshark's own source
    (packet.py, layers/base.py, layers/xml_layer.py, fields.py) and walked six adversarial paths:
    frame_info absent (not reachable — Packet.__init__ always sets it, so it degrades to the
    filter-name path), encap_type absent (AttributeError absorbed by getattr), unmapped,
    non-numeric, unknown filter name, and both present (encap wins, no fallthrough exists). No path
    returns a real DLT for unknown input.
  • The test fake is faithful at every point the code under test touches it. It does not replicate
    _sanitize_field_name, but the toolkit only ever calls getattr(frame_info, 'encap_type', None), so
    the simplification is behaviourally indistinguishable. No fidelity gap — which matters, because a
    wrong fake would have made the 26-failed/69-passed evidence worthless.
  • Test numbers reproduced to the exact figure, including plain unittest → Ran 14 tests … OK against
    the real merge-base 30bca999c, and coverage 42 0 14 0 100%.
  • breaking judged correct: filter names like ppp, lapd, raw, null that used to resolve
    silently (to a wrong DLT) now raise when frame.encap_type is unavailable.

One thing it explicitly did not verify, matching my own caveat: no Sphinx build was run, so the
new .. autodata:: rendering is UNVERIFIED-by-build. Structurally low-risk — the .rst line is
identical in shape to its neighbour and adds no hyperlink target — but unproven.

Ready for you to merge.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 27, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Wondering if the ENCAP_TYPE and DISPLAY_FILTER maps can be generated by a vendor script or maybe just a util script - so that we don't have to maintain them manually. They may exist in toolkit still - if generated by scripts.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Recorded, verbatim: "Wondering if the ENCAP_TYPE and DISPLAY_FILTER maps can be generated by a vendor
script or maybe just a util script - so that we don't have to maintain them manually. They may exist in
toolkit still - if generated by scripts."

Yes — and the generator effectively already exists, since that is how these tables were produced.
Every entry came from a sweep of editcap -F pcap -T <encap> → read the DLT at the pcap header's offset
20 → read frame.encap_type back with tshark -T pdml. It is a script that was run once; committing it
is the change you are asking for.

But a util/ script, not a vendor crawler — the vendor framework is the wrong shape here, measured
rather than assumed:

  1. Vendor._dest_path() is structurally hardwired to pcapkit/const/ (pcapkit/vendor/default.py:497).
    It anchors on the vendor package's own __file__ and mirrors the module's position under
    pcapkit.const
    . It cannot target pcapkit/toolkit/pyshark.py, which is where you want the maps to
    stay.
  2. Vendor.context() emits a whole enum class file (:425) — LINE(NAME, DOCS, FLAG, ENUM, MISS, …).
    These are two module-level dicts inside an existing hand-written module, not an enum class, so the
    template does not apply.
  3. Vendor.request() is an HTTP/CSV fetch (:442). This generator's source of truth is not a URL —
    it is the local tshark/editcap binaries plus a source capture. No vendor crawler has a local
    binary dependency, and adding one would make make vendor require Wireshark.

util/ already holds exactly this kind of thing — bump_version.py, changelog_md.py,
conda-build.py, conda-dist.py, wheel_rename.py — so a util/pyshark_encap_map.py that rewrites the
two blocks in place fits the existing convention without stretching the vendor framework.

Two things such a script has to handle, both learned the hard way while producing this table:

  • a PDML showname can contain / ("Encapsulation type: NULL/Loopback (15)"), which a naive regex
    drops — it silently lost 10 of 157 rows, including null itself
  • editcap -T's help header line contributes a bogus editcap: token, inflating a naive token split
    to 227 when the real count is 226; and splitting on whitespace rather than the <tok> - <desc> line
    shape gives 646

Filed as its own issue rather than folded into #850, since #850 is a defect fix and this is tooling —
say if you would rather it blocked the merge. #850 itself is unchanged and still review: good-to-go.

@JarryShaw
JarryShaw merged commit 506b92c into main Sep 27, 2026
31 checks passed
@JarryShaw
JarryShaw deleted the fix/843-encap-type-keying branch September 27, 2026 13:16
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 27, 2026
JarryShaw added a commit that referenced this pull request Sep 27, 2026
…ap_type

pcapkit/toolkit/pyshark.py resolved a frame's link type from
packet.layers[0].layer_name, a Wireshark display-filter name, with a
blanket Enum_LinkType.get(name.upper()) fallback. One filter name serves
several encapsulations, so that fallback returned wrong DLTs silently --
null for a DLT_LOOP capture, 101 (RAW) for rawip6.

Re-keyed onto frame.encap_type, which is distinct per encapsulation, and
the fallback is gone: an unrecognised encapsulation or filter name now
raises MissingKeyError. Both lookup tables were measured through
editcap/tshark round trips rather than transcribed from Wireshark source,
which is not on the build host: ENCAP_TYPE_TO_LINKTYPE grows to 152
entries and FILTER_NAME_TO_LINKTYPE grows from #838's 2 entries to 58.
JarryShaw added a commit that referenced this pull request Sep 27, 2026
Adds util/pyshark_encap_map.py, a generator that regenerates
ENCAP_TYPE_TO_LINKTYPE (152 entries) and FILTER_NAME_TO_LINKTYPE (58) in
place inside pcapkit/toolkit/pyshark.py, so the two tables #850 hand-built
stop being hand-maintained. Guards against its own worst failure mode: a
naive LinkType(dlt) lookup cannot detect an unmapped DLT, because
_missing_ mints a placeholder rather than raising, so the generator
snapshots every known value before any lookup.

The real-tshark sweep is gated on a new HAS_WIRESHARK flag and is
version-pinned -- editcap -T accepts 226 encapsulations on Wireshark
4.6.9, 224 on 4.2.2 (CI's Ubuntu noble) -- so the count assertions run
only under the measured version. One line of pyshark.py itself changes as
a result: the IPMB_LINUX table entry becomes I2C_LINUX, the canonical
name #848 gave value 209.

util/changelog_md.py regenerated CHANGELOG.md for all seven entries added
across this and the six preceding commits (#838, #846, #847, #848, #849,
#850, #853); `--check` exit 0.
JarryShaw added a commit that referenced this pull request Sep 27, 2026
#846 and #850

- #838's "the 21 whose vendor crawler leaves Vendor.process() unmodified"
  measures 22, not 21 -- pcapkit.const.hip.transport.Transport also has an
  unmodified process(), but its FLAG-bound range has no unassigned gap, so
  its _missing_ carries no bounded-range extend_enum branch to fix. The
  real discriminator is that branch, not the process() override; reworded,
  and the "other 84" split into the 83 that do override process() and the
  one that doesn't but has nothing to fix either. Re-derived independently
  against 5e25db2^..5e25db2 (not main, which would fold in #847's own
  edit to ipx/socket.py): 22 registries have no process() override, 21 of
  them have the bounded-range branch, matching
  tests/const/test_const_enum_no_mint.py's own REGISTRIES_WITH_UNASSIGNED_RANGES
  (21 entries) vs ALL_REGISTRIES (22, +hip.transport, with a NOTE explaining
  the exclusion).

- #846's "Only tests/integration/test_engine_runtime.py and
  test_engine_parity.py change" is false -- 30bca99's own numstat also
  touches .github/workflows/unit-tests.yml (17+/2-). Qualified to "only
  these two test files change".

- #850 gets the breaking marker: its own prose already says the fallback
  is gone and an unrecognised encapsulation or filter name now raises
  MissingKeyError instead of substituting a plausible DLT -- the same
  shape as the two existing markers at AppType.get (1.5.0.rst:2456) and
  the four .get()-backed enum fields (1.5.0.rst:2720). Restructured to
  lead with the marker and the subject, matching their wording and
  placement; the later restatement of the same fact is dropped.

None of these are code or test changes -- prose only, matching the
cross-review's own framing. #848 stays unmarked (its own "Not breaking"
paragraph holds); #838, #849 and #853 stay unmarked per explicit
instruction not to add markers beyond what was asked.
JarryShaw added a commit that referenced this pull request Sep 27, 2026
util/changelog_md.py regenerated from 1.5.0.rst after the #838/#846/#850
corrections; --check exit 0.
JarryShaw added a commit that referenced this pull request Sep 28, 2026
…ap_type

pcapkit/toolkit/pyshark.py resolved a frame's link type from
packet.layers[0].layer_name, a Wireshark display-filter name, with a
blanket Enum_LinkType.get(name.upper()) fallback. One filter name serves
several encapsulations, so that fallback returned wrong DLTs silently --
null for a DLT_LOOP capture, 101 (RAW) for rawip6.

Re-keyed onto frame.encap_type, which is distinct per encapsulation, and
the fallback is gone: an unrecognised encapsulation or filter name now
raises MissingKeyError. Both lookup tables were measured through
editcap/tshark round trips rather than transcribed from Wireshark source,
which is not on the build host: ENCAP_TYPE_TO_LINKTYPE grows to 152
entries and FILTER_NAME_TO_LINKTYPE grows from #838's 2 entries to 58.
JarryShaw added a commit that referenced this pull request Sep 28, 2026
Adds util/pyshark_encap_map.py, a generator that regenerates
ENCAP_TYPE_TO_LINKTYPE (152 entries) and FILTER_NAME_TO_LINKTYPE (58) in
place inside pcapkit/toolkit/pyshark.py, so the two tables #850 hand-built
stop being hand-maintained. Guards against its own worst failure mode: a
naive LinkType(dlt) lookup cannot detect an unmapped DLT, because
_missing_ mints a placeholder rather than raising, so the generator
snapshots every known value before any lookup.

The real-tshark sweep is gated on a new HAS_WIRESHARK flag and is
version-pinned -- editcap -T accepts 226 encapsulations on Wireshark
4.6.9, 224 on 4.2.2 (CI's Ubuntu noble) -- so the count assertions run
only under the measured version. One line of pyshark.py itself changes as
a result: the IPMB_LINUX table entry becomes I2C_LINUX, the canonical
name #848 gave value 209.

util/changelog_md.py regenerated CHANGELOG.md for all seven entries added
across this and the six preceding commits (#838, #846, #847, #848, #849,
#850, #853); `--check` exit 0.
JarryShaw added a commit that referenced this pull request Sep 28, 2026
#846 and #850

- #838's "the 21 whose vendor crawler leaves Vendor.process() unmodified"
  measures 22, not 21 -- pcapkit.const.hip.transport.Transport also has an
  unmodified process(), but its FLAG-bound range has no unassigned gap, so
  its _missing_ carries no bounded-range extend_enum branch to fix. The
  real discriminator is that branch, not the process() override; reworded,
  and the "other 84" split into the 83 that do override process() and the
  one that doesn't but has nothing to fix either. Re-derived independently
  against 5e25db2^..5e25db2 (not main, which would fold in #847's own
  edit to ipx/socket.py): 22 registries have no process() override, 21 of
  them have the bounded-range branch, matching
  tests/const/test_const_enum_no_mint.py's own REGISTRIES_WITH_UNASSIGNED_RANGES
  (21 entries) vs ALL_REGISTRIES (22, +hip.transport, with a NOTE explaining
  the exclusion).

- #846's "Only tests/integration/test_engine_runtime.py and
  test_engine_parity.py change" is false -- 30bca99's own numstat also
  touches .github/workflows/unit-tests.yml (17+/2-). Qualified to "only
  these two test files change".

- #850 gets the breaking marker: its own prose already says the fallback
  is gone and an unrecognised encapsulation or filter name now raises
  MissingKeyError instead of substituting a plausible DLT -- the same
  shape as the two existing markers at AppType.get (1.5.0.rst:2456) and
  the four .get()-backed enum fields (1.5.0.rst:2720). Restructured to
  lead with the marker and the subject, matching their wording and
  placement; the later restatement of the same fact is dropped.

None of these are code or test changes -- prose only, matching the
cross-review's own framing. #848 stays unmarked (its own "Not breaking"
paragraph holds); #838, #849 and #853 stay unmarked per explicit
instruction not to add markers beyond what was asked.
JarryShaw added a commit that referenced this pull request Sep 28, 2026
util/changelog_md.py regenerated from 1.5.0.rst after the #838/#846/#850
corrections; --check exit 0.
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
@JarryShaw JarryShaw moved this to Done in PyPCAPKit Oct 6, 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.

toolkit: pyshark link-type resolution silently returns a wrong DLT for ppp, null, lapd and raw

1 participant