From 7070f5a286f9bbec2a3b38f97c4a24e950816606 Mon Sep 17 00:00:00 2001
From: "Ariel Shaqed (Scolnicov)" <ariel.shaqed@gmail.com>
Date: Thu, 6 Feb 2025 12:45:01 +0200
Subject: [PATCH] Fix file handle leak in read (#139)

* [bug] Close file when reading
* Test close-on-read fix
---
 nfs_onread.go |  1 +
 nfs_test.go   | 93 +++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/nfs_onread.go b/nfs_onread.go
index 23b3e42..b34949e 100644
--- a/nfs_onread.go
+++ b/nfs_onread.go
@@ -49,6 +49,7 @@ func onRead(ctx context.Context, w *response, userHandle Handler) error {
 		}
 		return &NFSStatusError{NFSStatusAccess, err}
 	}
+	defer fh.Close()
 
 	resp := nfsReadResponse{}
 
diff --git a/nfs_test.go b/nfs_test.go
index 00177ca..833eba2 100644
--- a/nfs_test.go
+++ b/nfs_test.go
@@ -3,11 +3,15 @@ package nfs_test
 import (
 	"bytes"
 	"fmt"
+	"math/rand"
 	"net"
+	"os"
 	"reflect"
 	"sort"
+	"sync"
 	"testing"
 
+	"github.com/go-git/go-billy/v5"
 	nfs "github.com/willscott/go-nfs"
 	"github.com/willscott/go-nfs/helpers"
 	"github.com/willscott/go-nfs/helpers/memfs"
@@ -18,6 +22,75 @@ import (
 	"github.com/willscott/go-nfs-client/nfs/xdr"
 )
 
+type OpenArgs struct {
+	File string
+	Flag int
+	Perm os.FileMode
+}
+
+func (o *OpenArgs) String() string {
+	return fmt.Sprintf("\"%s\"; %05xd %s", o.File, o.Flag, o.Perm)
+}
+
+// NewTrackingFS wraps fs to detect file handle leaks.
+func NewTrackingFS(fs billy.Filesystem) *trackingFS {
+	return &trackingFS{Filesystem: fs, open: make(map[int64]OpenArgs)}
+}
+
+// trackingFS wraps a Filesystem to detect file handle leaks.
+type trackingFS struct {
+	billy.Filesystem
+	mu   sync.Mutex
+	open map[int64]OpenArgs
+}
+
+func (t *trackingFS) ListOpened() []OpenArgs {
+	t.mu.Lock()
+	defer t.mu.Unlock()
+	ret := make([]OpenArgs, 0, len(t.open))
+	for _, o := range t.open {
+		ret = append(ret, o)
+	}
+	return ret
+}
+
+func (t *trackingFS) Create(filename string) (billy.File, error) {
+	return t.OpenFile(filename, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0666)
+}
+
+func (t *trackingFS) Open(filename string) (billy.File, error) {
+	return t.OpenFile(filename, os.O_RDONLY, 0)
+}
+
+func (t *trackingFS) OpenFile(filename string, flag int, perm os.FileMode) (billy.File, error) {
+	open, err := t.Filesystem.OpenFile(filename, flag, perm)
+	if err != nil {
+		return nil, err
+	}
+	t.mu.Lock()
+	defer t.mu.Unlock()
+	id := rand.Int63()
+	t.open[id] = OpenArgs{filename, flag, perm}
+	closer := func() {
+		delete(t.open, id)
+	}
+	open = &trackingFile{
+		File:    open,
+		onClose: closer,
+	}
+	return open, err
+}
+
+type trackingFile struct {
+	billy.File
+	onClose func()
+}
+
+func (f *trackingFile) Close() error {
+	f.onClose()
+	return f.File.Close()
+}
+
 func TestNFS(t *testing.T) {
 	if testing.Verbose() {
 		util.DefaultLogger.SetDebug(true)
@@ -29,9 +102,17 @@ func TestNFS(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	mem := memfs.New()
+	mem := NewTrackingFS(memfs.New())
+
+	defer func() {
+		if opened := mem.ListOpened(); len(opened) > 0 {
+			t.Errorf("Unclosed files: %v", opened)
+		}
+	}()
+
 	// File needs to exist in the root for memfs to acknowledge the root exists.
-	_, _ = mem.Create("/test")
+	r, _ := mem.Create("/test")
+	r.Close()
 
 	handler := helpers.NewNullAuthHandler(mem)
 	cacheHelper := helpers.NewCachingHandler(handler, 1024)
@@ -78,12 +159,18 @@ func TestNFS(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer f.Close()
 	b := []byte("hello world")
 	_, err = f.Write(b)
 	if err != nil {
 		t.Fatal(err)
 	}
-	mf, _ := mem.Open("/helloworld.txt")
+
+	mf, err := target.Open("/helloworld.txt")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer mf.Close()
 	buf := make([]byte, len(b))
 	if _, err = mf.Read(buf[:]); err != nil {
 		t.Fatal(err)