Repository navigation
SeekableReader's position bookkeeping is not maintained across operations: a refused seek leaves _tell moved, SEEK_CUR/SEEK_END can drive it negative, and peek desyncs the buffer cursor #643
Description
SeekableReader maintains three position variables — _tell, _buffer_set/_buffer_cur, and self._buffer's own BytesIO cursor — and nothing keeps them in step across a failed seek, a negative-going seek, or a peek. Three separate defects, filed together because they are one problem: the bookkeeping has no invariant that survives an operation which does not complete normally.
The invariant that ought to hold is that self._buffer's octet 0 sits at absolute _buffer_set, and self._buffer.tell() tracks _tell - _buffer_set. seek is the only method that restores it, at pcapkit/corekit/io.py:298, and every path that skips that line leaves the object lying about where it is.
All three are silent wrong data, not crashes. None of them involves truncate, so none is affected by #633.
1. A refused seek leaves _tell already moved
seek assigns _tell in each whence branch before it validates the result. When the validation then refuses the seek, _tell has already been overwritten with the rejected target, and the resync at line 298 is never reached:
pcapkit/corekit/io.py:274 self._tell = offset # SEEK_SET
pcapkit/corekit/io.py:276 self._tell += offset # SEEK_CUR
pcapkit/corekit/io.py:278 self._tell = buf_end + offset # SEEK_END
pcapkit/corekit/io.py:301 raise SeekError(f'cannot seek before the beginning of the buffer: ...')
pcapkit/corekit/io.py:298 self._buffer.seek(self._tell - self._buffer_set, io.SEEK_SET) # the resync, skipped
A caller that catches SeekError and reasonably believes the position is unchanged instead reads from the wrong offset, with no exception at the read:
r = SeekableReader(io.BytesIO(b'ABCDEFGHIJKLMNOPQRSTUVWXYZ'), buffer_size=2)
r.read(13) # b'ABCDEFGHIJKLM'; _buffer_set=11, _buffer_cur=2
r.seek(8) # raises SeekError('cannot seek before the beginning of the buffer: 8 < 11')
r.tell() # 8 <- moved, despite the refusal
r.read(1) # b'L' <- wrong; offset 8 is b'I'Reproduced independently at a second parameterisation, so it is not an artefact of one buffer size:
r = SeekableReader(io.BytesIO(b'ABCDEFGHIJKLMNOPQRSTUVWXYZ'), buffer_size=4)
r.read(6) # b'ABCDEF'; _buffer_set=2, _buffer_cur=4
r.seek(0) # raises SeekError('cannot seek before the beginning of the buffer: 0 < 2')
r.tell() # 0 <- moved
r.read(1) # b'C' <- wrong; offset 0 is b'A'The invalid-whence branch at pcapkit/corekit/io.py:279-280 is correctly guarded — it raises before any of the three assignments run, and seek(0, 99) leaves tell() untouched. Only the three whence branches above are affected; noting it so nobody adds a redundant guard there.
2. SEEK_CUR and SEEK_END never check that the resulting _tell is non-negative
SEEK_SET rejects a negative offset outright at pcapkit/corekit/io.py:272-273. The other two branches do arithmetic and never look at the result, so an absolute position below zero is reachable:
r = SeekableReader(io.BytesIO(b'abcdef'), buffer_size=4)
r.seek(-1, io.SEEK_SET) # SeekError: negative seek value -1 <- clean, guardedr = SeekableReader(io.BytesIO(b'abcdef'), buffer_size=4)
r.seek(-100, io.SEEK_CUR) # SeekError: cannot seek before the beginning of the buffer: -100 < 0
r.tell() # -100 <- a negative absolute stream positionr = SeekableReader(io.BytesIO(b'abcdef'), buffer_size=4)
r.seek(-100, io.SEEK_END) # SeekError: cannot seek before the beginning of the buffer: -96 < 0
r.tell() # -96The asymmetry is the defect: the same nonsensical request is a clean negative seek value under one whence and a misleading cannot seek before the beginning of the buffer under the other two. The message conflates a position that has legitimately slid out of the buffer window — normal, and recoverable with buffer_save=True — with an absolute position that cannot exist.
A negative _tell then compounds with the - 1 in the buffered read paths (filed separately), and the result is a length-contract violation as well as wrong data. With _buffer_cur == 0, min(size, self._buffer_cur - 1) is min(1, -1) == -1, and BytesIO.read(-1) means read-to-EOF:
r = SeekableReader(io.BytesIO(bytes(range(50))), buffer_size=16)
r.seek(-5, io.SEEK_CUR) # SeekError: cannot seek before the beginning of the buffer: -5 < 0
r._tell # -5, and _buffer_cur is still 0
r.read(1) # b'\x00' * 16 <- 16 octets for a 1-octet request,
# all of them the never-populated buffer
r.tell() # 11read(1) returning sixteen octets of pristine zero-fill is the worst single observation in this issue: the data never came from the stream at all.
With buffer_save=True the same seek does not even raise
The refusal at line 301 is conditional on self._buffer_file is None. With a backing file, control falls to line 302 and seek returns the negative position as if it had succeeded; the failure then surfaces on an unrelated call as a bare OS error:
r = SeekableReader(io.BytesIO(bytes(range(50))), buffer_size=16,
buffer_save=True, buffer_path='/tmp/buf.bin')
r.seek(-5, io.SEEK_CUR) # returns -5 <- no exception at all
r.read(1) # OSError: [Errno 22] Invalid argumentThat OSError comes from temp_file.seek(self._tell, io.SEEK_SET) at pcapkit/corekit/io.py:372, inside read — i.e. from a line that is only reachable because seek accepted something it should not have.
3. peek desyncs self._buffer's cursor from _tell
peek must not advance the stream position, and indeed it never touches _tell — unlike read, read1 and readline it has no self._tell += len(buf). But its buffered branch reads through self._buffer, which does advance that BytesIO's own cursor:
pcapkit/corekit/io.py:492 buf = self._buffer.read(min(size, self._buffer_cur - 1))
pcapkit/corekit/io.py:503 return buf # no _tell update, and no cursor restore
So after a peek, _tell is right and self._buffer.tell() is wrong, and the next buffered read starts from the wrong place:
r = SeekableReader(io.BytesIO(bytes(range(50))), buffer_size=16)
r.read(8); r.seek(2)
r.read(3) # b'\x02\x03\x04' <- control, no peek: correctr = SeekableReader(io.BytesIO(bytes(range(50))), buffer_size=16)
r.read(8); r.seek(2)
r.peek(3) # b'\x02\x03\x04' correct preview
# but now _tell=2 while self._buffer.tell()=5
r.read(3) # b'\x05\x06\x07' <- WRONG; should equal the control
r.tell() # 5, in both runs -- tell() cannot distinguish themtell() reports the same number in both runs while the returned bytes differ, so there is no way for a caller to detect this.
The other branch, at pcapkit/corekit/io.py:480-485, has the same shape by a different route. When _tell is at or past the end of buffered content and the raw stream has no peek, peek does a real consuming self._stream.read(size) followed by self._write_buffer(buf) — and _write_buffer writes through the memoryview at line 145 rather than through self._buffer.write(), so self._buffer's cursor is again left where it was:
r = SeekableReader(NoPeekStream(bytes(range(50))), buffer_size=16) # non-seekable, no peek, no read1
r.read(8)
r.read(4) # b'\x08\t\n\x0b' <- control: correctr = SeekableReader(NoPeekStream(bytes(range(50))), buffer_size=16)
r.read(8)
r.peek(4) # b'\x08\t\n\x0b' correct preview
r.read(4) # b'\x00\x01\x02\x03' <- WRONG; replays the first four
# octets ever bufferedThis one is reachable from Extractor, on ordinary input
peek is not a latent API corner. Extractor calls it as the magic-number sniff:
pcapkit/foundation/extraction.py:966-968 if not self._ifile.seekable(): -> wrap in SeekableReader
pcapkit/foundation/extraction.py:991 self._magic = self._ifile.peek(4)[:4]
pcapkit/foundation/engines/pcapng.py:336 buffer = ext._ifile.peek(4)[:4]
So for any non-seekable input — a pipe, a socket, sys.stdin.buffer — peek(4) is the first operation performed on the stream, and it corrupts the read that follows it. Reproducing exactly that sequence, with a non-seekable stream that has no peek (as a pipe does not):
DATA = b'\xd4\xc3\xb2\xa1' + b'RESTOFHEADER' + b'FIRSTRECORD' # PCAP magic, then content
r = SeekableReader(Pipe(DATA), stream_closing=False) # as extraction.py:968 does
r.peek(4)[:4] # b'\xd4\xc3\xb2\xa1' correct magic, as read at :991
r.read(16) # b'\xd4\xc3\xb2RESTOFHEADERF'
# correct would be b'\xd4\xc3\xb2\xa1RESTOFHEADER'The \xa1 — the fourth octet of the PCAP magic number — is dropped from the middle, and a stray F from the following record is spliced onto the end to make the length up. The control run, identical but without the peek, returns b'\xd4\xc3\xb2\xa1RESTOFHEADER' correctly.
That last reproduction is a joint consequence of this defect and the - 1 off-by-one filed separately: the peek populates the buffer without advancing _tell, and the - 1 is what then drops an octet. Either fix alone would change the symptom; it is recorded in both issues rather than in only one.
Measured
On origin/main (375e9d411), CPython 3.14.7, tree asserted as the repository's own rather than trusting the editable install — pcapkit.__file__ printed and checked on every run, because the __editable__ finder on this machine resolves to a checkout behind origin/main. Every reproduction above is io.BytesIO (or a small non-seekable wrapper over one) with no fixtures.
Why nothing catches it
The suite exercises all three paths and discriminates none of them.
tests/corekit/test_io.py:34-41—test_seek_before_buffer_start_requires_saved_bufferdrives defect 1's exact path:read(6)thenassertRaises(SeekError)onseek(0). It never checkstell()afterwards and never reads afterwards, which is where the whole defect lives.tests/corekit/test_io.py:139-140—seek(-1, io.SEEK_CUR)andseek(-1, io.SEEK_END)are both asserted, but with offsets small enough that_tellstays non-negative. Defect 2 needs an offset large enough to cross zero, and no test uses one.tests/corekit/test_io.py:31,:117,:185,:208,:240— fivepeekassertions, all checking only the valuepeekreturns. Defect 3 is entirely in what the next read returns, which none of them looks at.
The same shape as #622, #601 and #618: a test that runs the line without discriminating the behaviour.
Notes
- Found as side-findings by the workers on SeekableReader.truncate pads on the wrong side and returns fewer octets than requested #622 (PR fix(corekit): keep SeekableReader's buffer content when truncating it (#622) #633) and Extractor._cleanup closes the caller-supplied stream and leaks the file it opened itself #610 / extract(no_eof=True) never returns: EOF is detected but nothing stops the loop #620 (PRs fix(foundation): close the input stream pcapkit opened, not the caller's (#610) #636 / fix(foundation): give
no_eofa way to stop, so extract() returns (#620) #639), several by fuzzing; re-verified here from scratch on375e9d411before filing. - fix(corekit): keep SeekableReader's buffer content when truncating it (#622) #633 is open and unmerged at the time of writing, so
pcapkit/corekit/io.pyonmaindoes not carry itstruncatefix. Everything above was measured againstmainas it stands, and no reproduction callstruncate, so none of it is affected either way. - Fixes in this file are known to be subtle, so no fix is proposed here. fix(corekit): keep SeekableReader's buffer content when truncating it (#622) #633's own first revision turned a loud
ValueErrorinto silent data corruption, because the buffer is a sliding window in which_buffer_set + _buffer_curmust stay invariant. The design is left to whoever picks this up. - The
- 1off-by-one that two of the reproductions above interact with is filed separately, as a distinct and independently fixable defect: four wrong expressions, versus the state-machine problem described here. - Related: SeekableReader.truncate pads on the wrong side and returns fewer octets than requested #622 (
truncatepadding, same file), FieldBase.unpack pads a short read with rjust regardless of byte order, silently corrupting little-endian values #604 / OptionField.unpack never returns for a well-formed HOPOPT header with an SMF_DPD option #431 (the padding-side family).
Metadata
Metadata
Assignees
Labels
Projects
- StatusShow more project fieldsDone
Activity
JarryShaw commentedon Sep 22, 2026
Companion issues from the same pass over
pcapkit/corekit/io.py, filed after this one so the numbers were not available above:min(size, self._buffer_cur - 1)off-by-one at four sites. This is the "filed separately" defect referred to in sections 2 and 3 above: it is what converts the negative_tellof section 2 into a 16-octet answer to a 1-octet request, and what drops the\xa1in section 3'sExtractorreproduction.truncateshould raise givenwritable()isFalse, plus the misspelledwriteableoverride. Minor, and a judgement call rather than a defect.The three are independently fixable; none blocks another.
fix(corekit): keep SeekableReader's position bookkeeping in step (#643,
fix(corekit): keep SeekableReader's position bookkeeping in step (#643,
fix(corekit): keep SeekableReader's position bookkeeping in step (#643,
fix(corekit): refuse truncate on a reader that reports itself unwrita…
fix(corekit): refuse truncate on a reader that reports itself unwrita…