More timeouts and bounding

Author: mitchpaulus

CHANGELOG.md

@@ -14,7 +14,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   watch (inotify on Linux, kqueue on macOS, `ReadDirectoryChangesW` on
   Windows) with a bounded stat-poll fallback. Clipboard writes are now atomic
   and serialized with an OS file lock, so concurrent instances can no longer
-  lose or corrupt clipboard entries.
+  lose or corrupt clipboard entries. Lock waits time out and clipboard reads
+  are capped in size and time, so a stuck or hostile process sharing the
+  clipboard file cannot hang or bloat other instances.
 
 ### Added
 

mshell/DirWatch.go

@@ -70,6 +70,12 @@ func (fm *FileManager) clipboardWatchLoop() {
 	}
 	defer watcher.Close()
 
+	// The watch only now exists: anything written before this point (startup,
+	// or the dormant gap before a restart) produced no event, so reconcile
+	// with the actual on-disk state once. Every later write is covered by an
+	// event because the watch was established before this refresh.
+	fm.applyClipboardChange(false)
+
 	for {
 		select {
 		case <-fm.watchDone:

mshell/DirWatch_linux.go

@@ -66,6 +66,12 @@ func (w *dirWatcher) readLoop() {
 				return
 			}
 			nameLen := int(event.Len)
+			if offset+unix.SizeofInotifyEvent+nameLen > n {
+				// A record claiming to extend past the read is malformed;
+				// the kernel never splits events, so stop parsing rather
+				// than slice past the data (which would panic the process).
+				break
+			}
 			name := ""
 			if nameLen > 0 {
 				nameBytes := buf[offset+unix.SizeofInotifyEvent : offset+unix.SizeofInotifyEvent+nameLen]

mshell/DirWatch_windows.go

@@ -134,16 +134,18 @@ func (w *dirWatcher) readLoop() {
 // NextEntryOffset uint32, Action uint32, FileNameLength uint32 (bytes),
 // FileName []uint16 (not NUL-terminated).
 func (w *dirWatcher) parseAndSend(buf []byte) {
-	offset := uint32(0)
+	// All arithmetic is in int (64-bit) so a malformed length or chain
+	// offset can never wrap and loop forever or slice out of range.
+	offset := 0
 	for {
-		if int(offset)+12 > len(buf) {
+		if offset+12 > len(buf) {
 			return
 		}
-		next := binary.LittleEndian.Uint32(buf[offset:])
-		nameLen := binary.LittleEndian.Uint32(buf[offset+8:])
+		next := int(binary.LittleEndian.Uint32(buf[offset:]))
+		nameLen := int(binary.LittleEndian.Uint32(buf[offset+8:]))
 		nameStart := offset + 12
 		nameEnd := nameStart + nameLen
-		if int(nameEnd) > len(buf) {
+		if nameEnd > len(buf) {
 			return
 		}
 		nameBytes := buf[nameStart:nameEnd]
@@ -155,6 +157,11 @@ func (w *dirWatcher) parseAndSend(buf []byte) {
 		if next == 0 {
 			return
 		}
+		if next < 12 || offset+next > len(buf) {
+			// A chain offset that is too small or points past the buffer is
+			// malformed; stop parsing rather than spin on it.
+			return
+		}
 		offset += next
 	}
 }

mshell/FileLock_unix.go

@@ -3,36 +3,53 @@
 package main
 
 import (
+	"fmt"
 	"os"
+	"path/filepath"
+	"time"
 
 	"golang.org/x/sys/unix"
 )
 
+const fileLockRetryInterval = 50 * time.Millisecond
+
+// openNonBlock is ORed into opens of the shared clipboard file so that a FIFO
+// planted at that path cannot block open(2) forever waiting for a writer. It
+// has no effect on regular files.
+const openNonBlock = unix.O_NONBLOCK
+
 // fileLock is an exclusive advisory lock backed by flock(2). The lock is tied
 // to the open file description, so the OS releases it automatically if the
 // process dies while holding it; no stale-lock cleanup is ever needed.
 type fileLock struct {
 	f *os.File
 }
 
-// acquireFileLock blocks until an exclusive lock on path is acquired,
-// creating the lock file if needed.
-func acquireFileLock(path string) (*fileLock, error) {
+// acquireFileLock acquires an exclusive lock on path, creating the lock file
+// if needed. The wait is bounded: it retries a non-blocking lock until
+// timeout, then fails, so a stuck or hostile lock holder can never hang the
+// caller indefinitely.
+func acquireFileLock(path string, timeout time.Duration) (*fileLock, error) {
 	f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0644)
 	if err != nil {
 		return nil, err
 	}
+	deadline := time.Now().Add(timeout)
 	for {
-		err = unix.Flock(int(f.Fd()), unix.LOCK_EX)
-		if err != unix.EINTR {
-			break
+		err = unix.Flock(int(f.Fd()), unix.LOCK_EX|unix.LOCK_NB)
+		if err == nil {
+			return &fileLock{f: f}, nil
 		}
+		if err != unix.EWOULDBLOCK && err != unix.EAGAIN && err != unix.EINTR {
+			f.Close()
+			return nil, err
+		}
+		if time.Now().After(deadline) {
+			f.Close()
+			return nil, fmt.Errorf("timed out waiting for lock on %s", filepath.Base(path))
+		}
+		time.Sleep(fileLockRetryInterval)
 	}
-	if err != nil {
-		f.Close()
-		return nil, err
-	}
-	return &fileLock{f: f}, nil
 }
 
 func (l *fileLock) release() {

mshell/FileLock_windows.go

@@ -3,34 +3,57 @@
 package main
 
 import (
+	"fmt"
 	"os"
+	"path/filepath"
+	"time"
 
 	"golang.org/x/sys/windows"
 )
 
+const fileLockRetryInterval = 50 * time.Millisecond
+
+// openNonBlock mirrors the unix constant ORed into clipboard opens. Windows
+// has no FIFO-in-the-filesystem equivalent whose open blocks, so it is a
+// no-op here.
+const openNonBlock = 0
+
 // fileLock is an exclusive lock backed by LockFileEx. The lock is tied to the
 // open handle, so the OS releases it automatically if the process dies while
 // holding it; no stale-lock cleanup is ever needed.
 type fileLock struct {
 	f *os.File
 }
 
-// acquireFileLock blocks until an exclusive lock on path is acquired,
-// creating the lock file if needed.
-func acquireFileLock(path string) (*fileLock, error) {
+// acquireFileLock acquires an exclusive lock on path, creating the lock file
+// if needed. The wait is bounded: it retries a non-blocking lock until
+// timeout, then fails, so a stuck or hostile lock holder can never hang the
+// caller indefinitely.
+func acquireFileLock(path string, timeout time.Duration) (*fileLock, error) {
 	f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0644)
 	if err != nil {
 		return nil, err
 	}
-	// Lock one byte at offset 0 (the offset lives in the Overlapped struct).
-	// Without LOCKFILE_FAIL_IMMEDIATELY this blocks until the lock is free.
-	ol := new(windows.Overlapped)
-	err = windows.LockFileEx(windows.Handle(f.Fd()), windows.LOCKFILE_EXCLUSIVE_LOCK, 0, 1, 0, ol)
-	if err != nil {
-		f.Close()
-		return nil, err
+	deadline := time.Now().Add(timeout)
+	for {
+		// Lock one byte at offset 0 (the offset lives in the Overlapped
+		// struct). LOCKFILE_FAIL_IMMEDIATELY keeps each attempt non-blocking.
+		ol := new(windows.Overlapped)
+		err = windows.LockFileEx(windows.Handle(f.Fd()),
+			windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, 0, 1, 0, ol)
+		if err == nil {
+			return &fileLock{f: f}, nil
+		}
+		if err != windows.ERROR_LOCK_VIOLATION {
+			f.Close()
+			return nil, err
+		}
+		if time.Now().After(deadline) {
+			f.Close()
+			return nil, fmt.Errorf("timed out waiting for lock on %s", filepath.Base(path))
+		}
+		time.Sleep(fileLockRetryInterval)
 	}
-	return &fileLock{f: f}, nil
 }
 
 func (l *fileLock) release() {