Skip to content

fix(corekit)!: to_dict() returns an InfoDict, so a repeated IPv6 extension header survives the dict rebuild (#1453) - #1466

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1453-to-dict-multidict
Oct 9, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1453-to-dict-multidict

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1453

to_dict() now returns InfoDict, a new OrderedMultiDict subclass, for every Info, so the return type does not depend on the packet. The dict interface, json and the dumpers see the first value per key, and items(multi=True) yields every value in wire order. Info.__update__ given a MultiDict adds to a key it already holds, IPv6 adds each extension header that way, and _lookup_exthdr walks items(multi=True).

Breaking (!), for callers of to_dict():

  • type(d) is dict is now False (isinstance(d, dict) still holds).
  • d == plain_dict is False when a key repeats.
  • yaml.safe_dump(d) raises RepresenterError.
  • A repeated header shows the first, not the last, in d[key], info.opts and the dumps.
  • Dumping costs 8–11% more on a loaded host (reviewer's figure).

Also touched: the InfoDict docs entry, and pcapkit/dumpkit/common.py, whose object_hook writes any OrderedMultiDict as a list of entries; one condition now sends an InfoDict to the dict branch, so all 92 dumps (23 captures × json/tree/plist/xml) are byte-identical to main at 94ae9664c.

Dict rebuilds before → after: dst-dst 56→64/64, hop-dst-dst 64→72, dst-dst-dst 56→72, Routing-Routing 56→64, dst-rt-dst 56→72, hop-dst-rt-dst 64→80. Byte-exact through Ethernet and Frame too. copy.copy(info) now gets its own __multi__. With main's sources, the new tests/protocols/test_to_dict_repeated_exthdr_1453_unit.py reports 34 failed, 3 passed; with this PR, 7 passed, 41 subtests.

@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: running A cross-review is in flight against the current head - no verdict yet labels Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.56% (unit tier, Python 3.14, 074e0c89b, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 957 2356 870 91.00%
pcapkit/corekit 2107 93 672 27 94.60%
pcapkit/dumpkit 242 2 88 2 98.79%
pcapkit/foundation 2648 98 930 48 94.80%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16899 222 4564 187 98.06%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 429 4 122 4 98.55%
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 dd2029c4e (Sonnet cross-review; the author ran on Opus). Two small items.

  1. Docs. docs/source/pcapkit/corekit/multidict.rst has no InfoDict entry. The new docstrings in infoclass.py cross-reference pcapkit.corekit.multidict.InfoDict, so those links will not resolve. Add an autoclass block with :show-inheritance: and items.
  2. copy.copy(info) shares __multi__. I confirmed this on this head with a dst-dst chain: the copy's __multi__ is the original's object. A later __update__ on the copy therefore leaks into the original (pcapkit/corekit/infoclass.py:447). Copy it per instance and add a test.

Holds:

Notes:

  • The measured dump cost is 8–11% on a loaded host, against the 7% reported.
  • type(d) is dict and yaml.safe_dump now fail. Mention both in the PR body under the !.

@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 9, 2026
…nsion header survives the dict rebuild (#1453)

to_dict() keyed the extension headers by name, so a second header of the
same type overwrote the first and from_data(info.to_dict()) rebuilt a
shorter packet: Destination Options twice, then UDP, came back 56 of 64.

- multidict: new InfoDict, an OrderedMultiDict with dict views and dict
  equality; the dict interface sees the first value per key.
- Info: to_dict() returns an InfoDict. __update__ given a MultiDict adds
  to a key already held instead of replacing it, and items(multi=True)
  yields every value in order. A copy.copy() gets its own bookkeeping.
- IPv6: each extension header is added through a MultiDict, and
  _lookup_exthdr walks items(multi=True).
- dumpkit: an InfoDict is written as a mapping, not as the entry list an
  OrderedMultiDict field gets, so every dump stays byte-identical.
- docs: InfoDict in the multidict page.

Legs corekit, foundation, dumpkit, protocols/internet, protocols (rest)
and project pass; the capture dumps are byte-identical to main.
@JarryShaw
JarryShaw force-pushed the fix/1453-to-dict-multidict branch from dd2029c to 074e0c8 Compare October 9, 2026 04:23
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment 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 074e0c89b. Round-2 cross-review on Sonnet; the author ran on Opus.

  • copy.copy leak: fixed. __copy__ recreates all three per-instance attributes that __new__ sets (__map__, __map_reverse__, __multi__). Disabling it makes test_info_copy_keeps_its_own_repeats fail. I re-checked myself that a copy shares neither __multi__ nor __map__ with the original.
  • Rebuild is byte-exact from both info and info.to_dict(), across ten independent extension-header chains: repeated Destination Options, Routing, and mixed chains starting with Hop-by-Hop. I also checked Destination Options ×2 followed by UDP: 68 of 68 octets on both paths.
  • Dumps are unchanged. All 92 dumps (23 captures × json/tree/plist/xml) are byte-identical to main per diff -rq.
  • Docs: the InfoDict entry matches the surrounding style.

Optional additions to the breaking-changes list, not blockers:

  • json.dumps(info.to_dict()) silently keeps only the first repeated header.
  • len(d) counts keys, not values.
  • Nested values are also InfoDict.

@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 8328fd3 into main Oct 9, 2026
42 checks passed
@JarryShaw
JarryShaw deleted the fix/1453-to-dict-multidict branch October 9, 2026 13:16
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 9, 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.

fix(ipv6): to_dict() collapses repeated extension headers, so the dict rebuild silently shortens the packet

1 participant