Skip to content

Commit

Permalink
fix(flock): race condition in acquire (#423)
Browse files Browse the repository at this point in the history
There was a race condition that enabled multiple processes to acquire
the same lock file. Consider the following sequence:

- Process A successfully acquires lock
- Process B enters acquire function
- Process B calls unix.Open
- Process A calls release(), deleting the lock file
- Process B calls unix.Flock on the still-open file descriptor,
successfully locking the now deleted file.
- Process C successfully acquires lock on newly created lock file

After this sequence both B and C hold the lock. To resolve this race, we
can simply avoid removing the file. This is what e.g. dpkg does: the
/var/lib/dpkg/lock file is always present under normal circumstances.

The issue was discovered by repeatedly running TestContinueFlock:
```
=== RUN   TestContinueFlock
    flock_test.go:85: Did not expect an error but got:
        remove /tmp/TestContinueFlock3236442975/001/lock: no such file or directory
--- FAIL: TestContinueFlock (0.10s)
```
That error, while otherwise benign, indicates that the two goroutines in
the test hit the race condition in question.
  • Loading branch information
nickajacks1 authored Dec 7, 2024
1 parent 10591db commit f25714e
Showing 1 changed file with 9 additions and 1 deletion.
10 changes: 9 additions & 1 deletion util/flock/flock.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ var getPID = os.Getpid
// updated.
//
// If the lock is held by another process, Acquire will block until the lock is released or the context is cancelled.
//
// The file is NOT deleted on release; doing so creates a race condition that allows multiple processes to acquire
// the same lock.
func Acquire(ctx context.Context, path, message string) (release func() error, err error) {
absPath, err := filepath.Abs(path)
if err != nil {
Expand Down Expand Up @@ -87,11 +90,16 @@ func acquire(path, message string) (release func() error, err error) {
return nil, errors.Wrapf(err, "marshal failed")
}

err = unix.Ftruncate(fd, 0)
if err != nil {
return nil, errors.Wrapf(err, "truncate failed")
}

_, err = unix.Write(fd, payload)
if err != nil {
return nil, errors.Wrapf(err, "write failed")
}
return func() error {
return errors.Join(os.Remove(path), unix.Flock(fd, unix.LOCK_UN), unix.Close(fd))
return errors.Join(unix.Flock(fd, unix.LOCK_UN), unix.Close(fd))
}, nil
}

0 comments on commit f25714e

Please # to comment.