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_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. 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_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_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_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_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) + } +} 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") + } +} diff --git a/nfs_test.go b/nfs_test.go index 833eba2..5c55b2b 100644 --- a/nfs_test.go +++ b/nfs_test.go @@ -2,7 +2,10 @@ package nfs_test import ( "bytes" + "errors" + "flag" "fmt" + "io" "math/rand" "net" "os" @@ -91,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") @@ -172,8 +182,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") 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) + } + } + }) + } +}