Land the context, connection-hook and reverse-cache race work on master

conn.go

@@ -45,6 +45,17 @@ type conn struct {
 func (c *conn) serve(ctx context.Context) {
 	connCtx, cancel := context.WithCancel(ctx)
 	defer cancel()
+
+	if hook := c.OnConnect; hook != nil {
+		connCtx, c.Conn = hook(connCtx, c.Conn)
+	}
+
+	defer func() {
+		if hook := c.OnDisconnect; hook != nil {
+			hook(connCtx, c.Conn)
+		}
+	}()
+
 	c.writeSerializer = make(chan []byte, 1)
 	go c.serializeWrites(connCtx)
 

handler.go

@@ -16,7 +16,7 @@ type Handler interface {
 
 	// Change can return 'nil' if filesystem is read-only
 	// If the returned value can be cast to `UnixChange`, mknod and link RPCs will be available.
-	Change(billy.Filesystem) billy.Change
+	Change(context.Context, billy.Filesystem) billy.Change
 
 	// Optional methods - generic helpers or trivial implementations can be sufficient depending on use case.
 
@@ -25,9 +25,9 @@ type Handler interface {
 
 	// represent file objects as opaque references
 	// Can be safely implemented via helpers/cachinghandler.
-	ToHandle(fs billy.Filesystem, path []string) []byte
-	FromHandle(fh []byte) (billy.Filesystem, []string, error)
-	InvalidateHandle(billy.Filesystem, []byte) error
+	ToHandle(cxt context.Context, fs billy.Filesystem, path []string) []byte
+	FromHandle(ctx context.Context, fh []byte) (billy.Filesystem, []string, error)
+	InvalidateHandle(context.Context, billy.Filesystem, []byte) error
 
 	// How many handles can be safely maintained by the handler.
 	HandleLimit() int

helpers/cachinghandler.go

@@ -1,10 +1,10 @@
 package helpers
 
 import (
+	"context"
 	"crypto/sha256"
 	"encoding/binary"
 	"io/fs"
-	"reflect"
 	"sync"
 
 	"github.com/willscott/go-nfs"
@@ -53,7 +53,7 @@ type entry struct {
 // ToHandle takes a file and represents it with an opaque handle to reference it.
 // In stateless nfs (when it's serving a unix fs) this can be the device + inode
 // but we can generalize with a stateful local cache of handed out IDs.
-func (c *CachingHandler) ToHandle(f billy.Filesystem, path []string) []byte {
+func (c *CachingHandler) ToHandle(ctx context.Context, f billy.Filesystem, path []string) []byte {
 	joinedPath := f.Join(path...)
 
 	if handle := c.searchReverseCache(f, joinedPath); handle != nil {
@@ -78,7 +78,7 @@ func (c *CachingHandler) ToHandle(f billy.Filesystem, path []string) []byte {
 }
 
 // FromHandle converts from an opaque handle to the file it represents
-func (c *CachingHandler) FromHandle(fh []byte) (billy.Filesystem, []string, error) {
+func (c *CachingHandler) FromHandle(ctx context.Context, fh []byte) (billy.Filesystem, []string, error) {
 	id, err := uuid.FromBytes(fh)
 	if err != nil {
 		return nil, []string{}, err
@@ -101,11 +101,21 @@ func (c *CachingHandler) FromHandle(fh []byte) (billy.Filesystem, []string, erro
 }
 
 func (c *CachingHandler) searchReverseCache(f billy.Filesystem, path string) []byte {
-	uuids := c.getReverseHandles(path)
+	// Hold RLock for entire iteration to prevent races with appendReverseHandle
+	// and evictReverseCache which modify the slice. This is safe because
+	// activeHandles.Get() has its own internal synchronization (LRU cache).
+	c.reverseHandlesMu.RLock()
+	defer c.reverseHandlesMu.RUnlock()
 
-	for _, id := range uuids {
+	for _, id := range c.reverseHandles[path] {
 		if candidate, ok := c.activeHandles.Get(id); ok {
-			if reflect.DeepEqual(candidate.f, f) {
+			// Use interface comparison instead of reflect.DeepEqual to avoid
+			// race conditions. reflect.DeepEqual traverses all internal fields
+			// of the filesystem, including mutable maps that can be modified
+			// concurrently by file operations. Interface comparison (==) only
+			// compares type and pointer, which is sufficient for checking if
+			// it's the same filesystem instance.
+			if candidate.f == f {
 				return id[:]
 			}
 		}
@@ -130,19 +140,13 @@ func (c *CachingHandler) evictReverseCache(path string, handle uuid.UUID) {
 	}
 }
 
-func (c *CachingHandler) getReverseHandles(path string) []uuid.UUID {
-	c.reverseHandlesMu.RLock()
-	defer c.reverseHandlesMu.RUnlock()
-	return c.reverseHandles[path]
-}
-
 func (c *CachingHandler) appendReverseHandle(path string, id uuid.UUID) {
 	c.reverseHandlesMu.Lock()
 	defer c.reverseHandlesMu.Unlock()
 	c.reverseHandles[path] = append(c.reverseHandles[path], id)
 }
 
-func (c *CachingHandler) InvalidateHandle(fs billy.Filesystem, handle []byte) error {
+func (c *CachingHandler) InvalidateHandle(ctx context.Context, fs billy.Filesystem, handle []byte) error {
 	//Remove from cache
 	id, _ := uuid.FromBytes(handle)
 	entry, ok := c.activeHandles.Get(id)

helpers/cachinghandler_test.go

@@ -5,6 +5,7 @@ import (
 	"sync"
 	"testing"
 
+	"github.com/go-git/go-billy/v5"
 	"github.com/willscott/go-nfs/helpers/memfs"
 )
 
@@ -30,11 +31,11 @@ func TestCachingHandlerConcurrentToHandle(t *testing.T) {
 				// Each goroutine creates handles for different paths
 				// but also accesses some shared paths to maximize contention
 				path := []string{fmt.Sprintf("file-%d-%d.txt", id, j)}
-				_ = cacheHandler.ToHandle(mem, path)
+				_ = cacheHandler.ToHandle(t.Context(), mem, path)
 
 				// Also access a shared path to increase contention
 				sharedPath := []string{fmt.Sprintf("shared-%d.txt", j%10)}
-				_ = cacheHandler.ToHandle(mem, sharedPath)
+				_ = cacheHandler.ToHandle(t.Context(), mem, sharedPath)
 			}
 		}(i)
 	}
@@ -56,7 +57,7 @@ func TestCachingHandlerConcurrentToHandleAndFromHandle(t *testing.T) {
 	handles := make([][]byte, 20)
 	for i := 0; i < 20; i++ {
 		path := []string{fmt.Sprintf("precreated-%d.txt", i)}
-		handles[i] = cacheHandler.ToHandle(mem, path)
+		handles[i] = cacheHandler.ToHandle(t.Context(), mem, path)
 	}
 
 	var wg sync.WaitGroup
@@ -68,7 +69,7 @@ func TestCachingHandlerConcurrentToHandleAndFromHandle(t *testing.T) {
 			defer wg.Done()
 			for j := 0; j < numOperations; j++ {
 				path := []string{fmt.Sprintf("new-file-%d-%d.txt", id, j)}
-				_ = cacheHandler.ToHandle(mem, path)
+				_ = cacheHandler.ToHandle(t.Context(), mem, path)
 			}
 		}(i)
 	}
@@ -79,7 +80,7 @@ func TestCachingHandlerConcurrentToHandleAndFromHandle(t *testing.T) {
 			defer wg.Done()
 			for j := 0; j < numOperations; j++ {
 				handle := handles[j%len(handles)]
-				_, _, _ = cacheHandler.FromHandle(handle)
+				_, _, _ = cacheHandler.FromHandle(t.Context(), handle)
 			}
 		}(i)
 	}
@@ -106,10 +107,10 @@ func TestCachingHandlerConcurrentInvalidateHandle(t *testing.T) {
 			defer wg.Done()
 			for j := 0; j < numOperations; j++ {
 				path := []string{fmt.Sprintf("invalidate-%d-%d.txt", id, j)}
-				handle := cacheHandler.ToHandle(mem, path)
+				handle := cacheHandler.ToHandle(t.Context(), mem, path)
 				// Immediately invalidate some handles
 				if j%3 == 0 {
-					_ = cacheHandler.InvalidateHandle(mem, handle)
+					_ = cacheHandler.InvalidateHandle(t.Context(), mem, handle)
 				}
 			}
 		}(i)
@@ -121,7 +122,129 @@ func TestCachingHandlerConcurrentInvalidateHandle(t *testing.T) {
 			defer wg.Done()
 			for j := 0; j < numOperations; j++ {
 				sharedPath := []string{fmt.Sprintf("shared-invalidate-%d.txt", j%20)}
-				_ = cacheHandler.ToHandle(mem, sharedPath)
+				_ = cacheHandler.ToHandle(t.Context(), mem, sharedPath)
+			}
+		}(i)
+	}
+
+	wg.Wait()
+}
+
+// TestCachingHandlerReflectDeepEqualRace tests for the race condition in
+// searchReverseCache where reflect.DeepEqual reads filesystem internal state
+// while another goroutine modifies it through file operations.
+//
+// The race occurs at line 108: reflect.DeepEqual(candidate.f, f)
+// reflect.DeepEqual traverses all internal fields of the filesystem objects,
+// including mutable maps that can be modified concurrently.
+//
+// Note: This test uses separate filesystems per writer goroutine to avoid
+// triggering races in memfs itself (which is not thread-safe).
+//
+// Run with: go test -race -run TestCachingHandlerReflectDeepEqualRace ./helpers/
+func TestCachingHandlerReflectDeepEqualRace(t *testing.T) {
+	// Create filesystem instances - one shared for reads, others for writes
+	const numWriterFS = 10
+	readerFS := memfs.New()
+	writerFilesystems := make([]billy.Filesystem, numWriterFS)
+	for i := range writerFilesystems {
+		writerFilesystems[i] = memfs.New()
+	}
+
+	handler := NewNullAuthHandler(readerFS)
+	cacheHandler := NewCachingHandler(handler, 1024).(*CachingHandler)
+
+	const numGoroutines = 10
+	const numOperations = 100
+
+	// Pre-populate cache with handles from all filesystems using same paths
+	// This ensures searchReverseCache will compare different FS instances
+	for j := 0; j < 10; j++ {
+		path := []string{fmt.Sprintf("shared-%d.txt", j)}
+		_ = cacheHandler.ToHandle(t.Context(), readerFS, path)
+		for _, fs := range writerFilesystems {
+			_ = cacheHandler.ToHandle(t.Context(), fs, path)
+		}
+	}
+
+	var wg sync.WaitGroup
+	wg.Add(numGoroutines * 2)
+
+	// Group 1: ToHandle calls triggering filesystem comparisons in searchReverseCache
+	// With reflect.DeepEqual, this would race with writers modifying FS internals
+	for i := 0; i < numGoroutines; i++ {
+		go func(id int) {
+			defer wg.Done()
+			for j := 0; j < numOperations; j++ {
+				// Alternate between reader and writer filesystems
+				var fs billy.Filesystem
+				if j%2 == 0 {
+					fs = readerFS
+				} else {
+					fs = writerFilesystems[j%numWriterFS]
+				}
+				path := []string{fmt.Sprintf("shared-%d.txt", j%10)}
+				_ = cacheHandler.ToHandle(t.Context(), fs, path)
+			}
+		}(i)
+	}
+
+	// Group 2: File operations on dedicated filesystems (one per goroutine)
+	// Each goroutine has its own filesystem to avoid memfs internal races
+	for i := 0; i < numGoroutines; i++ {
+		go func(id int) {
+			defer wg.Done()
+			fs := writerFilesystems[id%numWriterFS]
+			for j := 0; j < numOperations; j++ {
+				filename := fmt.Sprintf("/file-%d-%d.txt", id, j)
+				f, err := fs.Create(filename)
+				if err == nil {
+					_, _ = f.Write([]byte("data"))
+					_ = f.Close()
+				}
+			}
+		}(i)
+	}
+
+	wg.Wait()
+}
+
+// TestCachingHandlerSliceReferenceRace tests for the race condition where
+// getReverseHandles returns a slice reference that can be modified while
+// being iterated in searchReverseCache.
+//
+// The race occurs because:
+// 1. getReverseHandles returns c.reverseHandles[path] - a reference to the slice
+// 2. After releasing RLock, searchReverseCache iterates over this slice
+// 3. Concurrent appendReverseHandle/evictReverseCache modify the same slice
+//
+// Run with: go test -race -run TestCachingHandlerSliceReferenceRace ./helpers/
+func TestCachingHandlerSliceReferenceRace(t *testing.T) {
+	mem := memfs.New()
+	handler := NewNullAuthHandler(mem)
+	cacheHandler := NewCachingHandler(handler, 1024).(*CachingHandler)
+
+	const numGoroutines = 20
+	const numOperations = 500
+
+	var wg sync.WaitGroup
+	wg.Add(numGoroutines)
+
+	// All goroutines use the same small set of paths to maximize contention
+	// on the same reverseHandles slice entries
+	for i := 0; i < numGoroutines; i++ {
+		go func(id int) {
+			defer wg.Done()
+			for j := 0; j < numOperations; j++ {
+				// Use only 5 unique paths to maximize slice contention
+				path := []string{fmt.Sprintf("race-test-%d.txt", j%5)}
+				handle := cacheHandler.ToHandle(t.Context(), mem, path)
+
+				// Occasionally invalidate to trigger evictReverseCache
+				// while other goroutines are in searchReverseCache
+				if j%7 == 0 {
+					_ = cacheHandler.InvalidateHandle(t.Context(), mem, handle)
+				}
 			}
 		}(i)
 	}

helpers/nullauthhandler.go

@@ -27,7 +27,7 @@ func (h *NullAuthHandler) Mount(ctx context.Context, conn net.Conn, req nfs.Moun
 }
 
 // Change provides an interface for updating file attributes.
-func (h *NullAuthHandler) Change(fs billy.Filesystem) billy.Change {
+func (h *NullAuthHandler) Change(ctx context.Context, fs billy.Filesystem) billy.Change {
 	if c, ok := h.fs.(billy.Change); ok {
 		return c
 	}
@@ -40,16 +40,16 @@ func (h *NullAuthHandler) FSStat(ctx context.Context, f billy.Filesystem, s *nfs
 }
 
 // ToHandle handled by CachingHandler
-func (h *NullAuthHandler) ToHandle(f billy.Filesystem, s []string) []byte {
+func (h *NullAuthHandler) ToHandle(ctx context.Context, f billy.Filesystem, s []string) []byte {
 	return []byte{}
 }
 
 // FromHandle handled by CachingHandler
-func (h *NullAuthHandler) FromHandle([]byte) (billy.Filesystem, []string, error) {
+func (h *NullAuthHandler) FromHandle(context.Context, []byte) (billy.Filesystem, []string, error) {
 	return nil, []string{}, nil
 }
 
-func (c *NullAuthHandler) InvalidateHandle(billy.Filesystem, []byte) error {
+func (c *NullAuthHandler) InvalidateHandle(context.Context, billy.Filesystem, []byte) error {
 	return nil
 }
 

mount.go

@@ -39,7 +39,7 @@ func onMount(ctx context.Context, w *response, userHandle Handler) error {
 		return err
 	}
 
-	rootHndl := userHandle.ToHandle(handle, []string{})
+	rootHndl := userHandle.ToHandle(ctx, handle, []string{})
 
 	if status == MountStatusOk {
 		_ = xdr.Write(writer, rootHndl)

nfs_onaccess.go

@@ -14,7 +14,7 @@ func onAccess(ctx context.Context, w *response, userHandle Handler) error {
 	if err != nil {
 		return &NFSStatusError{NFSStatusInval, err}
 	}
-	fs, path, err := userHandle.FromHandle(roothandle)
+	fs, path, err := userHandle.FromHandle(ctx, roothandle)
 	if err != nil {
 		return &NFSStatusError{NFSStatusStale, err}
 	}

nfs_oncommit.go

@@ -18,7 +18,7 @@ func onCommit(ctx context.Context, w *response, userHandle Handler) error {
 	}
 	// The conn will drain the unread offset and count arguments.
 
-	fs, path, err := userHandle.FromHandle(handle)
+	fs, path, err := userHandle.FromHandle(ctx, handle)
 	if err != nil {
 		return &NFSStatusError{NFSStatusStale, err}
 	}

nfs_oncreate.go

@@ -47,7 +47,7 @@ func onCreate(ctx context.Context, w *response, userHandle Handler) error {
 		return &NFSStatusError{NFSStatusNotSupp, os.ErrInvalid}
 	}
 
-	fs, path, err := userHandle.FromHandle(obj.Handle)
+	fs, path, err := userHandle.FromHandle(ctx, obj.Handle)
 	if err != nil {
 		return &NFSStatusError{NFSStatusStale, err}
 	}
@@ -86,8 +86,8 @@ func onCreate(ctx context.Context, w *response, userHandle Handler) error {
 		return &NFSStatusError{NFSStatusAccess, err}
 	}
 
-	fp := userHandle.ToHandle(fs, newFile)
-	changer := userHandle.Change(fs)
+	fp := userHandle.ToHandle(ctx, fs, newFile)
+	changer := userHandle.Change(ctx, fs)
 	if err := attrs.Apply(changer, fs, newFilePath); err != nil {
 		Log.Errorf("Error applying attributes: %v\n", err)
 		return &NFSStatusError{NFSStatusIO, err}

nfs_onfsinfo.go

@@ -24,7 +24,7 @@ func onFSInfo(ctx context.Context, w *response, userHandle Handler) error {
 	if err != nil {
 		return &NFSStatusError{NFSStatusInval, err}
 	}
-	fs, path, err := userHandle.FromHandle(roothandle)
+	fs, path, err := userHandle.FromHandle(ctx, roothandle)
 	if err != nil {
 		return &NFSStatusError{NFSStatusStale, err}
 	}