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.
This commit is contained in:
@@ -76,6 +76,19 @@ type Cache struct {
|
|||||||
|
|
||||||
// In-memory cache of variant metadata (content type, size) to avoid reading .meta files
|
// In-memory cache of variant metadata (content type, size) to avoid reading .meta files
|
||||||
metaCache map[VariantKey]variantMeta
|
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.
|
// NewCache creates a new cache instance.
|
||||||
@@ -94,6 +107,7 @@ func NewCache(db *sql.DB, config CacheConfig) (*Cache, error) {
|
|||||||
evictionStop: make(chan struct{}),
|
evictionStop: make(chan struct{}),
|
||||||
evictionDone: make(chan struct{}),
|
evictionDone: make(chan struct{}),
|
||||||
metaCache: make(map[VariantKey]variantMeta),
|
metaCache: make(map[VariantKey]variantMeta),
|
||||||
|
contentLocks: newContentLock(),
|
||||||
}
|
}
|
||||||
|
|
||||||
if c.disabled {
|
if c.disabled {
|
||||||
|
|||||||
@@ -318,6 +318,10 @@ func (c *Cache) evictSourceBlob(ctx context.Context, contentHash ContentHash) er
|
|||||||
return fmt.Errorf("failed to commit eviction transaction: %w", err)
|
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.
|
// Only after the rows are gone may the files be removed.
|
||||||
for _, reference := range references {
|
for _, reference := range references {
|
||||||
if err := c.srcMetadata.Delete(reference.host, reference.pathHash); err != nil {
|
if err := c.srcMetadata.Delete(reference.host, reference.pathHash); err != nil {
|
||||||
|
|||||||
@@ -667,3 +667,106 @@ func TestStartEvictionReconcilesAccountingWithDisk(t *testing.T) {
|
|||||||
t.Errorf("stale accounting row without a file was not dropped (rows=%d)", n)
|
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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user