From c7e42c2850a367d61b58d0463aed6b9ee62411de Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 22 Sep 2026 18:16:11 -0400 Subject: [PATCH] fix(corekit): refuse truncate on a reader that reports itself unwritable (#645) **Behaviour change to a public API, twice over.** ``SeekableReader.truncate()`` now raises where it returned a size, and the misspelled ``writeable()`` is gone in favour of ``writable()``. * ``io.IOBase.writable`` gates two methods -- "If False, write() and truncate() will raise OSError" -- and ``writable()`` here returns False. ``write`` and ``writelines`` already honoured that; ``truncate`` returned its new size, so a caller that checked ``writable()`` first -- which is what the contract invites -- got a surprise either way round. It now raises ``UnsupportedOperation('truncate')``: the same refusal ``write`` uses, and the same message CPython's own C ``BufferedReader`` carries. * The resizing is not deleted, only made private, as ``_truncate_buffer``. It never touched the underlying stream -- it resizes a private lookback window -- and it is the only route to the "position dropped by the window" state that ``seek`` and the four buffered read paths must refuse, which #643 and #644 landed tests for. Deleting it would take those tests' mechanism with it. * ``writeable()`` was not an override of anything: ``'writable' in SeekableReader.__dict__`` was False and ``SeekableReader.writable is io.IOBase.writable`` was True, so ``io``, ``shutil`` and any third-party caller read the inherited value and never saw the one defined here. Both returned False, which is why there was no symptom. Renamed to the spelling the protocol uses; the value it reports is unchanged, and was always honest. * ``docs/source/pcapkit/corekit/io.rst`` follows the rename. The exception is ``pcapkit.utilities.exceptions.UnsupportedOperation``, which subclasses ``io.UnsupportedOperation`` and so is an ``OSError``. The in-library exception *is* the stdlib one here, so the house rule and the ABC agree rather than having to be traded off. tests/corekit/test_io.py: 31 -> 34 tests and 35 -> 41 subtests, module coverage 100% before and after (235 -> 237 statements, 0 missed either side). The three new tests fail on b34f132f6 with exit 1 -- 6 SUBFAILED and 1 FAILED -- and pass here with exit 0. tests/corekit/: 179 passed, 413 subtests, exit 0. Fixes #645. --- docs/source/pcapkit/corekit/io.rst | 2 +- pcapkit/corekit/io.py | 67 ++++++++--- tests/corekit/test_io.py | 171 ++++++++++++++++++++++++----- 3 files changed, 197 insertions(+), 43 deletions(-) diff --git a/docs/source/pcapkit/corekit/io.rst b/docs/source/pcapkit/corekit/io.rst index ac219f9175..7e53af26f9 100644 --- a/docs/source/pcapkit/corekit/io.rst +++ b/docs/source/pcapkit/corekit/io.rst @@ -22,7 +22,7 @@ implementation to :class:`io.BufferedReader`. .. automethod:: readline .. automethod:: readlines - .. automethod:: writeable + .. automethod:: writable .. automethod:: write .. automethod:: seekable diff --git a/pcapkit/corekit/io.py b/pcapkit/corekit/io.py index 2dfbb33476..7c051c8b4d 100644 --- a/pcapkit/corekit/io.py +++ b/pcapkit/corekit/io.py @@ -176,7 +176,8 @@ def _seek_buffer(self) -> 'int': SeekError: If the position lies before the window, whose octets are then gone for good -- the stream cannot be rewound to re-supply them. :meth:`seek` refuses that position for the same reason; it is reachable here only through - a :meth:`truncate` that moved the window's base past a position already set. + a :meth:`_truncate_buffer` that moved the window's base past a position already + set. """ buf_off = self._tell - self._buffer_set @@ -254,7 +255,7 @@ def readline(self, size: 'int | None' = -1, /) -> 'bytes': elif not size: # NOTE: a request for no octets is answered without consulting the window, # which has nothing to say about it. Asking anyway refuses a zero-length - # read from a position :meth:`truncate` has left behind the window -- a + # read from a position :meth:`_truncate_buffer` has left behind the window -- a # refusal over data that was never wanted, and not what this used to do. # # Truthiness rather than ``size == 0`` on purpose. It is the same test for @@ -401,16 +402,42 @@ def tell(self) -> 'int': def truncate(self, size: 'int | None' = None, /) -> 'int': """Resize the stream to the given ``size`` in bytes (or the current position if ``size`` is - not specified). The current stream position isn't changed. This resizing can extend or - reduce the current file size. In case of extension, the contents of the new file area - depend on the platform (on most systems, additional bytes are zero-filled). The new file - size is returned. + not specified). + + Raises: + UnsupportedOperation: Always, since the reader is not writable. Note: - Nothing here writes to the underlying stream -- :meth:`write` raises -- so what this - resizes is the buffer, not the stream behind it. The buffer is a sliding window over a - stream that cannot be seeked: its octet 0 sits at absolute offset ``_buffer_set``, its - content occupies ``[0:_buffer_cur]``, and everything past that is padding never read. + :meth:`io.IOBase.writable` gates this method as well as :meth:`write` -- "If + :data:`False`, :meth:`write` and :meth:`truncate` will raise :exc:`OSError`" -- and + :meth:`writable` here returns :data:`False`, so refusing is what the contract asks + for. It is also what CPython's own read-only buffered readers do: both the + accelerated :class:`io.BufferedReader` and the pure-Python + ``_pyio.BufferedReader`` raise :exc:`io.UnsupportedOperation`, the latter from + ``_BufferedIOMixin.truncate``'s ``_checkWritable()`` (#645). + + The resizing this used to perform is still reachable internally, as + :meth:`_truncate_buffer`. It never touched the underlying stream in the first place + -- it resizes a private lookback window -- which is why it survives under a private + name rather than being removed with the public method's behaviour. + + """ + raise UnsupportedOperation('truncate') + + def _truncate_buffer(self, size: 'int | None' = None, /) -> 'int': + """Resize the lookback buffer to the given ``size`` in bytes (or the current position if + ``size`` is not specified). The current stream position isn't changed. This resizing can + extend or reduce the current buffer size. In case of extension, the new area is + zero-filled. The new buffer size is returned. + + Note: + This is the internal half of what :meth:`truncate` used to do, which is all of it: + nothing here writes to the underlying stream -- :meth:`write` raises -- so what this + resizes is the buffer, not the stream behind it. That is why :meth:`truncate` refuses + (#645) while this remains: the operation is a private-window one, not an + :class:`io.IOBase` write. The buffer is a sliding window over a stream that cannot be + seeked: its octet 0 sits at absolute offset ``_buffer_set``, its content occupies + ``[0:_buffer_cur]``, and everything past that is padding never read. Two consequences for which octets survive. An extension appends its zero octets at the **tail**, the new area being by definition the region past the old end. A @@ -431,10 +458,10 @@ def truncate(self, size: 'int | None' = None, /) -> 'int': """ if size is None: - # NOTE: an unspecified size means the current position, per - # :meth:`io.IOBase.truncate`. The buffer is indexed relative to - # ``_buffer_set``, and the position may sit before it once a saved - # buffer has been rewound, in which case nothing is kept. + # NOTE: an unspecified size means the current position, following the + # :meth:`io.IOBase.truncate` convention this used to implement. The buffer + # is indexed relative to ``_buffer_set``, and the position may sit before it + # once a saved buffer has been rewound, in which case nothing is kept. size = max(self._tell - self._buffer_set, 0) if size < 0: raise TruncateError(f'negative size value {size}') @@ -462,9 +489,17 @@ def truncate(self, size: 'int | None' = None, /) -> 'int': self._buffer_cur = len(temp) - dropped return self._buffer_size - def writeable(self) -> 'bool': + def writable(self) -> 'bool': """Return :obj:`True` if the stream supports writing. If :obj:`False`, :meth:`write` and - :meth:`truncate` will raise :exc:`OSError`.""" + :meth:`truncate` will raise :exc:`OSError`. + + Note: + This was spelled ``writeable`` until #645, which is not how the :mod:`io` protocol + spells it, so it overrode nothing and :mod:`io` never consulted it -- the inherited + :meth:`io.IOBase.writable` answered instead. Both returned :data:`False`, so there + was no observable divergence to notice; the coincidence is what hid it. + + """ return False def writelines(self, lines: 'Iterable[Buffer]', /) -> 'None': diff --git a/tests/corekit/test_io.py b/tests/corekit/test_io.py index e04d37081e..1b29ffafb7 100644 --- a/tests/corekit/test_io.py +++ b/tests/corekit/test_io.py @@ -1,5 +1,6 @@ from __future__ import annotations +import _pyio import io import os import tempfile @@ -53,10 +54,10 @@ def test_saved_buffer_allows_rewinding_before_memory_window(self) -> None: if os.path.exists(path): os.unlink(path) - def test_truncate_rejects_negative_sizes(self) -> None: + def test_truncate_buffer_rejects_negative_sizes(self) -> None: reader = self.SeekableReader(io.BytesIO(b'abcdef'), buffer_size=4) with self.assertRaises(self.exceptions.TruncateError): - reader.truncate(-1) + reader._truncate_buffer(-1) self._close_reader(reader) def test_write_operations_raise_unsupported_operation(self) -> None: @@ -67,6 +68,124 @@ def test_write_operations_raise_unsupported_operation(self) -> None: reader.writelines([b'x']) self._close_reader(reader) + def test_truncate_refuses_on_a_stream_that_reports_itself_unwritable(self) -> None: + """Issue #645: ``truncate`` is gated on ``writable()``, and this reader is not writable. + + :meth:`io.IOBase.writable` documents the gate for both methods it guards -- "If + :data:`False`, :meth:`write` and :meth:`truncate` will raise :exc:`OSError`" -- and + :meth:`~pcapkit.corekit.io.SeekableReader.writable` returns :data:`False` here. Two of + the three writability-gated methods already honoured that: ``write`` and ``writelines`` + raise. ``truncate`` returned its new size instead, so a caller that checked + ``writable()`` first -- which is exactly what the contract invites -- got a surprise + either way round. + + Every form is asserted, since the omitted-size form takes a different path through the + method than an explicit size and only the explicit one would have been noticed. + + """ + for args in [(), (0,), (4,), (None,)]: + with self.subTest(args=args): + reader = self.SeekableReader(io.BytesIO(b'abcdef'), buffer_size=4) + self.assertFalse(reader.writable()) + + with self.assertRaises(self.exceptions.UnsupportedOperation) as caught: + reader.truncate(*args) + # NOTE: the contract names OSError, so the refusal has to be one. pcapkit's + # UnsupportedOperation subclasses io.UnsupportedOperation, which subclasses + # OSError, so the in-library exception satisfies the stdlib contract as it + # stands -- there is no choice to make between the two here. + self.assertIsInstance(caught.exception, OSError) + self.assertIsInstance(caught.exception, io.UnsupportedOperation) + self._close_reader(reader) + + def test_truncate_refuses_with_the_same_exception_the_stdlib_reader_raises(self) -> None: + """The property, rather than the behaviour: parity with CPython's own buffered reader. + + Asserting "it raises" pins the fix; asserting "it raises *what a read-only + :class:`io.BufferedReader` raises*" pins the reason for it, and is what stops this being + re-opened by an argument about which exception the contract means. The stdlib's type is + captured by *running* the same call on a real read-only file object rather than being + named here, so the assertion tracks CPython instead of restating a belief about it. + + Both implementations are checked. The accelerated :class:`io.BufferedReader` raises + ``io.UnsupportedOperation: truncate``; the pure-Python ``_pyio.BufferedReader`` + raises ``io.UnsupportedOperation: File or stream is not writable.`` from + ``_BufferedIOMixin.truncate``'s ``_checkWritable()``. The messages differ and the type + does not, which is why the type is what is asserted. + + """ + with tempfile.NamedTemporaryFile(delete=False) as temp: + temp.write(b'abcdef') + path = temp.name + try: + baselines = {} + + with open(path, 'rb') as accelerated: + self.assertIsInstance(accelerated, io.BufferedReader) + self.assertFalse(accelerated.writable()) + with self.assertRaises(OSError) as caught: + accelerated.truncate() + baselines['io.BufferedReader'] = type(caught.exception) + + class NonWritableRaw(_pyio.RawIOBase): + """A raw stream that reads and does not write, as ``SeekableReader``'s is.""" + + def readable(self) -> bool: + return True + + def writable(self) -> bool: + return False + + def readinto(self, buffer) -> int: + return 0 + + with _pyio.BufferedReader(NonWritableRaw()) as pure_python: + self.assertFalse(pure_python.writable()) + with self.assertRaises(OSError) as caught: + pure_python.truncate() + baselines['_pyio.BufferedReader'] = type(caught.exception) + + for name, expected in baselines.items(): + with self.subTest(baseline=name, expected=expected.__name__): + reader = self.SeekableReader(io.BytesIO(b'abcdef'), buffer_size=4) + # NOTE: the premise of the comparison -- both report themselves unwritable, + # so both are in the state the contract gates ``truncate`` on. + self.assertFalse(reader.writable()) + with self.assertRaises(expected): + reader.truncate() + self._close_reader(reader) + finally: + if os.path.exists(path): + os.unlink(path) + + def test_writable_is_the_method_the_io_protocol_consults(self) -> None: + """Issue #645: the override was spelled ``writeable``, so it overrode nothing. + + The :mod:`io` API spells it ``writable``. With the misspelling in place, + ``'writable' in SeekableReader.__dict__`` was :data:`False` and + ``SeekableReader.writable is io.IOBase.writable`` was :data:`True` -- so :mod:`io`, + :mod:`shutil` and any third-party caller read the inherited value and never saw the one + defined in this file. Both returned :data:`False`, which is the coincidence that hid it: + there was no symptom to notice, and editing the misspelled method would silently have + had no effect. + + The identity assertion is the load-bearing one. ``writable()`` returning :data:`False` + passed before the fix too, by inheritance, so a value-only test cannot tell the two + trees apart. + + """ + self.assertIn('writable', self.SeekableReader.__dict__) + self.assertIsNot(self.SeekableReader.writable, io.IOBase.writable) + self.assertNotIn('writeable', self.SeekableReader.__dict__) + + reader = self.SeekableReader(io.BytesIO(b'abcdef'), buffer_size=4) + # NOTE: the reader genuinely cannot write -- ``write`` raises -- so False is the honest + # answer, and the reporting was never the dishonest half. Only where it was *defined* + # was wrong, and the guard on ``truncate`` was missing. + self.assertFalse(reader.writable()) + self.assertFalse(hasattr(reader, 'writeable')) + self._close_reader(reader) + def test_detach_raises_when_underlying_stream_has_no_detach(self) -> None: reader = self.SeekableReader(io.BytesIO(b'abcdef'), buffer_size=4) with self.assertRaises((self.exceptions.UnsupportedOperation, io.UnsupportedOperation)): @@ -143,14 +262,14 @@ def test_seek_variants_warnings_and_truncate_sizes(self) -> None: warn.assert_called_once() self.assertTrue(reader.seekable()) - self.assertFalse(reader.writeable()) + self.assertFalse(reader.writable()) # NOTE: an omitted size means the current position, per :meth:`io.IOBase.truncate`. # The position is 6 and the buffer starts at 2, so 4 octets of it are kept. This # asserted 0 until issue #622, which is what an omitted size was resized to. self.assertEqual(reader._buffer_set, 2) - self.assertEqual(reader.truncate(None), 4) - self.assertEqual(reader.truncate(6), 6) - self.assertEqual(reader.truncate(2), 2) + self.assertEqual(reader._truncate_buffer(None), 4) + self.assertEqual(reader._truncate_buffer(6), 6) + self.assertEqual(reader._truncate_buffer(2), 2) self._close_reader(reader) def test_read_read1_and_peek_fallback_stream_methods(self) -> None: @@ -244,7 +363,7 @@ def close(self): self.assertEqual(reader.peek(2), b'ab') self._close_reader(reader) - def test_truncate_keeps_the_content_and_not_the_padding(self) -> None: + def test_truncate_buffer_keeps_the_content_and_not_the_padding(self) -> None: """Issue #622, verbatim: the octets already read survive a truncation. The buffer holds its content at ``[0:_buffer_cur]`` and nothing but unwritten @@ -258,7 +377,7 @@ def test_truncate_keeps_the_content_and_not_the_padding(self) -> None: reader = self.SeekableReader(io.BytesIO(b'abcde')) self.assertEqual(reader.read(4), b'abcd') - self.assertEqual(reader.truncate(8), 8) + self.assertEqual(reader._truncate_buffer(8), 8) self.assertEqual(reader.seek(0), 0) # NOTE: the stream holds five octets, so five is the whole of what an eight octet @@ -267,7 +386,7 @@ def test_truncate_keeps_the_content_and_not_the_padding(self) -> None: self.assertEqual(reader.read(8), b'abcde') self._close_reader(reader) - def test_truncate_pads_and_keeps_on_the_side_the_bookkeeping_expects(self) -> None: + def test_truncate_buffer_pads_and_keeps_on_the_side_the_bookkeeping_expects(self) -> None: """A truncation never keeps padding in preference to content. Each case distinguishes head from tail handling, since the expected buffer is the @@ -296,7 +415,7 @@ def test_truncate_pads_and_keeps_on_the_side_the_bookkeeping_expects(self) -> No reader = self.SeekableReader(io.BytesIO(b'abcdefghijkl'), buffer_size=buffer_size) self.assertEqual(reader.read(read_size), b'abcdefghijkl'[:read_size]) - self.assertEqual(reader.truncate(size), size) + self.assertEqual(reader._truncate_buffer(size), size) self.assertEqual(bytes(reader._buffer.getvalue()), expected) self.assertEqual(reader._buffer_set, expected_set) # NOTE: the content pointer indexes the buffer, so it cannot be left @@ -306,7 +425,7 @@ def test_truncate_pads_and_keeps_on_the_side_the_bookkeeping_expects(self) -> No self.assertEqual(reader._buffer_set + reader._buffer_cur, read_size) self._close_reader(reader) - def test_truncate_keeps_the_window_base_in_step_with_the_stream(self) -> None: + def test_truncate_buffer_keeps_the_window_base_in_step_with_the_stream(self) -> None: """A reduction that left ``_buffer_set`` alone made the next seek read wrong octets. ``seek`` treats ``_buffer_set + _buffer_cur`` as how far the stream has been @@ -323,7 +442,7 @@ def test_truncate_keeps_the_window_base_in_step_with_the_stream(self) -> None: reader = self.SeekableReader(io.BytesIO(b'abcdefghijklmnop'), buffer_size=8) self.assertEqual(reader.read(8), b'abcdefgh') - self.assertEqual(reader.truncate(3), 3) + self.assertEqual(reader._truncate_buffer(3), 3) # the three most recent octets, and a base that still accounts for the other five self.assertEqual(bytes(reader._buffer.getvalue()), b'fgh') @@ -334,7 +453,7 @@ def test_truncate_keeps_the_window_base_in_step_with_the_stream(self) -> None: self.assertEqual(reader.read(1), b'g') self._close_reader(reader) - def test_truncate_leaves_the_position_where_it_was(self) -> None: + def test_truncate_buffer_leaves_the_position_where_it_was(self) -> None: """:meth:`io.IOBase.truncate` does not move the position, and neither may this one. The truncation here is to the size the buffer already has, so its *content* is the @@ -349,25 +468,25 @@ def test_truncate_leaves_the_position_where_it_was(self) -> None: self.assertEqual(reader.seek(2), 2) self.assertEqual(reader._buffer.tell(), 2) - self.assertEqual(reader.truncate(8), 8) + self.assertEqual(reader._truncate_buffer(8), 8) self.assertEqual(reader.tell(), 2) self.assertEqual(reader._buffer.tell(), 2) self.assertEqual(reader.read(2), b'cd') self._close_reader(reader) - def test_truncate_without_a_size_resizes_to_the_current_position(self) -> None: + def test_truncate_buffer_without_a_size_resizes_to_the_current_position(self) -> None: """An omitted size means the current position, not zero.""" reader = self.SeekableReader(io.BytesIO(b'abcdefgh'), buffer_size=8) self.assertEqual(reader.read(5), b'abcde') self.assertEqual(reader.tell(), 5) - self.assertEqual(reader.truncate(), 5) + self.assertEqual(reader._truncate_buffer(), 5) self.assertEqual(reader._buffer_size, 5) self.assertEqual(bytes(reader._buffer.getvalue()), b'abcde') self._close_reader(reader) - def test_truncate_below_the_content_leaves_the_reader_usable(self) -> None: + def test_truncate_buffer_below_the_content_leaves_the_reader_usable(self) -> None: """A truncation has to bring ``_buffer_cur`` down with the buffer it indexes. Left above the new size it addressed octets the buffer no longer has, and the next @@ -378,16 +497,16 @@ def test_truncate_below_the_content_leaves_the_reader_usable(self) -> None: reader = self.SeekableReader(io.BytesIO(b'abcdefghijkl'), buffer_size=8) self.assertEqual(reader.read(6), b'abcdef') - self.assertEqual(reader.truncate(3), 3) + self.assertEqual(reader._truncate_buffer(3), 3) self.assertEqual(reader._buffer_cur, 3) self.assertEqual(reader._buffer_set, 3) self.assertEqual(reader.read(1), b'g') self._close_reader(reader) - def test_truncate_to_nothing_leaves_the_reader_usable(self) -> None: + def test_truncate_buffer_to_nothing_leaves_the_reader_usable(self) -> None: """Truncating the buffer away entirely still has to leave reads working. - ``truncate(0)`` is the only way to reach a buffer of no size: the constructor + ``_truncate_buffer(0)`` is the only way to reach a buffer of no size: the constructor refuses one, since ``io.BufferedReader`` rejects a non-positive ``buffer_size`` with ``ValueError: buffer size must be strictly positive``. The next read then went to ``_write_buffer``, whose ``buf[-self._buffer_size:]`` is ``buf[-0:]`` -- @@ -400,7 +519,7 @@ def test_truncate_to_nothing_leaves_the_reader_usable(self) -> None: """ reader = self.SeekableReader(io.BytesIO(b'abcde'), buffer_size=5) - self.assertEqual(reader.truncate(0), 0) + self.assertEqual(reader._truncate_buffer(0), 0) self.assertEqual(reader.read(1), b'a') self.assertEqual(reader.read(2), b'bc') self.assertEqual(reader.tell(), 3) @@ -743,7 +862,7 @@ def test_the_window_base_and_content_pointer_track_the_stream_consumption(self) ('seek', lambda: reader.seek(2), 2), ('readline', lambda: reader.readline(4), b'cdef'), ('read1', lambda: reader.read1(2), b'gh'), - ('truncate', lambda: reader.truncate(4), 4), + ('_truncate_buffer', lambda: reader._truncate_buffer(4), 4), ('read', lambda: reader.read(2), b'ij'), ] for name, operation, expected in operations: @@ -759,7 +878,7 @@ def test_the_window_base_and_content_pointer_track_the_stream_consumption(self) def test_a_position_the_window_has_dropped_is_refused_not_guessed(self) -> None: """A read from before the window raises, rather than answering from the wrong octet. - :meth:`truncate` advances the window's base past the octets it drops, which can + :meth:`_truncate_buffer` advances the window's base past the octets it drops, which can leave a position already set sitting before it. :meth:`seek` refuses that position outright -- the stream cannot be rewound to re-supply the octets, so there is nothing to read -- but the buffered read paths reached it too, by way of a position @@ -775,7 +894,7 @@ def test_a_position_the_window_has_dropped_is_refused_not_guessed(self) -> None: self.assertEqual(reader.read(4), b'abcd') self.assertEqual(reader.seek(1), 1) - self.assertEqual(reader.truncate(2), 2) + self.assertEqual(reader._truncate_buffer(2), 2) self.assertEqual(reader._buffer_set, 2) # the window now starts at 2 self.assertEqual(reader.tell(), 1) # and the position is behind it @@ -792,7 +911,7 @@ def test_a_zero_length_read_is_answered_without_consulting_the_window(self) -> N about a request that wants nothing: every one of these returned ``b''`` before the refusal existed, and a zero-length read failing on position grounds is not a behaviour either issue asked for. Checking the window before looking at the requested - size made all four of them raise from a position :meth:`truncate` had stranded. + size made all four of them raise from a position :meth:`_truncate_buffer` had stranded. The same position is asserted both ways round, which is what makes this a statement about the *size* rather than about the state: at size zero all four return, and the @@ -811,7 +930,7 @@ def test_a_zero_length_read_is_answered_without_consulting_the_window(self) -> N self.assertEqual(reader.read(4), b'abcd') self.assertEqual(reader.seek(1), 1) if stranded: - self.assertEqual(reader.truncate(2), 2) + self.assertEqual(reader._truncate_buffer(2), 2) self.assertEqual(reader._buffer_set, 2) self.assertLess(reader.tell(), reader._buffer_set)