fix: prevent data races in MemFs Rename and RemoveAll

Rename: If unRegisterWithParent or renameDescendants returns an error,
the function returns while holding an exclusive write lock, but the
deferred RUnlock runs instead of Unlock.

RemoveAll: Unsafe modification of data map under the assumption that all
entries belong to unregistered path. This implementaiton also raced with
non-standard implicit MkdirAll behavior of Create.

Author: mafredri

mem/file.go

@@ -113,6 +113,13 @@ func SetGID(f *FileData, gid int) {
 	f.Unlock()
 }
 
+func SetUIDGID(f *FileData, uid, gid int) {
+	f.Lock()
+	f.uid = uid
+	f.gid = gid
+	f.Unlock()
+}
+
 func GetFileInfo(f *FileData) *FileInfo {
 	return &FileInfo{f}
 }

memmap.go

@@ -55,11 +55,13 @@ func (*MemMapFs) Name() string { return "MemMapFS" }
 
 func (m *MemMapFs) Create(name string) (File, error) {
 	name = normalizePath(name)
-	m.mu.Lock()
 	file := mem.CreateFile(name)
-	m.getData()[name] = file
+
+	m.mu.Lock()
+	defer m.mu.Unlock()
+
 	m.registerWithParent(file, 0)
-	m.mu.Unlock()
+	m.getData()[name] = file
 	return mem.NewFileHandle(file), nil
 }
 
@@ -162,18 +164,17 @@ func (m *MemMapFs) Mkdir(name string, perm os.FileMode) error {
 	}
 
 	m.mu.Lock()
-	// Dobule check that it doesn't exist.
-	if _, ok := m.getData()[name]; ok {
-		m.mu.Unlock()
+	defer m.mu.Unlock()
+
+	if _, ok := m.getData()[name]; ok { // Recheck.
 		return &os.PathError{Op: "mkdir", Path: name, Err: ErrFileExists}
 	}
+
 	item := mem.CreateDir(name)
 	mem.SetMode(item, os.ModeDir|perm)
 	m.getData()[name] = item
 	m.registerWithParent(item, perm)
-	m.mu.Unlock()
-
-	return m.setFileMode(name, perm|os.ModeDir)
+	return nil
 }
 
 func (m *MemMapFs) MkdirAll(path string, perm os.FileMode) error {
@@ -253,8 +254,9 @@ func (m *MemMapFs) OpenFile(name string, flag int, perm os.FileMode) (File, erro
 	if err != nil {
 		return nil, err
 	}
+	fileData := file.(*mem.File).Data()
 	if flag == os.O_RDONLY {
-		file = mem.NewReadOnlyFileHandle(file.(*mem.File).Data())
+		file = mem.NewReadOnlyFileHandle(fileData)
 	}
 	if flag&os.O_APPEND > 0 {
 		_, err = file.Seek(0, io.SeekEnd)
@@ -271,7 +273,7 @@ func (m *MemMapFs) OpenFile(name string, flag int, perm os.FileMode) (File, erro
 		}
 	}
 	if chmod {
-		return file, m.setFileMode(name, perm)
+		mem.SetMode(fileData, perm)
 	}
 	return file, nil
 }
@@ -296,20 +298,19 @@ func (m *MemMapFs) Remove(name string) error {
 
 func (m *MemMapFs) RemoveAll(path string) error {
 	path = normalizePath(path)
-	m.mu.Lock()
-	m.unRegisterWithParent(path)
-	m.mu.Unlock()
 
-	m.mu.RLock()
-	defer m.mu.RUnlock()
+	// The lock must be held throughout to ensure atomic consistency
+	// between the parent registry and getData. This is a limitation
+	// of the current data model.
+	m.mu.Lock()
+	defer m.mu.Unlock()
 
+	if err := m.unRegisterWithParent(path); err != nil {
+		return nil
+	}
 	for p := range m.getData() {
 		if p == path || strings.HasPrefix(p, path+FilePathSeparator) {
-			m.mu.RUnlock()
-			m.mu.Lock()
 			delete(m.getData(), p)
-			m.mu.Unlock()
-			m.mu.RLock()
 		}
 	}
 	return nil
@@ -324,32 +325,32 @@ func (m *MemMapFs) Rename(oldname, newname string) error {
 	}
 
 	m.mu.RLock()
-	defer m.mu.RUnlock()
-	if _, ok := m.getData()[oldname]; ok {
-		m.mu.RUnlock()
-		m.mu.Lock()
-		err := m.unRegisterWithParent(oldname)
-		if err != nil {
-			return err
-		}
+	_, ok := m.getData()[oldname]
+	m.mu.RUnlock()
+	if !ok {
+		return &os.PathError{Op: "rename", Path: oldname, Err: ErrFileNotFound}
+	}
 
-		fileData := m.getData()[oldname]
-		mem.ChangeFileName(fileData, newname)
-		m.getData()[newname] = fileData
+	m.mu.Lock()
+	defer m.mu.Unlock()
 
-		err = m.renameDescendants(oldname, newname)
-		if err != nil {
-			return err
-		}
+	err := m.unRegisterWithParent(oldname)
+	if err != nil {
+		return err
+	}
 
-		delete(m.getData(), oldname)
+	fileData := m.getData()[oldname]
+	mem.ChangeFileName(fileData, newname)
+	m.getData()[newname] = fileData
 
-		m.registerWithParent(fileData, 0)
-		m.mu.Unlock()
-		m.mu.RLock()
-	} else {
-		return &os.PathError{Op: "rename", Path: oldname, Err: ErrFileNotFound}
+	err = m.renameDescendants(oldname, newname)
+	if err != nil {
+		return err
 	}
+
+	delete(m.getData(), oldname)
+
+	m.registerWithParent(fileData, 0)
 	return nil
 }
 
@@ -391,6 +392,7 @@ func (m *MemMapFs) Stat(name string) (os.FileInfo, error) {
 }
 
 func (m *MemMapFs) Chmod(name string, mode os.FileMode) error {
+	name = normalizePath(name)
 	mode &= chmodBits
 
 	m.mu.RLock()
@@ -400,25 +402,7 @@ func (m *MemMapFs) Chmod(name string, mode os.FileMode) error {
 		return &os.PathError{Op: "chmod", Path: name, Err: ErrFileNotFound}
 	}
 	prevOtherBits := mem.GetFileInfo(f).Mode() & ^chmodBits
-
-	mode = prevOtherBits | mode
-	return m.setFileMode(name, mode)
-}
-
-func (m *MemMapFs) setFileMode(name string, mode os.FileMode) error {
-	name = normalizePath(name)
-
-	m.mu.RLock()
-	f, ok := m.getData()[name]
-	m.mu.RUnlock()
-	if !ok {
-		return &os.PathError{Op: "chmod", Path: name, Err: ErrFileNotFound}
-	}
-
-	m.mu.Lock()
-	mem.SetMode(f, mode)
-	m.mu.Unlock()
-
+	mem.SetMode(f, prevOtherBits|mode)
 	return nil
 }
 
@@ -431,10 +415,7 @@ func (m *MemMapFs) Chown(name string, uid, gid int) error {
 	if !ok {
 		return &os.PathError{Op: "chown", Path: name, Err: ErrFileNotFound}
 	}
-
-	mem.SetUID(f, uid)
-	mem.SetGID(f, gid)
-
+	mem.SetUIDGID(f, uid, gid)
 	return nil
 }
 
@@ -447,11 +428,7 @@ func (m *MemMapFs) Chtimes(name string, atime time.Time, mtime time.Time) error
 	if !ok {
 		return &os.PathError{Op: "chtimes", Path: name, Err: ErrFileNotFound}
 	}
-
-	m.mu.Lock()
 	mem.SetModTime(f, mtime)
-	m.mu.Unlock()
-
 	return nil
 }
 

memmap_test.go

@@ -67,7 +67,7 @@ func TestPathErrors(t *testing.T) {
 
 	// fs.Create doesn't return an error
 
-	err = fs.Mkdir(path2, perm)
+	err = fs.MkdirAll(path2, perm)
 	if err != nil {
 		t.Error(err)
 	}
@@ -918,3 +918,93 @@ func TestMemMapFsRename(t *testing.T) {
 		}
 	}
 }
+
+// TestMemMapFsRenameLockCorruption reproduces a bug where Rename returns while
+// holding a write lock but the deferred RUnlock runs, causing:
+//
+//	RUnlock of unlocked RWMutex
+//
+// Test: go test . -race -count=10 -run=TestMemMapFsRenameLockCorruption
+func TestMemMapFsRenameLockCorruption(t *testing.T) {
+	for range 10 {
+		fs := NewMemMapFs()
+		if err := fs.MkdirAll("/a/b", 0o755); err != nil {
+			t.Fatal(err)
+		}
+		if _, err := fs.Create("/a/b/c"); err != nil {
+			t.Fatal(err)
+		}
+
+		var wg sync.WaitGroup
+		wg.Add(2)
+
+		go func() {
+			defer wg.Done()
+			fs.RemoveAll("/a/b")
+		}()
+
+		go func() {
+			defer wg.Done()
+			fs.Rename("/a/b/c", "/x")
+		}()
+
+		done := make(chan struct{})
+		go func() {
+			wg.Wait()
+			close(done)
+		}()
+
+		select {
+		case <-done:
+		case <-time.After(5 * time.Second):
+			t.Fatal("deadlock")
+		}
+	}
+}
+
+// TestMemMapFsRemoveAllCreateRace reproduces a bug where a RemoveAll and Create
+// race leaves the filesystem in an inconsistent state where a file exists
+// without its parent.
+//
+//	dir/file exists but /dir does not
+//
+// Test: go test . -race -count=100 -run=TestMemMapFsRemoveAllCreateRace
+func TestMemMapFsRemoveAllCreateRace(t *testing.T) {
+	for range 10 {
+		fs := NewMemMapFs()
+		if err := fs.MkdirAll("/dir", 0o755); err != nil {
+			t.Fatal(err)
+		}
+
+		var wg sync.WaitGroup
+		wg.Add(4)
+
+		go func() {
+			defer wg.Done()
+			fs.RemoveAll("/dir")
+		}()
+		go func() {
+			defer wg.Done()
+			fs.Create("/dir/file1")
+		}()
+		go func() {
+			defer wg.Done()
+			fs.Create("/dir/file2")
+		}()
+		go func() {
+			defer wg.Done()
+			fs.Create("/dir/file3")
+		}()
+
+		wg.Wait()
+
+		_, dirErr := fs.Stat("/dir")
+
+		for _, name := range []string{"/dir/file1", "/dir/file2", "/dir/file3"} {
+			_, fileErr := fs.Stat(name)
+			if dirErr != nil && fileErr == nil {
+				t.Errorf("%s exists but /dir does not", name)
+			}
+		}
+	}
+}