From 7d6390ed108e8695c40e6775dc1a5c6f38ba4d22 Mon Sep 17 00:00:00 2001
From: Pavel Kalinnikov <pavel@cockroachlabs.com>
Date: Mon, 15 Jan 2024 19:55:35 +0000
Subject: [PATCH] vfs: fix data race over memNode.name

This commit fixes the following data race over memNode.name:
- memFile.Stat().Name() reads memNode.name without a lock
- MemFS.Rename() writes memNode.name under MemFS.mu

The memNode.name field is fundamentally owned by MemFS, so must be
accessed under MemFS.mu lock. However, this lock is not accessible from
memNode.

After this commit, memNode no longer implements os.FileInfo. Instead,
the FileInfo role is now played by memFile (which has access to MemFS,
and can use its lock to safely access the name field).
---
 vfs/mem_fs.go | 50 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/vfs/mem_fs.go b/vfs/mem_fs.go
index fa44847b4f..eba75559ef 100644
--- a/vfs/mem_fs.go
+++ b/vfs/mem_fs.go
@@ -549,9 +549,11 @@ func (*MemFS) GetDiskUsage(string) (DiskUsage, error) {
 	return DiskUsage{}, ErrUnsupported
 }
 
-// memNode holds a file's data or a directory's children, and implements os.FileInfo.
+// memNode holds a file's data or a directory's children.
 type memNode struct {
-	name  string
+	// TODO(pav-kv): file name is a property of a file, not of a node. Multiple
+	// files with different names can hard link to the same node.
+	name  string // protected by MemFS.mu
 	isDir bool
 	refs  atomic.Int32
 
@@ -580,34 +582,38 @@ func newRootMemNode() *memNode {
 	}
 }
 
-func (f *memNode) IsDir() bool {
-	return f.isDir
+func (f *memFile) IsDir() bool {
+	return f.n.isDir
 }
 
-func (f *memNode) ModTime() time.Time {
-	f.mu.Lock()
-	defer f.mu.Unlock()
-	return f.mu.modTime
+func (f *memFile) ModTime() time.Time {
+	f.n.mu.Lock()
+	defer f.n.mu.Unlock()
+	return f.n.mu.modTime
 }
 
-func (f *memNode) Mode() os.FileMode {
-	if f.isDir {
+func (f *memFile) Mode() os.FileMode {
+	if f.n.isDir {
 		return os.ModeDir | 0755
 	}
 	return 0755
 }
 
-func (f *memNode) Name() string {
-	return f.name
+func (f *memFile) Name() string {
+	if fs := f.fs; fs != nil {
+		fs.mu.Lock()
+		defer fs.mu.Unlock()
+	}
+	return f.n.name
 }
 
-func (f *memNode) Size() int64 {
-	f.mu.Lock()
-	defer f.mu.Unlock()
-	return int64(len(f.mu.data))
+func (f *memFile) Size() int64 {
+	f.n.mu.Lock()
+	defer f.n.mu.Unlock()
+	return int64(len(f.n.mu.data))
 }
 
-func (f *memNode) Sys() interface{} {
+func (f *memFile) Sys() interface{} {
 	return nil
 }
 
@@ -657,7 +663,8 @@ func (f *memNode) resetToSyncedState() {
 	}
 }
 
-// memFile is a reader or writer of a node's data, and implements File.
+// memFile is a reader or writer of a node's data. It implements File and
+// os.FileInfo.
 type memFile struct {
 	n           *memNode
 	fs          *MemFS // nil for a standalone memFile
@@ -666,13 +673,16 @@ type memFile struct {
 	read, write bool
 }
 
+var _ os.FileInfo = (*memFile)(nil)
 var _ File = (*memFile)(nil)
 
 func (f *memFile) Close() error {
 	if n := f.n.refs.Add(-1); n < 0 {
 		panic(fmt.Sprintf("pebble: close of unopened file: %d", n))
 	}
-	f.n = nil
+	// TODO(pav-kv): set f.n to nil here, to detect double-close and
+	// use-after-close errors automatically. Currently this is not possible
+	// because the lifetime of Stat() can outlive the Close() of this file.
 	return nil
 }
 
@@ -769,7 +779,7 @@ func (f *memFile) Prefetch(offset int64, length int64) error { return nil }
 func (f *memFile) Preallocate(offset, length int64) error    { return nil }
 
 func (f *memFile) Stat() (os.FileInfo, error) {
-	return f.n, nil
+	return f, nil
 }
 
 func (f *memFile) Sync() error {