Make the directory verifier cache reachable and safe to share

handler.go

@@ -44,6 +44,10 @@ type UnixChange interface {
 
 // CachingHandler represents the optional caching work that a user may wish to over-ride with
 // their own implementations, but which can be otherwise provided through defaults.
+//
+// If your Handler wraps another one (for example to add logging or metrics), give it an
+// `Unwrap() Handler` method that returns the inner handler. The server follows Unwrap to find
+// a CachingHandler underneath.
 type CachingHandler interface {
 	VerifierFor(path string, contents []fs.FileInfo) uint64
 

helpers/cachinghandler.go

@@ -8,6 +8,7 @@ import (
 	"sync"
 
 	"github.com/willscott/go-nfs"
+	"github.com/willscott/go-nfs/file"
 
 	"github.com/go-git/go-billy/v5"
 	"github.com/google/uuid"
@@ -190,6 +191,12 @@ func hashPathAndContents(path string, contents []fs.FileInfo) uint64 {
 
 	for _, c := range contents {
 		vHash.Write([]byte(c.Name())) // Never fails according to the docs
+		// One CachingHandler serves every mounted filesystem, and two of them can have a
+		// directory with the same path and file names. Mixing in each file's ID gives
+		// them different verifiers.
+		if info := file.GetInfo(c); info != nil {
+			vHash.Write(binary.BigEndian.AppendUint64(nil, info.Fileid))
+		}
 	}
 
 	verify := vHash.Sum(nil)[0:8]
@@ -203,7 +210,7 @@ func (c *CachingHandler) VerifierFor(path string, contents []fs.FileInfo) uint64
 }
 
 func (c *CachingHandler) DataForVerifier(path string, id uint64) []fs.FileInfo {
-	if cache, ok := c.activeVerifiers.Get(id); ok {
+	if cache, ok := c.activeVerifiers.Get(id); ok && cache.path == path {
 		return cache.contents
 	}
 	return nil

helpers/cachinghandler_verifier_test.go

@@ -0,0 +1,52 @@
+package helpers
+
+import (
+	"io/fs"
+	"testing"
+	"time"
+
+	"github.com/willscott/go-nfs/file"
+)
+
+// dirEntry is a fake directory entry that carries a file ID, like a real stat result does.
+type dirEntry struct {
+	name   string
+	size   int64
+	fileid uint64
+}
+
+func (e dirEntry) Name() string       { return e.name }
+func (e dirEntry) Size() int64        { return e.size }
+func (e dirEntry) Mode() fs.FileMode  { return 0o644 }
+func (e dirEntry) ModTime() time.Time { return time.Time{} }
+func (e dirEntry) IsDir() bool        { return false }
+func (e dirEntry) Sys() any           { return file.FileInfo{Fileid: e.fileid} }
+
+// Two filesystems can have a directory with the same path and file names. Each must get
+// its own verifier, otherwise one would be shown the other's files.
+func TestVerifierForTellsApartFilesystemsWithTheSameNames(t *testing.T) {
+	c := NewCachingHandler(nil, 16).(*CachingHandler)
+
+	onA := []fs.FileInfo{dirEntry{name: "main.go", size: 10, fileid: 5001}}
+	onB := []fs.FileInfo{dirEntry{name: "main.go", size: 99000, fileid: 7002}}
+
+	verifierA := c.VerifierFor("/src", onA)
+	verifierB := c.VerifierFor("/src", onB)
+	if verifierA == verifierB {
+		t.Fatalf("listings with different file IDs share verifier %x", verifierA)
+	}
+
+	got := c.DataForVerifier("/src", verifierA)
+	if len(got) != 1 || got[0].Size() != 10 {
+		t.Errorf("listing for A = %v, want A's main.go of size 10", got)
+	}
+}
+
+func TestDataForVerifierChecksThePath(t *testing.T) {
+	c := NewCachingHandler(nil, 16).(*CachingHandler)
+
+	verifier := c.VerifierFor("/src", []fs.FileInfo{dirEntry{name: "main.go", fileid: 1}})
+	if got := c.DataForVerifier("/other", verifier); got != nil {
+		t.Errorf("verifier for /src returned a listing for /other: %v", got)
+	}
+}

nfs_onreaddir.go

@@ -12,6 +12,8 @@ import (
 	"sort"
 
 	"github.com/willscott/go-nfs-client/nfs/xdr"
+
+	"github.com/willscott/go-nfs/file"
 )
 
 type readDirArgs struct {
@@ -45,7 +47,7 @@ func onReadDir(ctx context.Context, w *response, userHandle Handler) error {
 		return &NFSStatusError{NFSStatusStale, err}
 	}
 
-	contents, verifier, err := getDirListingWithVerifier(ctx, userHandle, obj.Handle, obj.CookieVerif)
+	contents, verifier, err := getDirListingWithVerifier(ctx, userHandle, obj.Handle, obj.Cookie, obj.CookieVerif)
 	if err != nil {
 		return err
 	}
@@ -137,16 +139,20 @@ func onReadDir(ctx context.Context, w *response, userHandle Handler) error {
 	return nil
 }
 
-func getDirListingWithVerifier(ctx context.Context, userHandle Handler, fsHandle []byte, verifier uint64) ([]fs.FileInfo, uint64, error) {
+// getDirListingWithVerifier returns the entries of the directory a handle points to.
+// When the client continues a listing (cookie > 0), the copy cached on the first page is
+// used if there is one. A new listing (cookie 0) always reads the directory.
+func getDirListingWithVerifier(ctx context.Context, userHandle Handler, fsHandle []byte, cookie uint64, verifier uint64) ([]fs.FileInfo, uint64, error) {
 	// figure out what directory it is.
 	fs, p, err := userHandle.FromHandle(ctx, fsHandle)
 	if err != nil {
 		return nil, 0, &NFSStatusError{NFSStatusStale, err}
 	}
 
 	path := fs.Join(p...)
+	vh, caching := cachingHandlerOf(userHandle)
 	// see if the verifier has this dir cached:
-	if vh, ok := userHandle.(CachingHandler); verifier != 0 && ok {
+	if caching && cookie != 0 && verifier != 0 {
 		entries := vh.DataForVerifier(path, verifier)
 		if entries != nil {
 			return entries, verifier, nil
@@ -165,7 +171,7 @@ func getDirListingWithVerifier(ctx context.Context, userHandle Handler, fsHandle
 		return contents[i].Name() < contents[j].Name()
 	})
 
-	if vh, ok := userHandle.(CachingHandler); ok {
+	if caching {
 		// let the user handler make a verifier if it can.
 		v := vh.VerifierFor(path, contents)
 		return contents, v, nil
@@ -175,6 +181,22 @@ func getDirListingWithVerifier(ctx context.Context, userHandle Handler, fsHandle
 	return contents, id, nil
 }
 
+// cachingHandlerOf returns the CachingHandler inside h, following Unwrap through any
+// handlers that wrap it.
+func cachingHandlerOf(h Handler) (CachingHandler, bool) {
+	for h != nil {
+		if c, ok := h.(CachingHandler); ok {
+			return c, true
+		}
+		wrapper, ok := h.(interface{ Unwrap() Handler })
+		if !ok {
+			break
+		}
+		h = wrapper.Unwrap()
+	}
+	return nil, false
+}
+
 func hashPathAndContents(path string, contents []fs.FileInfo) uint64 {
 	//calculate a cookie-verifier.
 	vHash := sha256.New()
@@ -184,6 +206,11 @@ func hashPathAndContents(path string, contents []fs.FileInfo) uint64 {
 
 	for _, c := range contents {
 		vHash.Write([]byte(c.Name())) // Never fails according to the docs
+		// Two filesystems can have a directory with the same path and file names.
+		// Mixing in each file's ID gives them different verifiers.
+		if info := file.GetInfo(c); info != nil {
+			vHash.Write(binary.BigEndian.AppendUint64(nil, info.Fileid))
+		}
 	}
 
 	verify := vHash.Sum(nil)[0:8]

nfs_onreaddir_test.go

@@ -0,0 +1,101 @@
+package nfs
+
+import (
+	"context"
+	"io/fs"
+	"testing"
+
+	"github.com/go-git/go-billy/v5"
+	"github.com/go-git/go-billy/v5/memfs"
+)
+
+// dirHandler returns the directory "dir" on fs, whatever handle it is given.
+type dirHandler struct {
+	Handler
+	fs billy.Filesystem
+}
+
+func (h *dirHandler) FromHandle(context.Context, []byte) (billy.Filesystem, []string, error) {
+	return h.fs, []string{"dir"}, nil
+}
+
+// cachingDirHandler returns the same cached listing for any verifier.
+type cachingDirHandler struct {
+	*dirHandler
+	cached  []fs.FileInfo
+	lookups int
+}
+
+func (h *cachingDirHandler) VerifierFor(string, []fs.FileInfo) uint64 { return 42 }
+
+func (h *cachingDirHandler) DataForVerifier(string, uint64) []fs.FileInfo {
+	h.lookups++
+	return h.cached
+}
+
+// wrappingHandler wraps another handler, like logging or metrics middleware does.
+type wrappingHandler struct{ Handler }
+
+func (w wrappingHandler) Unwrap() Handler { return w.Handler }
+
+func newCachingDirHandler(t *testing.T) *cachingDirHandler {
+	t.Helper()
+
+	mem := memfs.New()
+	f, err := mem.Create("dir/on-disk")
+	if err != nil {
+		t.Fatal(err)
+	}
+	_ = f.Close()
+
+	cachedDir := memfs.New()
+	f, err = cachedDir.Create("cached")
+	if err != nil {
+		t.Fatal(err)
+	}
+	_ = f.Close()
+	cached, err := cachedDir.ReadDir("/")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	return &cachingDirHandler{dirHandler: &dirHandler{fs: mem}, cached: cached}
+}
+
+func names(entries []fs.FileInfo) []string {
+	out := make([]string, 0, len(entries))
+	for _, e := range entries {
+		out = append(out, e.Name())
+	}
+	return out
+}
+
+func TestDirListingFindsCachingHandlerThroughWrappers(t *testing.T) {
+	h := newCachingDirHandler(t)
+
+	entries, verifier, err := getDirListingWithVerifier(context.Background(), wrappingHandler{h}, nil, 5, 42)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := names(entries); len(got) != 1 || got[0] != "cached" {
+		t.Errorf("continuation page should come from the wrapped handler's cache, got %v", got)
+	}
+	if verifier != 42 {
+		t.Errorf("verifier = %d, want 42", verifier)
+	}
+}
+
+func TestDirListingFromCookieZeroReadsTheDirectory(t *testing.T) {
+	h := newCachingDirHandler(t)
+
+	entries, _, err := getDirListingWithVerifier(context.Background(), h, nil, 0, 42)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := names(entries); len(got) != 1 || got[0] != "on-disk" {
+		t.Errorf("a listing from cookie zero should read the directory, got %v", got)
+	}
+	if h.lookups != 0 {
+		t.Errorf("verifier cache consulted %d times for a cookie-zero listing", h.lookups)
+	}
+}

nfs_onreaddirplus.go

@@ -51,7 +51,7 @@ func onReadDirPlus(ctx context.Context, w *response, userHandle Handler) error {
 		return &NFSStatusError{NFSStatusStale, err}
 	}
 
-	contents, verifier, err := getDirListingWithVerifier(ctx, userHandle, obj.Handle, obj.CookieVerif)
+	contents, verifier, err := getDirListingWithVerifier(ctx, userHandle, obj.Handle, obj.Cookie, obj.CookieVerif)
 	if err != nil {
 		return err
 	}