From b598fbbf555db47578806d127b221ac1cd8dd854 Mon Sep 17 00:00:00 2001 From: Anish Athalye Date: Wed, 15 Jul 2020 13:48:32 -0400 Subject: [PATCH] Improve handling of mode bits - "/" should have mode `os.ModeDir|0755`, not `0000`. Among other things, this had resulted in `mode.IsDir()` returning false for root prior to this patch. - `Mkdir`, `MkdirAll`, and `OpenFile` shouldn't be allowed to set permissions that are otherwise illegal through `Chmod`. This mirrors what Go's `os` package does: it calls `syscallMode(mode)`, which effectively clears out the same bits that are disallowed by `Chmod`. - `MkdirAll` should use the given permissions for all intermediate directories that are created, not just for the final directory. Prior to this patch, intermediate directories were created with mode bits `0000`. Besides the permission bits being wrong, `mode.IsDir()` would return false for these directories prior to this patch. --- memmap.go | 23 +++++--- memmap_test.go | 147 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+), 8 deletions(-) diff --git a/memmap.go b/memmap.go index 6be0e9c2..bbcc2381 100644 --- a/memmap.go +++ b/memmap.go @@ -25,6 +25,8 @@ import ( "github.com/spf13/afero/mem" ) +const chmodBits = os.ModePerm | os.ModeSetuid | os.ModeSetgid | os.ModeSticky // Only a subset of bits are allowed to be changed. Documented under os.Chmod() + type MemMapFs struct { mu sync.RWMutex data map[string]*mem.FileData @@ -40,7 +42,9 @@ func (m *MemMapFs) getData() map[string]*mem.FileData { m.data = make(map[string]*mem.FileData) // Root should always exist, right? // TODO: what about windows? - m.data[FilePathSeparator] = mem.CreateDir(FilePathSeparator) + root := mem.CreateDir(FilePathSeparator) + mem.SetMode(root, os.ModeDir|0755) + m.data[FilePathSeparator] = root }) return m.data } @@ -52,7 +56,7 @@ func (m *MemMapFs) Create(name string) (File, error) { m.mu.Lock() file := mem.CreateFile(name) m.getData()[name] = file - m.registerWithParent(file) + m.registerWithParent(file, 0) m.mu.Unlock() return mem.NewFileHandle(file), nil } @@ -83,14 +87,14 @@ func (m *MemMapFs) findParent(f *mem.FileData) *mem.FileData { return pfile } -func (m *MemMapFs) registerWithParent(f *mem.FileData) { +func (m *MemMapFs) registerWithParent(f *mem.FileData, perm os.FileMode) { if f == nil { return } parent := m.findParent(f) if parent == nil { pdir := filepath.Dir(filepath.Clean(f.Name())) - err := m.lockfreeMkdir(pdir, 0777) + err := m.lockfreeMkdir(pdir, perm) if err != nil { //log.Println("Mkdir error:", err) return @@ -119,13 +123,15 @@ func (m *MemMapFs) lockfreeMkdir(name string, perm os.FileMode) error { } } else { item := mem.CreateDir(name) + mem.SetMode(item, os.ModeDir|perm) m.getData()[name] = item - m.registerWithParent(item) + m.registerWithParent(item, perm) } return nil } func (m *MemMapFs) Mkdir(name string, perm os.FileMode) error { + perm &= chmodBits name = normalizePath(name) m.mu.RLock() @@ -137,8 +143,9 @@ func (m *MemMapFs) Mkdir(name string, perm os.FileMode) error { m.mu.Lock() item := mem.CreateDir(name) + mem.SetMode(item, os.ModeDir|perm) m.getData()[name] = item - m.registerWithParent(item) + m.registerWithParent(item, perm) m.mu.Unlock() return m.setFileMode(name, perm|os.ModeDir) @@ -208,6 +215,7 @@ func (m *MemMapFs) lockfreeOpen(name string) (*mem.FileData, error) { } func (m *MemMapFs) OpenFile(name string, flag int, perm os.FileMode) (File, error) { + perm &= chmodBits chmod := false file, err := m.openWrite(name) if err == nil && (flag&os.O_EXCL > 0) { @@ -300,7 +308,7 @@ func (m *MemMapFs) Rename(oldname, newname string) error { delete(m.getData(), oldname) mem.ChangeFileName(fileData, newname) m.getData()[newname] = fileData - m.registerWithParent(fileData) + m.registerWithParent(fileData, 0) m.mu.Unlock() m.mu.RLock() } else { @@ -319,7 +327,6 @@ func (m *MemMapFs) Stat(name string) (os.FileInfo, error) { } func (m *MemMapFs) Chmod(name string, mode os.FileMode) error { - const chmodBits = os.ModePerm | os.ModeSetuid | os.ModeSetgid | os.ModeSticky // Only a subset of bits are allowed to be changed. Documented under os.Chmod() mode &= chmodBits m.mu.RLock() diff --git a/memmap_test.go b/memmap_test.go index 371096ff..6ce742cb 100644 --- a/memmap_test.go +++ b/memmap_test.go @@ -412,6 +412,116 @@ loop: } } +// root is a directory +func TestMemFsRootDirMode(t *testing.T) { + t.Parallel() + + fs := NewMemMapFs() + info, err := fs.Stat("/") + if err != nil { + t.Fatal(err) + } + if !info.IsDir() { + t.Error("should be a directory") + } + if !info.Mode().IsDir() { + t.Errorf("FileMode is not directory, is %s", info.Mode().String()) + } +} + +// MkdirAll creates intermediate directories with correct mode +func TestMemFsMkdirAllMode(t *testing.T) { + t.Parallel() + + fs := NewMemMapFs() + err := fs.MkdirAll("/a/b/c", 0755) + if err != nil { + t.Fatal(err) + } + info, err := fs.Stat("/a") + if err != nil { + t.Fatal(err) + } + if !info.Mode().IsDir() { + t.Error("/a: mode is not directory") + } + if info.Mode() != os.FileMode(os.ModeDir|0755) { + t.Errorf("/a: wrong permissions, expected drwxr-xr-x, got %s", info.Mode()) + } + info, err = fs.Stat("/a/b") + if err != nil { + t.Fatal(err) + } + if !info.Mode().IsDir() { + t.Error("/a/b: mode is not directory") + } + if info.Mode() != os.FileMode(os.ModeDir|0755) { + t.Errorf("/a/b: wrong permissions, expected drwxr-xr-x, got %s", info.Mode()) + } + info, err = fs.Stat("/a/b/c") + if err != nil { + t.Fatal(err) + } + if !info.Mode().IsDir() { + t.Error("/a/b/c: mode is not directory") + } + if info.Mode() != os.FileMode(os.ModeDir|0755) { + t.Errorf("/a/b/c: wrong permissions, expected drwxr-xr-x, got %s", info.Mode()) + } +} + +// MkdirAll does not change permissions of already-existing directories +func TestMemFsMkdirAllNoClobber(t *testing.T) { + t.Parallel() + + fs := NewMemMapFs() + err := fs.MkdirAll("/a/b/c", 0755) + if err != nil { + t.Fatal(err) + } + info, err := fs.Stat("/a/b") + if err != nil { + t.Fatal(err) + } + if info.Mode() != os.FileMode(os.ModeDir|0755) { + t.Errorf("/a/b: wrong permissions, expected drwxr-xr-x, got %s", info.Mode()) + } + err = fs.MkdirAll("/a/b/c/d/e/f", 0710) + // '/a/b' is unchanged + if err != nil { + t.Fatal(err) + } + info, err = fs.Stat("/a/b") + if err != nil { + t.Fatal(err) + } + if info.Mode() != os.FileMode(os.ModeDir|0755) { + t.Errorf("/a/b: wrong permissions, expected drwxr-xr-x, got %s", info.Mode()) + } + // new directories created with proper permissions + info, err = fs.Stat("/a/b/c/d") + if err != nil { + t.Fatal(err) + } + if info.Mode() != os.FileMode(os.ModeDir|0710) { + t.Errorf("/a/b/c/d: wrong permissions, expected drwx--x---, got %s", info.Mode()) + } + info, err = fs.Stat("/a/b/c/d/e") + if err != nil { + t.Fatal(err) + } + if info.Mode() != os.FileMode(os.ModeDir|0710) { + t.Errorf("/a/b/c/d/e: wrong permissions, expected drwx--x---, got %s", info.Mode()) + } + info, err = fs.Stat("/a/b/c/d/e/f") + if err != nil { + t.Fatal(err) + } + if info.Mode() != os.FileMode(os.ModeDir|0710) { + t.Errorf("/a/b/c/d/e/f: wrong permissions, expected drwx--x---, got %s", info.Mode()) + } +} + func TestMemFsDirMode(t *testing.T) { fs := NewMemMapFs() err := fs.Mkdir("/testDir1", 0644) @@ -503,3 +613,40 @@ func TestMemFsChmod(t *testing.T) { t.Error("chmod should not change file type. New mode =", info.Mode()) } } + +// can't use Mkdir to get around which permissions we're allowed to set +func TestMemFsMkdirModeIllegal(t *testing.T) { + t.Parallel() + + fs := NewMemMapFs() + err := fs.Mkdir("/a", os.ModeSocket|0755) + if err != nil { + t.Fatal(err) + } + info, err := fs.Stat("/a") + if err != nil { + t.Fatal(err) + } + if info.Mode() != os.FileMode(os.ModeDir|0755) { + t.Fatalf("should not be able to use Mkdir to set illegal mode: %s", info.Mode().String()) + } +} + +// can't use OpenFile to get around which permissions we're allowed to set +func TestMemFsOpenFileModeIllegal(t *testing.T) { + t.Parallel() + + fs := NewMemMapFs() + file, err := fs.OpenFile("/a", os.O_CREATE, os.ModeSymlink|0644) + if err != nil { + t.Fatal(err) + } + defer file.Close() + info, err := fs.Stat("/a") + if err != nil { + t.Fatal(err) + } + if info.Mode() != os.FileMode(0644) { + t.Fatalf("should not be able to use OpenFile to set illegal mode: %s", info.Mode().String()) + } +}