diff --git a/internal/rawfilelock/rawfilelock.go b/internal/rawfilelock/rawfilelock.go new file mode 100644 index 000000000..4f340ae3c --- /dev/null +++ b/internal/rawfilelock/rawfilelock.go @@ -0,0 +1,64 @@ +package rawfilelock + +import ( + "os" +) + +type LockType byte + +const ( + ReadLock LockType = iota + WriteLock +) + +type FileHandle = fileHandle + +// OpenLock opens a file for locking +// WARNING: This is the underlying file locking primitive of the OS; +// because closing FileHandle releases the lock, it is not suitable for use +// if there is any chance of two concurrent goroutines attempting to use the same lock. +// Most users should use the higher-level operations from internal/staging_lockfile or pkg/lockfile. +func OpenLock(path string, readOnly bool) (FileHandle, error) { + flags := os.O_CREATE + if readOnly { + flags |= os.O_RDONLY + } else { + flags |= os.O_RDWR + } + + fd, err := openHandle(path, flags) + if err == nil { + return fd, nil + } + + return fd, &os.PathError{Op: "open", Path: path, Err: err} +} + +// TryLockFile attempts to lock a file handle +func TryLockFile(fd FileHandle, lockType LockType) error { + return lockHandle(fd, lockType, true) +} + +// LockFile locks a file handle +func LockFile(fd FileHandle, lockType LockType) error { + return lockHandle(fd, lockType, false) +} + +// UnlockAndClose unlocks and closes a file handle +func UnlockAndCloseHandle(fd FileHandle) { + unlockAndCloseHandle(fd) +} + +// CloseHandle closes a file handle without unlocking +// +// WARNING: This is a last-resort function for error handling only! +// On Unix systems, closing a file descriptor automatically releases any locks, +// so "closing without unlocking" is impossible. This function will release +// the lock as a side effect of closing the file. +// +// This function should only be used in error paths where the lock state +// is already corrupted or when giving up on lock management entirely. +// Normal code should use UnlockAndCloseHandle instead. +func CloseHandle(fd FileHandle) { + closeHandle(fd) +} diff --git a/internal/rawfilelock/rawfilelock_test.go b/internal/rawfilelock/rawfilelock_test.go new file mode 100644 index 000000000..83354069e --- /dev/null +++ b/internal/rawfilelock/rawfilelock_test.go @@ -0,0 +1,86 @@ +package rawfilelock + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestOpenLock(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + prepare func() (path string, readOnly bool) + }{ + { + name: "file exists (read/write)", + prepare: func() (string, bool) { + tempFile, err := os.CreateTemp(t.TempDir(), "lock-") + require.NoError(t, err) + tempFile.Close() + return tempFile.Name(), false + }, + }, + { + name: "file exists readonly (readonly)", + prepare: func() (string, bool) { + tempFile, err := os.CreateTemp(t.TempDir(), "lock-") + require.NoError(t, err) + tempFile.Close() + return tempFile.Name(), true + }, + }, + { + name: "base dir exists (read/write)", + prepare: func() (string, bool) { + tempDir := os.TempDir() + require.DirExists(t, tempDir) + return filepath.Join(tempDir, "test-1.lock"), false + }, + }, + } { + path, readOnly := tc.prepare() + + fd, err := OpenLock(path, readOnly) + require.NoError(t, err, tc.name) + UnlockAndCloseHandle(fd) + + fd, err = OpenLock(path, readOnly) + require.NoError(t, err) + UnlockAndCloseHandle(fd) + + require.Nil(t, os.RemoveAll(path)) + } +} + +func TestOpenLockNotCreateParentDir(t *testing.T) { + tmpDir := t.TempDir() + lockPath := filepath.Join(tmpDir, "lockfile") + fd, err := OpenLock(lockPath, false) + require.NoError(t, err) + UnlockAndCloseHandle(fd) + + lockPath = filepath.Join(tmpDir, "subdir", "lockfile") + _, err = OpenLock(lockPath, false) + require.Error(t, err) +} + +func TestTryLockFileAndLockFile(t *testing.T) { + tmpFile, err := os.CreateTemp(t.TempDir(), "lockfile") + require.NoError(t, err) + defer os.Remove(tmpFile.Name()) + fd, err := OpenLock(tmpFile.Name(), false) + require.NoError(t, err) + + require.NoError(t, TryLockFile(fd, WriteLock)) + UnlockAndCloseHandle(fd) + + fd2, err := OpenLock(tmpFile.Name(), false) + require.NoError(t, err) + + require.NoError(t, LockFile(fd2, WriteLock)) + UnlockAndCloseHandle(fd2) +} diff --git a/internal/staging_lockfile/staging_lockfile_unix.go b/internal/rawfilelock/rawfilelock_unix.go similarity index 86% rename from internal/staging_lockfile/staging_lockfile_unix.go rename to internal/rawfilelock/rawfilelock_unix.go index 7a4e4dda9..268554076 100644 --- a/internal/staging_lockfile/staging_lockfile_unix.go +++ b/internal/rawfilelock/rawfilelock_unix.go @@ -1,6 +1,6 @@ //go:build !windows -package staging_lockfile +package rawfilelock import ( "time" @@ -16,9 +16,9 @@ func openHandle(path string, mode int) (fileHandle, error) { return fileHandle(fd), err } -func lockHandle(fd fileHandle, lType lockType, nonblocking bool) error { +func lockHandle(fd fileHandle, lType LockType, nonblocking bool) error { fType := unix.F_RDLCK - if lType != readLock { + if lType != ReadLock { fType = unix.F_WRLCK } lk := unix.Flock_t{ diff --git a/internal/staging_lockfile/staging_lockfile_windows.go b/internal/rawfilelock/rawfilelock_windows.go similarity index 88% rename from internal/staging_lockfile/staging_lockfile_windows.go rename to internal/rawfilelock/rawfilelock_windows.go index dc0c6da13..9c0d692f8 100644 --- a/internal/staging_lockfile/staging_lockfile_windows.go +++ b/internal/rawfilelock/rawfilelock_windows.go @@ -1,6 +1,6 @@ //go:build windows -package staging_lockfile +package rawfilelock import ( "golang.org/x/sys/windows" @@ -19,9 +19,9 @@ func openHandle(path string, mode int) (fileHandle, error) { return fileHandle(fd), err } -func lockHandle(fd fileHandle, lType lockType, nonblocking bool) error { +func lockHandle(fd fileHandle, lType LockType, nonblocking bool) error { flags := 0 - if lType != readLock { + if lType != ReadLock { flags = windows.LOCKFILE_EXCLUSIVE_LOCK } if nonblocking { diff --git a/internal/staging_lockfile/staging_lockfile.go b/internal/staging_lockfile/staging_lockfile.go index 94cf16fce..aeb8e7421 100644 --- a/internal/staging_lockfile/staging_lockfile.go +++ b/internal/staging_lockfile/staging_lockfile.go @@ -2,16 +2,10 @@ package staging_lockfile import ( "fmt" - "os" "path/filepath" "sync" -) -type lockType byte - -const ( - readLock lockType = iota - writeLock + "github.com/containers/storage/internal/rawfilelock" ) // StagingLockFile represents a file lock used to coordinate access to staging areas. @@ -30,7 +24,7 @@ type StagingLockFile struct { // stateMutex is used to synchronize concurrent accesses to the state below stateMutex *sync.Mutex locked bool - fd fileHandle + fd rawfilelock.FileHandle } var ( @@ -86,11 +80,11 @@ func getLockfile(path string) (*StagingLockFile, error) { // This function will be called at most once for each unique path within a process. func createStagingLockFileForPath(path string) (*StagingLockFile, error) { // Check if we can open the lock. - fd, err := openLock(path) + fd, err := rawfilelock.OpenLock(path, false) if err != nil { return nil, err } - unlockAndCloseHandle(fd) + rawfilelock.UnlockAndCloseHandle(fd) return &StagingLockFile{ file: path, @@ -100,30 +94,6 @@ func createStagingLockFileForPath(path string) (*StagingLockFile, error) { }, nil } -// openLock opens the file at the specified path with read-write access. -// It creates the file and any necessary parent directories if they don't exist. -// Returns a file handle that can be used for locking. -func openLock(path string) (fd fileHandle, err error) { - flags := os.O_CREATE - flags |= os.O_RDWR - - fd, err = openHandle(path, flags) - if err == nil { - return fd, nil - } - - // the directory of the lockfile seems to be removed, try to create it - if os.IsNotExist(err) { - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { - return fd, fmt.Errorf("creating lock file directory: %w", err) - } - - return openLock(path) - } - - return fd, &os.PathError{Op: "open", Path: path, Err: err} -} - // tryLock attempts to acquire an exclusive lock on the StagingLockFile without blocking. // It first tries to acquire the internal rwMutex, then opens and tries to lock the file. // Returns nil on success or an error if any step fails. @@ -136,15 +106,15 @@ func (l *StagingLockFile) tryLock() error { } l.stateMutex.Lock() defer l.stateMutex.Unlock() - fd, err := openLock(l.file) + fd, err := rawfilelock.OpenLock(l.file, false) if err != nil { rwMutexUnlocker() return err } l.fd = fd - if err = lockHandle(l.fd, writeLock, true); err != nil { - closeHandle(fd) + if err = rawfilelock.TryLockFile(l.fd, rawfilelock.WriteLock); err != nil { + rawfilelock.CloseHandle(fd) rwMutexUnlocker() return err } diff --git a/internal/staging_lockfile/staging_lockfile_test.go b/internal/staging_lockfile/staging_lockfile_test.go index 402f813b5..0f77f8f93 100644 --- a/internal/staging_lockfile/staging_lockfile_test.go +++ b/internal/staging_lockfile/staging_lockfile_test.go @@ -4,7 +4,6 @@ import ( "io" "os" "os/exec" - "path/filepath" "runtime" "sync" "sync/atomic" @@ -229,50 +228,3 @@ func TestLockfileMultiProcess(t *testing.T) { wg.Wait() assert.True(t, whighest == 1, "expected to have no more than one writer lock active at a time, had %d", whighest) } - -func TestOpenLock(t *testing.T) { - t.Parallel() - - for _, tc := range []struct { - name string - prepare func() (path string) - }{ - { - name: "file exists (read/write)", - prepare: func() string { - tempFile, err := os.CreateTemp("", "lock-") - require.NoError(t, err) - tempFile.Close() - return tempFile.Name() - }, - }, - { - name: "base dir exists (read/write)", - prepare: func() string { - tempDir := os.TempDir() - require.DirExists(t, tempDir) - return filepath.Join(tempDir, "test-1.lock") - }, - }, - { - name: "base dir not exists (read/write)", - prepare: func() string { - tempDir, err := os.MkdirTemp("", "lock-") - require.NoError(t, err) - return filepath.Join(tempDir, "subdir", "test-1.lock") - }, - }, - } { - path := tc.prepare() - - fd, err := openLock(path) - require.NoError(t, err, tc.name) - unlockAndCloseHandle(fd) - - fd, err = openLock(path) - require.NoError(t, err) - unlockAndCloseHandle(fd) - - require.Nil(t, os.RemoveAll(path)) - } -} diff --git a/pkg/lockfile/lockfile.go b/pkg/lockfile/lockfile.go index 52f6c0a62..dfe81c245 100644 --- a/pkg/lockfile/lockfile.go +++ b/pkg/lockfile/lockfile.go @@ -6,6 +6,8 @@ import ( "path/filepath" "sync" "time" + + "github.com/containers/storage/internal/rawfilelock" ) // A Locker represents a file lock where the file is used to cache an @@ -55,13 +57,6 @@ type Locker interface { AssertLockedForWriting() } -type lockType byte - -const ( - readLock lockType = iota - writeLock -) - // LockFile represents a file lock where the file is used to cache an // identifier of the last party that made changes to whatever's being protected // by the lock. @@ -79,12 +74,12 @@ type LockFile struct { stateMutex *sync.Mutex counter int64 lw LastWrite // A global value valid as of the last .Touch() or .Modified() - lockType lockType + lockType rawfilelock.LockType locked bool // The following fields are only modified on transitions between counter == 0 / counter != 0. // Thus, they can be safely accessed by users _that currently hold the LockFile_ without locking. // In other cases, they need to be protected using stateMutex. - fd fileHandle + fd rawfilelock.FileHandle } var ( @@ -129,12 +124,12 @@ func (l *LockFile) Lock() { if l.ro { panic("can't take write lock on read-only lock file") } - l.lock(writeLock) + l.lock(rawfilelock.WriteLock) } // RLock locks the lockfile as a reader. func (l *LockFile) RLock() { - l.lock(readLock) + l.lock(rawfilelock.ReadLock) } // TryLock attempts to lock the lockfile as a writer. Panic if the lock is a read-only one. @@ -142,12 +137,12 @@ func (l *LockFile) TryLock() error { if l.ro { panic("can't take write lock on read-only lock file") } - return l.tryLock(writeLock) + return l.tryLock(rawfilelock.WriteLock) } // TryRLock attempts to lock the lockfile as a reader. func (l *LockFile) TryRLock() error { - return l.tryLock(readLock) + return l.tryLock(rawfilelock.ReadLock) } // Unlock unlocks the lockfile. @@ -172,9 +167,9 @@ func (l *LockFile) Unlock() { l.locked = false // Close the file descriptor on the last unlock, releasing the // file lock. - unlockAndCloseHandle(l.fd) + rawfilelock.UnlockAndCloseHandle(l.fd) } - if l.lockType == readLock { + if l.lockType == rawfilelock.ReadLock { l.rwMutex.RUnlock() } else { l.rwMutex.Unlock() @@ -206,7 +201,7 @@ func (l *LockFile) AssertLockedForWriting() { l.AssertLocked() // Like AssertLocked, don’t even bother with l.stateMutex. - if l.lockType == readLock { + if l.lockType == rawfilelock.ReadLock { panic("internal error: lock is not held for writing") } } @@ -273,7 +268,7 @@ func (l *LockFile) Touch() error { return err } l.stateMutex.Lock() - if !l.locked || (l.lockType == readLock) { + if !l.locked || (l.lockType == rawfilelock.ReadLock) { panic("attempted to update last-writer in lockfile without the write lock") } defer l.stateMutex.Unlock() @@ -324,6 +319,24 @@ func getLockfile(path string, ro bool) (*LockFile, error) { return lockFile, nil } +// openLock opens a lock file at the specified path, creating the parent directory if it does not exist. +func openLock(path string, readOnly bool) (rawfilelock.FileHandle, error) { + fd, err := rawfilelock.OpenLock(path, readOnly) + if err == nil { + return fd, nil + } + + // the directory of the lockfile seems to be removed, try to create it + if os.IsNotExist(err) { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return fd, fmt.Errorf("creating lock file directory: %w", err) + } + + return openLock(path, readOnly) + } + return fd, &os.PathError{Op: "open", Path: path, Err: err} +} + // createLockFileForPath returns new *LockFile object, possibly (depending on the platform) // working inter-process and associated with the specified path. // @@ -343,11 +356,11 @@ func createLockFileForPath(path string, ro bool) (*LockFile, error) { if err != nil { return nil, err } - unlockAndCloseHandle(fd) + rawfilelock.UnlockAndCloseHandle(fd) - lType := writeLock + lType := rawfilelock.WriteLock if ro { - lType = readLock + lType = rawfilelock.ReadLock } return &LockFile{ @@ -362,40 +375,10 @@ func createLockFileForPath(path string, ro bool) (*LockFile, error) { }, nil } -// openLock opens the file at path and returns the corresponding file -// descriptor. The path is opened either read-only or read-write, -// depending on the value of ro argument. -// -// openLock will create the file and its parent directories, -// if necessary. -func openLock(path string, ro bool) (fd fileHandle, err error) { - flags := os.O_CREATE - if ro { - flags |= os.O_RDONLY - } else { - flags |= os.O_RDWR - } - fd, err = openHandle(path, flags) - if err == nil { - return fd, nil - } - - // the directory of the lockfile seems to be removed, try to create it - if os.IsNotExist(err) { - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { - return fd, fmt.Errorf("creating lock file directory: %w", err) - } - - return openLock(path, ro) - } - - return fd, &os.PathError{Op: "open", Path: path, Err: err} -} - // lock locks the lockfile via syscall based on the specified type and // command. -func (l *LockFile) lock(lType lockType) { - if lType == readLock { +func (l *LockFile) lock(lType rawfilelock.LockType) { + if lType == rawfilelock.ReadLock { l.rwMutex.RLock() } else { l.rwMutex.Lock() @@ -413,7 +396,7 @@ func (l *LockFile) lock(lType lockType) { // Optimization: only use the (expensive) syscall when // the counter is 0. In this case, we're either the first // reader lock or a writer lock. - if err := lockHandle(l.fd, lType, false); err != nil { + if err := rawfilelock.LockFile(l.fd, lType); err != nil { panic(err) } } @@ -424,10 +407,10 @@ func (l *LockFile) lock(lType lockType) { // lock locks the lockfile via syscall based on the specified type and // command. -func (l *LockFile) tryLock(lType lockType) error { +func (l *LockFile) tryLock(lType rawfilelock.LockType) error { var success bool var rwMutexUnlocker func() - if lType == readLock { + if lType == rawfilelock.ReadLock { success = l.rwMutex.TryRLock() rwMutexUnlocker = l.rwMutex.RUnlock } else { @@ -451,8 +434,8 @@ func (l *LockFile) tryLock(lType lockType) error { // Optimization: only use the (expensive) syscall when // the counter is 0. In this case, we're either the first // reader lock or a writer lock. - if err = lockHandle(l.fd, lType, true); err != nil { - closeHandle(fd) + if err = rawfilelock.TryLockFile(l.fd, lType); err != nil { + rawfilelock.CloseHandle(fd) rwMutexUnlocker() return err } diff --git a/pkg/lockfile/lockfile_test.go b/pkg/lockfile/lockfile_test.go index d8513575f..32dd5eb28 100644 --- a/pkg/lockfile/lockfile_test.go +++ b/pkg/lockfile/lockfile_test.go @@ -4,7 +4,6 @@ import ( "io" "os" "os/exec" - "path/filepath" "runtime" "sync" "sync/atomic" @@ -844,59 +843,3 @@ func TestLockfileMultiprocessModifiedSince(t *testing.T) { require.NoError(t, err) assert.False(t, modified) } - -func TestOpenLock(t *testing.T) { - t.Parallel() - - for _, tc := range []struct { - name string - prepare func() (path string, readOnly bool) - }{ - { - name: "file exists (read/write)", - prepare: func() (string, bool) { - tempFile, err := os.CreateTemp("", "lock-") - require.NoError(t, err) - tempFile.Close() - return tempFile.Name(), false - }, - }, - { - name: "file exists readonly (readonly)", - prepare: func() (string, bool) { - tempFile, err := os.CreateTemp("", "lock-") - require.NoError(t, err) - tempFile.Close() - return tempFile.Name(), true - }, - }, - { - name: "base dir exists (read/write)", - prepare: func() (string, bool) { - tempDir := os.TempDir() - require.DirExists(t, tempDir) - return filepath.Join(tempDir, "test-1.lock"), false - }, - }, - { - name: "base dir not exists (read/write)", - prepare: func() (string, bool) { - tempDir, err := os.MkdirTemp("", "lock-") - require.NoError(t, err) - return filepath.Join(tempDir, "subdir", "test-1.lock"), false - }, - }, - } { - path, readOnly := tc.prepare() - - fd, err := openLock(path, readOnly) - require.NoError(t, err, tc.name) - unlockAndCloseHandle(fd) - - fd, err = openLock(path, readOnly) - require.NoError(t, err) - unlockAndCloseHandle(fd) - - require.Nil(t, os.RemoveAll(path)) - } -} diff --git a/pkg/lockfile/lockfile_unix.go b/pkg/lockfile/lockfile_unix.go index 885f2f88a..14c27c51f 100644 --- a/pkg/lockfile/lockfile_unix.go +++ b/pkg/lockfile/lockfile_unix.go @@ -9,8 +9,6 @@ import ( "golang.org/x/sys/unix" ) -type fileHandle uintptr - // GetLastWrite returns a LastWrite value corresponding to current state of the lock. // This is typically called before (_not after_) loading the state when initializing a consumer // of the data protected by the lock. @@ -66,41 +64,3 @@ func (l *LockFile) TouchedSince(when time.Time) bool { touched := time.Unix(mtim.Unix()) return when.Before(touched) } - -func openHandle(path string, mode int) (fileHandle, error) { - mode |= unix.O_CLOEXEC - fd, err := unix.Open(path, mode, 0o644) - return fileHandle(fd), err -} - -func lockHandle(fd fileHandle, lType lockType, nonblocking bool) error { - fType := unix.F_RDLCK - if lType != readLock { - fType = unix.F_WRLCK - } - lk := unix.Flock_t{ - Type: int16(fType), - Whence: int16(unix.SEEK_SET), - Start: 0, - Len: 0, - } - cmd := unix.F_SETLKW - if nonblocking { - cmd = unix.F_SETLK - } - for { - err := unix.FcntlFlock(uintptr(fd), cmd, &lk) - if err == nil || nonblocking { - return err - } - time.Sleep(10 * time.Millisecond) - } -} - -func unlockAndCloseHandle(fd fileHandle) { - unix.Close(int(fd)) -} - -func closeHandle(fd fileHandle) { - unix.Close(int(fd)) -} diff --git a/pkg/lockfile/lockfile_windows.go b/pkg/lockfile/lockfile_windows.go index 0cc1c50cc..e66f7bfbb 100644 --- a/pkg/lockfile/lockfile_windows.go +++ b/pkg/lockfile/lockfile_windows.go @@ -14,8 +14,6 @@ const ( allBytes = ^uint32(0) ) -type fileHandle windows.Handle - // GetLastWrite returns a LastWrite value corresponding to current state of the lock. // This is typically called before (_not after_) loading the state when initializing a consumer // of the data protected by the lock. @@ -73,37 +71,3 @@ func (l *LockFile) TouchedSince(when time.Time) bool { } return when.Before(stat.ModTime()) } - -func openHandle(path string, mode int) (fileHandle, error) { - mode |= windows.O_CLOEXEC - fd, err := windows.Open(path, mode, windows.S_IWRITE) - return fileHandle(fd), err -} - -func lockHandle(fd fileHandle, lType lockType, nonblocking bool) error { - flags := 0 - if lType != readLock { - flags = windows.LOCKFILE_EXCLUSIVE_LOCK - } - if nonblocking { - flags |= windows.LOCKFILE_FAIL_IMMEDIATELY - } - ol := new(windows.Overlapped) - if err := windows.LockFileEx(windows.Handle(fd), uint32(flags), reserved, allBytes, allBytes, ol); err != nil { - if nonblocking { - return err - } - panic(err) - } - return nil -} - -func unlockAndCloseHandle(fd fileHandle) { - ol := new(windows.Overlapped) - windows.UnlockFileEx(windows.Handle(fd), reserved, allBytes, allBytes, ol) - closeHandle(fd) -} - -func closeHandle(fd fileHandle) { - windows.Close(windows.Handle(fd)) -}