fix(file): write every byte or fail; evict a payload that shrank under read (LAB-2682) - #272
Open
27Bslash6 wants to merge 8 commits into
Open
fix(file): write every byte or fail; evict a payload that shrank under read (LAB-2682)#27227Bslash6 wants to merge 8 commits into
27Bslash6 wants to merge 8 commits into
Conversation
…in zero-copy in default gate (LAB-770) Expert-panel findings applied: - deserialize() catches BufferError so a non-u8 exporter (e.g. numpy float array) rejected at the PyO3 boundary still raises SerializationError, as documented (pre-change bytes() coercion surfaced these as ValueError) - retrieve(): single detach/map_err tail; SAFETY comment states the data-race residual is UB accepted per the hashlib GIL-release precedent; empty-buffer arm documents the from_raw_parts non-null requirement - new non-slow tracemalloc test pins the zero-copy borrow (<1.5x payload) so a to_vec revert fails the default gate, not just the slow suite - dropped a redundant equivalence assert
…p pip for PYSEC-2026-3721 (LAB-770) CodeRabbit correctly flagged that readonly() describes the view, not the backing storage: memoryview(bytearray).toreadonly() passed the old gate, making the detached read a data race (UB). retrieve() now takes &Bound<PyAny>: a bytes argument borrows via the safe as_bytes API; a buffer-protocol argument borrows only when a pointer-range check proves its memory lies inside the immutable bytes object its view exports (.obj) — attribute trust alone is spoofable by a PEP 688 __buffer__ exporter with a decoy .obj, so the proof is the pointer range, held alive across the GIL release. Everything else copies (pre-LAB-770 semantics). The production shape — unwrap's memoryview over the bytes envelope — still takes the zero-copy path (end-to-end 3.5x guard green). Regression tests: readonly-view-over-bytearray and spoofed-.obj exporter round-trip via the copy path; zero-copy test docstring now states its tracemalloc blind spot honestly (a Rust-side copy is invisible to it). Also: uv lock pip 26.1.2 -> 26.2.1 — pip-audit fails the Python Dependency CVEs check on PYSEC-2026-3721 (CVE-2026-13346); red on main for the same reason, this unblocks it here.
… (LAB-770) A single read(2) may return fewer bytes than asked (POSIX; Linux caps one call at ~2 GiB), and values above MMAP_MAX_BYTES take this path, so a >2 GiB payload was silently truncated into a spurious integrity failure. One helper, _read_fully, now backs all six fd reads. Error semantics are unchanged: EOF still yields a short result the header/checksum checks reject as before. The single-chunk join aliases its input, so the 3.5x read-peak guard holds. Kody review follow-up on #267.
…ffer's short header (LAB-770) Expert-panel re-run over the whole #267 diff (the earlier panels predated a8b7960, so the pointer-range borrow it introduced had never been reviewed). The gate already proved the buffer's memory lies inside the base `bytes`, so the offset into that object is computable — which makes the borrow expressible as an ordinary slice. from_raw_parts is gone; this was the crate's only unsafe block. Same pointer, same zero-copy path, same 3.5x guard, now bounds-checked by the compiler with no SAFETY comment that can drift from the code. The range check also uses checked_add: the old `ptr + item_count` could wrap past the bound it was supposed to enforce (unreachable from Python, but the comment claimed a proof the arithmetic did not deliver). This turns the two gate tests from tautologies into real ones. They asserted only round-trip equality, which held on either branch, so nothing failed if the gate was weakened. Verified by deleting the containment check and re-running: the spoofed-.obj test now aborts on the slice bounds check, where the raw borrow passed green while reading out of bounds. Also from the panel: - get_buffer was the one header read of five without a length check; st_size is sampled before the read, so a file truncated in between made header[2] raise IndexError straight past this backend's OSError handling. - deserialize's BufferError comment had the mechanism backwards: bytes() did not reject a numpy float array, it coerced it to raw bytes that envelope validation then rejected. Same contract, different cause. - test_retrieve_memoryview_is_zero_copy claimed what its own docstring admits tracemalloc cannot see; renamed to _adds_no_python_copy. Two findings deliberately left out of scope, filed separately: set() ignores os.write's return value (the destructive mirror of the short-read bug this PR fixed), and get() never unlinks a truncated payload the way its sibling corruption branches do.
…r read (LAB-2682) set() discarded os.write's return value. Linux caps one write(2) at ~2 GiB and max_value_mb allows far more, so a large value was silently truncated, fsync'd and renamed into place as a successful set. The truncated ciphertext then failed AES-GCM as tamper-class, which encryption_fail_closed retains as evidence forever: a permanent false tamper alarm from a benign short write. - _write_fully(fd, data): loop over short writes via a non-copying memoryview, raise EIO on zero progress; twin of _read_fully. Backs set() and refresh_ttl()'s in-place expiry rewrite so there is one write idiom. - get(): a payload shorter than st_size - HEADER_SIZE (file shrank between fstat and read) is unlinked and returned as a miss, like the header corruption branches, instead of reaching the envelope's integrity check. - Same-length modifications are untouched: they still surface through the envelope and the fail-closed policy (tests pin both directions, including end-to-end on a real FileBackend under fail_closed=True).
…r; doc wording (LAB-2682) Panel findings applied: - get/exists/get_ttl/refresh_ttl released the flock on an fd they had already closed. flock(LOCK_UN) on a closed fd is EBADF and swallowed, but if the number was reused by another thread in between it unlocks a stranger's file. Guard on fd_closed; close already drops the lock. - The truncation-vs-tamper rationale was stated five times; the get() branch comment is now the single anchor, others point at it. - Cut test_refresh_ttl_loops_over_short_writes: an 8-byte in-place write to a regular file does not short; set()'s two tests cover _write_fully. - docs: short writes are resumed, not retried; expiry is not structural breakage; the length reference is st_size, the header has no length.
|
Warning Review limit reachedNext included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 106 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (3)
Comment |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
27Bslash6
dismissed
kodus-27b[bot]’s stale review
September 2, 2026 02:14
Finding rebutted in the review thread and resolved: the rule targets assert used for production input validation, every cited line is a pytest assertion in a test body. No src/ assert added by this diff.
27Bslash6
dismissed
kodus-27b[bot]’s stale review
September 2, 2026 02:18
The base branch was changed.
…rite # Conflicts: # src/cachekit/backends/file/backend.py
Contributor
Author
|
Resolved |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes LAB-2682. Stacked on #267 (base is its branch; retargets to
mainwhen it merges) so the write fix reuses its_read_fullytwin.Problem
FileBackend.set()discardedos.write's return value. Linux caps onewrite(2)at ~2 GiB whilemax_value_mballows far more, so a large value was silently truncated,fsync'd and renamed into place as a successful set. The truncated ciphertext then failed AES-GCM, which the envelope deliberately classifies as tamper-class, andencryption_fail_closedretained it as evidence forever: a permanent false tamper alarm from a benign short write. The end-to-end test in this PR reproduces exactly that log line against the pre-fix code ("L2 cache decrypt AUTHENTICATION failure ... failing closed") from a one-byte truncation.Change
_write_fully(fd, data): loops over short writes via a non-copying memoryview, raises EIO on zero progress; twin of_read_fully. Backsset()andrefresh_ttl()'s in-place expiry rewrite.get(): a payload shorter thanst_size - HEADER_SIZE(the file shrank between fstat and read) is unlinked and returned as a miss, matching the four header-corruption branches, instead of reaching the envelope's integrity check.FileBackendunderfail_closed=True.get/exists/get_ttl/refresh_ttlno longer release the flock on an fd they already closed (reused fd number hazard).docs/backends/file.mdgains a "Corruption vs. tampering" limitation note.Not covered, by design: a file truncated at rest (consistent
st_size) is indistinguishable from tampering at the backend, since the header carries no length field (format shared with cachekit-rs). That case stays tamper-class per the Rust binding's provenance rule. AC1 removes the only benign source of such files.Verification
pytest tests/unit -m "not slow": 1954 passed locally.