From a32fc10673156b8be832b8651d1791ca0e48b932 Mon Sep 17 00:00:00 2001 From: Pavel Snajdr Date: Sat, 10 Oct 2026 05:04:01 +0000 Subject: [PATCH 1/6] fix(nfs): close write files on seek and write errors A WRITE request owns the file opened by its handler. Seek and partial-write failures previously returned without closing it, leaving retirement to backend finalizers instead of request completion. Close once on both failure exits, preserve the primary error identity and NFS status, and include any close failure as diagnostic context. Keep the normal success and close-error paths unchanged; use APIs compatible with the declared Go version. Loopback regressions use real temporary files and cover open, seek, partial-write, success and close-error paths. Restoring the previous handler fails the owned-close and closed-file assertions. Full normal tests, a full race run, vet and the consuming proxy race regression pass; an earlier repeated full race attempt hit client source-port collisions. No claim is made that every backend physically retires a resource after a failed Close. --- nfs_onwrite.go | 7 ++ nfs_write_lifetime_test.go | 158 +++++++++++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+) create mode 100644 nfs_write_lifetime_test.go diff --git a/nfs_onwrite.go b/nfs_onwrite.go index 861ef16..8ada93c 100644 --- a/nfs_onwrite.go +++ b/nfs_onwrite.go @@ -3,6 +3,7 @@ package nfs import ( "bytes" "context" + "fmt" "io" "math" "os" @@ -70,6 +71,9 @@ func onWrite(ctx context.Context, w *response, userHandle Handler) error { } if req.Offset > 0 { if _, err := file.Seek(int64(req.Offset), io.SeekStart); err != nil { + if closeErr := file.Close(); closeErr != nil { + err = fmt.Errorf("%w; closing write file: %v", err, closeErr) + } return &NFSStatusError{NFSStatusIO, err} } } @@ -79,6 +83,9 @@ func onWrite(ctx context.Context, w *response, userHandle Handler) error { } writtenCount, err := file.Write(req.Data[:end]) if err != nil { + if closeErr := file.Close(); closeErr != nil { + err = fmt.Errorf("%w; closing write file: %v", err, closeErr) + } Log.Errorf("Error writing: %v", err) return &NFSStatusError{statusFromWriteError(err), err} } diff --git a/nfs_write_lifetime_test.go b/nfs_write_lifetime_test.go new file mode 100644 index 0000000..63cd046 --- /dev/null +++ b/nfs_write_lifetime_test.go @@ -0,0 +1,158 @@ +package nfs_test + +import ( + "errors" + "io" + "os" + "path/filepath" + "sync" + "sync/atomic" + "syscall" + "testing" + + "github.com/go-git/go-billy/v5" + "github.com/go-git/go-billy/v5/osfs" + nfs "github.com/willscott/go-nfs" + "github.com/willscott/go-nfs-client/nfs/rpc" +) + +type writeLifetimeFS struct { + billy.Filesystem + openErr, seekErr, writeErr, closeErr error + mu sync.Mutex + files []*writeLifetimeFile +} + +func (fs *writeLifetimeFS) OpenFile(name string, flag int, perm os.FileMode) (billy.File, error) { + if flag != os.O_RDWR { + return fs.Filesystem.OpenFile(name, flag, perm) + } + if fs.openErr != nil { + return nil, fs.openErr + } + file, err := fs.Filesystem.OpenFile(name, flag, perm) + if err != nil { + return nil, err + } + wrapped := &writeLifetimeFile{File: file, owner: fs} + fs.mu.Lock() + fs.files = append(fs.files, wrapped) + fs.mu.Unlock() + return wrapped, nil +} + +type writeLifetimeFile struct { + billy.File + owner *writeLifetimeFS + closes int32 +} + +func (f *writeLifetimeFile) Seek(offset int64, whence int) (int64, error) { + if f.owner.seekErr != nil { + return 0, f.owner.seekErr + } + return f.File.Seek(offset, whence) +} + +func (f *writeLifetimeFile) Write(data []byte) (int, error) { + if f.owner.writeErr != nil { + // Preserve a real partial write before injecting the backend failure. + n, err := f.File.Write(data[:1]) + if err != nil { + return n, err + } + return n, f.owner.writeErr + } + return f.File.Write(data) +} + +func (f *writeLifetimeFile) Close() error { + atomic.AddInt32(&f.closes, 1) + if err := f.File.Close(); err != nil { + return err + } + // This fault reports an error after the real descriptor has closed; it + // does not imply that every billy implementation closes on an error. + return f.owner.closeErr +} + +func TestOnWriteClosesOwnedFile(t *testing.T) { + for _, tc := range []struct { + name string + openErr, seekErr, writeErr, closeErr error + want nfs.NFSStatus + }{ + {name: "success", want: nfs.NFSStatusOk}, + {name: "open error", openErr: syscall.EACCES, want: nfs.NFSStatusAccess}, + {name: "seek error", seekErr: syscall.EIO, want: nfs.NFSStatusIO}, + {name: "partial write error", writeErr: syscall.EIO, want: nfs.NFSStatusIO}, + {name: "seek and close errors", seekErr: syscall.EIO, closeErr: syscall.ENOSPC, want: nfs.NFSStatusIO}, + {name: "write and close errors", writeErr: syscall.ENOSPC, closeErr: syscall.EIO, want: nfs.NFSStatusNoSPC}, + {name: "close error", closeErr: syscall.EIO, want: nfs.NFSStatusIO}, + } { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, "payload"), []byte("original"), 0600); err != nil { + t.Fatal(err) + } + fs := &writeLifetimeFS{Filesystem: osfs.New(root), openErr: tc.openErr, seekErr: tc.seekErr, writeErr: tc.writeErr, closeErr: tc.closeErr} + target := serveFS(t, fs) + // Retain files for the oracle so GC cannot hide a missed Close. On + // a failing baseline, reclaim only after the assertions below. + t.Cleanup(func() { + fs.mu.Lock() + defer fs.mu.Unlock() + for _, file := range fs.files { + if atomic.LoadInt32(&file.closes) == 0 { + _ = file.Close() + } + } + }) + _, handle, err := target.Lookup("/payload") + if err != nil { + t.Fatal(err) + } + type writeArgs struct { + rpc.Header + Handle []byte + Offset uint64 + Count uint32 + How uint32 + Data []byte + } + data := []byte("updated") + status := callStatus(t, target, nfs.NFSProcedureWrite, &writeArgs{Header: header(nfs.NFSProcedureWrite), Handle: handle, Offset: 1, Count: uint32(len(data)), How: 0, Data: data}) + if status != tc.want { + t.Errorf("WRITE status: got %v, want %v", status, tc.want) + } + fs.mu.Lock() + opened := append([]*writeLifetimeFile(nil), fs.files...) + fs.mu.Unlock() + wantOpened := 1 + if tc.openErr != nil { + wantOpened = 0 + } + if len(opened) != wantOpened { + t.Fatalf("WRITE opened %d files, want %d", len(opened), wantOpened) + } + if tc.openErr == nil && tc.seekErr == nil { + wantData := "oupdated" + if tc.writeErr != nil { + wantData = "ouiginal" + } + actual, err := os.ReadFile(filepath.Join(root, "payload")) + if err != nil || string(actual) != wantData { + t.Errorf("real file contents: got %q, error %v; want %q", actual, err, wantData) + } + } + for _, file := range opened { + if got := atomic.LoadInt32(&file.closes); got != 1 { + t.Errorf("WRITE called Close %d times, want exactly 1", got) + } + if _, err := file.File.Seek(0, io.SeekStart); !errors.Is(err, os.ErrClosed) { + t.Errorf("WRITE left its real file open: Seek error %v, want os.ErrClosed", err) + } + } + }) + } +} From f209314c19cea1282713f4dfbed2d71590acd6b5 Mon Sep 17 00:00:00 2001 From: Pavel Snajdr Date: Sat, 10 Oct 2026 17:51:58 +0000 Subject: [PATCH 2/6] fix(nfs): preserve READ file boundaries and EOF Clamp large reads with unsigned offset comparisons so reads beyond EOF do not underflow into a larger backend buffer. Do not cast unrepresentable protocol offsets into a negative ReaderAt offset. Report known EOF even when a complete or zero-byte backend read returns nil, reuse the post-read attributes, and advertise the actual READ cap in FSINFO. Real-file loopback tests cover Count/Data/EOF, stat/read error statuses, file close, a fixed three-call client bound, and the advertised limit. Normal/race3, all-package normal/race and vet pass; four production omissions fail the intended assertions and restored race3 passes. Correct the existing full-read test to accept complete data with EOF while rejecting short or unrelated-error reads. Old client repeated-read cursor behavior after EOF, concurrent file mutation, minimum-Go runtime and kernel clients are not qualified here. --- nfs_onfsinfo.go | 4 +- nfs_onread.go | 30 +++- nfs_read_boundary_test.go | 326 ++++++++++++++++++++++++++++++++++++++ nfs_test.go | 6 +- 4 files changed, 357 insertions(+), 9 deletions(-) create mode 100644 nfs_read_boundary_test.go diff --git a/nfs_onfsinfo.go b/nfs_onfsinfo.go index 55ec15d..760c0f6 100644 --- a/nfs_onfsinfo.go +++ b/nfs_onfsinfo.go @@ -51,8 +51,8 @@ func onFSInfo(ctx context.Context, w *response, userHandle Handler) error { } res := fsinfores{ - Rtmax: 1 << 30, - Rtpref: 1 << 30, + Rtmax: MaxRead, + Rtpref: MaxRead, Rtmult: 4096, Wtmax: 1 << 30, Wtpref: 1 << 30, diff --git a/nfs_onread.go b/nfs_onread.go index a0dcb9e..b390f61 100644 --- a/nfs_onread.go +++ b/nfs_onread.go @@ -52,28 +52,48 @@ func onRead(ctx context.Context, w *response, userHandle Handler) error { defer fh.Close() resp := nfsReadResponse{} + var fileSize uint64 + haveSize := false if obj.Count > CheckRead { info, err := fs.Stat(fs.Join(path...)) if err != nil { return &NFSStatusError{NFSStatusAccess, err} } - if info.Size()-int64(obj.Offset) < int64(obj.Count) { - obj.Count = uint32(uint64(info.Size()) - obj.Offset) + if info.Size() >= 0 { + fileSize, haveSize = uint64(info.Size()), true + if obj.Offset >= fileSize { + obj.Count = 0 + } else if remaining := fileSize - obj.Offset; remaining < uint64(obj.Count) { + obj.Count = uint32(remaining) + } } } if obj.Count > MaxRead { obj.Count = MaxRead } + // billy.ReaderAt takes a signed offset. A larger protocol offset is + // beyond every representable file size and must not wrap negative. + if obj.Offset > uint64(1<<63-1) { + obj.Count = 0 + resp.EOF = 1 + } resp.Data = make([]byte, obj.Count) // todo: multiple reads if size isn't full - cnt, err := fh.ReadAt(resp.Data, int64(obj.Offset)) + var cnt int + if obj.Count != 0 { + cnt, err = fh.ReadAt(resp.Data, int64(obj.Offset)) + } if err != nil && !errors.Is(err, io.EOF) { return &NFSStatusError{NFSStatusIO, err} } resp.Count = uint32(cnt) resp.Data = resp.Data[:resp.Count] - if errors.Is(err, io.EOF) { + attrs := tryStat(fs, path) + if attrs != nil && attrs.Type == FileTypeRegular { + fileSize, haveSize = attrs.Filesize, true + } + if errors.Is(err, io.EOF) || haveSize && (obj.Offset >= fileSize || uint64(resp.Count) >= fileSize-obj.Offset) { resp.EOF = 1 } @@ -81,7 +101,7 @@ func onRead(ctx context.Context, w *response, userHandle Handler) error { if err := xdr.Write(writer, uint32(NFSStatusOk)); err != nil { return &NFSStatusError{NFSStatusServerFault, err} } - if err := WritePostOpAttrs(writer, tryStat(fs, path)); err != nil { + if err := WritePostOpAttrs(writer, attrs); err != nil { return &NFSStatusError{NFSStatusServerFault, err} } diff --git a/nfs_read_boundary_test.go b/nfs_read_boundary_test.go new file mode 100644 index 0000000..fc8ef8d --- /dev/null +++ b/nfs_read_boundary_test.go @@ -0,0 +1,326 @@ +package nfs_test + +import ( + "bytes" + "context" + "errors" + "io" + "net" + "os" + "path/filepath" + "sync" + "sync/atomic" + "syscall" + "testing" + "time" + + "github.com/go-git/go-billy/v5" + "github.com/go-git/go-billy/v5/osfs" + nfs "github.com/willscott/go-nfs" + nfsc "github.com/willscott/go-nfs-client/nfs" + "github.com/willscott/go-nfs-client/nfs/rpc" + "github.com/willscott/go-nfs-client/nfs/xdr" + "github.com/willscott/go-nfs/helpers" +) + +type readBoundaryFS struct { + billy.Filesystem + mu sync.Mutex + files []*readBoundaryFile + statError, attrError, readError int32 +} + +type readBoundaryFile struct { + billy.File + owner *readBoundaryFS + lengths []int + closed chan struct{} + closeOnce sync.Once + closes int32 +} + +func (fs *readBoundaryFS) Open(name string) (billy.File, error) { + file, err := fs.Filesystem.Open(name) + if err != nil { + return nil, err + } + wrapped := &readBoundaryFile{File: file, owner: fs, closed: make(chan struct{})} + fs.mu.Lock() + fs.files = append(fs.files, wrapped) + fs.mu.Unlock() + return wrapped, nil +} + +func (fs *readBoundaryFS) Stat(name string) (os.FileInfo, error) { + if atomic.LoadInt32(&fs.statError) != 0 { + return nil, syscall.EACCES + } + return fs.Filesystem.Stat(name) +} + +func (fs *readBoundaryFS) Lstat(name string) (os.FileInfo, error) { + if atomic.LoadInt32(&fs.attrError) != 0 { + return nil, syscall.EIO + } + return fs.Filesystem.Lstat(name) +} + +func (f *readBoundaryFile) ReadAt(p []byte, off int64) (int, error) { + f.owner.mu.Lock() + f.lengths = append(f.lengths, len(p)) + f.owner.mu.Unlock() + if atomic.LoadInt32(&f.owner.readError) != 0 { + return 0, syscall.EIO + } + return f.File.ReadAt(p, off) +} + +func (f *readBoundaryFile) Close() error { + atomic.AddInt32(&f.closes, 1) + err := f.File.Close() + f.closeOnce.Do(func() { close(f.closed) }) + return err +} + +type readBoundaryConn struct { + net.Conn + disconnected chan struct{} +} + +type readBoundaryListener struct { + net.Listener + mu sync.Mutex + conns []*readBoundaryConn +} + +func (l *readBoundaryListener) Accept() (net.Conn, error) { + conn, err := l.Listener.Accept() + if err != nil { + return nil, err + } + wrapped := &readBoundaryConn{Conn: conn, disconnected: make(chan struct{})} + l.mu.Lock() + l.conns = append(l.conns, wrapped) + l.mu.Unlock() + return wrapped, nil +} + +func awaitReadBoundary(t *testing.T, done <-chan struct{}, what string) { + t.Helper() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Errorf("%s did not settle", what) + } +} + +func serveReadBoundary(t *testing.T, fs billy.Filesystem) *nfsc.Target { + t.Helper() + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + tracked := &readBoundaryListener{Listener: listener} + ctx, cancel := context.WithCancel(context.Background()) + server := &nfs.Server{ + Handler: helpers.NewCachingHandler(helpers.NewNullAuthHandler(fs), 16), + Context: ctx, + OnDisconnect: func(_ context.Context, conn net.Conn) { + close(conn.(*readBoundaryConn).disconnected) + }, + } + acceptDone := make(chan struct{}) + go func() { defer close(acceptDone); _ = server.Serve(tracked) }() + t.Cleanup(func() { + cancel() + _ = listener.Close() + awaitReadBoundary(t, acceptDone, "server accept loop") + tracked.mu.Lock() + connections := append([]*readBoundaryConn(nil), tracked.conns...) + tracked.mu.Unlock() + for _, conn := range connections { + _ = conn.Close() + awaitReadBoundary(t, conn.disconnected, "server request loop") + } + }) + var client *rpc.Client + for attempt := 0; attempt < 8; attempt++ { + client, err = rpc.DialTCP("tcp", listener.Addr().String(), false) + if err == nil || !errors.Is(err, syscall.EADDRINUSE) { + break + } + } + if err != nil { + t.Fatal(err) + } + t.Cleanup(client.Close) + mount := nfsc.Mount{Client: client} + target, err := mount.Mount("/", rpc.AuthNull) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = mount.Unmount() }) + return target +} + +func TestReadFileBoundary(t *testing.T) { + payload := []byte("payload") + for _, tc := range []struct { + name string + data []byte + offset uint64 + count uint32 + want []byte + wantEOF uint32 + wantStatus nfs.NFSStatus + statError, attrError, readError bool + }{ + {name: "large tail", data: payload, offset: 1, count: nfs.CheckRead + 1, want: payload[1:], wantEOF: 1}, + {name: "at EOF", data: payload, offset: uint64(len(payload)), count: nfs.CheckRead + 1, wantEOF: 1}, + {name: "beyond EOF", data: payload, offset: uint64(len(payload) + 1), count: nfs.CheckRead + 1, wantEOF: 1}, + {name: "empty", count: nfs.CheckRead + 1, wantEOF: 1}, + {name: "post attributes unavailable", data: payload, count: nfs.CheckRead + 1, want: payload, wantEOF: 1, attrError: true}, + {name: "stat error", data: payload, count: nfs.CheckRead + 1, wantStatus: nfs.NFSStatusAccess, statError: true}, + {name: "read error", data: payload, count: 2, wantStatus: nfs.NFSStatusIO, readError: true}, + {name: "small short read", data: payload, offset: 1, count: 10, want: payload[1:], wantEOF: 1}, + {name: "within file", data: payload, offset: 1, count: 2, want: payload[1:3]}, + {name: "exact file size", data: payload, count: uint32(len(payload)), want: payload, wantEOF: 1}, + {name: "zero before EOF", data: payload}, + {name: "zero at EOF", data: payload, offset: uint64(len(payload)), wantEOF: 1}, + {name: "offset outside signed range", data: payload, offset: uint64(1 << 63), count: 1, wantEOF: 1}, + {name: "maximum offset", data: payload, offset: ^uint64(0), count: nfs.CheckRead + 1, wantEOF: 1}, + } { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, "payload"), tc.data, 0600); err != nil { + t.Fatal(err) + } + fs := &readBoundaryFS{Filesystem: osfs.New(root)} + target := serveReadBoundary(t, fs) + _, handle, err := target.Lookup("/payload") + if err != nil { + t.Fatal(err) + } + if tc.statError { + atomic.StoreInt32(&fs.statError, 1) + } + if tc.attrError { + atomic.StoreInt32(&fs.attrError, 1) + } + if tc.readError { + atomic.StoreInt32(&fs.readError, 1) + } + res, err := target.Call(&struct { + rpc.Header + Handle []byte + Offset uint64 + Count uint32 + }{header(nfs.NFSProcedureRead), handle, tc.offset, tc.count}) + if err != nil { + t.Fatal(err) + } + var status uint32 + var result struct { + Attr nfsc.PostOpAttr + Count uint32 + EOF uint32 + } + if err := xdr.Read(res, &status); err != nil { + t.Fatal(err) + } + if status != uint32(tc.wantStatus) { + t.Errorf("READ status must match: got %d want %d", status, tc.wantStatus) + } + if status == uint32(nfs.NFSStatusOk) { + if err := xdr.Read(res, &result); err != nil { + t.Fatal(err) + } + dataLength, err := xdr.ReadUint32(res) + if err != nil || dataLength > tc.count { + t.Fatalf("READ opaque length=%d request=%d err=%v", dataLength, tc.count, err) + } + data := make([]byte, dataLength) + if dataLength != 0 { + if _, err := io.ReadFull(res, data); err != nil { + t.Fatal(err) + } + } + if result.Count != uint32(len(data)) || !bytes.Equal(data, tc.want) { + t.Errorf("READ Count/Data disagreement: count=%d data=%q want=%q", result.Count, data, tc.want) + } + if result.EOF != tc.wantEOF { + t.Errorf("READ must truthfully report EOF: got %d want %d", result.EOF, tc.wantEOF) + } + } + fs.mu.Lock() + files := append([]*readBoundaryFile(nil), fs.files...) + fs.mu.Unlock() + if len(files) != 1 { + t.Fatalf("READ must own one real file, got %d", len(files)) + } + awaitReadBoundary(t, files[0].closed, "READ file Close") + if atomic.LoadInt32(&files[0].closes) != 1 { + t.Errorf("READ must close its real file exactly once") + } + fs.mu.Lock() + for _, length := range files[0].lengths { + if uint64(length) > uint64(tc.count) || (tc.offset >= uint64(len(tc.data)) && tc.count > nfs.CheckRead && length != 0) { + t.Errorf("READ allocation must not grow past the request at EOF: backend length=%d request=%d", length, tc.count) + } + } + fs.mu.Unlock() + }) + } +} + +func TestReadClientReachesEOF(t *testing.T) { + root := t.TempDir() + payload := []byte("payload") + if err := os.WriteFile(filepath.Join(root, "payload"), payload, 0600); err != nil { + t.Fatal(err) + } + target := serveReadBoundary(t, osfs.New(root)) + file, err := target.Open("/payload") + if err != nil { + t.Fatal(err) + } + buf := make([]byte, nfs.CheckRead+1) + var data []byte + reachedEOF := false + zeroProgress := 0 + reads := 0 + // Fixed three-call bound: never start an unbounded copy/read loop. + for attempt := 0; attempt < 3; attempt++ { + n, err := file.Read(buf) + reads++ + data = append(data, buf[:n]...) + if errors.Is(err, io.EOF) { + reachedEOF = true + break + } + if err != nil { + t.Fatal(err) + } + if n == 0 { + zeroProgress++ + } + } + t.Logf("bounded real client: reads=%d payload=%d EOF=%v zero-progress=%d", reads, len(data), reachedEOF, zeroProgress) + if reads != 1 { + t.Errorf("real client must receive terminal EOF with the payload READ: got %d calls", reads) + } + if !bytes.Equal(data, payload) || !reachedEOF || zeroProgress != 0 { + t.Errorf("real client must reach EOF without zero progress: data=%q EOF=%v zero-progress=%d", data, reachedEOF, zeroProgress) + } +} + +func TestReadAdvertisedLimit(t *testing.T) { + target := serveReadBoundary(t, osfs.New(t.TempDir())) + info, err := target.FSInfo() + if err != nil { + t.Fatal(err) + } + if info.RTMax != nfs.MaxRead || info.RTPref > nfs.MaxRead { + t.Errorf("FSINFO must advertise the actual READ limit: max=%d pref=%d enforced=%d", info.RTMax, info.RTPref, nfs.MaxRead) + } +} diff --git a/nfs_test.go b/nfs_test.go index 833eba2..7b374e7 100644 --- a/nfs_test.go +++ b/nfs_test.go @@ -2,7 +2,9 @@ package nfs_test import ( "bytes" + "errors" "fmt" + "io" "math/rand" "net" "os" @@ -172,8 +174,8 @@ func TestNFS(t *testing.T) { } defer mf.Close() buf := make([]byte, len(b)) - if _, err = mf.Read(buf[:]); err != nil { - t.Fatal(err) + if n, err := mf.Read(buf[:]); n != len(b) || err != nil && !errors.Is(err, io.EOF) { + t.Fatalf("READ count=%d want=%d err=%v", n, len(b), err) } if !bytes.Equal(buf, b) { t.Fatal("written does not match expected") From cc5dba21129ad4cf4c99e123c1062cd40e680263 Mon Sep 17 00:00:00 2001 From: Pavel Snajdr Date: Sat, 10 Oct 2026 19:21:53 +0000 Subject: [PATCH 3/6] fix(nfs): qualify terminal reads with the corrected client A terminal NFS READ can return data with EOF. The old client's sequential Read kept its previous cursor on that result, replaying consumed bytes on a later read and rereading the old prefix after subsequent file growth. Select the published correction in willscott/go-nfs-client#6 for this module's ordinary build/test path. Add real filesystem/TCP server-client regressions for zero and nonzero initial positions, cursor advancement, next-read no-replay, sequential append after EOF, and position-independent ReadAt. Reuse the existing server fixture instead of a separate wire implementation. Focused normal/race count3, all-package normal/race, vet and module tidying pass. Restoring the old native client reproduces six cursor, six replay and six growth failures across count3; restored race count3 passes. One earlier full-normal run encountered an existing WRITE fixture source-port collision, retained separately. The temporary public-fork replacement applies only when this repository is the main module. Downstream consumers must select the corrected client explicitly; upstream default-branch landing and the canonical pin remain separate delivery obligations. No minimum-Go execution, kernel-client, concurrent-mutation, aggregate-pressure or release qualification is implied. --- go.mod | 3 ++ go.sum | 4 +- nfs_read_client_test.go | 85 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 nfs_read_client_test.go diff --git a/go.mod b/go.mod index 2776eb9..14f7f6d 100644 --- a/go.mod +++ b/go.mod @@ -19,3 +19,6 @@ require ( github.com/polydawn/rio v0.0.0-20220823181337-7c31ad9831a4 // indirect github.com/warpfork/go-errcat v0.0.0-20180917083543-335044ffc86e // indirect ) + +// Preserve terminal READ cursor progress until the client fix is available upstream. +replace github.com/willscott/go-nfs-client => github.com/snajpa/go-nfs-client v0.0.0-20261010180736-6436a64df37a diff --git a/go.sum b/go.sum index 4ccc8db..59727d3 100644 --- a/go.sum +++ b/go.sum @@ -25,12 +25,12 @@ github.com/rasky/go-xdr v0.0.0-20170124162913-1a41d1a06c93/go.mod h1:Nfe4efndBz4 github.com/rogpeppe/go-internal v1.11.0 h1:cWPaGQEPrBb5/AsnsZesgZZ9yb1OQ+GOISoDNXVBh4M= github.com/smartystreets/assertions v1.2.0 h1:42S6lae5dvLc7BrLu/0ugRtcFVjoJNMC/N3yZFZkDFs= github.com/smartystreets/goconvey v1.7.2 h1:9RBaZCeXEQ3UselpuwUQHltGVXvdwm6cv1hgR6gDIPg= +github.com/snajpa/go-nfs-client v0.0.0-20261010180736-6436a64df37a h1:ksGJkAfNUhqZATloV9tlTik6+xx/Lg3e+txpko5RMX0= +github.com/snajpa/go-nfs-client v0.0.0-20261010180736-6436a64df37a/go.mod h1:Tq++Lr/FgiS3X48q5FETemXiSLGuYMQT2sPjYNPJSwA= github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= github.com/warpfork/go-errcat v0.0.0-20180917083543-335044ffc86e h1:FIB2fi7XJGHIdf5rWNsfFQqatIKxutT45G+wNuMQNgs= github.com/warpfork/go-errcat v0.0.0-20180917083543-335044ffc86e/go.mod h1:/qe02xr3jvTUz8u/PV0FHGpP8t96OQNP7U9BJMwMLEw= github.com/warpfork/go-wish v0.0.0-20200122115046-b9ea61034e4a h1:G++j5e0OC488te356JvdhaM8YS6nMsjLAYF7JxCv07w= -github.com/willscott/go-nfs-client v0.0.0-20240104095149-b44639837b00 h1:U0DnHRZFzoIV1oFEZczg5XyPut9yxk9jjtax/9Bxr/o= -github.com/willscott/go-nfs-client v0.0.0-20240104095149-b44639837b00/go.mod h1:Tq++Lr/FgiS3X48q5FETemXiSLGuYMQT2sPjYNPJSwA= github.com/willscott/memphis v0.0.0-20241203204924-a148a489d367 h1:A9hsyc7kKeultwdUhS99FVq2S4xT6QVZqOEptPGjHpM= github.com/willscott/memphis v0.0.0-20241203204924-a148a489d367/go.mod h1:mAQkn9EwN7WZdbH1DnV+9Nmr3oMjPbG4a0zDM2yI2iA= golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 h1:2dVuKD2vS7b0QIHQbpyTISPd0LeHDbnYEryqj5Q1ug8= diff --git a/nfs_read_client_test.go b/nfs_read_client_test.go new file mode 100644 index 0000000..50a7923 --- /dev/null +++ b/nfs_read_client_test.go @@ -0,0 +1,85 @@ +package nfs_test + +import ( + "bytes" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "testing" + + "github.com/go-git/go-billy/v5/osfs" + nfs "github.com/willscott/go-nfs" +) + +func TestRealReadClientTerminalCursor(t *testing.T) { + for _, prefix := range []int{0, 3} { + t.Run(fmt.Sprintf("prefix_%d", prefix), func(t *testing.T) { + root := t.TempDir() + name := filepath.Join(root, "payload") + payload := []byte("payload") + if err := os.WriteFile(name, payload, 0600); err != nil { + t.Fatal(err) + } + target := serveReadBoundary(t, osfs.New(root)) + file, err := target.Open("/payload") + if err != nil { + t.Fatal(err) + } + if prefix > 0 { + b := make([]byte, prefix) + if n, err := file.Read(b); n != prefix || err != nil || !bytes.Equal(b, payload[:prefix]) { + t.Fatalf("prefix read n=%d err=%v data=%q", n, err, b) + } + } + buf := make([]byte, nfs.CheckRead+1) + n, err := file.Read(buf) + if n != len(payload)-prefix || !errors.Is(err, io.EOF) || !bytes.Equal(buf[:n], payload[prefix:]) { + t.Fatalf("terminal read n=%d err=%v data=%q", n, err, buf[:n]) + } + if pos, err := file.Seek(0, io.SeekCurrent); pos != int64(len(payload)) || err != nil { + t.Errorf("actual terminal cursor must advance: pos=%d want=%d err=%v", pos, len(payload), err) + } + if n, err := file.Read(buf); n != 0 || !errors.Is(err, io.EOF) { + t.Errorf("actual next READ must not repeat payload: n=%d err=%v", n, err) + } + appended := []byte("appended") + writer, err := os.OpenFile(name, os.O_WRONLY|os.O_APPEND, 0600) + if err != nil { + t.Fatal(err) + } + _, writeErr := writer.Write(appended) + closeErr := writer.Close() + if writeErr != nil || closeErr != nil { + t.Fatalf("append write=%v close=%v", writeErr, closeErr) + } + if n, err := file.Read(buf); n != len(appended) || !errors.Is(err, io.EOF) || !bytes.Equal(buf[:n], appended) { + t.Errorf("READ after file growth must start at previous end: n=%d err=%v data=%q", n, err, buf[:n]) + } + }) + } +} + +func TestRealReadAtCursorUnaffected(t *testing.T) { + root := t.TempDir() + payload := []byte("payload") + if err := os.WriteFile(filepath.Join(root, "payload"), payload, 0600); err != nil { + t.Fatal(err) + } + target := serveReadBoundary(t, osfs.New(root)) + file, err := target.Open("/payload") + if err != nil { + t.Fatal(err) + } + buf := make([]byte, nfs.CheckRead+1) + if n, err := file.ReadAt(buf, 0); n != 7 || !errors.Is(err, io.EOF) || !bytes.Equal(buf[:n], payload) { + t.Fatalf("ReadAt n=%d err=%v", n, err) + } + if pos, err := file.Seek(0, io.SeekCurrent); pos != 0 || err != nil { + t.Errorf("ReadAt must not move cursor: pos=%d err=%v", pos, err) + } + if n, err := file.Read(buf); n != 7 || !errors.Is(err, io.EOF) || !bytes.Equal(buf[:n], payload) { + t.Fatalf("Read after ReadAt n=%d err=%v", n, err) + } +} From 1e9c53c45e2b3fbd0331d9c30e33a21f8856d9a9 Mon Sep 17 00:00:00 2001 From: Pavel Snajdr Date: Sun, 11 Oct 2026 01:38:21 +0000 Subject: [PATCH 4/6] test(nfs): initialize client logging before RPC receivers Verbose root-package tests can race in the NFS client logger. The READ tests start RPC receive goroutines; a later TestNFS turns debugging on by writing the logger's unsynchronized flag. Closing the connection does not join that reader, so changing the flag in a later test is unsafe even without parallel tests. Parse test flags and configure verbose client logging in TestMain before any test starts. Production NFS code and default nonverbose logging remain unchanged. The Go 1.19.13 root race run fails at the logger update before this change and passes afterwards. The root suite also passes normal and race runs on Go 1.26.8. A repeated shuffled race run reported no logger race but hit existing random source-port bind collisions, which this fixture correction does not address. --- nfs_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/nfs_test.go b/nfs_test.go index 7b374e7..5c55b2b 100644 --- a/nfs_test.go +++ b/nfs_test.go @@ -3,6 +3,7 @@ package nfs_test import ( "bytes" "errors" + "flag" "fmt" "io" "math/rand" @@ -93,10 +94,17 @@ func (f *trackingFile) Close() error { return f.File.Close() } -func TestNFS(t *testing.T) { +// Configure the client logger before any test starts its RPC receiver. +// Changing its unsynchronized debug flag in TestNFS races with earlier readers. +func TestMain(m *testing.M) { + flag.Parse() if testing.Verbose() { util.DefaultLogger.SetDebug(true) } + os.Exit(m.Run()) +} + +func TestNFS(t *testing.T) { // make an empty in-memory server. listener, err := net.Listen("tcp", "localhost:0") From 3a68f3e147521ce735f9b8f56af2cfbb8ee32bb5 Mon Sep 17 00:00:00 2001 From: Pavel Snajdr Date: Thu, 15 Oct 2026 01:55:39 +0200 Subject: [PATCH 5/6] fix(nfs): compare mounted filesystem identity for rename A filesystem wrapper can carry a different request context for each handle lookup. Deep comparison of those wrappers can reject RENAME on the same mount and read mutable filesystem fields concurrently. Unwrap middleware before comparing filesystem identity. Accept only non-nil pointers of the same type that name the same instance; reject value implementations rather than comparing interface fields that may contain non-comparable values. Keep the existing TestMain logger setup before RPC receivers start. Add rename controls for nested wrappers, distinct mounts, typed nils, non-comparable interface fields and pointer-backed filesystem objects. These controls have been source-inspected and formatted, not compiled or executed for this draft. Normal/race and actual consumer verification remain required; this does not establish production readiness. --- nfs_onrename.go | 29 +++++- nfs_rename_identity_test.go | 176 ++++++++++++++++++++++++++++++++++++ 2 files changed, 203 insertions(+), 2 deletions(-) create mode 100644 nfs_rename_identity_test.go diff --git a/nfs_onrename.go b/nfs_onrename.go index cb96793..6a4d3d5 100644 --- a/nfs_onrename.go +++ b/nfs_onrename.go @@ -32,8 +32,10 @@ func onRename(ctx context.Context, w *response, userHandle Handler) error { if err != nil { return &NFSStatusError{NFSStatusStale, err} } - // check the two fs are the same - if !reflect.DeepEqual(fs, fs2) { + // Middleware can attach a different request context to each lookup. Only + // the mounted filesystem's identity determines whether RENAME is allowed; + // comparing wrapper contents also races mutable filesystem state. + if !sameFilesystem(fs, fs2) { return &NFSStatusError{NFSStatusNotSupp, os.ErrPermission} } @@ -102,3 +104,26 @@ func onRename(ctx context.Context, w *response, userHandle Handler) error { } return nil } + +func sameFilesystem(left, right billy.Filesystem) bool { + unwrap := func(fs billy.Filesystem) billy.Filesystem { + for { + value := reflect.ValueOf(fs) + if !value.IsValid() || (value.Kind() == reflect.Ptr && value.IsNil()) { + return nil + } + wrapper, ok := fs.(interface{ Unwrap() billy.Filesystem }) + if !ok { + return fs + } + fs = wrapper.Unwrap() + } + } + left, right = unwrap(left), unwrap(right) + typ := reflect.TypeOf(left) + // Only nonnil pointer instances have object identity we can prove without + // reading mutable fields. A struct's type can be comparable while one of + // its interface fields contains a non-comparable value, so type-level + // comparability alone does not make interface equality safe. + return typ != nil && typ == reflect.TypeOf(right) && typ.Kind() == reflect.Ptr && left == right +} diff --git a/nfs_rename_identity_test.go b/nfs_rename_identity_test.go new file mode 100644 index 0000000..1dfd0d0 --- /dev/null +++ b/nfs_rename_identity_test.go @@ -0,0 +1,176 @@ +package nfs + +import ( + "bytes" + "context" + "errors" + "io" + "os" + "testing" + + "github.com/go-git/go-billy/v5" + "github.com/go-git/go-billy/v5/memfs" + "github.com/willscott/go-nfs-client/nfs/xdr" +) + +type renameRequestFS struct { + billy.Filesystem + ctx context.Context +} + +func (fs *renameRequestFS) Unwrap() billy.Filesystem { return fs.Filesystem } + +type renameIdentityHandler struct { + Handler + from, to billy.Filesystem + invalidated bool +} + +func (h *renameIdentityHandler) FromHandle(_ context.Context, handle []byte) (billy.Filesystem, []string, error) { + if bytes.Equal(handle, []byte{1}) { + return h.from, []string{"from"}, nil + } + return h.to, []string{"to"}, nil +} + +func (h *renameIdentityHandler) ToHandle(context.Context, billy.Filesystem, []string) []byte { + return []byte{3} +} + +func (h *renameIdentityHandler) InvalidateHandle(context.Context, billy.Filesystem, []byte) error { + h.invalidated = true + return nil +} + +func TestRenameUsesMountedFilesystemIdentity(t *testing.T) { + for _, differentMount := range []bool{false, true} { + name := "request-wrappers" + if differentMount { + name = "different-mounts-with-same-root" + } + t.Run(name, func(t *testing.T) { + left := memfs.New() + right := left + if differentMount { + right = memfs.New() + } + if left.Root() != right.Root() { + t.Fatal("test requires mounts with the same root string") + } + if err := left.MkdirAll("from", 0755); err != nil { + t.Fatal(err) + } + if err := right.MkdirAll("to", 0755); err != nil { + t.Fatal(err) + } + file, err := left.Create("from/source") + if err != nil { + t.Fatal(err) + } + _, writeErr := file.Write([]byte("preserved data")) + closeErr := file.Close() + if writeErr != nil || closeErr != nil { + t.Fatalf("write source: %v; close: %v", writeErr, closeErr) + } + oldCtx, cancel := context.WithCancel(context.Background()) + cancel() + handler := &renameIdentityHandler{ + from: &renameRequestFS{Filesystem: left, ctx: oldCtx}, + to: &renameRequestFS{Filesystem: &renameRequestFS{Filesystem: right, ctx: context.Background()}, ctx: context.Background()}, + } + body := new(bytes.Buffer) + for _, arg := range []DirOpArg{{Handle: []byte{1}, Filename: []byte("source")}, {Handle: []byte{2}, Filename: []byte("destination")}} { + if err := xdr.Write(body, arg); err != nil { + t.Fatal(err) + } + } + writer := new(bytes.Buffer) + err = onRename(context.Background(), &response{req: &request{Body: body}, writer: writer, responded: true}, handler) + if differentMount { + var status *NFSStatusError + if !errors.As(err, &status) || status.NFSStatus != NFSStatusNotSupp || !errors.Is(err, os.ErrPermission) { + t.Fatalf("cross-mount RENAME = %v, want NFS3ERR_NOTSUPP", err) + } + if handler.invalidated { + t.Fatal("rejected rename invalidated a handle") + } + if _, err := left.Stat("from/source"); err != nil { + t.Fatal("rejected rename changed the source:", err) + } + if _, err := right.Stat("to/destination"); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("rejected rename created a destination: %v", err) + } + return + } + if err != nil { + t.Fatal("same-mount rename through different request wrappers:", err) + } + var status uint32 + if err := xdr.Read(writer, &status); err != nil || status != uint32(NFSStatusOk) { + t.Fatalf("rename response = %d, %v", status, err) + } + if !handler.invalidated { + t.Fatal("successful rename did not invalidate the old handle") + } + if _, err := left.Stat("from/source"); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("source still exists after rename: %v", err) + } + file, err = right.Open("to/destination") + if err != nil { + t.Fatal(err) + } + data, readErr := io.ReadAll(file) + closeErr = file.Close() + if readErr != nil || closeErr != nil { + t.Fatalf("read destination: %v; close: %v", readErr, closeErr) + } + if string(data) != "preserved data" { + t.Fatalf("renamed data = %q", data) + } + }) + } +} + +// A slice-valued filesystem cannot be compared for object identity, even when +// two values embed the same backend. Do not inspect its mutable contents. +type nonComparableRenameFS struct { + billy.Filesystem + markers []int +} + +// The struct type is comparable, but its interface can contain a value that +// is not. Type.Comparable alone does not make interface equality panic-safe. +type comparableRenameFS struct { + billy.Filesystem +} + +func TestFilesystemIdentityRejectsUnprovableIdentity(t *testing.T) { + fs := nonComparableRenameFS{Filesystem: memfs.New(), markers: []int{1}} + if sameFilesystem(fs, fs) { + t.Fatal("non-comparable filesystem was accepted by content equality") + } + if sameFilesystem(nil, nil) { + t.Fatal("nil filesystems have no mounted identity") + } + valueWithInterface := comparableRenameFS{Filesystem: fs} + if sameFilesystem(valueWithInterface, valueWithInterface) { + t.Fatal("comparable value with a non-comparable interface has no provable object identity") + } + comparableValue := comparableRenameFS{Filesystem: memfs.New()} + if sameFilesystem(comparableValue, comparableValue) { + t.Fatal("value equality does not establish mounted object identity") + } + var nilWrapper *renameRequestFS + if sameFilesystem(nilWrapper, nilWrapper) { + t.Fatal("typed nil wrapper has no mounted identity") + } + if sameFilesystem(&renameRequestFS{Filesystem: nilWrapper}, nilWrapper) { + t.Fatal("nested typed nil wrapper has no mounted identity") + } + // Pointer identity remains safe even if the pointed-to object's fields + // include an interface with a slice-valued dynamic filesystem. + object := &comparableRenameFS{Filesystem: fs} + if !sameFilesystem(object, object) { + t.Fatal("the exact same nonnil mounted object must retain its identity") + } +} From dd0fe7872adbdf83d14638a4390ecf7d34a2126f Mon Sep 17 00:00:00 2001 From: Pavel Snajdr Date: Mon, 19 Oct 2026 16:34:32 +0000 Subject: [PATCH 6/6] test(nfs): unwrap the rename errno filesystem wrapper The rename errno table wraps the mounted filesystem to inject LinkError values. The mounted-filesystem identity check rejects this value wrapper before Rename runs because the wrapper does not expose its filesystem. All nine cases consequently return NOTSUPP instead of the injected errno. Add Unwrap to the test wrapper so identity checks reach the same embedded mount while Rename still calls the wrapper's error-injection method. Production code and every existing errno assertion remain unchanged. The nine errno cases fail on the preceding head and pass with this change. With Go 1.26.9 on Linux/amd64, go test ./... and go test -race ./... each pass all 89 tests without skips, including the TCP client/server tests. go vet ./... and go mod verify also pass. These checks do not establish kernel-mounted NFS or downstream integration; current hosted CI and human review remain required. Reverting this test-only commit restores the previous wrapper and its reproducible failures. --- nfs_errors_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nfs_errors_test.go b/nfs_errors_test.go index 3d1611e..28ea405 100644 --- a/nfs_errors_test.go +++ b/nfs_errors_test.go @@ -187,6 +187,8 @@ func (r renameErrFS) Rename(oldpath, newpath string) error { return r.Filesystem.Rename(oldpath, newpath) } +func (r renameErrFS) Unwrap() billy.Filesystem { return r.Filesystem } + // TestOnRenameStatusFromErrno checks that each errno a rename can report // reaches the client as the status RFC 1813 gives RENAME for it, rather than // as an NFS3ERR_IO the client can only pass on as EIO.