diff --git a/containers.go b/containers.go index f72b015c49..2182cbb98c 100644 --- a/containers.go +++ b/containers.go @@ -191,7 +191,7 @@ func (r *containerStore) startWritingWithReload(canReload bool) error { }() if canReload { - if err := r.ReloadIfChanged(); err != nil { + if err := r.reloadIfChanged(true); err != nil { return err } } @@ -222,7 +222,7 @@ func (r *containerStore) startReading() error { } }() - if err := r.ReloadIfChanged(); err != nil { + if err := r.reloadIfChanged(false); err != nil { return err } @@ -235,14 +235,17 @@ func (r *containerStore) stopReading() { r.lockfile.Unlock() } -// ReloadIfChanged reloads the contents of the store from disk if it is changed. -func (r *containerStore) ReloadIfChanged() error { +// reloadIfChanged reloads the contents of the store from disk if it is changed. +// +// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true +// if it is held for writing. +func (r *containerStore) reloadIfChanged(lockedForWriting bool) error { r.loadMut.Lock() defer r.loadMut.Unlock() modified, err := r.lockfile.Modified() if err == nil && modified { - return r.Load() + return r.load(lockedForWriting) } return err } @@ -267,9 +270,11 @@ func (r *containerStore) datapath(id, key string) string { return filepath.Join(r.datadir(id), makeBigDataBaseName(key)) } -// Load reloads the contents of the store from disk. It should be called -// with the lock held. -func (r *containerStore) Load() error { +// load reloads the contents of the store from disk. +// +// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true +// if it is held for writing. +func (r *containerStore) load(lockedForWriting bool) error { needSave := false rpath := r.containerspath() data, err := os.ReadFile(rpath) @@ -302,17 +307,19 @@ func (r *containerStore) Load() error { r.bylayer = layers r.byname = names if needSave { + if !lockedForWriting { + // Eventually, the callers should be modified to retry with a write lock, instead. + return errors.New("container store is inconsistent and the current caller does not hold a write lock") + } return r.Save() } return nil } // Save saves the contents of the store to disk. It should be called with -// the lock held. +// the lock held, locked for writing. func (r *containerStore) Save() error { - if !r.lockfile.Locked() { - return errors.New("container store is not locked") - } + r.lockfile.AssertLockedForWriting() rpath := r.containerspath() if err := os.MkdirAll(filepath.Dir(rpath), 0700); err != nil { return err @@ -347,7 +354,7 @@ func newContainerStore(dir string) (rwContainerStore, error) { return nil, err } defer cstore.stopWriting() - if err := cstore.Load(); err != nil { + if err := cstore.load(true); err != nil { return nil, err } return &cstore, nil diff --git a/images.go b/images.go index 5f3640a18e..ea32cc34b4 100644 --- a/images.go +++ b/images.go @@ -1,7 +1,6 @@ package storage import ( - "errors" "fmt" "os" "path/filepath" @@ -154,7 +153,7 @@ type rwImageStore interface { } type imageStore struct { - lockfile Locker + lockfile Locker // lockfile.IsReadWrite can be used to distinguish between read-write and read-only image stores. dir string images []*Image idindex *truncindex.TruncIndex @@ -209,7 +208,7 @@ func (r *imageStore) startWritingWithReload(canReload bool) error { }() if canReload { - if err := r.ReloadIfChanged(); err != nil { + if err := r.reloadIfChanged(true); err != nil { return err } } @@ -244,7 +243,7 @@ func (r *imageStore) startReadingWithReload(canReload bool) error { }() if canReload { - if err := r.ReloadIfChanged(); err != nil { + if err := r.reloadIfChanged(false); err != nil { return err } } @@ -264,14 +263,17 @@ func (r *imageStore) stopReading() { r.lockfile.Unlock() } -// ReloadIfChanged reloads the contents of the store from disk if it is changed. -func (r *imageStore) ReloadIfChanged() error { +// reloadIfChanged reloads the contents of the store from disk if it is changed. +// +// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true +// if it is held for writing. +func (r *imageStore) reloadIfChanged(lockedForWriting bool) error { r.loadMut.Lock() defer r.loadMut.Unlock() modified, err := r.lockfile.Modified() if err == nil && modified { - return r.Load() + return r.load(lockedForWriting) } return err } @@ -336,9 +338,11 @@ func (i *Image) recomputeDigests() error { return nil } -// Load reloads the contents of the store from disk. It should be called -// with the lock held. -func (r *imageStore) Load() error { +// load reloads the contents of the store from disk. +// +// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true +// if it is held for writing. +func (r *imageStore) load(lockedForWriting bool) error { shouldSave := false rpath := r.imagespath() data, err := os.ReadFile(rpath) @@ -376,7 +380,8 @@ func (r *imageStore) Load() error { image.ReadOnly = !r.lockfile.IsReadWrite() } } - if shouldSave && (!r.lockfile.IsReadWrite() || !r.lockfile.Locked()) { + if shouldSave && (!r.lockfile.IsReadWrite() || !lockedForWriting) { + // Eventually, the callers should be modified to retry with a write lock if IsReadWrite && !lockedForWriting, instead. return ErrDuplicateImageNames } r.images = images @@ -391,14 +396,12 @@ func (r *imageStore) Load() error { } // Save saves the contents of the store to disk. It should be called with -// the lock held. +// the lock held, locked for writing. func (r *imageStore) Save() error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify the image store at %q: %w", r.imagespath(), ErrStoreIsReadOnly) } - if !r.lockfile.Locked() { - return errors.New("image store is not locked for writing") - } + r.lockfile.AssertLockedForWriting() rpath := r.imagespath() if err := os.MkdirAll(filepath.Dir(rpath), 0700); err != nil { return err @@ -433,7 +436,7 @@ func newImageStore(dir string) (rwImageStore, error) { return nil, err } defer istore.stopWriting() - if err := istore.Load(); err != nil { + if err := istore.load(true); err != nil { return nil, err } return &istore, nil @@ -456,7 +459,7 @@ func newROImageStore(dir string) (roImageStore, error) { return nil, err } defer istore.stopReading() - if err := istore.Load(); err != nil { + if err := istore.load(false); err != nil { return nil, err } return &istore, nil diff --git a/layers.go b/layers.go index f291fcddbd..032f4683ea 100644 --- a/layers.go +++ b/layers.go @@ -331,7 +331,7 @@ func (r *layerStore) startWritingWithReload(canReload bool) error { }() if canReload { - if err := r.ReloadIfChanged(); err != nil { + if err := r.reloadIfChanged(true); err != nil { return err } } @@ -366,7 +366,7 @@ func (r *layerStore) startReadingWithReload(canReload bool) error { }() if canReload { - if err := r.ReloadIfChanged(); err != nil { + if err := r.reloadIfChanged(false); err != nil { return err } } @@ -420,14 +420,17 @@ func (r *layerStore) Modified() (bool, error) { return tmodified, nil } -// ReloadIfChanged reloads the contents of the store from disk if it is changed. -func (r *layerStore) ReloadIfChanged() error { +// reloadIfChanged reloads the contents of the store from disk if it is changed. +// +// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true +// if it is held for writing. +func (r *layerStore) reloadIfChanged(lockedForWriting bool) error { r.loadMut.Lock() defer r.loadMut.Unlock() modified, err := r.Modified() if err == nil && modified { - return r.Load() + return r.load(lockedForWriting) } return err } @@ -448,9 +451,11 @@ func (r *layerStore) layerspath() string { return filepath.Join(r.layerdir, "layers.json") } -// Load reloads the contents of the store from disk. It should be called -// with the lock held. -func (r *layerStore) Load() error { +// load reloads the contents of the store from disk. +// +// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true +// if it is held for writing. +func (r *layerStore) load(lockedForWriting bool) error { shouldSave := false rpath := r.layerspath() info, err := os.Stat(rpath) @@ -499,7 +504,8 @@ func (r *layerStore) Load() error { } err = nil } - if shouldSave && (!r.lockfile.IsReadWrite() || !r.lockfile.Locked()) { + if shouldSave && (!r.lockfile.IsReadWrite() || !lockedForWriting) { + // Eventually, the callers should be modified to retry with a write lock if IsReadWrite && !lockedForWriting, instead. return ErrDuplicateLayerNames } r.layers = layers @@ -520,7 +526,7 @@ func (r *layerStore) Load() error { // Last step: as we’re writable, try to remove anything that a previous // user of this storage area marked for deletion but didn't manage to // actually delete. - if r.lockfile.Locked() { + if lockedForWriting { for _, layer := range r.layers { if layer.Flags == nil { layer.Flags = make(map[string]interface{}) @@ -581,7 +587,7 @@ func (r *layerStore) loadMounts() error { } // Save saves the contents of the store to disk. It should be called with -// the lock held. +// the lock held, locked for writing. func (r *layerStore) Save() error { r.mountsLockfile.Lock() defer r.mountsLockfile.Unlock() @@ -595,9 +601,7 @@ func (r *layerStore) saveLayers() error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify the layer store at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } - if !r.lockfile.Locked() { - return errors.New("layer store is not locked for writing") - } + r.lockfile.AssertLockedForWriting() rpath := r.layerspath() if err := os.MkdirAll(filepath.Dir(rpath), 0700); err != nil { return err @@ -616,9 +620,7 @@ func (r *layerStore) saveMounts() error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify the layer store at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } - if !r.mountsLockfile.Locked() { - return errors.New("layer store mount information is not locked for writing") - } + r.mountsLockfile.AssertLockedForWriting() mpath := r.mountspath() if err := os.MkdirAll(filepath.Dir(mpath), 0700); err != nil { return err @@ -675,7 +677,7 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri return nil, err } defer rlstore.stopWriting() - if err := rlstore.Load(); err != nil { + if err := rlstore.load(true); err != nil { return nil, err } return &rlstore, nil @@ -700,7 +702,7 @@ func newROLayerStore(rundir string, layerdir string, driver drivers.Driver) (roL return nil, err } defer rlstore.stopReading() - if err := rlstore.Load(); err != nil { + if err := rlstore.load(false); err != nil { return nil, err } return &rlstore, nil diff --git a/pkg/lockfile/lockfile.go b/pkg/lockfile/lockfile.go index 7cd2051d58..f639357f4a 100644 --- a/pkg/lockfile/lockfile.go +++ b/pkg/lockfile/lockfile.go @@ -40,8 +40,13 @@ type Locker interface { // IsReadWrite() checks if the lock file is read-write IsReadWrite() bool - // Locked() checks if lock is locked for writing by a thread in this process - Locked() bool + // AssertLocked() can be used by callers that _know_ that they hold the lock (for reading or writing), for sanity checking. + // It might do nothing at all, or it may panic if the caller is not the owner of this lock. + AssertLocked() + + // AssertLocked() can be used by callers that _know_ that they hold the lock locked for writing, for sanity checking. + // It might do nothing at all, or it may panic if the caller is not the owner of this lock for writing. + AssertLockedForWriting() } var ( diff --git a/pkg/lockfile/lockfile_test.go b/pkg/lockfile/lockfile_test.go index 431695fd83..d15193bf6d 100644 --- a/pkg/lockfile/lockfile_test.go +++ b/pkg/lockfile/lockfile_test.go @@ -201,16 +201,20 @@ func TestLockfileName(t *testing.T) { assert.NotEmpty(t, l.name, "lockfile name should be recorded correctly") - assert.False(t, l.Locked(), "Locked() said we have a write lock") + // l.Assert* are NOT usable for determining lock state if there are concurrent users of the lock. + // It’s just about acceptable for these smoke tests. + assert.Panics(t, l.AssertLocked) l.RLock() - assert.False(t, l.Locked(), "Locked() said we have a write lock") + l.AssertLocked() + assert.Panics(t, l.AssertLockedForWriting) l.Unlock() assert.NotEmpty(t, l.name, "lockfile name should be recorded correctly") l.Lock() - assert.True(t, l.Locked(), "Locked() said we didn't have a write lock") + l.AssertLocked() + l.AssertLockedForWriting() l.Unlock() assert.NotEmpty(t, l.name, "lockfile name should be recorded correctly") @@ -222,7 +226,8 @@ func TestLockfileRead(t *testing.T) { defer os.Remove(l.name) l.RLock() - assert.False(t, l.Locked(), "Locked() said we have a write lock") + l.AssertLocked() + assert.Panics(t, l.AssertLockedForWriting) l.Unlock() } @@ -232,7 +237,8 @@ func TestROLockfileRead(t *testing.T) { defer os.Remove(l.name) l.RLock() - assert.False(t, l.Locked(), "Locked() said we have a write lock") + l.AssertLocked() + assert.Panics(t, l.AssertLockedForWriting) l.Unlock() } @@ -242,7 +248,8 @@ func TestLockfileWrite(t *testing.T) { defer os.Remove(l.name) l.Lock() - assert.True(t, l.Locked(), "Locked() said we didn't have a write lock") + l.AssertLocked() + l.AssertLockedForWriting() l.Unlock() } @@ -255,7 +262,8 @@ func TestROLockfileWrite(t *testing.T) { assert.NotNil(t, recover(), "Should have panicked trying to take a write lock using a read lock") }() l.Lock() - assert.False(t, l.Locked(), "Locked() said we have a write lock") + l.AssertLocked() + assert.Panics(t, l.AssertLockedForWriting) l.Unlock() } diff --git a/pkg/lockfile/lockfile_unix.go b/pkg/lockfile/lockfile_unix.go index b908ce8227..21378e9420 100644 --- a/pkg/lockfile/lockfile_unix.go +++ b/pkg/lockfile/lockfile_unix.go @@ -213,11 +213,33 @@ func (l *lockfile) Unlock() { l.stateMutex.Unlock() } -// Locked checks if lockfile is locked for writing by a thread in this process. -func (l *lockfile) Locked() bool { - l.stateMutex.Lock() - defer l.stateMutex.Unlock() - return l.locked && (l.locktype == unix.F_WRLCK) +func (l *lockfile) AssertLocked() { + // DO NOT provide a variant that returns the value of l.locked. + // + // If the caller does not hold the lock, l.locked might nevertheless be true because another goroutine does hold it, and + // we can’t tell the difference. + // + // Hence, this “AssertLocked” method, which exists only for sanity checks. + + // Don’t even bother with l.stateMutex: The caller is expected to hold the lock, and in that case l.locked is constant true + // with no possible writers. + // If the caller does not hold the lock, we are violating the locking/memory model anyway, and accessing the data + // without the lock is more efficient for callers, and potentially more visible to lock analysers for incorrect callers. + if !l.locked { + panic("internal error: lock is not held by the expected owner") + } +} + +func (l *lockfile) AssertLockedForWriting() { + // DO NOT provide a variant that returns the current lock state. + // + // The same caveats as for AssertLocked apply equally. + + l.AssertLocked() + // Like AssertLocked, don’t even bother with l.stateMutex. + if l.locktype != unix.F_WRLCK { + panic("internal error: lock is not held for writing") + } } // Touch updates the lock file with the UID of the user. diff --git a/pkg/lockfile/lockfile_windows.go b/pkg/lockfile/lockfile_windows.go index 9849f94dee..cc898d82b5 100644 --- a/pkg/lockfile/lockfile_windows.go +++ b/pkg/lockfile/lockfile_windows.go @@ -47,8 +47,23 @@ func (l *lockfile) Unlock() { l.mu.Unlock() } -func (l *lockfile) Locked() bool { - return l.locked +func (l *lockfile) AssertLocked() { + // DO NOT provide a variant that returns the value of l.locked. + // + // If the caller does not hold the lock, l.locked might nevertheless be true because another goroutine does hold it, and + // we can’t tell the difference. + // + // Hence, this “AssertLocked” method, which exists only for sanity checks. + if !l.locked { + panic("internal error: lock is not held by the expected owner") + } +} + +func (l *lockfile) AssertLockedForWriting() { + // DO NOT provide a variant that returns the current lock state. + // + // The same caveats as for AssertLocked apply equally. + l.AssertLocked() // The current implementation does not distinguish between read and write locks. } func (l *lockfile) Modified() (bool, error) {