Skip to content

fix(protocols): handle declared lengths above sys.maxsize as overruns (#1975) - #1979

Merged
JarryShaw merged 3 commits into
mainfrom
fix/1975-declared-length-overflow
Oct 20, 2026
Merged

JarryShaw merged 3 commits into
mainfrom
fix/1975-declared-length-overflow

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — a bug fix

Description of your pull request and other information

A declared length above sys.maxsize, such as a PCAP incl_len of 2**31 - 16 on GraalPy or 32-bit CPython, used to crash a read or seek with OverflowError before the overrun handling could see it. Every length-driven read is now bounded at sys.maxsize, so a hostile claim comes back short and gets the same "captured length runs past the data" or "block truncated" handling as on 64-bit CPython. Claims that fit are read exactly as before.

The new test_declared_length_past_maxsize_1975_unit.py (ALL) simulates a 32-bit Py_ssize_t on CPython. It fails on main (12 failed) and passes here. On GraalPy 25.4.4, the two #1541 modules and the new one pass with no skips, while main fails 14 with OverflowError. The third-party engines are left as they are, as best effort under #1687.

Closes #1975

…#1975)

Where Py_ssize_t is 32 bits wide (GraalPy, 32-bit CPython) a read or seek
past sys.maxsize raises OverflowError instead of coming back short, so a
PCAP incl_len or PCAP-NG length reaching that far crashed the reader before
the overrun handling could keep the record as captured.

- Schema.unpack bounds its payload and field reads at sys.maxsize.
- Frame.read bounds the record read, and a next-record offset past
  sys.maxsize becomes the end of the octets read.
- PCAPNG.read and _read_block_cut bound the block read.
- SeekableReader._fill bounds a READ_CHUNK of -1 at sys.maxsize.
- Lift #1974's sys.maxsize skips from the #1541 tests, and add a test
  that runs the narrow build's limit on any interpreter.

Claims that fit read exactly as before.

Closes #1975
@JarryShaw JarryShaw added bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix) labels Oct 20, 2026
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed bug Issues reporting a defect (set by the bug report template; a default, not an assessment) labels Oct 20, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 20, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

When the desired read is beyond sys.maxsize or sys.maxint, should'nt we make multiple reads to satisfy the request instead of cutting short and bounding at the limit?

@JarryShaw

Copy link
Copy Markdown
Owner Author

Partly yes. I measured on GraalPy 25.4.4, where sys.maxsize is 2**31−1:

  • A single read above sys.maxsize raises OverflowError, and so does bytes(2**31). No object can hold more than sys.maxsize octets, so several reads could not be joined into one payload on these builds. Bounding the payload read is unavoidable.
  • A seek past sys.maxsize works, both absolute and relative.

The PR gets the second part wrong. When a record's next offset lies past sys.maxsize, Frame.read falls back to the end of the octets it read. On a file that really holds such a record, that misaligns every later record. So I'll change it to keep the payload bounded (with the truncation warning) but advance the stream to the declared end of the record:

  • by relative seeks when the stream is seekable;
  • by chunked read-and-discard when it isn't.

The PCAP-NG block reads get the same treatment. Later records then parse exactly as they do on 64-bit CPython.

@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 20, 2026
… of truncating (#1975)

A record or block the file really holds past sys.maxsize was read only to
the limit, and the next one was then parsed from inside it.

- corekit.io.advance moves a stream forward any distance: relative seeks of
  at most sys.maxsize, or chunked read-and-discard on a SeekableReader or a
  non-seekable stream.
- Frame.read and PCAPNG.read call it when the bounded read came back whole,
  so the next record starts at the declared end; a short read is unchanged.
- The PCAP and PCAP-NG engines cap the read-ahead at sys.maxsize, and the
  PCAP frame schema reads no more than the header and payload can hold in
  one read, so a SeekableReader keeps the record start in its window.
- SeekableReader._fill bounds any READ_CHUNK at sys.maxsize.
- Tests cover a sparse file holding a record past a lowered limit, seekable
  and through a pipe, for PCAP and PCAP-NG.

Closes #1975
@JarryShaw

Copy link
Copy Markdown
Owner Author

Pushed 9e0c3719d. A record or block that really extends past sys.maxsize is now moved past whole, so the next one parses at its declared offset. The payload stays bounded and warned.

  • A new pcapkit.corekit.io.advance moves by relative seeks, or by chunked read-and-discard on a SeekableReader or a pipe.
  • Frame.read and PCAPNG.read call it.
  • On the pipe path, the pcap/pcapng engines' _bound_read_ahead, the PCAP frame schema's packet length and SeekableReader._fill are also bounded at sys.maxsize.
  • A file shorter than its claim behaves as before.

The new sparse-stream tests fail on 0fd43fb67 (5 failed) and pass here. They run seekable, through a pipe, and with READ_CHUNK=-1. GraalPy 25.4.4 passes 16. A delta review is running.

@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 20, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 9e0c3719d. A Sonnet delta review probed advance across BytesIO, SeekableReader, and pipes with zero, negative, exact and past-EOF counts. It found that advance only runs when a read came back whole but the declared length is longer, so 64-bit behaviour is unchanged. The sparse-stream tests check the next record's content. The PR also passes merged with #1983 and main. Open point: advance has only one internal caller. Should it stay public in pcapkit.corekit.io, or become _advance? Non-blocking: for a record that really extends past the limit, the kept 'runs past the data' warning is misleading.

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

Copy link
Copy Markdown
Contributor

Coverage: 91.52% (unit tier, Python 3.15, 9e0c3719d, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19597 881 2618 867 92.11%
pcapkit/corekit 2540 71 808 37 96.54%
pcapkit/dumpkit 276 3 100 3 98.40%
pcapkit/foundation 2918 23 1016 25 98.78%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17423 224 4586 189 98.11%
pcapkit/toolkit 872 36 298 10 95.21%
pcapkit/utilities 522 2 158 4 99.12%
pcapkit/vendor 4431 1995 1018 167 51.73%

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

@JarryShaw

Copy link
Copy Markdown
Owner Author

use advance

@JarryShaw

Copy link
Copy Markdown
Owner Author

Keeping advance public, as you said. No change needed.

@JarryShaw
JarryShaw merged commit 04fc1b1 into main Oct 20, 2026
59 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 20, 2026
@JarryShaw
JarryShaw deleted the fix/1975-declared-length-overflow branch October 20, 2026 17:07
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 20, 2026
@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(foundation): a record length above sys.maxsize crashes Frame.read with OverflowError on 32-bit-Py_ssize_t builds

1 participant