fix resource leak in os.(*File).Close

Signed-off-by: Achille Roussel <[email protected]>

11 files changed, 168 insertions, 40 deletions

diff --git a/src/os/dir_darwin.go b/src/os/dir_darwin.go
index 0578d0b67..3883a45c6 100644
--- a/src/os/dir_darwin.go
+++ b/src/os/dir_darwin.go
@@ -147,7 +147,7 @@ func darwinOpenDir(fd syscallFd) (uintptr, string, error) {
 	return dir, "", nil
 }
 
-// Implemented in syscall/syscall_darwin.go.
+// Implemented in syscall/syscall_libc_darwin_*.go.
 
 //go:linkname closedir syscall.closedir
 func closedir(dir uintptr) (err error)
diff --git a/src/os/dir_other.go b/src/os/dir_other.go
index ddf28c6b5..5b83ca441 100644
--- a/src/os/dir_other.go
+++ b/src/os/dir_other.go
@@ -13,6 +13,9 @@ import (
 type dirInfo struct {
 }
 
+func (*dirInfo) close() {
+}
+
 func (f *File) readdir(n int, mode readdirMode) (names []string, dirents []DirEntry, infos []FileInfo, err error) {
 	return nil, nil, nil, &PathError{Op: "readdir unimplemented", Err: syscall.ENOTDIR}
 }
diff --git a/src/os/dir_unix.go b/src/os/dir_unix.go
index a23f00e4c..f290ea38e 100644
--- a/src/os/dir_unix.go
+++ b/src/os/dir_unix.go
@@ -8,43 +8,29 @@ package os
 
 import (
 	"io"
-	"sync"
 	"syscall"
 	"unsafe"
 )
 
 // Auxiliary information if the File describes a directory
 type dirInfo struct {
-	buf  *[]byte // buffer for directory I/O
-	nbuf int     // length of buf; return value from Getdirentries
-	bufp int     // location of next record in buf.
+	nbuf int             // length of buf; return value from Getdirentries
+	bufp int             // location of next record in buf.
+	buf  [blockSize]byte // buffer for directory I/O
 }
 
 const (
 	// More than 5760 to work around https://golang.org/issue/24015.
-	blockSize = 8192
+	blockSize = 8192 - 2*unsafe.Sizeof(int(0))
 )
 
-var dirBufPool = sync.Pool{
-	New: func() interface{} {
-		// The buffer must be at least a block long.
-		buf := make([]byte, blockSize)
-		return &buf
-	},
-}
-
 func (d *dirInfo) close() {
-	if d.buf != nil {
-		dirBufPool.Put(d.buf)
-		d.buf = nil
-	}
 }
 
 func (f *File) readdir(n int, mode readdirMode) (names []string, dirents []DirEntry, infos []FileInfo, err error) {
 	// If this file has no dirinfo, create one.
 	if f.dirinfo == nil {
 		f.dirinfo = new(dirInfo)
-		f.dirinfo.buf = dirBufPool.Get().(*[]byte)
 	}
 	d := f.dirinfo
 
@@ -66,7 +52,7 @@ func (f *File) readdir(n int, mode readdirMode) (names []string, dirents []DirEn
 		if d.bufp >= d.nbuf {
 			d.bufp = 0
 			var errno error
-			d.nbuf, errno = syscall.ReadDirent(syscallFd(f.handle.(unixFileHandle)), *d.buf)
+			d.nbuf, errno = syscall.ReadDirent(syscallFd(f.handle.(unixFileHandle)), d.buf[:])
 			if d.nbuf < 0 {
 				errno = handleSyscallError(errno)
 			}
@@ -79,7 +65,7 @@ func (f *File) readdir(n int, mode readdirMode) (names []string, dirents []DirEn
 		}
 
 		// Drain the buffer
-		buf := (*d.buf)[d.bufp:d.nbuf]
+		buf := d.buf[d.bufp:d.nbuf]
 		reclen, ok := direntReclen(buf)
 		if !ok || reclen > uint64(len(buf)) {
 			break
diff --git a/src/os/file.go b/src/os/file.go
index 359d82b1b..f26bf3cea 100644
--- a/src/os/file.go
+++ b/src/os/file.go
@@ -42,11 +42,11 @@ var lstat = Lstat
 func Mkdir(path string, perm FileMode) error {
 	fs, suffix := findMount(path)
 	if fs == nil {
-		return &PathError{"mkdir", path, ErrNotExist}
+		return &PathError{Op: "mkdir", Path: path, Err: ErrNotExist}
 	}
 	err := fs.Mkdir(suffix, perm)
 	if err != nil {
-		return &PathError{"mkdir", path, err}
+		return &PathError{Op: "mkdir", Path: path, Err: err}
 	}
 	return nil
 }
@@ -65,7 +65,7 @@ func fixCount(n int, err error) (int, error) {
 func Remove(path string) error {
 	fs, suffix := findMount(path)
 	if fs == nil {
-		return &PathError{"remove", path, ErrNotExist}
+		return &PathError{Op: "remove", Path: path, Err: ErrNotExist}
 	}
 	err := fs.Remove(suffix)
 	if err != nil {
@@ -84,11 +84,11 @@ func (f *File) Name() string {
 func OpenFile(name string, flag int, perm FileMode) (*File, error) {
 	fs, suffix := findMount(name)
 	if fs == nil {
-		return nil, &PathError{"open", name, ErrNotExist}
+		return nil, &PathError{Op: "open", Path: name, Err: ErrNotExist}
 	}
 	handle, err := fs.OpenFile(suffix, flag, perm)
 	if err != nil {
-		return nil, &PathError{"open", name, err}
+		return nil, &PathError{Op: "open", Path: name, Err: err}
 	}
 	return NewFile(handle, name), nil
 }
@@ -106,10 +106,14 @@ func Create(name string) (*File, error) {
 // Read reads up to len(b) bytes from the File. It returns the number of bytes
 // read and any error encountered. At end of file, Read returns 0, io.EOF.
 func (f *File) Read(b []byte) (n int, err error) {
-	n, err = f.handle.Read(b)
+	if f.handle == nil {
+		err = ErrClosed
+	} else {
+		n, err = f.handle.Read(b)
+	}
 	// TODO: want to always wrap, like upstream, but ReadFile() compares against exactly io.EOF?
 	if err != nil && err != io.EOF {
-		err = &PathError{"read", f.name, err}
+		err = &PathError{Op: "read", Path: f.name, Err: err}
 	}
 	return
 }
@@ -123,13 +127,16 @@ func (f *File) ReadAt(b []byte, offset int64) (n int, err error) {
 	if offset < 0 {
 		return 0, &PathError{Op: "readat", Path: f.name, Err: errNegativeOffset}
 	}
+	if f.handle == nil {
+		return 0, &PathError{Op: "readat", Path: f.name, Err: ErrClosed}
+	}
 
 	for len(b) > 0 {
 		m, e := f.handle.ReadAt(b, offset)
 		if e != nil {
 			// TODO: want to always wrap, like upstream, but TestReadAtEOF compares against exactly io.EOF?
 			if e != io.EOF {
-				err = &PathError{"readat", f.name, e}
+				err = &PathError{Op: "readat", Path: f.name, Err: e}
 			} else {
 				err = e
 			}
@@ -146,9 +153,13 @@ func (f *File) ReadAt(b []byte, offset int64) (n int, err error) {
 // Write writes len(b) bytes to the File. It returns the number of bytes written
 // and an error, if any. Write returns a non-nil error when n != len(b).
 func (f *File) Write(b []byte) (n int, err error) {
-	n, err = f.handle.Write(b)
+	if f.handle == nil {
+		err = ErrClosed
+	} else {
+		n, err = f.handle.Write(b)
+	}
 	if err != nil {
-		err = &PathError{"write", f.name, err}
+		err = &PathError{Op: "write", Path: f.name, Err: err}
 	}
 	return
 }
@@ -160,14 +171,33 @@ func (f *File) WriteString(s string) (n int, err error) {
 }
 
 func (f *File) WriteAt(b []byte, off int64) (n int, err error) {
-	return 0, ErrNotImplemented
+	if f.handle == nil {
+		err = ErrClosed
+	} else {
+		err = ErrNotImplemented
+	}
+	err = &PathError{Op: "writeat", Path: f.name, Err: err}
+	return
 }
 
 // Close closes the File, rendering it unusable for I/O.
 func (f *File) Close() (err error) {
-	err = f.handle.Close()
+	if f.handle == nil {
+		err = ErrClosed
+	} else {
+		// Some platforms manage extra state other than the system handle which
+		// needs to be released when the file is closed. For example, darwin
+		// files have a DIR object holding a dup of the file descriptor.
+		//
+		// These platform-specific logic is provided by the (*file).close method
+		// which is why we do not call the handle's Close method directly.
+		err = f.file.close()
+		if err == nil {
+			f.handle = nil
+		}
+	}
 	if err != nil {
-		err = &PathError{"close", f.name, err}
+		err = &PathError{Op: "close", Path: f.name, Err: err}
 	}
 	return
 }
@@ -182,11 +212,24 @@ func (f *File) Close() (err error) {
 // system; you can seek to the beginning of the directory on Unix-like
 // operating systems, but not on Windows.
 func (f *File) Seek(offset int64, whence int) (ret int64, err error) {
-	return f.handle.Seek(offset, whence)
+	if f.handle == nil {
+		err = ErrClosed
+	} else {
+		ret, err = f.handle.Seek(offset, whence)
+	}
+	if err != nil {
+		err = &PathError{Op: "seek", Path: f.name, Err: err}
+	}
+	return
 }
 
-func (f *File) SyscallConn() (syscall.RawConn, error) {
-	return nil, ErrNotImplemented
+func (f *File) SyscallConn() (conn syscall.RawConn, err error) {
+	if f.handle == nil {
+		err = ErrClosed
+	} else {
+		err = ErrNotImplemented
+	}
+	return
 }
 
 // fd is an internal interface that is used to try a type assertion in order to
@@ -201,12 +244,17 @@ func (f *File) Fd() uintptr {
 	if ok {
 		return handle.Fd()
 	}
-	return 0
+	return ^uintptr(0)
 }
 
 // Truncate is a stub, not yet implemented
-func (f *File) Truncate(size int64) error {
-	return &PathError{"truncate", f.name, ErrNotImplemented}
+func (f *File) Truncate(size int64) (err error) {
+	if f.handle == nil {
+		err = ErrClosed
+	} else {
+		err = ErrNotImplemented
+	}
+	return &PathError{Op: "truncate", Path: f.name, Err: err}
 }
 
 // LinkError records an error during a link or symlink or rename system call and
diff --git a/src/os/file_anyos_test.go b/src/os/file_anyos_test.go
index 205ac299f..a1e8425f1 100644
--- a/src/os/file_anyos_test.go
+++ b/src/os/file_anyos_test.go
@@ -3,6 +3,7 @@
 package os_test
 
 import (
+	"errors"
 	"io"
 	. "os"
 	"runtime"
@@ -120,3 +121,59 @@ func TestFd(t *testing.T) {
 		t.Errorf("File descriptor contents not equal to file contents.")
 	}
 }
+
+// closeTests is the list of tests used to validate that after calling Close,
+// calling any method of File returns ErrClosed.
+var closeTests = map[string]func(*File) error{
+	"Close": func(f *File) error {
+		return f.Close()
+	},
+	"Read": func(f *File) error {
+		_, err := f.Read(nil)
+		return err
+	},
+	"ReadAt": func(f *File) error {
+		_, err := f.ReadAt(nil, 0)
+		return err
+	},
+	"Seek": func(f *File) error {
+		_, err := f.Seek(0, 0)
+		return err
+	},
+	"SyscallConn": func(f *File) error {
+		_, err := f.SyscallConn()
+		return err
+	},
+	"Truncate": func(f *File) error {
+		return f.Truncate(0)
+	},
+	"Write": func(f *File) error {
+		_, err := f.Write(nil)
+		return err
+	},
+	"WriteAt": func(f *File) error {
+		_, err := f.WriteAt(nil, 0)
+		return err
+	},
+	"WriteString": func(f *File) error {
+		_, err := f.WriteString("")
+		return err
+	},
+}
+
+func TestClose(t *testing.T) {
+	f := newFile("TestClose.txt", t)
+
+	if err := f.Close(); err != nil {
+		t.Error("unexpected error closing the file:", err)
+	}
+	if fd := f.Fd(); fd != ^uintptr(0) {
+		t.Error("unexpected file handle after closing the file:", fd)
+	}
+
+	for name, test := range closeTests {
+		if err := test(f); !errors.Is(err, ErrClosed) {
+			t.Errorf("unexpected error returned by calling %s on a closed file: %v", name, err)
+		}
+	}
+}
diff --git a/src/os/file_other.go b/src/os/file_other.go
index 12b21b838..7e4642c0b 100644
--- a/src/os/file_other.go
+++ b/src/os/file_other.go
@@ -33,6 +33,10 @@ type file struct {
 	name   string
 }
 
+func (f *file) close() error {
+	return f.handle.Close()
+}
+
 func NewFile(fd uintptr, name string) *File {
 	return &File{&file{stdioFileHandle(fd), name}}
 }
diff --git a/src/os/file_unix.go b/src/os/file_unix.go
index 271071e32..6cb6f7f01 100644
--- a/src/os/file_unix.go
+++ b/src/os/file_unix.go
@@ -42,6 +42,14 @@ type file struct {
 	dirinfo *dirInfo // nil unless directory being read
 }
 
+func (f *file) close() (err error) {
+	if f.dirinfo != nil {
+		f.dirinfo.close()
+		f.dirinfo = nil
+	}
+	return f.handle.Close()
+}
+
 func NewFile(fd uintptr, name string) *File {
 	return &File{&file{unixFileHandle(fd), name, nil}}
 }
diff --git a/src/os/file_windows.go b/src/os/file_windows.go
index a6838a642..572a82f27 100644
--- a/src/os/file_windows.go
+++ b/src/os/file_windows.go
@@ -39,6 +39,10 @@ type file struct {
 	name   string
 }
 
+func (f *file) close() error {
+	return f.handle.Close()
+}
+
 func NewFile(fd uintptr, name string) *File {
 	return &File{&file{unixFileHandle(fd), name}}
 }
diff --git a/src/syscall/syscall_libc_darwin.go b/src/syscall/syscall_libc_darwin.go
index a8c98f214..0b908255a 100644
--- a/src/syscall/syscall_libc_darwin.go
+++ b/src/syscall/syscall_libc_darwin.go
@@ -267,6 +267,14 @@ func Pipe2(fds []int, flags int) (err error) {
 	return
 }
 
+func closedir(dir uintptr) (err error) {
+	e := libc_closedir(unsafe.Pointer(dir))
+	if e != 0 {
+		err = getErrno()
+	}
+	return
+}
+
 func readdir_r(dir uintptr, entry *Dirent, result **Dirent) (err error) {
 	e1 := libc_readdir_r(unsafe.Pointer(dir), unsafe.Pointer(entry), unsafe.Pointer(result))
 	if e1 != 0 {
diff --git a/src/syscall/syscall_libc_darwin_amd64.go b/src/syscall/syscall_libc_darwin_amd64.go
index d890c6624..1f5528ec5 100644
--- a/src/syscall/syscall_libc_darwin_amd64.go
+++ b/src/syscall/syscall_libc_darwin_amd64.go
@@ -17,6 +17,11 @@ import (
 //export fdopendir$INODE64
 func libc_fdopendir(fd int32) unsafe.Pointer
 
+// int closedir(struct DIR * buf);
+//
+//export closedir
+func libc_closedir(unsafe.Pointer) int32
+
 // int readdir_r(struct DIR * buf, struct dirent *entry, struct dirent **result);
 //
 //export readdir_r$INODE64
diff --git a/src/syscall/syscall_libc_darwin_arm64.go b/src/syscall/syscall_libc_darwin_arm64.go
index 49f3c598c..f9ce3c4e3 100644
--- a/src/syscall/syscall_libc_darwin_arm64.go
+++ b/src/syscall/syscall_libc_darwin_arm64.go
@@ -11,6 +11,11 @@ import (
 //export fdopendir
 func libc_fdopendir(fd int32) unsafe.Pointer
 
+// int closedir(struct DIR * buf);
+//
+//export closedir
+func libc_closedir(unsafe.Pointer) int32
+
 // int readdir_r(struct DIR * buf, struct dirent *entry, struct dirent **result);
 //
 //export readdir_r