Track paths and provide a rename hook.

This change also adds extensive testing to the p9 package via mocks. The sanity
checks and type checks are moved from the gofer into the core package, where
they can be more easily validated.

PiperOrigin-RevId: 218296768
Change-Id: I4fc3c326e7bf1e0e140a454cbacbcc6fd617ab55

4 files changed, 35 insertions, 149 deletions

diff --git a/runsc/fsgofer/BUILD b/runsc/fsgofer/BUILD
index 24e172f48..f28e4fa77 100644
--- a/runsc/fsgofer/BUILD
+++ b/runsc/fsgofer/BUILD
@@ -1,7 +1,7 @@
-package(licenses = ["notice"])  # Apache 2.0
-
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
+package(licenses = ["notice"])  # Apache 2.0
+
 go_library(
     name = "fsgofer",
     srcs = [
diff --git a/runsc/fsgofer/filter/BUILD b/runsc/fsgofer/filter/BUILD
index 40f4f2205..c7848d10c 100644
--- a/runsc/fsgofer/filter/BUILD
+++ b/runsc/fsgofer/filter/BUILD
@@ -1,7 +1,7 @@
-package(licenses = ["notice"])  # Apache 2.0
-
 load("@io_bazel_rules_go//go:def.bzl", "go_library")
 
+package(licenses = ["notice"])  # Apache 2.0
+
 go_library(
     name = "filter",
     srcs = [
diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go
index e03bb7752..fd913831a 100644
--- a/runsc/fsgofer/fsgofer.go
+++ b/runsc/fsgofer/fsgofer.go
@@ -26,7 +26,6 @@ import (
 	"math"
 	"os"
 	"path"
-	"strings"
 	"sync"
 	"syscall"
 
@@ -181,18 +180,6 @@ func (a *attachPoint) makeQID(stat syscall.Stat_t) p9.QID {
 	}
 }
 
-func isNameValid(name string) bool {
-	if name == "" || name == "." || name == ".." {
-		log.Warningf("Invalid name: %s", name)
-		return false
-	}
-	if strings.IndexByte(name, '/') >= 0 {
-		log.Warningf("Invalid name: %s", name)
-		return false
-	}
-	return true
-}
-
 // localFile implements p9.File wrapping a local file. The underlying file
 // is opened during Walk() and stored in 'controlFile' to be used with other
 // operations. The mode in which the file is opened varies depending on the
@@ -228,11 +215,7 @@ type localFile struct {
 	// attachPoint is the attachPoint that serves this localFile.
 	attachPoint *attachPoint
 
-	// mu protects 'hostPath' when file is renamed.
-	mu sync.Mutex
-
-	// TODO: hostPath is not safe to use as path needs to be walked
-	// everytime (and can change underneath us). Remove all usages.
+	// hostPath will be safely updated by the Renamed hook.
 	hostPath string
 
 	// controlFile is opened when localFile is created and it's never nil.
@@ -246,6 +229,7 @@ type localFile struct {
 	// if localFile isn't opened.
 	mode p9.OpenFlags
 
+	// ft is the fileType for this file.
 	ft fileType
 
 	// readDirMu protects against concurrent Readdir calls.
@@ -296,10 +280,7 @@ func openAnyFile(parent *localFile, name string) (*os.File, string, error) {
 		return nil, "", extractErrno(err)
 	}
 
-	parent.mu.Lock()
-	defer parent.mu.Unlock()
 	newPath := path.Join(parent.hostPath, name)
-
 	return os.NewFile(uintptr(fd), newPath), newPath, nil
 }
 
@@ -382,13 +363,10 @@ func (l *localFile) Open(mode p9.OpenFlags) (*fd.FD, p9.QID, uint32, error) {
 		log.Debugf("Open reopening file, mode: %v, %q", mode, l.controlFile.Name())
 		var err error
 
-		l.mu.Lock()
 		newFile, err = os.OpenFile(l.hostPath, openFlags|mode.OSFlags(), 0)
 		if err != nil {
-			l.mu.Unlock()
 			return nil, p9.QID{}, 0, extractErrno(err)
 		}
-		l.mu.Unlock()
 	}
 
 	stat, err := stat(int(newFile.Fd()))
@@ -418,9 +396,6 @@ func (l *localFile) Create(name string, mode p9.OpenFlags, perm p9.FileMode, uid
 		}
 		return nil, nil, p9.QID{}, 0, syscall.EBADF
 	}
-	if !isNameValid(name) {
-		return nil, nil, p9.QID{}, 0, syscall.EINVAL
-	}
 
 	// Use a single file for both 'controlFile' and 'openedFile'. Mode must include read for control
 	// and whichever else was requested by caller. Note that resulting file might have a wider mode
@@ -452,9 +427,6 @@ func (l *localFile) Create(name string, mode p9.OpenFlags, perm p9.FileMode, uid
 		return nil, nil, p9.QID{}, 0, extractErrno(err)
 	}
 
-	l.mu.Lock()
-	defer l.mu.Unlock()
-
 	cPath := path.Join(l.hostPath, name)
 	f := os.NewFile(uintptr(fd), cPath)
 	c := &localFile{
@@ -477,10 +449,6 @@ func (l *localFile) Mkdir(name string, perm p9.FileMode, uid p9.UID, gid p9.GID)
 		return p9.QID{}, syscall.EBADF
 	}
 
-	if !isNameValid(name) {
-		return p9.QID{}, syscall.EINVAL
-	}
-
 	if err := syscall.Mkdirat(l.controlFD(), name, uint32(perm.Permissions())); err != nil {
 		return p9.QID{}, extractErrno(err)
 	}
@@ -517,9 +485,6 @@ func (l *localFile) Walk(names []string) ([]p9.QID, p9.File, error) {
 			return nil, nil, extractErrno(err)
 		}
 
-		l.mu.Lock()
-		defer l.mu.Unlock()
-
 		c := &localFile{
 			attachPoint: l.attachPoint,
 			hostPath:    l.hostPath,
@@ -532,10 +497,6 @@ func (l *localFile) Walk(names []string) ([]p9.QID, p9.File, error) {
 	var qids []p9.QID
 	last := l
 	for _, name := range names {
-		if !isNameValid(name) {
-			return nil, nil, syscall.EINVAL
-		}
-
 		f, path, err := openAnyFile(last, name)
 		if err != nil {
 			return nil, nil, extractErrno(err)
@@ -761,15 +722,15 @@ func (l *localFile) SetAttr(valid p9.SetAttrMask, attr p9.SetAttr) error {
 	return err
 }
 
-// Remove implements p9.File.
-//
-// This is deprecated in favor of UnlinkAt.
-func (*localFile) Remove() error {
-	return syscall.ENOSYS
+// Rename implements p9.File; this should never be called.
+func (l *localFile) Rename(p9.File, string) error {
+	panic("rename called directly")
 }
 
-// Rename implements p9.File.
-func (l *localFile) Rename(directory p9.File, name string) error {
+// RenameAt implements p9.File.RenameAt.
+//
+// TODO: change to renameat(2).
+func (l *localFile) RenameAt(oldName string, directory p9.File, newName string) error {
 	conf := l.attachPoint.conf
 	if conf.ROMount {
 		if conf.PanicOnWrite {
@@ -777,34 +738,16 @@ func (l *localFile) Rename(directory p9.File, name string) error {
 		}
 		return syscall.EBADF
 	}
-	if !isNameValid(name) {
-		return syscall.EINVAL
-	}
-
-	l.mu.Lock()
-	defer l.mu.Unlock()
 
-	// TODO: change to renameat(2)
-	parent := directory.(*localFile)
-	newPath := path.Join(parent.hostPath, name)
-	if err := syscall.Rename(l.hostPath, newPath); err != nil {
+	newParent := directory.(*localFile)
+	oldPath := path.Join(l.hostPath, oldName)
+	newPath := path.Join(newParent.hostPath, newName)
+	if err := syscall.Rename(oldPath, newPath); err != nil {
 		return extractErrno(err)
 	}
-
-	// Update path on success.
-	// TODO: this doesn't cover cases where any of the
-	// parents have been renamed.
-	l.hostPath = newPath
 	return nil
 }
 
-// RenameAt implements p9.File.RenameAt.
-//
-// Code still uses [deprecated] Rename().
-func (*localFile) RenameAt(_ string, _ p9.File, _ string) error {
-	return syscall.ENOSYS
-}
-
 // ReadAt implements p9.File.
 func (l *localFile) ReadAt(p []byte, offset uint64) (int, error) {
 	if l.mode != p9.ReadOnly && l.mode != p9.ReadWrite {
@@ -848,9 +791,6 @@ func (l *localFile) Symlink(target, newName string, uid p9.UID, gid p9.GID) (p9.
 		}
 		return p9.QID{}, syscall.EBADF
 	}
-	if !isNameValid(newName) {
-		return p9.QID{}, syscall.EINVAL
-	}
 
 	if err := unix.Symlinkat(target, l.controlFD(), newName); err != nil {
 		return p9.QID{}, extractErrno(err)
@@ -882,9 +822,6 @@ func (l *localFile) Link(target p9.File, newName string) error {
 		}
 		return syscall.EBADF
 	}
-	if !isNameValid(newName) {
-		return syscall.EINVAL
-	}
 
 	targetFile := target.(*localFile)
 	if err := unix.Linkat(targetFile.controlFD(), "", l.controlFD(), newName, linux.AT_EMPTY_PATH); err != nil {
@@ -909,9 +846,7 @@ func (l *localFile) UnlinkAt(name string, flags uint32) error {
 		}
 		return syscall.EBADF
 	}
-	if !isNameValid(name) {
-		return syscall.EINVAL
-	}
+
 	if err := unix.Unlinkat(l.controlFD(), name, int(flags)); err != nil {
 		return extractErrno(err)
 	}
@@ -1000,6 +935,11 @@ func (l *localFile) Close() error {
 	return err
 }
 
+// Renamed implements p9.Renamed.
+func (l *localFile) Renamed(newDir p9.File, newName string) {
+	l.hostPath = path.Join(newDir.(*localFile).hostPath, newName)
+}
+
 // extractErrno tries to determine the errno.
 func extractErrno(err error) syscall.Errno {
 	if err == nil {
diff --git a/runsc/fsgofer/fsgofer_test.go b/runsc/fsgofer/fsgofer_test.go
index 48860f952..34033245b 100644
--- a/runsc/fsgofer/fsgofer_test.go
+++ b/runsc/fsgofer/fsgofer_test.go
@@ -415,22 +415,22 @@ func TestLink(t *testing.T) {
 
 func TestROMountChecks(t *testing.T) {
 	runCustom(t, allTypes, roConfs, func(t *testing.T, s state) {
-		if _, _, _, _, err := s.file.Create("..", p9.ReadWrite, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != syscall.EBADF {
+		if _, _, _, _, err := s.file.Create("some_file", p9.ReadWrite, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != syscall.EBADF {
 			t.Errorf("%v: Create() should have failed, got: %v, expected: syscall.EBADF", s, err)
 		}
-		if _, err := s.file.Mkdir("..", 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != syscall.EBADF {
+		if _, err := s.file.Mkdir("some_dir", 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != syscall.EBADF {
 			t.Errorf("%v: MkDir() should have failed, got: %v, expected: syscall.EBADF", s, err)
 		}
-		if err := s.file.Rename(s.file, ".."); err != syscall.EBADF {
+		if err := s.file.RenameAt("some_file", s.file, "other_file"); err != syscall.EBADF {
 			t.Errorf("%v: Rename() should have failed, got: %v, expected: syscall.EBADF", s, err)
 		}
-		if _, err := s.file.Symlink("some_place", "..", p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != syscall.EBADF {
+		if _, err := s.file.Symlink("some_place", "some_symlink", p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != syscall.EBADF {
 			t.Errorf("%v: Symlink() should have failed, got: %v, expected: syscall.EBADF", s, err)
 		}
-		if err := s.file.UnlinkAt("..", 0); err != syscall.EBADF {
+		if err := s.file.UnlinkAt("some_file", 0); err != syscall.EBADF {
 			t.Errorf("%v: UnlinkAt() should have failed, got: %v, expected: syscall.EBADF", s, err)
 		}
-		if err := s.file.Link(s.file, ".."); err != syscall.EBADF {
+		if err := s.file.Link(s.file, "some_link"); err != syscall.EBADF {
 			t.Errorf("%v: Link() should have failed, got: %v, expected: syscall.EBADF", s, err)
 		}
 
@@ -445,12 +445,12 @@ func TestROMountChecks(t *testing.T) {
 func TestROMountPanics(t *testing.T) {
 	conf := Config{ROMount: true, PanicOnWrite: true}
 	runCustom(t, allTypes, []Config{conf}, func(t *testing.T, s state) {
-		assertPanic(t, func() { s.file.Create("..", p9.ReadWrite, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())) })
-		assertPanic(t, func() { s.file.Mkdir("..", 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())) })
-		assertPanic(t, func() { s.file.Rename(s.file, "..") })
-		assertPanic(t, func() { s.file.Symlink("some_place", "..", p9.UID(os.Getuid()), p9.GID(os.Getgid())) })
-		assertPanic(t, func() { s.file.UnlinkAt("..", 0) })
-		assertPanic(t, func() { s.file.Link(s.file, "..") })
+		assertPanic(t, func() { s.file.Create("some_file", p9.ReadWrite, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())) })
+		assertPanic(t, func() { s.file.Mkdir("some_dir", 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())) })
+		assertPanic(t, func() { s.file.RenameAt("some_file", s.file, "other_file") })
+		assertPanic(t, func() { s.file.Symlink("some_place", "some_symlink", p9.UID(os.Getuid()), p9.GID(os.Getgid())) })
+		assertPanic(t, func() { s.file.UnlinkAt("some_file", 0) })
+		assertPanic(t, func() { s.file.Link(s.file, "some_link") })
 
 		valid := p9.SetAttrMask{Size: true}
 		attr := p9.SetAttr{Size: 0}
@@ -458,60 +458,6 @@ func TestROMountPanics(t *testing.T) {
 	})
 }
 
-func TestInvalidName(t *testing.T) {
-	runCustom(t, []fileType{regular}, rwConfs, func(t *testing.T, s state) {
-		if _, _, _, _, err := s.file.Create("..", p9.ReadWrite, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != syscall.EINVAL {
-			t.Errorf("%v: Create() should have failed, got: %v, expected: syscall.EINVAL", s, err)
-		}
-		if _, _, err := s.file.Walk([]string{".."}); err != syscall.EINVAL {
-			t.Errorf("%v: Walk() should have failed, got: %v, expected: syscall.EINVAL", s, err)
-		}
-		if _, err := s.file.Mkdir("..", 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != syscall.EINVAL {
-			t.Errorf("%v: MkDir() should have failed, got: %v, expected: syscall.EINVAL", s, err)
-		}
-		if err := s.file.Rename(s.file, ".."); err != syscall.EINVAL {
-			t.Errorf("%v: Rename() should have failed, got: %v, expected: syscall.EINVAL", s, err)
-		}
-		if _, err := s.file.Symlink("some_place", "..", p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != syscall.EINVAL {
-			t.Errorf("%v: Symlink() should have failed, got: %v, expected: syscall.EINVAL", s, err)
-		}
-		if err := s.file.UnlinkAt("..", 0); err != syscall.EINVAL {
-			t.Errorf("%v: UnlinkAt() should have failed, got: %v, expected: syscall.EINVAL", s, err)
-		}
-		if err := s.file.Link(s.file, ".."); err != syscall.EINVAL {
-			t.Errorf("%v: Link() should have failed, got: %v, expected: syscall.EINVAL", s, err)
-		}
-	})
-}
-
-func TestIsNameValid(t *testing.T) {
-	valid := []string{
-		"name",
-		"123",
-		"!@#$%^&*()",
-		".name",
-		"..name",
-		"...",
-	}
-	for _, s := range valid {
-		if got := isNameValid(s); !got {
-			t.Errorf("isNameValid(%s) failed, got: %v, expected: true", s, got)
-		}
-	}
-	invalid := []string{
-		".",
-		"..",
-		"name/name",
-		"/name",
-		"name/",
-	}
-	for _, s := range invalid {
-		if got := isNameValid(s); got {
-			t.Errorf("isNameValid(%s) failed, got: %v, expected: false", s, got)
-		}
-	}
-}
-
 func TestWalkNotFound(t *testing.T) {
 	runCustom(t, []fileType{directory}, allConfs, func(t *testing.T, s state) {
 		if _, _, err := s.file.Walk([]string{"nobody-here"}); err != syscall.ENOENT {