From 76acf921ba0652b21f9c66a167ce5e821eec3ce5 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 31 Aug 2026 05:07:52 +1000 Subject: [PATCH 1/6] review(panel): keep SerializationError contract for non-u8 buffers, pin 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 --- rust/src/python_bindings.rs | 38 +++++- src/cachekit/backends/file/backend.py | 22 ++-- .../serializers/standard_serializer.py | 7 +- .../test_byte_storage_error_injection.py | 118 ++++++++++++++++++ tests/performance/test_large_object_memory.py | 22 ++-- 5 files changed, 182 insertions(+), 25 deletions(-) diff --git a/rust/src/python_bindings.rs b/rust/src/python_bindings.rs index 0a612321..4c7dd005 100644 --- a/rust/src/python_bindings.rs +++ b/rust/src/python_bindings.rs @@ -52,13 +52,45 @@ impl PyByteStorage { /// Retrieve and validate stored bytes /// /// Args: - /// envelope_bytes: Serialized StorageEnvelope bytes + /// envelope_bytes: Serialized StorageEnvelope — any buffer-protocol object + /// (`bytes`, `memoryview`, `bytearray`), so callers holding a zero-copy + /// `memoryview` (SerializationWrapper.unwrap) never re-coerce to `bytes` (LAB-770) /// /// Returns: /// Tuple[bytes, str]: (original_data, format_identifier) - pub fn retrieve(&self, py: Python, envelope_bytes: &[u8]) -> PyResult<(Vec, String)> { + pub fn retrieve( + &self, + py: Python, + envelope_bytes: PyBuffer, + ) -> PyResult<(Vec, String)> { + let owned: Vec; + let data: &[u8] = if envelope_bytes.readonly() && envelope_bytes.is_c_contiguous() { + if envelope_bytes.item_count() == 0 { + // buf_ptr may be NULL for an empty buffer; from_raw_parts requires non-null. + &[] + } else { + // SAFETY: readonly + C-contiguous checked above, and `envelope_bytes` holds + // the Py_buffer view alive for the whole call (resizing an exported bytearray + // raises BufferError in the mutator, so the pointer cannot dangle). Residual: + // a thread mutating memory behind a readonly view over a still-mutable + // exporter during the detached read is a data race on this slice — UB — + // accepted per CPython hashlib's own GIL-release idiom; the slice is parsed + // once into an owned envelope in safe Rust, so a torn read fails checksum + // rather than corrupting memory. + unsafe { + std::slice::from_raw_parts( + envelope_bytes.buf_ptr() as *const u8, + envelope_bytes.item_count(), + ) + } + } + } else { + // Writable or non-contiguous exporter: copy to owned bytes (fail-safe fallback). + owned = envelope_bytes.to_vec(py)?; + &owned + }; // Detach from the GIL for decompression + checksum (see store()). - py.detach(|| self.inner.retrieve(envelope_bytes)) + py.detach(|| self.inner.retrieve(data)) .map_err(|e| PyValueError::new_err(format!("Retrieval failed: {}", e))) } diff --git a/src/cachekit/backends/file/backend.py b/src/cachekit/backends/file/backend.py index 14373c90..0e9a4971 100644 --- a/src/cachekit/backends/file/backend.py +++ b/src/cachekit/backends/file/backend.py @@ -167,11 +167,14 @@ def get(self, key: str) -> bytes | None: self._acquire_file_lock(fd, exclusive=False) try: - # Read entire file - file_data = os.read(fd, os.fstat(fd).st_size) + # Header-first read (LAB-770): reading the 14-byte header separately, + # then the payload in one os.read, avoids the full-payload + # file_data[HEADER_SIZE:] slice copy (~1x payload off the read peak). + st_size = os.fstat(fd).st_size + header = os.read(fd, HEADER_SIZE) # Validate header - if len(file_data) < HEADER_SIZE: + if len(header) < HEADER_SIZE: # Corrupted file, delete it os.close(fd) fd_closed = True @@ -179,10 +182,10 @@ def get(self, key: str) -> bytes | None: return None # Parse header - magic = file_data[0:2] - version = file_data[2] - # flags = struct.unpack(">H", file_data[4:6])[0] # uint16 BE (reserved for future) - expiry_timestamp = struct.unpack(">Q", file_data[6:14])[0] # uint64 BE + magic = header[0:2] + version = header[2] + # flags = struct.unpack(">H", header[4:6])[0] # uint16 BE (reserved for future) + expiry_timestamp = struct.unpack(">Q", header[6:14])[0] # uint64 BE # Validate magic and version if magic != MAGIC or version != FORMAT_VERSION: @@ -200,9 +203,8 @@ def get(self, key: str) -> bytes | None: self._safe_unlink(file_path) return None - # Extract payload - payload = file_data[HEADER_SIZE:] - return payload + # Read payload directly — exactly the bytes after the header + return os.read(fd, st_size - HEADER_SIZE) finally: self._release_file_lock(fd) diff --git a/src/cachekit/serializers/standard_serializer.py b/src/cachekit/serializers/standard_serializer.py index 649819ae..489f5ab4 100644 --- a/src/cachekit/serializers/standard_serializer.py +++ b/src/cachekit/serializers/standard_serializer.py @@ -328,7 +328,8 @@ def deserialize(self, data: bytes | memoryview, metadata: SerializationMetadata >>> result == {"test": 123} True """ - data = bytes(data) # coerce unwrap's zero-copy memoryview; no-op when already bytes (Rust retrieve needs bytes) + # No bytes() coercion: Rust retrieve accepts the buffer protocol (LAB-770), so + # unwrap's zero-copy memoryview flows through without a full-payload copy. try: if self.enable_integrity_checking: # Unwrap ByteStorage envelope (decompress + validate integrity) @@ -342,7 +343,9 @@ def deserialize(self, data: bytes | memoryview, metadata: SerializationMetadata except SerializationError: # Re-raise SerializationError (integrity check failure) without swallowing raise - except (msgpack.exceptions.UnpackException, ValueError, TypeError) as e: + except (msgpack.exceptions.UnpackException, ValueError, TypeError, BufferError) as e: + # BufferError: a non-u8 buffer exporter (e.g. numpy float array) rejected at the + # PyO3 boundary — pre-LAB-770 the bytes() coercion surfaced these as ValueError. raise SerializationError(f"Failed to deserialize MessagePack data: {e}") from e diff --git a/tests/critical/test_byte_storage_error_injection.py b/tests/critical/test_byte_storage_error_injection.py index 33b481d5..1da8b954 100644 --- a/tests/critical/test_byte_storage_error_injection.py +++ b/tests/critical/test_byte_storage_error_injection.py @@ -375,3 +375,121 @@ def test_final_envelope_size_security_check(self): error_msg = str(exc_info.value).lower() assert "exceeds maximum size" in error_msg or "too large" in error_msg + + +class TestByteStorageBufferProtocol: + """retrieve() accepts the buffer protocol (LAB-770) — no bytes() coercion needed. + + The zero-copy read path hands retrieve() a memoryview (SerializationWrapper.unwrap + slices one past the frame header); the PyO3 boundary must take it directly, plus + fall back to a copy for writable or non-contiguous exporters. + """ + + def test_retrieve_accepts_readonly_memoryview(self): + """Zero-copy path: memoryview over bytes round-trips identically to bytes.""" + from cachekit._rust_serializer import ByteStorage + + storage = ByteStorage("msgpack") + payload = b"buffer-protocol-roundtrip" * 1000 + envelope = storage.store(payload, None) + + data, fmt = storage.retrieve(memoryview(envelope)) + assert data == payload + assert fmt == "msgpack" + + def test_retrieve_accepts_offset_memoryview(self): + """The exact shape unwrap produces: a view sliced past a frame prefix.""" + from cachekit._rust_serializer import ByteStorage + + storage = ByteStorage("msgpack") + payload = b"offset-view" * 500 + envelope = storage.store(payload, None) + + framed = b"JUNKHDR" + envelope + data, _ = storage.retrieve(memoryview(framed)[7:]) + assert data == payload + + def test_retrieve_accepts_writable_buffer(self): + """Copy-fallback path: bytearray (writable exporter) still round-trips.""" + from cachekit._rust_serializer import ByteStorage + + storage = ByteStorage("msgpack") + payload = b"writable-exporter" * 500 + envelope = storage.store(payload, None) + + data, _ = storage.retrieve(bytearray(envelope)) + assert data == payload + data, _ = storage.retrieve(memoryview(bytearray(envelope))) + assert data == payload + + def test_retrieve_accepts_non_contiguous_view(self): + """Copy-fallback path: a strided view is copied, not misread.""" + from cachekit._rust_serializer import ByteStorage + + storage = ByteStorage("msgpack") + payload = b"strided-view" * 500 + envelope = storage.store(payload, None) + + interleaved = bytes(b for byte in envelope for b in (byte, 0xFF)) + data, _ = storage.retrieve(memoryview(interleaved)[::2]) + assert data == payload + + def test_retrieve_rejects_corrupt_memoryview(self): + """Error semantics are unchanged for buffer-protocol inputs.""" + from cachekit._rust_serializer import ByteStorage + + storage = ByteStorage("msgpack") + with pytest.raises(ValueError): + storage.retrieve(memoryview(b"not an envelope")) + + def test_retrieve_memoryview_is_zero_copy(self): + """The readonly path BORROWS — a bytes() coercion or to_vec creeping back fails here. + + tracemalloc sees only Python-heap allocations: retrieve's output bytes (~1x + payload for incompressible input). A revert to copy-the-envelope adds another + ~1x. This is the non-slow guard; the end-to-end bound lives in + tests/performance/test_large_object_memory.py. + """ + import gc + import os + import tracemalloc + + from cachekit._rust_serializer import ByteStorage + + storage = ByteStorage("msgpack") + payload = os.urandom(8 * 1024 * 1024) # incompressible: envelope ~= payload + envelope = storage.store(payload, None) + view = memoryview(envelope) + + gc.collect() + tracemalloc.start() + data, _ = storage.retrieve(view) + peak = tracemalloc.get_traced_memory()[1] + tracemalloc.stop() + + assert data == payload + assert peak / len(payload) < 1.5, ( + f"retrieve(memoryview) peak {peak / len(payload):.2f}x payload — the zero-copy borrow " + f"regressed to a full envelope copy (expected ~1x: just the output bytes)" + ) + + def test_standard_serializer_deserialize_memoryview(self): + """End-to-end: deserialize() takes unwrap's memoryview without re-coercing.""" + from cachekit.serializers.standard_serializer import StandardSerializer + + serializer = StandardSerializer() + obj = {"key": [1, 2, 3], "blob": b"x" * 4096} + data, _ = serializer.serialize(obj) + + assert serializer.deserialize(memoryview(data)) == obj + + def test_standard_serializer_deserialize_non_u8_buffer_raises_serialization_error(self): + """A non-u8 exporter (rejected as BufferError at the PyO3 boundary) keeps the + documented SerializationError contract — pre-LAB-770 the bytes() coercion + surfaced these as ValueError -> SerializationError.""" + np = pytest.importorskip("numpy") + from cachekit.serializers.base import SerializationError + from cachekit.serializers.standard_serializer import StandardSerializer + + with pytest.raises(SerializationError): + StandardSerializer().deserialize(np.zeros(4)) diff --git a/tests/performance/test_large_object_memory.py b/tests/performance/test_large_object_memory.py index a1b0df38..ceb22bf1 100644 --- a/tests/performance/test_large_object_memory.py +++ b/tests/performance/test_large_object_memory.py @@ -312,13 +312,14 @@ def test_file_backend_bytes_read_python_allocations_bounded(tmp_path: Path) -> N """END-TO-END read through FileBackend.get() (default serializer, no mmap). This is the path most cached functions take (anything that isn't a plaintext - Arrow DataFrame). Measured cost today: ~5x payload on the Python heap — - FileBackend.get's two full-payload copies (os.read + the file_data[14:] slice, - the exact copies #169 calls out), StandardSerializer.deserialize's ``bytes(data)`` - re-coercion of the envelope's zero-copy memoryview (Rust retrieve needs bytes), - the decompressed msgpack document, and the unpacked output. The bound pins that: - one MORE full-payload copy (~6x) fails. Tightening below 5x means fixing those - copies (separate ticket per #169 — this test is the measurement). + Arrow DataFrame). Measured cost today: ~3x payload on the Python heap — + FileBackend.get's single payload os.read (header read separately, LAB-770), + the decompressed msgpack document, and the unpacked output. The two avoidable + copies #169 called out are gone: the file_data[14:] slice (header-first read) + and deserialize's ``bytes(data)`` coercion (Rust retrieve takes the buffer + protocol, so unwrap's zero-copy memoryview flows through). The bound pins + that: one full-payload copy creeping back (~4x) fails. ~3x is the floor — + decompress + unpack are inherent (retrieve returns owned bytes by construction). """ payload = np.random.default_rng(0).bytes(50 * _MB) # incompressible: envelope ~= payload size backend, operation = _file_read_stack(tmp_path / "cache", "default") @@ -333,9 +334,10 @@ def test_file_backend_bytes_read_python_allocations_bounded(tmp_path: Path) -> N assert hit is not None, "end-to-end File read missed (errors read as miss — check logs)" assert hit[1] == payload - assert peak / len(payload) < 5.7, ( - f"File-backend bytes read peak {peak / len(payload):.2f}x payload — an additional full-payload " - f"read-side copy crept in (known cost ~5x: os.read + slice + bytes() coercion + decode + output)" + assert peak / len(payload) < 3.5, ( + f"File-backend bytes read peak {peak / len(payload):.2f}x payload — a full-payload read-side " + f"copy crept back in (known cost ~3x: payload os.read + decode + output; LAB-770 removed " + f"the header slice and the bytes() coercion)" ) From a8b7960dbb81e09a57f3def1bb5d902eee568e10 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Tue, 1 Sep 2026 22:27:32 +1000 Subject: [PATCH 2/6] fix(ffi): borrow only buffers provably backed by immutable bytes; bump pip for PYSEC-2026-3721 (LAB-770) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: 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. --- rust/src/python_bindings.rs | 65 ++++++++++++------- .../test_byte_storage_error_injection.py | 51 +++++++++++++-- uv.lock | 6 +- 3 files changed, 92 insertions(+), 30 deletions(-) diff --git a/rust/src/python_bindings.rs b/rust/src/python_bindings.rs index 4c7dd005..699b86ab 100644 --- a/rust/src/python_bindings.rs +++ b/rust/src/python_bindings.rs @@ -61,33 +61,52 @@ impl PyByteStorage { pub fn retrieve( &self, py: Python, - envelope_bytes: PyBuffer, + envelope_bytes: &Bound<'_, PyAny>, ) -> PyResult<(Vec, String)> { let owned: Vec; - let data: &[u8] = if envelope_bytes.readonly() && envelope_bytes.is_c_contiguous() { - if envelope_bytes.item_count() == 0 { - // buf_ptr may be NULL for an empty buffer; from_raw_parts requires non-null. - &[] + let buf: PyBuffer; + let base_bytes: Option>; + let data: &[u8] = if let Ok(b) = envelope_bytes.cast::() { + // `bytes` is immutable and kept alive by the Bound for the whole call: + // a safe zero-copy borrow with no data-race exposure. + b.as_bytes() + } else { + buf = PyBuffer::get(envelope_bytes)?; + // Zero-copy is only sound when the BACKING STORAGE is provably immutable — + // readonly() describes the view, not the exporter (memoryview(bytearray) + // .toreadonly() passes it while another thread can still mutate the bytearray + // during the detached read below: a data race, UB). The proof is a pointer- + // range check, not attribute trust: `.obj` naming a bytes object is spoofable + // by a PEP 688 __buffer__ exporter with a decoy attribute, so we borrow only + // when the buffer memory PROVABLY lies inside that immutable bytes object — + // true for the memoryview-over-bytes shape SerializationWrapper.unwrap + // produces, impossible to fake with memory the bytes doesn't own. `base_bytes` + // is held at function scope so the backing bytes outlives the detached read + // even if the exporter drops its own references mid-call. + base_bytes = envelope_bytes + .getattr("obj") + .ok() + .and_then(|base| base.cast_into::().ok()); + let provably_immutable = base_bytes.as_ref().is_some_and(|base| { + let start = base.as_bytes().as_ptr() as usize; + let ptr = buf.buf_ptr() as usize; + buf.readonly() + && buf.is_c_contiguous() + && buf.item_count() > 0 + && ptr >= start + && ptr + buf.item_count() <= start + base.as_bytes().len() + }); + if provably_immutable { + // SAFETY: non-null (len > 0), C-contiguous, element type validated u8 by + // PyBuffer extraction, and the range check above proves the memory sits + // inside an immutable `bytes` object kept alive by `base_bytes`. + unsafe { std::slice::from_raw_parts(buf.buf_ptr() as *const u8, buf.item_count()) } } else { - // SAFETY: readonly + C-contiguous checked above, and `envelope_bytes` holds - // the Py_buffer view alive for the whole call (resizing an exported bytearray - // raises BufferError in the mutator, so the pointer cannot dangle). Residual: - // a thread mutating memory behind a readonly view over a still-mutable - // exporter during the detached read is a data race on this slice — UB — - // accepted per CPython hashlib's own GIL-release idiom; the slice is parsed - // once into an owned envelope in safe Rust, so a torn read fails checksum - // rather than corrupting memory. - unsafe { - std::slice::from_raw_parts( - envelope_bytes.buf_ptr() as *const u8, - envelope_bytes.item_count(), - ) - } + // Mutable, non-bytes-backed, non-contiguous, or empty exporter: copy to + // owned bytes (fail-safe fallback; empty also sidesteps NULL buf_ptr). + owned = buf.to_vec(py)?; + &owned } - } else { - // Writable or non-contiguous exporter: copy to owned bytes (fail-safe fallback). - owned = envelope_bytes.to_vec(py)?; - &owned }; // Detach from the GIL for decompression + checksum (see store()). py.detach(|| self.inner.retrieve(data)) diff --git a/tests/critical/test_byte_storage_error_injection.py b/tests/critical/test_byte_storage_error_injection.py index 1da8b954..14d21d6d 100644 --- a/tests/critical/test_byte_storage_error_injection.py +++ b/tests/critical/test_byte_storage_error_injection.py @@ -422,6 +422,47 @@ def test_retrieve_accepts_writable_buffer(self): data, _ = storage.retrieve(memoryview(bytearray(envelope))) assert data == payload + def test_retrieve_readonly_view_over_mutable_exporter_round_trips(self): + """A readonly VIEW whose backing storage is still mutable must not be borrowed + across the GIL release (data race). It takes the copy path — readonly() alone + is not the zero-copy gate; the exporter must be immutable bytes.""" + from cachekit._rust_serializer import ByteStorage + + storage = ByteStorage("msgpack") + payload = b"readonly-view-mutable-backing" * 500 + envelope = storage.store(payload, None) + + ro_view = memoryview(bytearray(envelope)).toreadonly() + assert ro_view.readonly + data, _ = storage.retrieve(ro_view) + assert data == payload + + def test_retrieve_spoofed_obj_attribute_round_trips(self): + """A PEP 688 exporter with a decoy bytes `.obj` attribute must not trick the + zero-copy gate: the pointer-range proof sees its memory is NOT inside the + decoy bytes and takes the copy path. Round-trip stays correct.""" + import sys + + if sys.version_info < (3, 12): + pytest.skip("__buffer__ protocol requires Python 3.12+") + from cachekit._rust_serializer import ByteStorage + + storage = ByteStorage("msgpack") + payload = b"spoofed-exporter" * 500 + envelope = storage.store(payload, None) + + class Spoof: + obj = b"decoy-bytes-not-the-buffer" + + def __init__(self, backing: bytearray) -> None: + self.backing = backing + + def __buffer__(self, flags: int) -> memoryview: + return memoryview(self.backing).toreadonly() + + data, _ = storage.retrieve(Spoof(bytearray(envelope))) + assert data == payload + def test_retrieve_accepts_non_contiguous_view(self): """Copy-fallback path: a strided view is copied, not misread.""" from cachekit._rust_serializer import ByteStorage @@ -443,12 +484,14 @@ def test_retrieve_rejects_corrupt_memoryview(self): storage.retrieve(memoryview(b"not an envelope")) def test_retrieve_memoryview_is_zero_copy(self): - """The readonly path BORROWS — a bytes() coercion or to_vec creeping back fails here. + """No PYTHON-side full-envelope copy on the memoryview path (LAB-770). tracemalloc sees only Python-heap allocations: retrieve's output bytes (~1x - payload for incompressible input). A revert to copy-the-envelope adds another - ~1x. This is the non-slow guard; the end-to-end bound lives in - tests/performance/test_large_object_memory.py. + payload for incompressible input). A bytes(data)-style coercion creeping back + in front of retrieve adds another ~1x and fails here. Known blind spot: a + Rust-side copy (to_vec) is invisible to tracemalloc (measured: borrow and + copy paths both read 1.00x), so the borrow itself is pinned by review of + retrieve() in rust/src/python_bindings.rs, not by this suite. """ import gc import os diff --git a/uv.lock b/uv.lock index 4f9df255..49b3a464 100644 --- a/uv.lock +++ b/uv.lock @@ -1283,11 +1283,11 @@ wheels = [ [[package]] name = "pip" -version = "26.1.2" +version = "26.2.1" source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/01/91/47e7d486260f618783899587af63ccf7980fb60245c3e63dd4571c6b57ad/pip-26.1.2.tar.gz", hash = "sha256:f49cd134c61cf2fd75e0ce2676db03e4054504a5a4986d00f8299ae632dc4605", size = 1840799, upload-time = "2026-05-31T17:33:58.56Z" } +sdist = { url = "https://files.pythonhosted.org/packages/ae/15/4500e320e6b101ec3b719ae85b697d9940b6cda672bc555bd6016fc60c6f/pip-26.2.1.tar.gz", hash = "sha256:f6ad667e89a1fe78046c8f13232b247200f5258d7828f3f7883d660878e0813f", size = 1848877, upload-time = "2026-08-04T22:51:14.148Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/5d/95/6b5cb3461ea5673ba0995989746db58eb18b91b54dbf331e72f569540946/pip-26.1.2-py3-none-any.whl", hash = "sha256:382ff9f685ee3bc25864f820aa50505825f10f5458ffff07e30a6d96e5715cab", size = 1813144, upload-time = "2026-05-31T17:33:56.772Z" }, + { url = "https://files.pythonhosted.org/packages/f3/6e/1736e5b4ae2b778ef2f81c47d797de9f891d4d8acb047a24ca37a60294dd/pip-26.2.1-py3-none-any.whl", hash = "sha256:71138adf1f4ca900cdb7d289c21b7494329f2332b6d85f0e1c42108c0384ed3e", size = 1816632, upload-time = "2026-08-04T22:51:12.472Z" }, ] [[package]] From cf5e81a2780e90a98194ce89fdb5a6bcbc8f709a Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Wed, 2 Sep 2026 09:44:33 +1000 Subject: [PATCH 3/6] fix(file): loop os.read over short reads on every header/payload read (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. --- src/cachekit/backends/file/backend.py | 34 +++++++++++++++++++----- tests/unit/backends/test_file_backend.py | 21 +++++++++++++++ 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/cachekit/backends/file/backend.py b/src/cachekit/backends/file/backend.py index 0e9a4971..51952d12 100644 --- a/src/cachekit/backends/file/backend.py +++ b/src/cachekit/backends/file/backend.py @@ -56,6 +56,26 @@ MMAP_MAX_BYTES: int = 512 * 1024 * 1024 # 512 MB +def _read_fully(fd: int, n: int) -> bytes: + """Read ``n`` bytes from ``fd``, looping over short reads; stops early only at EOF. + + A single read(2) may return fewer bytes than asked (POSIX permits it; Linux caps one call at + ~2 GiB), so a lone ``os.read`` can silently truncate a large payload into a spurious integrity + failure. A file shorter than ``n`` still yields a short result, so callers' header/integrity + validation sees truncation exactly as before. CPython's single-chunk ``join`` returns the chunk + itself, so the one-read case (every payload the kernel serves whole) adds no copy and the + LAB-770 read-peak bound holds; only a genuinely short-read payload pays a join copy. + """ + chunks: list[bytes] = [] + while n > 0: + chunk = os.read(fd, n) + if not chunk: + break + chunks.append(chunk) + n -= len(chunk) + return b"".join(chunks) + + class _MmapHandle: """Owns a read-only mmap of a cache file plus a memoryview of its payload (past the 14-byte header). Zero-copy: the view aliases mapped pages, never a heap copy. @@ -168,10 +188,10 @@ def get(self, key: str) -> bytes | None: try: # Header-first read (LAB-770): reading the 14-byte header separately, - # then the payload in one os.read, avoids the full-payload + # then the payload via _read_fully, avoids the full-payload # file_data[HEADER_SIZE:] slice copy (~1x payload off the read peak). st_size = os.fstat(fd).st_size - header = os.read(fd, HEADER_SIZE) + header = _read_fully(fd, HEADER_SIZE) # Validate header if len(header) < HEADER_SIZE: @@ -204,7 +224,7 @@ def get(self, key: str) -> bytes | None: return None # Read payload directly — exactly the bytes after the header - return os.read(fd, st_size - HEADER_SIZE) + return _read_fully(fd, st_size - HEADER_SIZE) finally: self._release_file_lock(fd) @@ -274,7 +294,7 @@ def get_buffer(self, key: str) -> _MmapHandle | None: if st_size < HEADER_SIZE: self._safe_unlink(file_path) return None - header = os.read(fd, HEADER_SIZE) + header = _read_fully(fd, HEADER_SIZE) if header[0:2] != MAGIC or header[2] != FORMAT_VERSION: self._safe_unlink(file_path) return None @@ -539,7 +559,7 @@ def exists(self, key: str) -> bool: try: # Read header only - header_data = os.read(fd, HEADER_SIZE) + header_data = _read_fully(fd, HEADER_SIZE) if len(header_data) < HEADER_SIZE: # Corrupted, clean up @@ -666,7 +686,7 @@ async def get_ttl(self, key: str) -> int | None: try: self._acquire_file_lock(fd, exclusive=False) try: - header = os.read(fd, HEADER_SIZE) + header = _read_fully(fd, HEADER_SIZE) if len(header) < HEADER_SIZE or header[0:2] != MAGIC or header[2] != FORMAT_VERSION: os.close(fd) fd_closed = True @@ -725,7 +745,7 @@ async def refresh_ttl(self, key: str, ttl: int) -> bool: try: self._acquire_file_lock(fd, exclusive=True) try: - header = os.read(fd, HEADER_SIZE) + header = _read_fully(fd, HEADER_SIZE) if len(header) < HEADER_SIZE or header[0:2] != MAGIC or header[2] != FORMAT_VERSION: os.close(fd) fd_closed = True diff --git a/tests/unit/backends/test_file_backend.py b/tests/unit/backends/test_file_backend.py index 7d2c7efe..3005d3e6 100644 --- a/tests/unit/backends/test_file_backend.py +++ b/tests/unit/backends/test_file_backend.py @@ -807,6 +807,27 @@ def test_get_corrupted_truncated_file(self, backend: FileBackend, config: FileBa assert result is None assert not os.path.exists(file_path) + def test_get_survives_short_reads(self, backend: FileBackend, monkeypatch: pytest.MonkeyPatch) -> None: + """read(2) may return fewer bytes than asked (POSIX; Linux caps one call at ~2 GiB). + + Force every os.read to hand back at most 5 bytes, so the 14-byte header and the payload + each need several calls. get() must return the full value, not a truncated one that + would fail downstream integrity as spurious corruption. + """ + real_read = os.read + calls: list[int] = [] + + def short_read(fd: int, n: int) -> bytes: + calls.append(n) + return real_read(fd, min(n, 5)) + + payload = bytes(range(256)) * 8 + backend.set("short_read_key", payload) + monkeypatch.setattr(os, "read", short_read) + + assert backend.get("short_read_key") == payload + assert len(calls) > 2, "short reads were not exercised" + def test_get_expired_ttl_deletes_file(self, backend: FileBackend, config: FileBackendConfig) -> None: """Test get deletes expired files.""" key = "expired_key" From 7ff57594def091a06c6525670e6f8c020663e259 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Wed, 2 Sep 2026 10:44:03 +1000 Subject: [PATCH 4/6] review(panel): replace the raw borrow with a safe slice; guard get_buffer's short header (LAB-770) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- rust/src/python_bindings.rs | 73 +++++++++++-------- src/cachekit/backends/file/backend.py | 5 +- .../serializers/standard_serializer.py | 3 +- .../test_byte_storage_error_injection.py | 25 +++++-- 4 files changed, 68 insertions(+), 38 deletions(-) diff --git a/rust/src/python_bindings.rs b/rust/src/python_bindings.rs index 699b86ab..d89cf553 100644 --- a/rust/src/python_bindings.rs +++ b/rust/src/python_bindings.rs @@ -21,6 +21,29 @@ impl Default for PyByteStorage { } } +/// Offset into `base` at which `buf`'s memory starts, iff `buf` is a plain, immutable, +/// C-contiguous window onto that `bytes` object — the shape `SerializationWrapper.unwrap` +/// produces. `Some(off)` means `&base.as_bytes()[off..off + buf.item_count()]` is exactly +/// the buffer's bytes, so the caller can borrow it with a safe slice instead of a raw one. +/// +/// Every conjunct is load-bearing: `readonly` + a `bytes` base rule out a mutable exporter +/// racing the GIL-released read; `is_c_contiguous` rules out a strided view whose logical +/// bytes are not the contiguous span; the range check rules out a spoofed `.obj` naming a +/// decoy `bytes` the memory does not belong to; and `checked_add` keeps that range check +/// honest against a forged `Py_buffer` length rather than wrapping past it. +fn borrowable_offset(buf: &PyBuffer, base: &Bound<'_, PyBytes>) -> Option { + let bytes = base.as_bytes(); + let start = bytes.as_ptr() as usize; + let ptr = buf.buf_ptr() as usize; + let end = ptr.checked_add(buf.item_count())?; + (buf.readonly() + && buf.is_c_contiguous() + && buf.item_count() > 0 + && ptr >= start + && end <= start + bytes.len()) + .then(|| ptr - start) +} + #[pymethods] impl PyByteStorage { #[new] @@ -68,44 +91,34 @@ impl PyByteStorage { let base_bytes: Option>; let data: &[u8] = if let Ok(b) = envelope_bytes.cast::() { // `bytes` is immutable and kept alive by the Bound for the whole call: - // a safe zero-copy borrow with no data-race exposure. + // a zero-copy borrow with no data-race exposure. b.as_bytes() } else { buf = PyBuffer::get(envelope_bytes)?; - // Zero-copy is only sound when the BACKING STORAGE is provably immutable — - // readonly() describes the view, not the exporter (memoryview(bytearray) - // .toreadonly() passes it while another thread can still mutate the bytearray - // during the detached read below: a data race, UB). The proof is a pointer- - // range check, not attribute trust: `.obj` naming a bytes object is spoofable - // by a PEP 688 __buffer__ exporter with a decoy attribute, so we borrow only - // when the buffer memory PROVABLY lies inside that immutable bytes object — - // true for the memoryview-over-bytes shape SerializationWrapper.unwrap - // produces, impossible to fake with memory the bytes doesn't own. `base_bytes` - // is held at function scope so the backing bytes outlives the detached read - // even if the exporter drops its own references mid-call. + // Borrowing across the GIL release below is only sound when the BACKING + // STORAGE is immutable — readonly() describes the view, not the exporter + // (memoryview(bytearray).toreadonly() passes it while another thread can + // still mutate the bytearray). Attribute trust is not enough either: a + // PEP 688 __buffer__ exporter can name a decoy `bytes` in `.obj`. So the + // gate is a containment proof (borrowable_offset), and its payoff is that + // the borrow becomes expressible as an ORDINARY SLICE of that `bytes` — + // bounds-checked by Rust, no `unsafe`, nothing for a stale comment to + // misstate. Anything unproven falls back to a copy. base_bytes = envelope_bytes .getattr("obj") .ok() .and_then(|base| base.cast_into::().ok()); - let provably_immutable = base_bytes.as_ref().is_some_and(|base| { - let start = base.as_bytes().as_ptr() as usize; - let ptr = buf.buf_ptr() as usize; - buf.readonly() - && buf.is_c_contiguous() - && buf.item_count() > 0 - && ptr >= start - && ptr + buf.item_count() <= start + base.as_bytes().len() + let borrowed = base_bytes.as_ref().and_then(|base| { + borrowable_offset(&buf, base) + .map(|off| &base.as_bytes()[off..off + buf.item_count()]) }); - if provably_immutable { - // SAFETY: non-null (len > 0), C-contiguous, element type validated u8 by - // PyBuffer extraction, and the range check above proves the memory sits - // inside an immutable `bytes` object kept alive by `base_bytes`. - unsafe { std::slice::from_raw_parts(buf.buf_ptr() as *const u8, buf.item_count()) } - } else { - // Mutable, non-bytes-backed, non-contiguous, or empty exporter: copy to - // owned bytes (fail-safe fallback; empty also sidesteps NULL buf_ptr). - owned = buf.to_vec(py)?; - &owned + match borrowed { + Some(slice) => slice, + None => { + // Mutable, non-bytes-backed, non-contiguous, or empty exporter. + owned = buf.to_vec(py)?; + &owned + } } }; // Detach from the GIL for decompression + checksum (see store()). diff --git a/src/cachekit/backends/file/backend.py b/src/cachekit/backends/file/backend.py index 51952d12..dead0cde 100644 --- a/src/cachekit/backends/file/backend.py +++ b/src/cachekit/backends/file/backend.py @@ -295,7 +295,10 @@ def get_buffer(self, key: str) -> _MmapHandle | None: self._safe_unlink(file_path) return None header = _read_fully(fd, HEADER_SIZE) - if header[0:2] != MAGIC or header[2] != FORMAT_VERSION: + # Re-check the length: st_size above was sampled before the read, so a + # file truncated in between yields a short header here and header[2] + # would raise IndexError straight past this backend's OSError handling. + if len(header) < HEADER_SIZE or header[0:2] != MAGIC or header[2] != FORMAT_VERSION: self._safe_unlink(file_path) return None expiry_timestamp = struct.unpack(">Q", header[6:14])[0] diff --git a/src/cachekit/serializers/standard_serializer.py b/src/cachekit/serializers/standard_serializer.py index 489f5ab4..404c7797 100644 --- a/src/cachekit/serializers/standard_serializer.py +++ b/src/cachekit/serializers/standard_serializer.py @@ -345,7 +345,8 @@ def deserialize(self, data: bytes | memoryview, metadata: SerializationMetadata raise except (msgpack.exceptions.UnpackException, ValueError, TypeError, BufferError) as e: # BufferError: a non-u8 buffer exporter (e.g. numpy float array) rejected at the - # PyO3 boundary — pre-LAB-770 the bytes() coercion surfaced these as ValueError. + # PyO3 boundary. Pre-LAB-770 bytes() coerced these to raw bytes and envelope + # validation rejected the garbage as ValueError; same contract, new cause. raise SerializationError(f"Failed to deserialize MessagePack data: {e}") from e diff --git a/tests/critical/test_byte_storage_error_injection.py b/tests/critical/test_byte_storage_error_injection.py index 14d21d6d..66037a41 100644 --- a/tests/critical/test_byte_storage_error_injection.py +++ b/tests/critical/test_byte_storage_error_injection.py @@ -425,7 +425,12 @@ def test_retrieve_accepts_writable_buffer(self): def test_retrieve_readonly_view_over_mutable_exporter_round_trips(self): """A readonly VIEW whose backing storage is still mutable must not be borrowed across the GIL release (data race). It takes the copy path — readonly() alone - is not the zero-copy gate; the exporter must be immutable bytes.""" + is not the zero-copy gate; the exporter must be immutable bytes. + + Round-trip equality is all this one can assert: the bytearray base fails the + `.obj`-is-`bytes` cast before borrowable_offset is consulted, so a weakened + range check would not show up here. The spoof test below is what pins that. + """ from cachekit._rust_serializer import ByteStorage storage = ByteStorage("msgpack") @@ -439,8 +444,16 @@ def test_retrieve_readonly_view_over_mutable_exporter_round_trips(self): def test_retrieve_spoofed_obj_attribute_round_trips(self): """A PEP 688 exporter with a decoy bytes `.obj` attribute must not trick the - zero-copy gate: the pointer-range proof sees its memory is NOT inside the - decoy bytes and takes the copy path. Round-trip stays correct.""" + zero-copy gate: the containment proof sees its memory is NOT inside the decoy + bytes and takes the copy path. Round-trip stays correct. + + This test IS load-bearing for that proof. Because the borrow is a plain slice + of the base `bytes`, dropping the range check does not silently misread here: + the offset lands outside the decoy and the slice bounds check aborts (verified + by deleting the check and re-running — the old raw-pointer borrow passed green + under the same sabotage, reading out of bounds). The strided test below pins + is_c_contiguous() the same way. + """ import sys if sys.version_info < (3, 12): @@ -483,7 +496,7 @@ def test_retrieve_rejects_corrupt_memoryview(self): with pytest.raises(ValueError): storage.retrieve(memoryview(b"not an envelope")) - def test_retrieve_memoryview_is_zero_copy(self): + def test_retrieve_memoryview_adds_no_python_copy(self): """No PYTHON-side full-envelope copy on the memoryview path (LAB-770). tracemalloc sees only Python-heap allocations: retrieve's output bytes (~1x @@ -528,8 +541,8 @@ def test_standard_serializer_deserialize_memoryview(self): def test_standard_serializer_deserialize_non_u8_buffer_raises_serialization_error(self): """A non-u8 exporter (rejected as BufferError at the PyO3 boundary) keeps the - documented SerializationError contract — pre-LAB-770 the bytes() coercion - surfaced these as ValueError -> SerializationError.""" + documented SerializationError contract — pre-LAB-770 bytes() coerced these to + raw bytes and envelope validation rejected them as ValueError.""" np = pytest.importorskip("numpy") from cachekit.serializers.base import SerializationError from cachekit.serializers.standard_serializer import StandardSerializer From d49848f8ca8f6dc82ac1607d9ef222d740cb9f87 Mon Sep 17 00:00:00 2001 From: Winston Date: Wed, 2 Sep 2026 10:58:42 +1000 Subject: [PATCH 5/6] fix(file): write every byte or fail; evict a payload that shrank under 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). --- docs/backends/file.md | 2 + src/cachekit/backends/file/backend.py | 40 +++++- tests/unit/backends/test_file_backend.py | 170 +++++++++++++++++++++++ 3 files changed, 207 insertions(+), 5 deletions(-) diff --git a/docs/backends/file.md b/docs/backends/file.md index 549ff627..f9ae2c94 100644 --- a/docs/backends/file.md +++ b/docs/backends/file.md @@ -141,6 +141,8 @@ the cached payload is left untouched. 5. **Disk space**: FileBackend will evict least-recently-used entries when reaching 90% capacity. Ensure sufficient disk space beyond max_size_mb for temporary writes. +6. **Corruption vs. tampering**: `set()` writes every byte or raises `BackendError` (short `write(2)` calls are retried, never silently truncated into a "successful" file). On read, a structurally broken file — short header, bad magic or version, expired, or a payload shorter than the size the file reported — is deleted and treated as a miss. Payload *content* is not checked here: a same-length modification is served as-is, and the serialization envelope (xxHash3 checksum, or the AES-256-GCM tag for encrypted values) decides whether it is corruption or tampering under your `encryption_fail_closed` policy. + ## Performance Characteristics ``` diff --git a/src/cachekit/backends/file/backend.py b/src/cachekit/backends/file/backend.py index dead0cde..700ba3c6 100644 --- a/src/cachekit/backends/file/backend.py +++ b/src/cachekit/backends/file/backend.py @@ -76,6 +76,24 @@ def _read_fully(fd: int, n: int) -> bytes: return b"".join(chunks) +def _write_fully(fd: int, data: bytes) -> None: + """Write all of ``data`` to ``fd``, looping over short writes; the write twin of ``_read_fully``. + + A single write(2) may store fewer bytes than asked (POSIX permits it; Linux caps one call at + ~2 GiB) and ``os.write`` only reports the count, so a lone call can silently truncate a large + value that is then fsync'd and renamed into place as a "successful" set. A truncated + encrypted payload later fails AES-GCM as tamper-class, which fail-closed retains as evidence + forever, so the write side must land every byte or fail. The memoryview slice advances + without copying. A write that makes no progress raises rather than spinning. + """ + view = memoryview(data) + while view: + written = os.write(fd, view) + if written == 0: + raise OSError(errno.EIO, f"write made no progress with {view.nbytes} bytes remaining") + view = view[written:] + + class _MmapHandle: """Owns a read-only mmap of a cache file plus a memoryview of its payload (past the 14-byte header). Zero-copy: the view aliases mapped pages, never a heap copy. @@ -223,8 +241,20 @@ def get(self, key: str) -> bytes | None: self._safe_unlink(file_path) return None - # Read payload directly — exactly the bytes after the header - return _read_fully(fd, st_size - HEADER_SIZE) + # Read payload directly — exactly the bytes after the header. + payload_size = st_size - HEADER_SIZE + payload = _read_fully(fd, payload_size) + if len(payload) < payload_size: + # The file shrank between fstat and read: truncated underneath us. + # Treat it as corruption like the header branches above (unlink, + # miss) instead of handing a short payload to the envelope, whose + # AES-GCM check would classify it as tampering and, under + # encryption_fail_closed, retain the entry as evidence forever. + os.close(fd) + fd_closed = True + self._safe_unlink(file_path) + return None + return payload finally: self._release_file_lock(fd) @@ -378,8 +408,8 @@ def set(self, key: str, value: bytes, ttl: int | None = None) -> None: self._acquire_file_lock(fd, exclusive=True) try: - # Write all data - os.write(fd, file_data) + # Write all data (loops over short writes; never a silent partial file) + _write_fully(fd, file_data) # fsync to ensure data is on disk os.fsync(fd) @@ -768,7 +798,7 @@ async def refresh_ttl(self, key: str, ttl: int) -> bool: # power loss yields a wrong expiry, never a corrupt payload (magic/version # are untouched), so the entry just expires early/late. No rewrite-rename. os.lseek(fd, 6, os.SEEK_SET) - os.write(fd, struct.pack(">Q", new_expiry)) + _write_fully(fd, struct.pack(">Q", new_expiry)) os.fsync(fd) return True finally: diff --git a/tests/unit/backends/test_file_backend.py b/tests/unit/backends/test_file_backend.py index 3005d3e6..51cc77e9 100644 --- a/tests/unit/backends/test_file_backend.py +++ b/tests/unit/backends/test_file_backend.py @@ -1822,3 +1822,173 @@ def boom(*_a, **_k): monkeypatch.setattr(backend_mod.mmap, "mmap", boom) with pytest.raises(BackendError): backend.get_buffer("k") + + +@pytest.mark.unit +class TestShortIO: + """POSIX short I/O on the write path, and a file shrinking under a read (LAB-2682). + + A single write(2) may store fewer bytes than asked (Linux caps one call at ~2 GiB), and a + file can shrink between fstat and read. Neither is tampering, and neither may reach the + envelope's integrity check: for encrypted entries that check deliberately classifies any + truncated ciphertext as tamper-class (see rust/src/python_bindings.rs), and under + ``encryption_fail_closed`` it retains the entry as evidence forever. The backend is the only + layer that can tell truncation apart from tampering, because only it knows how many bytes it + meant to write (set) or how many the file claimed to hold (get). + """ + + def test_set_loops_over_short_writes(self, backend: FileBackend, monkeypatch: pytest.MonkeyPatch) -> None: + """Force every os.write to accept at most 5 bytes: set() must still land the whole value.""" + real_write = os.write + calls: list[int] = [] + + def short_write(fd: int, data: bytes) -> int: + calls.append(len(data)) + return real_write(fd, memoryview(data)[:5]) + + payload = bytes(range(256)) * 8 + monkeypatch.setattr(os, "write", short_write) + backend.set("short_write_key", payload) + monkeypatch.undo() + + assert len(calls) > 2, "short writes were not exercised" + assert os.path.getsize(backend._key_to_path("short_write_key")) == HEADER_SIZE + len(payload) + assert backend.get("short_write_key") == payload + + def test_set_zero_progress_write_raises_and_leaves_nothing_behind( + self, backend: FileBackend, config: FileBackendConfig, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A write that makes no progress must fail loudly, not spin or report success.""" + from cachekit.backends.errors import BackendError + + monkeypatch.setattr(os, "write", lambda fd, data: 0) + with pytest.raises(BackendError): + backend.set("stuck_key", b"payload") + monkeypatch.undo() + + assert backend.get("stuck_key") is None + assert not list(Path(config.cache_dir).rglob("*.tmp.*")), "temp file left behind" + + async def test_refresh_ttl_loops_over_short_writes(self, backend: FileBackend, monkeypatch: pytest.MonkeyPatch) -> None: + """The in-place 8-byte expiry rewrite shares the write idiom with set().""" + real_write = os.write + backend.set("ttl_key", b"v", ttl=10) + monkeypatch.setattr(os, "write", lambda fd, data: real_write(fd, memoryview(data)[:1])) + assert await backend.refresh_ttl("ttl_key", 1000) is True + monkeypatch.undo() + + remaining = await backend.get_ttl("ttl_key") + assert remaining is not None and remaining > 900 + + def test_get_file_shrunk_under_read_is_corruption_not_a_hit( + self, backend: FileBackend, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Payload shorter than st_size - HEADER_SIZE: unlink and miss, like the sibling corruption branches.""" + key = "shrunk_key" + payload = bytes(range(256)) * 8 + backend.set(key, payload) + file_path = backend._key_to_path(key) + real_read = os.read + + def truncating_read(fd: int, n: int) -> bytes: + if n > HEADER_SIZE: # the payload read: shrink the file after fstat, before read + os.truncate(file_path, HEADER_SIZE + 3) + return real_read(fd, n) + + monkeypatch.setattr(os, "read", truncating_read) + assert backend.get(key) is None + assert not os.path.exists(file_path) + + def test_get_modified_payload_is_served_not_evicted(self, backend: FileBackend) -> None: + """AC4 at the backend layer: a same-length modification is not the backend's call. + + Content integrity belongs to the envelope (xxHash3 checksum / AES-GCM tag), so the + bytes are handed back unchanged and the file stays for the fail policy to judge. + """ + key = "tampered_key" + payload = bytes(range(256)) * 8 + backend.set(key, payload) + file_path = backend._key_to_path(key) + + with open(file_path, "r+b") as f: + f.seek(HEADER_SIZE + 10) + f.write(b"\xff") + tampered = bytearray(payload) + tampered[10] = 0xFF + + assert backend.get(key) == bytes(tampered) + assert os.path.exists(file_path) + + +@pytest.mark.unit +class TestShortIOFailClosed: + """End-to-end on a real FileBackend: truncation and tampering stay distinguishable under fail-closed (AC4).""" + + _HEX_KEY = "a" * 64 + + def _decorated(self, config: FileBackendConfig) -> tuple[Any, list[int]]: + from cachekit import cache + + backend = FileBackend(config) + calls: list[int] = [] + + @cache( + backend=backend, + ttl=300, + l1_enabled=False, + encryption=True, + single_tenant_mode=True, + master_key=self._HEX_KEY, + fail_closed=True, + ) + def get_value(x: int) -> dict: + calls.append(x) + return {"result": x} + + return get_value, calls + + @staticmethod + def _only_cache_file(config: FileBackendConfig) -> str: + files = [str(p) for p in Path(config.cache_dir).rglob("*") if p.is_file()] + assert len(files) == 1, files + return files[0] + + def test_tampered_entry_raises_and_is_retained(self, config: FileBackendConfig) -> None: + from cachekit.serializers.encryption_wrapper import DecryptionAuthenticationError + + get_value, _ = self._decorated(config) + assert get_value(1) == {"result": 1} + file_path = self._only_cache_file(config) + + with open(file_path, "r+b") as f: # flip the last ciphertext byte, length unchanged + f.seek(-1, os.SEEK_END) + last = f.read(1)[0] + f.seek(-1, os.SEEK_END) + f.write(bytes([last ^ 0xFF])) + + with pytest.raises(DecryptionAuthenticationError): + get_value(1) + assert os.path.exists(file_path), "tamper evidence must be retained" + + def test_entry_shrunk_under_read_is_a_miss_not_a_tamper_alarm( + self, config: FileBackendConfig, monkeypatch: pytest.MonkeyPatch + ) -> None: + get_value, calls = self._decorated(config) + assert get_value(1) == {"result": 1} + file_path = self._only_cache_file(config) + real_read = os.read + + def truncating_read(fd: int, n: int) -> bytes: + if n > HEADER_SIZE: # one shot: shrink the file between fstat and the payload read + monkeypatch.setattr(os, "read", real_read) + # Drop exactly one ciphertext byte: the envelope frame stays structurally valid, + # so without the backend's length check this reaches AES-GCM, which rejects + # truncated ciphertext as tamper-class and fail-closed would retain it forever. + os.truncate(file_path, os.path.getsize(file_path) - 1) + return real_read(fd, n) + + monkeypatch.setattr(os, "read", truncating_read) + assert get_value(1) == {"result": 1} # clean miss: recomputed and rewritten, no raise + assert calls == [1, 1] + assert get_value(1) == {"result": 1} # the rewritten entry serves + assert calls == [1, 1] From a501c9c1f6b978e79247b797a79ed197b1c74362 Mon Sep 17 00:00:00 2001 From: Winston Date: Wed, 2 Sep 2026 11:07:13 +1000 Subject: [PATCH 6/6] review(panel): skip flock release after close; single rationale anchor; 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. --- docs/backends/file.md | 2 +- src/cachekit/backends/file/backend.py | 20 ++++++++++--------- tests/unit/backends/test_file_backend.py | 25 ++++-------------------- 3 files changed, 16 insertions(+), 31 deletions(-) diff --git a/docs/backends/file.md b/docs/backends/file.md index f9ae2c94..74c9e769 100644 --- a/docs/backends/file.md +++ b/docs/backends/file.md @@ -141,7 +141,7 @@ the cached payload is left untouched. 5. **Disk space**: FileBackend will evict least-recently-used entries when reaching 90% capacity. Ensure sufficient disk space beyond max_size_mb for temporary writes. -6. **Corruption vs. tampering**: `set()` writes every byte or raises `BackendError` (short `write(2)` calls are retried, never silently truncated into a "successful" file). On read, a structurally broken file — short header, bad magic or version, expired, or a payload shorter than the size the file reported — is deleted and treated as a miss. Payload *content* is not checked here: a same-length modification is served as-is, and the serialization envelope (xxHash3 checksum, or the AES-256-GCM tag for encrypted values) decides whether it is corruption or tampering under your `encryption_fail_closed` policy. +6. **Corruption vs. tampering**: `set()` writes every byte or raises `BackendError` (short `write(2)` calls are resumed until every byte lands, never silently truncated into a "successful" file). On read, an expired entry or a structurally broken file — short header, bad magic or version, or a payload shorter than the file's own `st_size` implies — is deleted and treated as a miss. Payload *content* is not checked here: a same-length modification is served as-is, and the serialization envelope (xxHash3 checksum, or the AES-256-GCM tag for encrypted values) decides whether it is corruption or tampering under your `encryption_fail_closed` policy. ## Performance Characteristics diff --git a/src/cachekit/backends/file/backend.py b/src/cachekit/backends/file/backend.py index 700ba3c6..d15ac705 100644 --- a/src/cachekit/backends/file/backend.py +++ b/src/cachekit/backends/file/backend.py @@ -81,10 +81,8 @@ def _write_fully(fd: int, data: bytes) -> None: A single write(2) may store fewer bytes than asked (POSIX permits it; Linux caps one call at ~2 GiB) and ``os.write`` only reports the count, so a lone call can silently truncate a large - value that is then fsync'd and renamed into place as a "successful" set. A truncated - encrypted payload later fails AES-GCM as tamper-class, which fail-closed retains as evidence - forever, so the write side must land every byte or fail. The memoryview slice advances - without copying. A write that makes no progress raises rather than spinning. + value that is then fsync'd and renamed into place as a "successful" set (see the + truncated-payload branch in ``get`` for what that costs). Raises EIO on zero progress. """ view = memoryview(data) while view: @@ -257,7 +255,8 @@ def get(self, key: str) -> bytes | None: return payload finally: - self._release_file_lock(fd) + if not fd_closed: # close already dropped the flock; a reused fd number is a stranger's + self._release_file_lock(fd) finally: if not fd_closed: os.close(fd) @@ -408,7 +407,7 @@ def set(self, key: str, value: bytes, ttl: int | None = None) -> None: self._acquire_file_lock(fd, exclusive=True) try: - # Write all data (loops over short writes; never a silent partial file) + # Write all data _write_fully(fd, file_data) # fsync to ensure data is on disk @@ -624,7 +623,8 @@ def exists(self, key: str) -> bool: return True finally: - self._release_file_lock(fd) + if not fd_closed: # close already dropped the flock; a reused fd number is a stranger's + self._release_file_lock(fd) finally: if not fd_closed: os.close(fd) @@ -739,7 +739,8 @@ async def get_ttl(self, key: str) -> int | None: return None return int(remaining) # whole-second granularity, matching Redis TTL finally: - self._release_file_lock(fd) + if not fd_closed: # close already dropped the flock; a reused fd number is a stranger's + self._release_file_lock(fd) finally: if not fd_closed: os.close(fd) @@ -802,7 +803,8 @@ async def refresh_ttl(self, key: str, ttl: int) -> bool: os.fsync(fd) return True finally: - self._release_file_lock(fd) + if not fd_closed: # close already dropped the flock; a reused fd number is a stranger's + self._release_file_lock(fd) finally: if not fd_closed: os.close(fd) diff --git a/tests/unit/backends/test_file_backend.py b/tests/unit/backends/test_file_backend.py index 51cc77e9..83bd3630 100644 --- a/tests/unit/backends/test_file_backend.py +++ b/tests/unit/backends/test_file_backend.py @@ -1828,13 +1828,8 @@ def boom(*_a, **_k): class TestShortIO: """POSIX short I/O on the write path, and a file shrinking under a read (LAB-2682). - A single write(2) may store fewer bytes than asked (Linux caps one call at ~2 GiB), and a - file can shrink between fstat and read. Neither is tampering, and neither may reach the - envelope's integrity check: for encrypted entries that check deliberately classifies any - truncated ciphertext as tamper-class (see rust/src/python_bindings.rs), and under - ``encryption_fail_closed`` it retains the entry as evidence forever. The backend is the only - layer that can tell truncation apart from tampering, because only it knows how many bytes it - meant to write (set) or how many the file claimed to hold (get). + Neither is tampering; see the truncated-payload branch in ``FileBackend.get`` for why the + backend, not the envelope, must be the layer that says so. """ def test_set_loops_over_short_writes(self, backend: FileBackend, monkeypatch: pytest.MonkeyPatch) -> None: @@ -1869,17 +1864,6 @@ def test_set_zero_progress_write_raises_and_leaves_nothing_behind( assert backend.get("stuck_key") is None assert not list(Path(config.cache_dir).rglob("*.tmp.*")), "temp file left behind" - async def test_refresh_ttl_loops_over_short_writes(self, backend: FileBackend, monkeypatch: pytest.MonkeyPatch) -> None: - """The in-place 8-byte expiry rewrite shares the write idiom with set().""" - real_write = os.write - backend.set("ttl_key", b"v", ttl=10) - monkeypatch.setattr(os, "write", lambda fd, data: real_write(fd, memoryview(data)[:1])) - assert await backend.refresh_ttl("ttl_key", 1000) is True - monkeypatch.undo() - - remaining = await backend.get_ttl("ttl_key") - assert remaining is not None and remaining > 900 - def test_get_file_shrunk_under_read_is_corruption_not_a_hit( self, backend: FileBackend, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -1981,9 +1965,8 @@ def test_entry_shrunk_under_read_is_a_miss_not_a_tamper_alarm( def truncating_read(fd: int, n: int) -> bytes: if n > HEADER_SIZE: # one shot: shrink the file between fstat and the payload read monkeypatch.setattr(os, "read", real_read) - # Drop exactly one ciphertext byte: the envelope frame stays structurally valid, - # so without the backend's length check this reaches AES-GCM, which rejects - # truncated ciphertext as tamper-class and fail-closed would retain it forever. + # Drop exactly one ciphertext byte so the frame stays structurally valid and the + # short ciphertext would reach AES-GCM (tamper-class) without the backend's check. os.truncate(file_path, os.path.getsize(file_path) - 1) return real_read(fd, n)