fix(lockutil): harden cross-platform lock files

Author: gnanam1990

internal/daemon/lock.go

@@ -65,5 +65,20 @@ func readPidFile(path string) (int, error) {
 	if err != nil {
 		return 0, err
 	}
-	return strconv.Atoi(strings.TrimSpace(string(data)))
+	metadata := strings.TrimSpace(string(data))
+	if pid, err := strconv.Atoi(metadata); err == nil {
+		return pid, nil
+	}
+	pidText, sequenceText, ok := strings.Cut(metadata, "-")
+	if !ok || pidText == "" || sequenceText == "" || strings.Contains(sequenceText, "-") {
+		return 0, fmt.Errorf("daemon: parse lock PID metadata %q", metadata)
+	}
+	if _, err := strconv.ParseUint(sequenceText, 10, 64); err != nil {
+		return 0, fmt.Errorf("daemon: parse lock holder sequence: %w", err)
+	}
+	pid, err := strconv.Atoi(pidText)
+	if err != nil {
+		return 0, fmt.Errorf("daemon: parse lock PID: %w", err)
+	}
+	return pid, nil
 }

internal/daemon/lock_test.go

@@ -87,6 +87,35 @@ func TestLockReleaseKeepsStableFile(t *testing.T) {
 	}
 }
 
+func TestReadPidFileMetadataFormats(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "daemon.lock")
+	for _, test := range []struct {
+		name     string
+		metadata string
+		wantPID  int
+		wantErr  bool
+	}{
+		{name: "legacy PID", metadata: "4242\n", wantPID: 4242},
+		{name: "PID and holder sequence", metadata: "4242-7\n", wantPID: 4242},
+		{name: "missing sequence", metadata: "4242-", wantErr: true},
+		{name: "non-numeric sequence", metadata: "4242-next", wantErr: true},
+		{name: "extra component", metadata: "4242-7-8", wantErr: true},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			if err := os.WriteFile(path, []byte(test.metadata), 0o600); err != nil {
+				t.Fatal(err)
+			}
+			got, err := readPidFile(path)
+			if (err != nil) != test.wantErr {
+				t.Fatalf("readPidFile() error = %v, wantErr %v", err, test.wantErr)
+			}
+			if !test.wantErr && got != test.wantPID {
+				t.Fatalf("readPidFile() = %d, want %d", got, test.wantPID)
+			}
+		})
+	}
+}
+
 func TestProcessAliveSelfAndDead(t *testing.T) {
 	if !osProcessAlive(os.Getpid()) {
 		t.Fatal("osProcessAlive(self) = false, want true")

internal/lockutil/lockutil.go

@@ -9,6 +9,8 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
+	"strings"
 	"sync"
 	"sync/atomic"
 )
@@ -38,7 +40,27 @@ type FileLock struct {
 // waiting. A contended lock returns ErrLockHeld. The kernel releases a lock if
 // its process exits, so an old unlocked file never needs stale reclamation.
 func TryAcquireFileLock(path string) (*FileLock, error) {
-	file, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600)
+	return TryAcquireFileLockAt(filepath.Dir(path), path)
+}
+
+// TryAcquireFileLockAt attempts to lock path while confining every component
+// below root to a handle-relative traversal. root is the caller's trusted
+// boundary and may itself be reached through a legitimate link; links and
+// reparse points below it are rejected before metadata can be written.
+func TryAcquireFileLockAt(root, path string) (*FileLock, error) {
+	if root == "" {
+		return nil, errors.New("lockutil: lock root is empty")
+	}
+	relative, err := filepath.Rel(root, path)
+	if err != nil {
+		return nil, fmt.Errorf("lockutil: resolve lock path relative to root: %w", err)
+	}
+	relative = filepath.Clean(relative)
+	if relative == "." || relative == ".." || filepath.IsAbs(relative) || strings.HasPrefix(relative, ".."+string(filepath.Separator)) {
+		return nil, fmt.Errorf("lockutil: lock path %q escapes root %q", path, root)
+	}
+
+	file, err := openLockFileAt(root, relative, path)
 	if err != nil {
 		return nil, fmt.Errorf("lockutil: open lock file: %w", err)
 	}

internal/lockutil/lockutil_other.go

@@ -4,13 +4,78 @@ package lockutil
 
 import (
 	"errors"
+	"fmt"
 	"os"
+	"path/filepath"
+	"strings"
 
 	"golang.org/x/sys/unix"
 )
 
 type platformLockState struct{}
 
+func openLockFileAt(root, relative, displayPath string) (_ *os.File, resultErr error) {
+	parentFD, err := unix.Open(root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
+	if err != nil {
+		return nil, fmt.Errorf("open lock root: %w", err)
+	}
+	defer func() {
+		if parentFD >= 0 {
+			resultErr = errors.Join(resultErr, unix.Close(parentFD))
+		}
+	}()
+
+	components := strings.Split(filepath.Clean(relative), string(filepath.Separator))
+	for _, component := range components[:len(components)-1] {
+		nextFD, openErr := unix.Openat(parentFD, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0)
+		if openErr != nil {
+			return nil, fmt.Errorf("open lock path component %q: %w", component, openErr)
+		}
+		if closeErr := unix.Close(parentFD); closeErr != nil {
+			_ = unix.Close(nextFD)
+			parentFD = -1
+			return nil, fmt.Errorf("close lock path component: %w", closeErr)
+		}
+		parentFD = nextFD
+	}
+
+	name := components[len(components)-1]
+	fd := -1
+	// Darwin can transiently return ENOENT when many callers race to create the
+	// same O_NOFOLLOW entry. The bound parent handle cannot be redirected, so a
+	// bounded retry preserves first-use contention without weakening no-follow.
+	for attempt := 0; attempt < 10; attempt++ {
+		fd, err = unix.Openat(parentFD, name, unix.O_CREAT|unix.O_RDWR|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0o600)
+		if !errors.Is(err, unix.ENOENT) {
+			break
+		}
+	}
+	if err != nil {
+		return nil, err
+	}
+	file := os.NewFile(uintptr(fd), displayPath)
+	if file == nil {
+		_ = unix.Close(fd)
+		return nil, errors.New("convert lock handle to file")
+	}
+	var stat unix.Stat_t
+	if err := unix.Fstat(fd, &stat); err != nil {
+		return nil, errors.Join(fmt.Errorf("inspect lock file: %w", err), file.Close())
+	}
+	if stat.Mode&unix.S_IFMT != unix.S_IFREG {
+		return nil, errors.Join(errors.New("refusing non-regular lock file"), file.Close())
+	}
+	if stat.Nlink != 1 {
+		return nil, errors.Join(errors.New("refusing multiply-linked lock file"), file.Close())
+	}
+	if err := unix.Close(parentFD); err != nil {
+		parentFD = -1
+		return nil, errors.Join(fmt.Errorf("close lock parent: %w", err), file.Close())
+	}
+	parentFD = -1
+	return file, nil
+}
+
 func tryLockFile(file *os.File) (platformLockState, bool, error) {
 	err := unix.Flock(int(file.Fd()), unix.LOCK_EX|unix.LOCK_NB)
 	if err == nil {

internal/lockutil/lockutil_test.go

@@ -10,6 +10,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"strings"
+	"sync"
 	"testing"
 )
 
@@ -77,6 +78,77 @@ func TestFileLockMetadata(t *testing.T) {
 	}
 }
 
+func TestFileLockConcurrentFirstAcquisition(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "lock")
+	const contenders = 200
+	start := make(chan struct{})
+	results := make(chan error, contenders)
+	locks := make(chan *FileLock, contenders)
+	var ready sync.WaitGroup
+	ready.Add(contenders)
+	for range contenders {
+		go func() {
+			ready.Done()
+			<-start
+			lock, err := TryAcquireFileLock(path)
+			if lock != nil {
+				locks <- lock
+			}
+			results <- err
+		}()
+	}
+	ready.Wait()
+	close(start)
+	for range contenders {
+		err := <-results
+		if err != nil && !errors.Is(err, ErrLockHeld) {
+			t.Errorf("concurrent first acquire: %v", err)
+		}
+	}
+	close(locks)
+	for lock := range locks {
+		if err := lock.Release(); err != nil {
+			t.Error(err)
+		}
+	}
+}
+
+func TestFileLockRefusesMultiplyLinkedEntry(t *testing.T) {
+	dir := t.TempDir()
+	target := filepath.Join(dir, "target")
+	lockPath := filepath.Join(dir, "lock")
+	const sentinel = "do not overwrite"
+	if err := os.WriteFile(target, []byte(sentinel), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Link(target, lockPath); err != nil {
+		t.Fatalf("create redirected hard link: %v", err)
+	}
+	if lock, err := TryAcquireFileLock(lockPath); err == nil {
+		_ = lock.Release()
+		t.Fatal("TryAcquireFileLock accepted a multiply-linked entry")
+	}
+	data, err := os.ReadFile(target)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(data) != sentinel {
+		t.Fatalf("redirected target was modified: %q", data)
+	}
+}
+
+func TestFileLockAtRejectsEscapingPath(t *testing.T) {
+	base := t.TempDir()
+	outside := filepath.Join(filepath.Dir(base), "outside.lock")
+	if lock, err := TryAcquireFileLockAt(base, outside); err == nil {
+		_ = lock.Release()
+		t.Fatal("TryAcquireFileLockAt accepted a path outside its root")
+	}
+	if _, err := os.Stat(outside); !errors.Is(err, os.ErrNotExist) {
+		t.Fatalf("escaping path was created: %v", err)
+	}
+}
+
 // TestFileLockReleasedByProcessExit proves crash recovery is provided by the
 // kernel lock lifetime. The child exits without calling Release; the next
 // process can then lock the same persistent file without moving or deleting it.