Skip to content

fix(dumpkit): JSON output of DPKT/Scapy frames grows without bound and fills the disk - #1381

Merged
JarryShaw merged 1 commit into
mainfrom
w1372-fallback-cycle
Oct 8, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
w1372-fallback-cycle

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort): mypy and isort are clean on common.py; pylint adds nothing new except the existing too-many-statements on make_dumper, now 85 statements against 65 before
  • make test passes, and a test case covers the change: see below for the modules and legs actually run
  • Added a changelog entry: N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect

Closes #1372

First bad commit: 9093061b3 (#1305). Found with git bisect run from 13aa677be (bad) back to 11ae68ebf (good), running the foundation leg with a 50 MB file cap. #1335 and #1336 are not the cause.

Root cause. In the plain-unittest leg, the default run at test_extraction_record_header_unit.py:63 ends up with an Ethernet info object (pcapkit.protocols.data.link.ethernet.Ethernet) whose class does not match the dumper's Info. That is cross-import pollution. So object_hook never matches it and the object goes to _append_fallback. Its vars() include __next_type__, which is a protocol class.

Before #1305, vars(cls) was a mappingproxy that was written out with str(). #1305 changed this to new_value.update(vars(value)), so every class became a real dict and was expanded. That walks typing generics, schemas, fields, enum types, int and the descriptors of each.

Nothing is memoised, so every shared node is expanded again on every path to it. Output therefore grows exponentially while the stack stays shallow. At 3,000 fallback calls the stack was 64 frames deep against a recursion limit of 1,000, and wrapper_descriptor had been expanded 931 times. That is why no RecursionError was raised.

Fix (pcapkit/dumpkit/common.py).

  • A class, module or routine is written as its qualified name. That alone removes the blow-up.
  • An object already on the current expansion path is written as <circular reference: T>, so a cycle terminates. A shared object that is not part of a cycle is still written in full every time it appears, so no data is dropped.
  • Safety net: if one top-level fallback object expands into more than FALLBACK_LIMIT = 10_000 objects, the dumper raises UnsupportedCall instead of writing a marker. Every fixture stays far below that: across the foundation, dumpkit and interface legs no fallback object expands into more than 7, so the budget never fires on a fixture.

These rules depend on no particular engine. Schema change: a class, module or function in fallback output is now written as its name.

Second issue. test_runtime_engines was wrong, not the code. #1305 passes version= on purpose (#1255), so the test now expects version=VersionInfo(2, 4).

Evidence (all runs under ulimit -f 204800):

  • New tests/dumpkit/test_fallback_cycle_unit.py: 4 passed. With origin/main's common.py restored, 3 fail (cycle and class: RecursionError; budget: nothing raised). The shared-object test passes there too, as it should: it pins that nothing is lost.
  • util/run_unittest_leg.py foundation: 483 test(s), 0 failure(s), 0 error(s). On main it was 2 failure(s), 2 error(s), Errno 27. Within the leg, out.json is now at most 25,767 bytes.
  • util/run_unittest_leg.py dumpkit: 220 test(s), 0 failure(s), 0 error(s).
  • pytest: test_runtime_engines 5 passed, test_extraction_record_header_unit 10 passed, test_common_unit 14 passed, test_dumper_roundtrip_unit 7 passed, tests/project 389 passed and 1 skipped.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on eabeaab6d: NEEDS CHANGES — one design point (Sonnet reviewed; author Opus). The reviewer rated this a nit; I am making it blocking because of the owner's #1266 ruling that JSON and plist stay the round-trip formats.

The seen memo drops data. A shared but non-cyclic object inside a fallback value is written in full once, then as <repeated reference: T> on every later appearance — and the PR's own test pins that. The blow-up came from walking classes' attribute tables, and rendering classes/modules/functions as qualified names removes it by itself. So keep cycle detection on the current path only, expand repeats in full, and drop seen. If a safety net is still wanted, make it an explicit size or depth budget that raises a clear in-library error rather than silently substituting a marker.

Everything else held:

  • Cause confirmed: swapping only common.py between 9093061b3^ and 9093061b3 flips test_runtime_engines + test_extraction_record_header_unit from 15 OK to 2 Errno 27 errors. With logging, Ethernet/IPv4/TCP info reach the fallback only after a purging module (test_runtime_engines, registry/test_foundation, engines/test_engine_base) has run.
  • No real capture hits a marker: 22 fixtures × 4 engines × json/plist = 176 files, byte-identical to main; even in a simulated polluted mode, no <circular or <repeated appears in any of them.
  • The tracker is safe: state lives on self, and the resets are in finally.
  • Tests: the version=VersionInfo(2, 4) test fix is right (fix(dumpkit): pcap output drops the input's global-header version #1255). The new test fails 3 on main's common.py. foundation leg 483/0, dumpkit 219/0, tests/project 389.

The re-import pollution that sends real Info objects to the fallback is out of scope here; I will file it with #1332's findings.

@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 7, 2026
…d fills the disk

- _append_fallback writes a class, module or routine as its qualified
  name instead of expanding it through vars(), which since #1305 turned a
  class's mappingproxy into a dict and walked the whole type graph.
- An object already on the current expansion path is written as a
  <circular reference: ...> marker, so a cycle terminates; a shared
  object is still expanded in full each time it is met.
- A top-level fallback object expanding into more than FALLBACK_LIMIT
  (10000) objects raises UnsupportedCall instead of writing on.
- test_runtime_engines: expect the version= keyword #1305 added to
  PCAP's _open_output call.

Closes #1372
@JarryShaw
JarryShaw force-pushed the w1372-fallback-cycle branch from eabeaab to b89db14 Compare October 7, 2026 21:56
@JarryShaw JarryShaw added 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 labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round-2 verdict on b89db1461: GOOD TO GO (Sonnet reviewed; author Opus). Round 1's data-loss concern is resolved.

  • Path-only cycles behave correctly. Self-, 2- and 3-cycles get <circular reference: T> where the object meets itself. In a diamond (A→B, A→C, B→D, C→D), D is written in full twice with no marker. Shared pairs under sibling branches expand identically. The path entry is popped in finally, and both counters are reset after a raise.
  • No data lost. Json and plist dumps of 23 captures under the default, dpkt and scapy engines (138 files) are byte-identical to main. In the leg-polluted mode, main still hits RecursionError on arp.pcap and fills the cap on ipv4.pcap. This PR completes every run, and its output is a strict superset with classes shown as qualified names. No marker appears in any fixture dump.
  • The budget never fires. 30,000 top-level objects in one container do not raise, and a single 2¹⁵-node expansion does. The re-measured peak over 23 captures × 3 engines × json/plist × 3 modes is 13 expansions, against a limit of 10,000. The FALLBACK_LIMIT comment's "no more than 7" should say 13.
  • Tests and legs. foundation runs 483 tests with 0 failures, so all three of fix(dumpkit): JSON output of DPKT/Scapy frames grows without bound and fills the disk #1372's failures now pass. dumpkit gives 220/0 and tests/project 389. The largest out.json seen across the legs is 369 KB. The revert proof gives 3 failed and 1 passed; the passing test is the shared-object guard, as expected.

Nit, worth a follow-up: UnsupportedCall subclasses AttributeError, so hasattr and getattr(..., default) swallow the budget error; the reviewer demonstrated hasattr returning False. A ValueError-based class would fit better; json raises ValueError for circular input.

@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 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.98% (unit tier, Python 3.14, b89db1461, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18777 1032 2348 843 90.27%
pcapkit/corekit 1949 84 606 20 94.91%
pcapkit/dumpkit 225 2 80 2 98.69%
pcapkit/foundation 2443 102 852 39 94.39%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16013 206 4178 177 98.06%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

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

@JarryShaw
JarryShaw merged commit 8c8ce87 into main Oct 8, 2026
40 checks passed
@JarryShaw
JarryShaw deleted the w1372-fallback-cycle branch October 8, 2026 01:02
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 8, 2026
JarryShaw added a commit that referenced this pull request Oct 15, 2026
…1803)

Entries:
- add #1791 (breaking: pack() leaves its instance unchanged), #1778
  (Protocol in protocol.__all__), #1781 (PortEnumField miss with a
  nameless proto), #1779 (vendor.ipx _path), #1780 (examples READMEs to
  Markdown, build leftovers) and #1790 (36 ambiguous cross-references)
- fold #1776 into the RPL 0x23 entry and #1381 into the dump-schema one

Accuracy:
- the benchmark and legacy_smoke READMEs are README.md since #1780
- EngineBase stays out of __all__, as #1016 settled
- the #575 peek calls get() only once the value resolves
- the #647 and #764 entries point at #775/#860, under which nothing mints
- the #861 entry names #869 and #874 for the EnumRegistry move

Figures:
- preamble: 685 citations, #340 to #1791
- process.rst pin: 298 entries; CHANGELOG.md regenerated

Refs #1103
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 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(dumpkit): JSON output of DPKT/Scapy frames grows without bound and fills the disk

1 participant