diff --git a/docs/backends/file.md b/docs/backends/file.md index 549ff62..74c9e76 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 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 dead0cd..d15ac70 100644 --- a/src/cachekit/backends/file/backend.py +++ b/src/cachekit/backends/file/backend.py @@ -76,6 +76,22 @@ 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 (see the + truncated-payload branch in ``get`` for what that costs). Raises EIO on zero progress. + """ + 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,11 +239,24 @@ 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) + 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) @@ -379,7 +408,7 @@ def set(self, key: str, value: bytes, ttl: int | None = None) -> None: try: # Write all data - os.write(fd, file_data) + _write_fully(fd, file_data) # fsync to ensure data is on disk os.fsync(fd) @@ -594,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) @@ -709,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) @@ -768,11 +799,12 @@ 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: - 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 3005d3e..83bd363 100644 --- a/tests/unit/backends/test_file_backend.py +++ b/tests/unit/backends/test_file_backend.py @@ -1822,3 +1822,156 @@ 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). + + 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: + """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" + + 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 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) + + 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]