diff --git a/mem/file.go b/mem/file.go index c77fcd40..4a3093c5 100644 --- a/mem/file.go +++ b/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} } diff --git a/memmap.go b/memmap.go index ed92f564..4097bebd 100644 --- a/memmap.go +++ b/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) + m.getData()[name] = item + 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 } diff --git a/memmap_test.go b/memmap_test.go index 873cd904..76fa2a4d 100644 --- a/memmap_test.go +++ b/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) + } + } + } +}