From 90b2f6fa66c78ff7577220a97919d1be6e4d76dd Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 00:45:39 +0000 Subject: [PATCH] test: add failing test for evictSourceBlob unlink-vs-store TOCTOU Adds an instrumentation seam (evictSourceBlobTestHook, fired after the row-deletion transaction commits and before the content file is unlinked) and a test that pauses eviction there while a concurrent StoreSource for identical content bytes races it. Currently red: the store completes immediately instead of being excluded, which is exactly the window the review flagged between evictSourceBlob's commit and its unlink. --- internal/imgcache/cache.go | 14 ++++ internal/imgcache/eviction.go | 4 ++ internal/imgcache/eviction_test.go | 103 +++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+) diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index 390597a..0a54392 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -76,6 +76,19 @@ type Cache struct { // In-memory cache of variant metadata (content type, size) to avoid reading .meta files metaCache map[VariantKey]variantMeta + + // contentLocks serializes StoreSource and evictSourceBlob per + // content hash, closing the race window between an eviction's row + // deletion and its file unlink against a concurrent store of + // identical content. + contentLocks *contentLock + + // evictSourceBlobTestHook, when set, is invoked by evictSourceBlob + // after its row-deletion transaction commits and before the + // content file is unlinked. It exists solely so tests can + // deterministically pause inside that window to exercise + // concurrent stores against it; production code leaves it nil. + evictSourceBlobTestHook func(ContentHash) } // NewCache creates a new cache instance. @@ -94,6 +107,7 @@ func NewCache(db *sql.DB, config CacheConfig) (*Cache, error) { evictionStop: make(chan struct{}), evictionDone: make(chan struct{}), metaCache: make(map[VariantKey]variantMeta), + contentLocks: newContentLock(), } if c.disabled { diff --git a/internal/imgcache/eviction.go b/internal/imgcache/eviction.go index 068d8ae..d68708a 100644 --- a/internal/imgcache/eviction.go +++ b/internal/imgcache/eviction.go @@ -318,6 +318,10 @@ func (c *Cache) evictSourceBlob(ctx context.Context, contentHash ContentHash) er return fmt.Errorf("failed to commit eviction transaction: %w", err) } + if c.evictSourceBlobTestHook != nil { + c.evictSourceBlobTestHook(contentHash) + } + // Only after the rows are gone may the files be removed. for _, reference := range references { if err := c.srcMetadata.Delete(reference.host, reference.pathHash); err != nil { diff --git a/internal/imgcache/eviction_test.go b/internal/imgcache/eviction_test.go index 0a16d05..86dd9ef 100644 --- a/internal/imgcache/eviction_test.go +++ b/internal/imgcache/eviction_test.go @@ -667,3 +667,106 @@ func TestStartEvictionReconcilesAccountingWithDisk(t *testing.T) { t.Errorf("stale accounting row without a file was not dropped (rows=%d)", n) } } + +// TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent exercises +// the exact TOCTOU window between evictSourceBlob's row-deletion +// transaction commit and its content file unlink: a concurrent +// StoreSource for a different source path whose content hashes to the +// same value (real SHA-256 dedup, not a contrived case) must not be +// able to insert a fresh row referencing the file while eviction is +// mid-unlink, and must not lose its own store once eviction has fully +// released the content hash. +func TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent(t *testing.T) { + cache, _ := newEvictionTestCache(t, 1<<30) + ctx := context.Background() + + content := bytes.Repeat([]byte{0x55}, 400) + + hash := storeEvictionTestSource(t, cache, "race.example.com", "/first.jpg", content) + + proceed := make(chan struct{}) + storeAttempted := make(chan struct{}) + + cache.evictSourceBlobTestHook = func(gotHash ContentHash) { + if gotHash != hash { + t.Errorf("test hook invoked for hash %s, want %s", gotHash, hash) + } + + close(storeAttempted) + <-proceed + } + + evictDone := make(chan error, 1) + + go func() { + evictDone <- cache.evictSourceBlob(ctx, hash) + }() + + // Wait until eviction has committed its delete transaction and is + // paused (inside the test hook) immediately before unlinking the + // content file: exactly the window the review flagged. + <-storeAttempted + + storeDone := make(chan error, 1) + + go func() { + req := &ImageRequest{ + SourceHost: "race.example.com", + SourcePath: "/dup.jpg", + Format: FormatJPEG, + Quality: 85, + FitMode: FitCover, + } + result := &httpfetcher.FetchResult{ + StatusCode: 200, + ContentType: "image/jpeg", + ContentLength: int64(len(content)), + Headers: map[string][]string{"Content-Type": {"image/jpeg"}}, + } + + _, err := cache.StoreSource(ctx, req, bytes.NewReader(content), result) + storeDone <- err + }() + + // The concurrent store must not be able to complete while eviction + // still holds the content hash (i.e. before the file is unlinked): + // if it could, it would insert a row referencing a file about to be + // removed out from under it. + select { + case err := <-storeDone: + t.Fatalf("StoreSource for identical content completed (err=%v) while eviction "+ + "still held the content hash open between commit and unlink; the store and "+ + "the evict of identical content are not mutually exclusive", err) + case <-time.After(200 * time.Millisecond): + // Expected: the store is blocked behind eviction's exclusion. + } + + close(proceed) + + if err := <-evictDone; err != nil { + t.Fatalf("evictSourceBlob failed: %v", err) + } + + if err := <-storeDone; err != nil { + t.Fatalf("StoreSource failed: %v", err) + } + + assertNoDanglingReferences(t, cache) + + dupHash, _, err := cache.LookupSource(ctx, &ImageRequest{ + SourceHost: "race.example.com", + SourcePath: "/dup.jpg", + }) + if err != nil { + t.Fatalf("LookupSource failed: %v", err) + } + + if dupHash == "" { + t.Fatal("re-stored blob was lost: the store legitimately ran after eviction " + + "released the content hash and must have recreated the file and row") + } + + if !cache.srcContent.Exists(dupHash) { + t.Errorf("source_content/source_metadata references %s but its file is missing", dupHash) + } +}