fix(corekit): keep SeekableReader's buffer content when truncating it (#622)

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** -- `_missing_` in the four Mobility Header flag enumerations ended in `return cls(value)`, the same constructor that had just failed to find the value, so every in-range value that is not already a member re-entered `_missing_` unbounded and raised `RecursionError`. `BindingACKFlag`, `BindingUpdateFlag`, `HandoverACKFlag` and `HandoverInitiateFlag` are `IntFlag` types whose members are single bits, so the hole was not some exotic bit pattern: `F(0)` -- no flags set, the most ordinary value a flag octet can carry -- recursed on all four, and so did every composite of two defined bits, such as `BindingACKFlag(0x06)`. Defining `_missing_` at all is what caused it, because it shadowed the `aenum` `Flag` machinery that resolves exactly those values; `pcapkit/const/tcp/flags.py` defines no `_missing_` and has never had the defect. All four now end in `return super()._missing_(value)`, which is what `pcapkit/vendor/default.py` emits for every other generated enumeration and what 75 of the 117 modules under `pcapkit/const/` already do, so `F(0)` is an empty flag and `BindingACKFlag(0x06)` is `S|D`. The `extend_enum` idiom that `pcapkit/const/pcapng/record_type.py` and `secrets_type.py` use to mint a member for an unassigned integer -- the only other two modules whose `_missing_` ends in `return cls(value)`, and which do not recurse precisely because that `extend_enum` runs first -- was considered and rejected for a flag type: naming `0x06` `Unassigned_0x06` would hide the composite and pollute `_member_map_` with an entry per bit pattern, up to 65536 of them for `BindingUpdateFlag`. The range guard above it is untouched, so an out-of-range or non-integer value is still the same `ValueError`. Fixed in the four `pcapkit/vendor/mh/` templates and regenerated, the `pcapkit/const/mh/` modules being generated output that the next crawl would otherwise revert (#623).
 - **Fixed** -- the HIP `SOLUTION` builder sized the parameter with `4 + math.ceil(max(random.bit_length(), solution.bit_length()) / 4)`, which is an invalid shorthand for the two fields it actually has to describe. [RFC 7401 Section 5.2.5](https://datatracker.ietf.org/doc/html/rfc7401#section-5.2.5) gives the parameter's `Length` as `4 + RHASH_len / 4` over a `Random #I` and a `Puzzle solution #J` of `RHASH_len / 8` octets **each**, and `/ 4` equals twice `/ 8` only because `RHASH_len` -- the natural output length of a hash function, in bits -- is a whole number of octets. Applied to an arbitrary `bit_length()` the identity fails, and the builder emitted an *odd* contents width, which its own reader then refused: `_read_param_solution`'s `(schema.len - 4) % 2` guard exists because `SolutionParameter` splits that width into two equal `(len - 4) // 2` halves, so an odd width cannot be framed at all. `random=0x1` with `solution=0xfff` declared `len=7` and raised `ProtocolError: HIPv2: [ParamNo 321] invalid format` on input the library had itself produced. The undersized length was the worse half of it: because both fields take their width from that same `len`, `solution=0xfff` was packed into the one octet it allowed and came back as `0xff` -- silent truncation, nothing raised. Fixed to `4 + 2 * math.ceil(max(...) / 8)`, the form the sibling `PUZZLE` builder already uses, which is even by construction and so can never trip the guard. Loosening the reader was rejected as the alternative: an odd contents width has no meaning in the wire format, since the RFC makes the two fields equal-width and says nothing about which would take an extra octet, and a laxer guard would still have truncated the value (#608).
 - **Added** -- `SOLUTION` parameter width coverage in `tests/protocols/internet/test_hip_unit.py`: eight widths asserting the declared length, the reader's acceptance and the construct-pack-parse cycle, plus a 57-bit case pinning the 20-octet length [RFC 5201 Section 5.2.5](https://datatracker.ietf.org/doc/html/rfc5201#section-5.2.5) requires of HIPv1. Five of the eight widths -- 1, 9, 12, 17 and 25 bits -- are deliberately *not* multiples of eight, because at a multiple of eight the defective `ceil(bits / 4)` coincides with the correct width, which is why every fixture that reached this builder passed through it unharmed; the round-trip case in `examples/generators/options.py` supplies no `random` or `solution` at all, so it exercised the formula at zero bits. 57 bits is what discriminates on the HIPv1 path for the same reason 64 does not: both formulas give 20 at a full-width value (#608).

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** -- ``_missing_`` in the four Mobility Header flag enumerations ended in
   ``return cls(value)``, the same constructor that had just failed to find the value,
   so every in-range value that is not already a member re-entered ``_missing_``