From 9197b6300a21312c24a767507cddb18a7233ce38 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 00:46:45 +0000 Subject: [PATCH] fix: close TOCTOU window between blob eviction commit and unlink StoreSource now hashes content itself and holds the per-hash contentLock across the whole store (file write plus accounting row inserts); evictSourceBlob holds the same lock across its whole operation (row deletion transaction through file unlink). A concurrent store and eviction of identical content bytes can no longer interleave: either runs to completion before the other starts, so a fresh row can never be left pointing at a file the other side is mid-unlink on. ContentStorage gains StoreHashed for callers that need the hash before writing; Store is refactored to share the write-if-absent logic with it, with no change to its existing behavior or signature. internal/imgcache/eviction_test.go: TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent proves it: pauses eviction (via evictSourceBlobTestHook) in the exact window between commit and unlink, asserts a concurrent StoreSource for identical content blocks rather than completing, then verifies no dangling reference and that the store's data survives once eviction releases the hash. --- internal/imgcache/cache.go | 31 ++++++++++++++++++++++++-- internal/imgcache/eviction.go | 11 ++++++++++ internal/imgcache/storage.go | 41 ++++++++++++++++++++++++++++------- 3 files changed, 73 insertions(+), 10 deletions(-) diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index 0a54392..d50e858 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -2,7 +2,9 @@ package imgcache import ( "context" + "crypto/sha256" "database/sql" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -215,8 +217,33 @@ func (c *Cache) StoreSource( return "", nil } - // Store content - contentHash, size, err := c.srcContent.Store(content) + // Hash the content ourselves (rather than via srcContent.Store, + // which would hash internally) so the content hash is known before + // any file or database work happens: that lets the entire store be + // serialized, per hash, against a concurrent eviction of the same + // content below. + data, err := io.ReadAll(content) + if err != nil { + return "", fmt.Errorf("failed to read source content: %w", err) + } + + sum := sha256.Sum256(data) + contentHash := ContentHash(hex.EncodeToString(sum[:])) + + // Hold the content hash's lock for the whole store operation. A + // concurrent eviction of this exact hash (the real SHA-256 dedup + // case: a different source path whose bytes hash identically) + // deletes the accounting rows and unlinks the file inside the same + // lock, so the two can never interleave: either this store + // completes first (and a subsequent eviction removes it together + // with its rows and file, correctly), or eviction completes first + // (and this store finds the file already gone and recreates it + // fresh) — never a fresh row left pointing at a file eviction is + // mid-unlink on. + unlock := c.contentLocks.Lock(string(contentHash)) + defer unlock() + + size, err := c.srcContent.StoreHashed(contentHash, data) if err != nil { return "", fmt.Errorf("failed to store source content: %w", err) } diff --git a/internal/imgcache/eviction.go b/internal/imgcache/eviction.go index d68708a..7574822 100644 --- a/internal/imgcache/eviction.go +++ b/internal/imgcache/eviction.go @@ -291,7 +291,18 @@ type sourceReference struct { // removed together with all of its references, and database rows never // point at deleted files. The JSON metadata sidecars for the removed // rows are deleted afterwards. +// +// The whole operation holds the content hash's lock (the same one +// StoreSource holds for its full store), so a concurrent store of +// identical content bytes can never observe the file gone but a row +// still present, or insert a fresh row between this transaction's +// commit and the file unlink below: it either runs entirely before +// this eviction starts, or is blocked until this eviction (row +// deletion and unlink together) has fully completed. func (c *Cache) evictSourceBlob(ctx context.Context, contentHash ContentHash) error { + unlock := c.contentLocks.Lock(string(contentHash)) + defer unlock() + references, err := c.sourceReferences(ctx, contentHash) if err != nil { return err diff --git a/internal/imgcache/storage.go b/internal/imgcache/storage.go index 9891074..ed1ea4c 100644 --- a/internal/imgcache/storage.go +++ b/internal/imgcache/storage.go @@ -65,24 +65,49 @@ func (s *ContentStorage) Store(r io.Reader) (hash ContentHash, size int64, err e hash = ContentHash(hex.EncodeToString(h[:])) size = int64(len(data)) + if err := s.writeIfAbsent(hash, data); err != nil { + return "", 0, err + } + + return hash, size, nil +} + +// StoreHashed writes pre-hashed content to storage at the path derived +// from hash, without recomputing it. Callers that already know the +// hash before writing (e.g. because they must hold a hash-keyed lock +// across the whole store operation) use this instead of Store. Like +// Store, it is idempotent: content already on disk at that path is +// left untouched. +func (s *ContentStorage) StoreHashed(hash ContentHash, data []byte) (size int64, err error) { + if err := s.writeIfAbsent(hash, data); err != nil { + return 0, err + } + + return int64(len(data)), nil +} + +// writeIfAbsent writes data to the path derived from hash, unless +// content already exists there, via a temp-file-plus-rename so +// concurrent readers never observe a partial file. +func (s *ContentStorage) writeIfAbsent(hash ContentHash, data []byte) (err error) { // Build path: /// path := s.hashToPath(hash) // Check if already exists - if _, err := os.Stat(path); err == nil { - return hash, size, nil + if _, statErr := os.Stat(path); statErr == nil { + return nil } // Create directory structure dir := filepath.Dir(path) if err := os.MkdirAll(dir, StorageDirPerm); err != nil { - return "", 0, fmt.Errorf("failed to create directory: %w", err) + return fmt.Errorf("failed to create directory: %w", err) } // Write to temp file first, then rename for atomicity tmpFile, err := os.CreateTemp(dir, ".tmp-*") if err != nil { - return "", 0, fmt.Errorf("failed to create temp file: %w", err) + return fmt.Errorf("failed to create temp file: %w", err) } tmpPath := tmpFile.Name() @@ -95,20 +120,20 @@ func (s *ContentStorage) Store(r io.Reader) (hash ContentHash, size int64, err e if _, err := tmpFile.Write(data); err != nil { _ = tmpFile.Close() - return "", 0, fmt.Errorf("failed to write content: %w", err) + return fmt.Errorf("failed to write content: %w", err) } if err := tmpFile.Close(); err != nil { - return "", 0, fmt.Errorf("failed to close temp file: %w", err) + return fmt.Errorf("failed to close temp file: %w", err) } // Atomic rename //nolint:gosec // G703: paths from internal SHA256 hashes if err := os.Rename(filepath.Clean(tmpPath), filepath.Clean(path)); err != nil { - return "", 0, fmt.Errorf("failed to rename temp file: %w", err) + return fmt.Errorf("failed to rename temp file: %w", err) } - return hash, size, nil + return nil } // Load returns a reader for the content with the given hash.