Merge pull request #250 from anishathalye/improve-mode

Improve handling of mode bits

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()

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())
+	}
+}