Skip to content

fix(corekit): a length-less SchemaField passes -1 to its schema, so every list item warns 'packet length < 0' - #1157

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1148-schemafield-negative-length
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1148-schemafield-negative-length

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — run on the touched files: isort clean; mypy/pylint report nothing in the changed lines
  • make test passes, and a test case covers the change — only the modules listed below, not the full suite
  • Added a changelog entry — N/A, added centrally after the wave

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1148. SchemaField.unpack passed its "undeclared" -1 on as the nested schema's length, so __length__ started negative and every sub-field warned. It now measures what is left in the buffer and passes that as a declared length. Passing None would be simpler, but prepare() would then turn a buffer that has run out into its quiet StreamEOFError ("end of capture"). Measured: that would change a truncated LOCATOR_SET from FieldValueError to StreamEOFError. Fixed-length items are unchanged.

Also rewrites the stale ContextRequestOption note in pcapkit/protocols/schema/internet/mh.py (docstring only; code AST unchanged), which justified the opaque requests run with the -1 warnings this PR removes. The opaque run is kept because a nested-schema list would only warn on an entry that overruns the option and zero-pad its value, where MH._read_opt_cr raises ProtocolError.

probe (SchemaWarning count) before after
HIP LOCATOR_SET, 2 / 3 locators 12 / 18 0 / 0
MH CGA Parameters, 1 item 6 0
HIP LOCATOR_SET, truncated by 10 FieldValueError + 17 FieldValueError + 6
MH CGA, truncated by 3 FieldValueError + 11 FieldValueError + 7
fixed-length item declared 3 < schema 4 2 2 (unchanged)

New tests/corekit/test_fields_misc_schema_length_1148_unit.py: 5 passed. Without the fix, 3 tests fail (5 subfailures). With the None variant, the truncation test fails with StreamEOFError. Passing (one module per process): test_fields_misc, _misc_packet_context, _collections, _field, test_option_roundtrip_unit, test_hip_unit, test_hip_locator_set_length_unit, test_mh_unit, test_ipv4_unit, test_httpv2_payload_length_unit, plus tests/project (385 passed, 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 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 1e78f8acf: NEEDS CHANGES (ran on Sonnet; author Opus). One stale comment needs fixing.

The note at pcapkit/protocols/schema/internet/mh.py:1371-1383 (ContextRequestOption) says a length-less SchemaField is passed -1 and warns once per field. After this PR that is false. Reword it to describe the new behaviour, or remove it.

Everything else checks out:

  • Buffers: the remainder is measured with tell/seek exactly as prepare already does. Parsed items and the final position are identical on bytes, BytesIO, a real file and SeekableReader.
  • Fixed lengths: a declared length takes the unchanged path.
  • Truncation: truncated input still raises FieldValueError. Swapping in a None sentinel fails the guard test.
  • Tests: 19 protocol modules give identical pass counts. Every frame's info hash is identical across 7 captures.
  • Speed: neutral. A 44,100-frame capture took 16.63 s before and 16.56 s after, and its 7,200 warnings per run drop to 0.

@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 6, 2026
…very list item warns 'packet length < 0'

SchemaField.unpack passed its "undeclared" length of -1 on to the nested
schema, seeding __length__ negative, so every sub-field of every item
warned. It now measures the remainder of the buffer and passes that as a
declared length. Passing None instead would let prepare() turn an
exhausted buffer into its quiet StreamEOFError, ending an extraction on a
truncated item; a declared zero keeps that a FieldValueError as before.

Closes #1148
@JarryShaw
JarryShaw force-pushed the fix/1148-schemafield-negative-length branch from 1e78f8a to e233a73 Compare October 6, 2026 21:15
@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 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on e233a7390: GOOD TO GO (ran on Sonnet; author Opus, round 2)

The reviewer's round 1 was GOOD TO GO with the stale mh.py note as a nit. I made it NEEDS CHANGES myself, because the PR made that comment false. It is now fixed.

  • Note claims, both probed. A valid ContextRequestOption with type 22 and type 19 entries parses with 0 warnings. A length-less nested list that overruns only warns and zero-pads, while _read_opt_cr raises ProtocolError (mh.py:4497-4502). So the opaque-run design still has a reason.
  • AST guard: mh.py is equal to round 1 once bare strings are stripped. The delta is docstring-only, and misc.py and the tests are unchanged.

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

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.75% (unit tier, Python 3.14, e233a7390, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1035 2342 846 90.23%
pcapkit/corekit 1878 91 580 22 94.34%
pcapkit/dumpkit 136 0 40 0 100.00%
pcapkit/foundation 2425 145 842 34 92.56%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15659 188 3944 163 98.18%
pcapkit/toolkit 487 65 144 1 85.42%
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 90602c4 into main Oct 6, 2026
40 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 6, 2026
@JarryShaw
JarryShaw deleted the fix/1148-schemafield-negative-length branch October 6, 2026 22:39
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 25, 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(corekit): a length-less SchemaField passes -1 to its schema, so every list item warns 'packet length < 0'

1 participant