From 478296afee1d8de52c6a4782daaeb4e6d63d0247 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 22 Sep 2026 00:56:59 -0400 Subject: [PATCH] fix(corekit): keep SeekableReader's buffer content when truncating it (#622) * `SeekableReader.truncate` kept padding in preference to content. The buffer is a sliding window whose content occupies `[0:_buffer_cur]`, with never-read padding behind it, so `temp[-size:]` -- slicing the buffer rather than the content -- kept the padding and discarded the octets actually read, and `temp.rjust(size)` prefixed the new zeros, displacing the content past where `_buffer_set` and `_buffer_cur` address it. An extension now appends at the tail, which is what `io.IOBase.truncate` means by "the contents of the new file area", and a reduction keeps the most recent octets, which is the lookback a window holds. * A reduction advances `_buffer_set` past the octets it drops, so that `_buffer_set + _buffer_cur` still equals how far the stream has been consumed -- the quantity `seek` reads as its licence to fetch more. Clamping `_buffer_cur` alone left that sum short of the stream, and the next forward `seek` then spliced in octets from the wrong absolute offset and said nothing. * `read` capped a buffered read at `min(size, self._buffer_cur - 1)`, a count less one measured from the start of the buffer rather than the run remaining from the position being read from. That is one octet short at the start of the buffer -- and the shortfall was made up from the stream past the octet it had skipped, dropping it -- and reaches into the padding anywhere further in. * Three more of the method's contract: the position was reset to the start of the buffer rather than left where it was, an omitted `size` resized to `0` rather than to the current position, and `_buffer_cur` was left addressing octets a reduced buffer no longer had. * `_write_buffer`'s `buf[-self._buffer_size:]` is `buf[-0:]` for a buffer of no size -- the whole of the octets just read rather than none of them -- which raised `ValueError` after a `truncate(0)`. Now counted from the front. Only `truncate` can reach a zero-length buffer; the constructor refuses one. * Eight new tests in `tests/corekit/test_io.py`, all over `io.BytesIO` with no fixture. The existing `truncate(None)` assertion expected `0` and now expects the current position. `tests/corekit/test_io.py` goes 13 failed / 14 passed / 5 subtests failed / exit 1 to 22 passed / 6 subtests passed / exit 0. `tests/corekit` 167 passed / 378 subtests / exit 0, `tests/foundation/test_extraction.py` -- the one place that wraps a non-seekable stream in this class -- 12 passed / 28 subtests / exit 0. Fuzzing 3000 random operation sequences against the buffer's invariants, including that `_buffer_set + _buffer_cur` tracks the stream's real consumption, goes 2372 inconsistent states and 683 undocumented exceptions to none of either. Coverage of `pcapkit/corekit/io.py` holds at 100% statement and 100% branch. `util/changelog_md.py --check` exits 0. Fixes #622 --- CHANGELOG.md | 1 + docs/source/changelog/1.5.0.rst | 47 ++++++++ pcapkit/corekit/io.py | 71 ++++++++++-- tests/corekit/test_io.py | 195 +++++++++++++++++++++++++++++++- 4 files changed, 303 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e595d2846..3061453b41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,7 @@ This is the resolution of #548, which reported `TransType.L2TP` (115) as registe - **Fixed** -- the one assertion #604 left pinning the old padding side, which had been red on `mainline` since #621 merged. `TCPUDPUnitTests.test_a_truncated_option_still_parses_its_declared_length` expected a truncated TCP option's `data` as the synthesised zero octets *followed by* the real ones, which is what `rjust()` produced; #621 made the padding `ljust()` everywhere but could not retarget this file, since another change (#612) owned it at the time and editing it concurrently risked discarding work that has since landed. The real octets now come first for both parametrised widths, and the docstring above the assertion says tail-padding rather than left-padding. Test-only: no library code changes, and the sibling case in `tests/protocols/internet/test_ipv4_unit.py` was already retargeted in #621. Measured against `main` at `2221c2d8f`: two subtest failures before, none after (#604). - **Fixed** -- `main` went red the moment #604's `ljust()` landed, because `TCPUDPUnitTests.test_a_truncated_option_still_parses_its_declared_length` still pinned the head-padded short read that fix removed. The `Reserved_79` option declaring `length=12` over 6 real octets now reports `aabbccddeeff00000000` where the test expected `00000000aabbccddeeff`, so both subtests -- `declared_length=12` and `=32` -- failed on that one assertion while the 17 other cases in the file stayed green: the parse itself never changed, only which end the synthesised zeros sit at. The expectation is inverted, and the docstring above it -- which said the short read was *left*-padded and described the value as four zero octets followed by the six real ones -- is corrected to match, since a docstring that contradicts its own assertion is how the stale expectation survived in the first place. The inputs do discriminate: `trailing` is non-zero and the pad width is 4 and 24, so neither subtest would hold under the other order. #621 left this file alone deliberately, because #612 owned it at the time, and merged two minutes ahead of the cross-review verdict that named it (#604, #621). - **Fixed** -- `util/bump_version.py` left `CITATION.cff` naming the previous release. Nothing else in the repository maintains that file -- no workflow, hook or packaging file mentions it -- so every bump since it landed in #615 would have stranded the `version` and `date-released` it renders as GitHub's "Cite this repository" button and that citation managers, Zenodo and dependency inventories read directly. Both fields now move with `__version__`. They have the same standing, since the file's own header says both describe the newest *published* release, and moving only one would assert that 1.5.0b5 was released on the day 1.5.0b4 was; the date is taken in UTC, because seven of the thirty most recent bumps were made late evening in US-Eastern where a local date is a day behind the publish it describes. That the two are the same day at all is measured rather than assumed: the bump is what triggers `create-release.yml`, the median gap to the PyPI upload is three minutes, and the UTC calendar dates agree 30 times out of 30. The rewrite is line-oriented, so the comment header, key ordering and each field's existing quoting survive -- `cff-version` and a `references` entry's own `version` are anchored out at column zero -- and the result is checked with `cffconvert --validate`. An absent file is reported on stderr and skipped rather than failing the vendor cron before its `git commit`, which would discard the whole registry crawl for the sake of a documentation file; a file present with no `version` field raises instead, before anything is written, because rewriting nothing while reporting success is the staleness this fixes. Two things came with it. The script gains a `main()` guard, having previously run the entire bump at import, which is why it had no testable surface; and the `import pcapkit` fallback in its version reader, which returned `"1.5.0b4'\n"` -- closing quote and newline included, which `packaging` rejects -- is fixed, a path that had never worked and went unnoticed because the only caller installs the package first. A new gate asserts the committed file still names the packaged version, covering the version changes made by hand, which never run this script at all -- 40 of the 159 commits that have moved `__version__` on `main`, a quarter over the project's life and 11 of the most recent 25 (#625). +- **Fixed** -- `SeekableReader.truncate` put its padding where the reader's own bookkeeping says the content is, and `read` then returned one octet fewer than the buffer held. Same family as #604, but two defects rather than one, and the padding side is only half of it. The buffer keeps its content at `[0:_buffer_cur]` with unwritten padding behind it, so a reduction has to keep the octets it has and an extension has to append at the *tail* -- the latter is what `io.IOBase.truncate` means by "the contents of the new file area", the area past the old end. It did neither: `temp[-size:]` sliced the buffer rather than the content, so it kept the trailing padding and discarded the octets actually read, and `temp.rjust(size)` prefixed the new zeros, displacing the content past where `_buffer_set` and `_buffer_cur` address it. Separately, `read` capped a buffered read at `min(size, self._buffer_cur - 1)`, a count less one measured from the start of the buffer rather than the run remaining from the position being read from -- one octet short at the start of the buffer, and reaching past the content into the padding anywhere further in. The shortfall was then made up from the stream, *past* the octet that had been skipped, which both dropped that octet and left the return short. The reported symptom needed both: `read(4)`, `truncate(8)`, `seek(0)`, `read(8)` over `b'abcde'` returned `b'\x00\x00\x00e'`, four octets of an eight octet request with three of them padding, and returns `b'abcde'` now -- five being the whole of what a five octet stream can answer with. Three more of the method's contract were wrong and are fixed with it: the position was reset to the start of the buffer rather than left alone, so a read after a truncation resumed from the wrong octet; an omitted `size` resized to `0` rather than to the current position; and `_buffer_cur` was left addressing octets a reduced buffer no longer had, so the next read raised `ValueError: memoryview assignment: lvalue and rvalue have different structures` from `_write_buffer` rather than returning anything. `truncate(0)` raised the same `ValueError` by a second route, and `_write_buffer` is fixed with it: `buf[-self._buffer_size:]` is `buf[-0:]` for a buffer of no size at all -- the whole of the octets just read rather than none of them -- so it is now counted from the front. That state is reachable only through `truncate`, since the constructor refuses a non-positive `buffer_size`. A reduction also advances `_buffer_set` past the octets it drops, keeping `_buffer_set + _buffer_cur` equal to how far the stream has been consumed, which `seek` reads as its licence to fetch more. Clamping `_buffer_cur` alone made that sum *under*-report the stream, and the next forward `seek` then spliced in octets from the wrong absolute offset and said nothing: `read(8)`, `truncate(3)`, `seek(6)`, `read(1)` over `b'abcdefghijklmnop'` returned `b'l'` where `b'g'` is the octet at offset 6 -- worse than the `ValueError` it replaced, being silent. All three came out of fuzzing random operation sequences against the buffer's own invariants, none from reading the code: of 3000 rounds, 2372 left the bookkeeping inconsistent before and 683 raised an undocumented exception, and none of either do now. Latent in this library rather than live -- nothing here calls `truncate`, confirmed by grep, though it is public on a public class -- and invisible to the existing tests, which asserted the return value and never the content (#622). - **Fixed** -- `LICENSE`'s copyright notice began the term at 2018, a year after the work it covers. The repository's first commit is `c57f7d0b7` "Initial commit", dated 2017-11-07, so the notice understated the term and contradicted the only other copyright site in the tree: `docs/source/conf.py` computes its Sphinx footer as `f'2017-{datetime.date.today().year}, Jarry Shaw'` and has said 2017 for as long as it has existed. The two now agree on where the term starts. The wrong start year survived every maintenance pass the line has had, because each looked only at the other end of the range: `bc836cfa2` (2020-05-31) introduced `2018-2020` when the BSD-3-Clause text replaced MPL 2.0 -- the file until then was stock MPL text carrying no author notice at all, so 2018 is the first start year the project ever asserted and it was already a year late when it was written -- and the range was then bumped by hand three times, to `2018-2022`, `2018-2023` and `2018-2026` (#615), each bump correcting the end year and copying `2018` forward untouched. The end year is **dropped** rather than automated, which is the other half of the change: the notice ships inside the sdist and the wheel, so its text is fixed at build time in every copy already downloaded and cannot be computed the way the docs footer is; copyright subsists from creation whether or not a notice names the current year; and BSD-3-Clause's canonical form is `Copyright (c) `, singular. So a range here is six years of hand maintenance on one line buying nothing, and no workflow, script hook or other automation is added in its place -- that was considered and rejected, since automating a value that need not be current is worse than not carrying it. `conf.py` is deliberately untouched, a self-maintaining docs footer showing a range being both conventional and correct. The licence body is unaltered: the whole-file diff is one hunk at line 3, and the remaining 28 lines carry the canonical BSD-3-Clause wording verbatim. Three things distinguish the file from SPDX's bare `licenseText`, and all three predate this change -- the `BSD 3-Clause License` title line, which the opensource.org template carries and SPDX omits; the `*` bullets in place of `1.`/`2.`/`3.`; and the extra `All rights reserved.` line -- and the `DAMAGE.` that #615 repaired from a stray `DAMAGE.s` is still clean (#630). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 8b2afd6a38..263c5850a9 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -1063,6 +1063,53 @@ pull requests between #326 and #509. version, covering the version changes made by hand, which never run this script at all -- 40 of the 159 commits that have moved ``__version__`` on ``main``, a quarter over the project's life and 11 of the most recent 25 (#625). +* **Fixed** -- ``SeekableReader.truncate`` put its padding where the reader's own + bookkeeping says the content is, and ``read`` then returned one octet fewer than + the buffer held. Same family as #604, but two defects rather than one, and the + padding side is only half of it. The buffer keeps its content at + ``[0:_buffer_cur]`` with unwritten padding behind it, so a reduction has to keep + the octets it has and an extension has to append at the *tail* -- the latter is + what ``io.IOBase.truncate`` means by "the contents of the new file area", the area + past the old end. It did neither: ``temp[-size:]`` sliced the buffer rather than + the content, so it kept the trailing padding and discarded the octets actually + read, and ``temp.rjust(size)`` prefixed the new zeros, displacing the content past + where ``_buffer_set`` and ``_buffer_cur`` address it. Separately, ``read`` + capped a buffered read at ``min(size, self._buffer_cur - 1)``, a count less one + measured from the start of the buffer rather than the run remaining from the + position being read from -- one octet short at the start of the buffer, and + reaching past the content into the padding anywhere further in. The shortfall was + then made up from the stream, *past* the octet that had been skipped, which both + dropped that octet and left the return short. The reported symptom needed both: + ``read(4)``, ``truncate(8)``, ``seek(0)``, ``read(8)`` over ``b'abcde'`` returned + ``b'\x00\x00\x00e'``, four octets of an eight octet request with three of them + padding, and returns ``b'abcde'`` now -- five being the whole of what a five octet + stream can answer with. Three more of the method's contract were wrong and are + fixed with it: the position was reset to the start of the buffer rather than left + alone, so a read after a truncation resumed from the wrong octet; an omitted + ``size`` resized to ``0`` rather than to the current position; and + ``_buffer_cur`` was left addressing octets a reduced buffer no longer had, so the + next read raised + ``ValueError: memoryview assignment: lvalue and rvalue have different structures`` + from ``_write_buffer`` rather than returning anything. ``truncate(0)`` raised the + same ``ValueError`` by a second route, and ``_write_buffer`` is fixed with it: + ``buf[-self._buffer_size:]`` is ``buf[-0:]`` for a buffer of no size at all -- + the whole of the octets just read rather than none of them -- so it is now counted + from the front. That state is reachable only through ``truncate``, since the + constructor refuses a non-positive ``buffer_size``. A reduction also advances + ``_buffer_set`` past the octets it drops, keeping ``_buffer_set + _buffer_cur`` + equal to how far the stream has been consumed, which ``seek`` reads as its licence + to fetch more. Clamping ``_buffer_cur`` alone made that sum *under*-report the + stream, and the next forward ``seek`` then spliced in octets from the wrong + absolute offset and said nothing: ``read(8)``, ``truncate(3)``, ``seek(6)``, + ``read(1)`` over ``b'abcdefghijklmnop'`` returned ``b'l'`` where ``b'g'`` is the + octet at offset 6 -- worse than the ``ValueError`` it replaced, being silent. All + three came out of fuzzing random operation sequences against the buffer's own + invariants, none from reading the code: of 3000 rounds, 2372 left the bookkeeping + inconsistent before and 683 raised an undocumented exception, and none of either + do now. Latent in this library rather + than live -- nothing here calls ``truncate``, confirmed by grep, though it is + public on a public class -- and invisible to the existing tests, which asserted + the return value and never the content (#622). * **Fixed** -- ``LICENSE``'s copyright notice began the term at 2018, a year after the work it covers. The repository's first commit is ``c57f7d0b7`` "Initial diff --git a/pcapkit/corekit/io.py b/pcapkit/corekit/io.py index 2be91eff2d..1ae8cc3bbf 100644 --- a/pcapkit/corekit/io.py +++ b/pcapkit/corekit/io.py @@ -131,7 +131,10 @@ def _write_buffer(self, buf: 'bytes', /) -> 'None': if self._buffer_cur > self._buffer_size: if buf_len >= self._buffer_size: - self._buffer_view[:] = buf[-self._buffer_size:] + # NOTE: the last ``_buffer_size`` octets, counted from the front rather + # than as ``buf[-self._buffer_size:]``, which for a buffer of no size at + # all is ``buf[-0:]`` -- the whole of ``buf``, not none of it. + self._buffer_view[:] = buf[buf_len - self._buffer_size:] else: self._buffer_view[:-buf_len] = self._buffer_view[old_ptr - (self._buffer_size - buf_len):old_ptr] self._buffer_view[-buf_len:] = buf @@ -316,22 +319,62 @@ def truncate(self, size: 'int | None' = None, /) -> 'int': 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.""" + size is returned. + + 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. + + 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 + reduction below the content keeps the **most recent** ``size`` octets and advances + ``_buffer_set`` past the ones it drops, because what this holds is lookback: the + octets it can still answer for are the ones just read, and the stream is already + beyond them. Padding is never kept in preference to content either way. + + Advancing ``_buffer_set`` is what keeps ``_buffer_set + _buffer_cur`` equal to how far + the stream has actually been consumed, which :meth:`seek` relies on to decide whether + it may read ahead to fill a gap. A reduction that shrank the window without moving its + base would leave that sum short of the stream, and the next forward :meth:`seek` would + splice in octets from the wrong absolute offset without complaining. + + Octets dropped by a reduction are gone for good, since the stream cannot be rewound to + re-supply them. A position left among them is then before the window, which + :meth:`seek` refuses as it refuses any other. + + """ if size is None: - size = 0 + # 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. + size = max(self._tell - self._buffer_set, 0) if size < 0: raise TruncateError(f'negative size value {size}') + + # NOTE: the position isn't changed by a truncation, but rebuilding the buffer + # resets it, so it is read here and put back below -- moved down by whatever + # the window's base moved up, so that it still denotes the same octet. + buffer_pos = self._buffer.tell() self._buffer_view.release() - temp = self._buffer.getvalue() - if size > self._buffer_size: - self._buffer = io.BytesIO(temp.rjust(size, b'\x00')) - else: - # keep the last ``size`` bytes - self._buffer = io.BytesIO(temp[-size:] if size else b'') + # NOTE: only ``[0:_buffer_cur]`` is content. Slicing the buffer itself would + # keep padding that was never read and count it as though it were data. + temp = self._buffer.getvalue()[:self._buffer_cur] + dropped = max(len(temp) - size, 0) + + self._buffer = io.BytesIO(temp[dropped:].ljust(size, b'\x00')) self._buffer_view = self._buffer.getbuffer() + self._buffer.seek(max(buffer_pos - dropped, 0), io.SEEK_SET) self._buffer_size = size + # NOTE: ``_buffer_set + _buffer_cur`` is unchanged by construction -- the base + # gains exactly what the content pointer loses -- so the stream's consumption + # point still reads correctly out of the pair. + self._buffer_set += dropped + self._buffer_cur = len(temp) - dropped return self._buffer_size def writeable(self) -> 'bool': @@ -372,7 +415,15 @@ def read(self, size: 'int | None' = -1, /) -> 'bytes': temp_file.seek(self._tell, io.SEEK_SET) buf = temp_file.read(size) else: - buf = self._buffer.read(min(size, self._buffer_cur - 1)) + # NOTE: ``_buffer_cur`` counts the octets written into the buffer, so what + # is available from here is the run between the current position and the end + # of that content. That count less one is neither: at the start of the + # buffer it is one octet short, and the shortfall is then made up from the + # stream -- past the octet that was skipped, losing it -- while further in + # it reaches beyond the content and hands the padding behind it back as + # data, which is also what an uncapped read does. + buf_rem = self._buffer_set + self._buffer_cur - self._tell + buf = self._buffer.read(buf_rem if size < 0 else min(size, buf_rem)) size_rem = -1 if size < 0 or (size_rem := size - len(buf)) > 0: diff --git a/tests/corekit/test_io.py b/tests/corekit/test_io.py index 971e881d4b..264ac59378 100644 --- a/tests/corekit/test_io.py +++ b/tests/corekit/test_io.py @@ -144,7 +144,11 @@ def test_seek_variants_warnings_and_truncate_sizes(self) -> None: self.assertTrue(reader.seekable()) self.assertFalse(reader.writeable()) - self.assertEqual(reader.truncate(None), 0) + # 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._close_reader(reader) @@ -240,6 +244,195 @@ 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: + """Issue #622, verbatim: the octets already read survive a truncation. + + The buffer holds its content at ``[0:_buffer_cur]`` and nothing but unwritten + padding after it, so slicing the *buffer's* tail for the octets to keep -- rather + than the content's -- kept the padding and threw the content away. ``b'abcde'`` + discriminates between the two: the content's last four octets are ``b'abcd'``, + the buffer's are ``b'\\x00\\x00\\x00\\x00'``, which uniform data could not tell + apart. + + """ + reader = self.SeekableReader(io.BytesIO(b'abcde')) + + self.assertEqual(reader.read(4), b'abcd') + self.assertEqual(reader.truncate(8), 8) + self.assertEqual(reader.seek(0), 0) + + # NOTE: the stream holds five octets, so five is the whole of what an eight octet + # request can be answered with; it returned b'\x00\x00\x00e' before the fix -- four + # octets, the content displaced by the padding, and one octet of it lost outright. + 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: + """A truncation never keeps padding in preference to content. + + Each case distinguishes head from tail handling, since the expected buffer is the + same octets in a different place: growing ``b'abcd'`` to 8 gives + ``b'abcd\\x00\\x00\\x00\\x00'`` one way round and ``b'\\x00\\x00\\x00\\x00abcd'`` the + other. The sizes are deliberately 3, 5, 7 and 8 rather than all multiples of one + number, so a fix that is off by a constant cannot pass the table. + + A reduction below the content keeps the most recent octets and advances + ``_buffer_set`` past the dropped ones, which is what holds + ``_buffer_set + _buffer_cur`` at the stream's consumption point -- asserted here as + ``read_size``, since that is how many octets each case has taken off the stream. + + """ + cases = [ + # (buffer_size, octets read, truncate size, expected buffer, expected _buffer_set) + (4, 4, 8, b'abcd\x00\x00\x00\x00', 0), # grown: the new area is at the tail + (4, 4, 3, b'bcd', 1), # reduced below the content + (8, 6, 3, b'def', 3), # reduced well below the content + (8, 3, 5, b'abc\x00\x00', 0), # reduced, but above the content + (8, 5, 7, b'abcde\x00\x00', 0), # grown by one octet only + (8, 4, 8, b'abcd\x00\x00\x00\x00', 0), # unchanged in size: a control + ] + for buffer_size, read_size, size, expected, expected_set in cases: + with self.subTest(buffer_size=buffer_size, read_size=read_size, size=size): + 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(bytes(reader._buffer.getvalue()), expected) + self.assertEqual(reader._buffer_set, expected_set) + # NOTE: the content pointer indexes the buffer, so it cannot be left + # pointing past the end of it. + self.assertLessEqual(reader._buffer_cur, reader._buffer_size) + # NOTE: and the pair still has to say how far the stream has been read. + 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: + """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 + consumed, and reads ahead from there to fill a gap. A reduction that shrank the + window without advancing its base left that sum short of the stream -- here it + would say 3 where 8 octets had been read -- so the fill fetched the octets at 8 + and labelled them as the ones at 3. Nothing raised; ``read(1)`` at offset 6 simply + returned ``b'l'`` instead of ``b'g'``. + + The data is non-uniform so the mislabelled octets are distinguishable from the + right ones, which is the whole of what this test turns on. + + """ + reader = self.SeekableReader(io.BytesIO(b'abcdefghijklmnop'), buffer_size=8) + + self.assertEqual(reader.read(8), b'abcdefgh') + self.assertEqual(reader.truncate(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') + self.assertEqual(reader._buffer_set, 5) + self.assertEqual(reader._buffer_cur, 3) + + self.assertEqual(reader.seek(6), 6) + self.assertEqual(reader.read(1), b'g') + self._close_reader(reader) + + def test_truncate_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 + same either way and only the position can account for the difference: reading from + a position reset to zero returns ``b'ab'``, reading from the preserved position + returns ``b'cd'``. + + """ + reader = self.SeekableReader(io.BytesIO(b'abcdefgh'), buffer_size=8) + + self.assertEqual(reader.read(4), b'abcd') + self.assertEqual(reader.seek(2), 2) + self.assertEqual(reader._buffer.tell(), 2) + + self.assertEqual(reader.truncate(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: + """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._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: + """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 + read raised ``ValueError: memoryview assignment: lvalue and rvalue have different + structures`` from :meth:`_write_buffer` rather than returning anything. + + """ + 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._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: + """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 + 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:]`` -- + the whole of the octets just read rather than none of them -- and assigning + those to a buffer with no room raised ``ValueError``. + + Reading forward is all that can still work: with nothing buffered there is no + lookback, so seeking back has to fail, and it does. + + """ + reader = self.SeekableReader(io.BytesIO(b'abcde'), buffer_size=5) + + self.assertEqual(reader.truncate(0), 0) + self.assertEqual(reader.read(1), b'a') + self.assertEqual(reader.read(2), b'bc') + self.assertEqual(reader.tell(), 3) + with self.assertRaises(self.exceptions.SeekError): + reader.seek(0) + self._close_reader(reader) + + def test_buffered_read_returns_every_buffered_octet(self) -> None: + """A read served from the buffer stops at the content, not one octet short of it. + + The existing round trip at :meth:`test_buffered_readline_read_read1_and_peek_paths` + cannot see this: it seeks to 1, where ``_buffer_cur - 1`` happens to equal the + octets actually available, so the two candidate caps agree. Seeking to 0 separates + them -- four octets are available and the old cap allowed three, making up the + fourth from the stream *past* the octet it had skipped. + + """ + reader = self.SeekableReader(io.BytesIO(b'abcde'), buffer_size=8) + self.assertEqual(reader.read(4), b'abcd') + self.assertEqual(reader.seek(0), 0) + self.assertEqual(reader.read(5), b'abcde') # was b'abce': four octets, d dropped + self.assertEqual(reader.tell(), 5) + self._close_reader(reader) + + # NOTE: an unbounded read must not hand back the padding behind the content + # either; this returned b'abcd\x00\x00\x00\x00e' before the fix. + reader = self.SeekableReader(io.BytesIO(b'abcde'), buffer_size=8) + self.assertEqual(reader.read(4), b'abcd') + self.assertEqual(reader.seek(0), 0) + self.assertEqual(reader.read(-1), b'abcde') + self._close_reader(reader) + def test_saved_readline_and_empty_buffer_refill_edges(self) -> None: reader = self.SeekableReader(io.BytesIO(b'abc\ndef'), buffer_size=4, buffer_save=True) self.assertEqual(reader.read(6), b'abc\nde')