runsc: Promote getExecutablePathInternal to getExecutablePath.

Remove GetExecutablePath (the non-internal version).  This makes path handling
more consistent between exec, root, and child containers.

The new getExecutablePath now uses MountNamespace.FindInode, which is more
robust than Walking the Dirent tree ourselves.

This also removes the last use of lstat(2) in the sentry, so that can be
removed from the filters.

PiperOrigin-RevId: 211683110
Change-Id: Ic8ec960fc1c267aa7d310b8efe6e900c88a9207a

7 files changed, 69 insertions, 86 deletions

diff --git a/runsc/boot/controller.go b/runsc/boot/controller.go
index fdb6be5b1..ec1110059 100644
--- a/runsc/boot/controller.go
+++ b/runsc/boot/controller.go
@@ -228,6 +228,16 @@ func (cm *containerManager) Start(args *StartArgs, _ *struct{}) error {
 // Execute runs a command on a created or running sandbox.
 func (cm *containerManager) Execute(e *control.ExecArgs, waitStatus *uint32) error {
 	log.Debugf("containerManager.Execute: %+v", *e)
+
+	if e.Filename == "" {
+		rootCtx := cm.l.rootProcArgs.NewContext(cm.l.k)
+		rootMns := cm.l.k.RootMountNamespace()
+		var err error
+		if e.Filename, err = getExecutablePath(rootCtx, rootMns, e.Argv[0], e.Envv); err != nil {
+			return fmt.Errorf("error getting executable path for %q: %v", e.Argv[0], err)
+		}
+	}
+
 	proc := control.Proc{Kernel: cm.l.k}
 	if err := proc.Exec(e, waitStatus); err != nil {
 		return fmt.Errorf("error executing: %+v: %v", e, err)
diff --git a/runsc/boot/filter/config.go b/runsc/boot/filter/config.go
index 113023bdd..f864b1f45 100644
--- a/runsc/boot/filter/config.go
+++ b/runsc/boot/filter/config.go
@@ -164,10 +164,7 @@ var allowedSyscalls = seccomp.SyscallRules{
 			seccomp.AllowAny{}, /* winsize struct */
 		},
 	},
-	syscall.SYS_LSEEK: {},
-	// TODO: Remove SYS_LSTAT when executable lookup moves
-	// into the gofer.
-	syscall.SYS_LSTAT:   {},
+	syscall.SYS_LSEEK:   {},
 	syscall.SYS_MADVISE: {},
 	syscall.SYS_MINCORE: {},
 	syscall.SYS_MMAP: []seccomp.Rule{
diff --git a/runsc/boot/fs.go b/runsc/boot/fs.go
index 20d0e42ef..4a11b30f1 100644
--- a/runsc/boot/fs.go
+++ b/runsc/boot/fs.go
@@ -701,13 +701,13 @@ func setFileSystemForProcess(procArgs *kernel.CreateProcessArgs, spec *specs.Spe
 	return nil
 }
 
-// GetExecutablePathInternal traverses the *container's* filesystem to resolve
-// exec's absolute path. For example, if the container is being served files by
-// the fsgofer serving /foo/bar as the container root, it will search within
+// getExecutablePath traverses the *container's* filesystem to resolve exec's
+// absolute path. For example, if the container is being served files by the
+// fsgofer serving /foo/bar as the container root, it will search within
 // /foo/bar, not the host root.
 // TODO: Unit test this.
-func GetExecutablePathInternal(ctx context.Context, procArgs *kernel.CreateProcessArgs) (string, error) {
-	exec := filepath.Clean(procArgs.Filename)
+func getExecutablePath(ctx context.Context, mns *fs.MountNamespace, filename string, env []string) (string, error) {
+	exec := filepath.Clean(filename)
 
 	// Don't search PATH if exec is a path to a file (absolute or relative).
 	if strings.IndexByte(exec, '/') >= 0 {
@@ -716,31 +716,52 @@ func GetExecutablePathInternal(ctx context.Context, procArgs *kernel.CreateProce
 
 	// Search the PATH for a file whose name matches the one we are looking
 	// for.
-	pathDirs := specutils.GetPath(procArgs.Envv)
+	pathDirs := specutils.GetPath(env)
 	for _, p := range pathDirs {
-		// Walk to the end of the path.
-		curDir := procArgs.Root
-		for _, pc := range strings.Split(p, "/") {
-			var err error
-			if curDir, err = curDir.Walk(ctx, curDir, pc); err != nil {
-				break
-			}
-		}
-		if curDir == nil {
+		// Try to find the binary inside path p.
+		binPath := path.Join(p, filename)
+		root := fs.RootFromContext(ctx)
+		defer root.DecRef()
+		d, err := mns.FindInode(ctx, root, nil, binPath, linux.MaxSymlinkTraversals)
+		if err == syserror.ENOENT || err == syserror.EACCES {
 			continue
 		}
-		// Check for the executable in the path directory.
-		dirent, err := curDir.Walk(ctx, curDir, exec)
 		if err != nil {
-			continue
+			return "", fmt.Errorf("FindInode(%q) failed: %v", binPath, err)
 		}
-		// Check whether we can read and execute the file in question.
-		if err := dirent.Inode.CheckPermission(ctx, fs.PermMask{Read: true, Execute: true}); err != nil {
-			log.Infof("Found executable at %q, but user cannot execute it: %v", path.Join(p, exec), err)
+		defer d.DecRef()
+
+		// Check whether we can read and execute the found file.
+		if err := d.Inode.CheckPermission(ctx, fs.PermMask{Read: true, Execute: true}); err != nil {
+			log.Infof("Found executable at %q, but user cannot execute it: %v", binPath, err)
 			continue
 		}
 		return path.Join("/", p, exec), nil
 	}
 
-	return "", fmt.Errorf("could not find executable %s in path %v", exec, pathDirs)
+	return "", fmt.Errorf("could not find executable %q in path %v", exec, pathDirs)
+}
+
+// setExecutablePath sets the procArgs.Filename by searching the PATH for an
+// executable matching the procArgs.Argv[0].
+func setExecutablePath(ctx context.Context, mns *fs.MountNamespace, procArgs *kernel.CreateProcessArgs) error {
+	if procArgs.Filename != "" {
+		// Sanity check.
+		if !path.IsAbs(procArgs.Filename) {
+			return fmt.Errorf("filename must be absolute: %q", procArgs.Filename)
+		}
+		// Nothing to set.
+		return nil
+	}
+
+	if len(procArgs.Argv) == 0 {
+		return fmt.Errorf("Argv must not be empty")
+	}
+
+	f, err := getExecutablePath(ctx, mns, procArgs.Argv[0], procArgs.Envv)
+	if err != nil {
+		return err
+	}
+	procArgs.Filename = f
+	return nil
 }
diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go
index 74d0c2534..2733c4d69 100644
--- a/runsc/boot/loader.go
+++ b/runsc/boot/loader.go
@@ -271,16 +271,8 @@ func newProcess(spec *specs.Spec, creds *auth.Credentials, utsns *kernel.UTSName
 		return kernel.CreateProcessArgs{}, fmt.Errorf("error creating limits: %v", err)
 	}
 
-	// Get the executable path, which is a bit tricky because we have to
-	// inspect the environment PATH which is relative to the root path.
-	exec, err := specutils.GetExecutablePath(spec.Process.Args[0], spec.Root.Path, spec.Process.Env)
-	if err != nil {
-		return kernel.CreateProcessArgs{}, fmt.Errorf("error getting executable path: %v", err)
-	}
-
 	// Create the process arguments.
 	procArgs := kernel.CreateProcessArgs{
-		Filename:             exec,
 		Argv:                 spec.Process.Args,
 		Envv:                 spec.Process.Env,
 		WorkingDirectory:     spec.Process.Cwd, // Defaults to '/' if empty.
@@ -365,7 +357,7 @@ func (l *Loader) run() error {
 	// If we are restoring, we do not want to create a process.
 	// l.restore is set by the container manager when a restore call is made.
 	if !l.restore {
-		err := setFileSystemForProcess(
+		if err := setFileSystemForProcess(
 			&l.rootProcArgs,
 			l.spec,
 			l.conf,
@@ -374,10 +366,16 @@ func (l *Loader) run() error {
 			l.rootProcArgs.Credentials,
 			l.rootProcArgs.Limits,
 			l.k,
-			"" /* CID, which isn't needed for the root container */)
-		if err != nil {
+			"" /* CID, which isn't needed for the root container */); err != nil {
 			return err
 		}
+
+		rootCtx := l.rootProcArgs.NewContext(l.k)
+		rootMns := l.k.RootMountNamespace()
+		if err := setExecutablePath(rootCtx, rootMns, &l.rootProcArgs); err != nil {
+			return fmt.Errorf("error setting executable path for %+v: %v", l.rootProcArgs, err)
+		}
+
 		// Create the root container init task.
 		if _, err := l.k.CreateProcess(l.rootProcArgs); err != nil {
 			return fmt.Errorf("failed to create init process: %v", err)
@@ -443,7 +441,7 @@ func (l *Loader) startContainer(k *kernel.Kernel, spec *specs.Spec, conf *Config
 		ioFDs = append(ioFDs, fd)
 	}
 
-	err = setFileSystemForProcess(
+	if err := setFileSystemForProcess(
 		&procArgs,
 		spec,
 		conf,
@@ -452,13 +450,14 @@ func (l *Loader) startContainer(k *kernel.Kernel, spec *specs.Spec, conf *Config
 		creds,
 		procArgs.Limits,
 		k,
-		cid)
-	if err != nil {
+		cid); err != nil {
 		return 0, fmt.Errorf("failed to create new process: %v", err)
 	}
 
-	if procArgs.Filename, err = GetExecutablePathInternal(procArgs.NewContext(k), &procArgs); err != nil {
-		return 0, err
+	ctx := procArgs.NewContext(l.k)
+	mns := k.RootMountNamespace()
+	if err := setExecutablePath(ctx, mns, &procArgs); err != nil {
+		return 0, fmt.Errorf("error setting executable path for %+v: %v", procArgs, err)
 	}
 
 	tg, err := l.k.CreateProcess(procArgs)
diff --git a/runsc/cmd/exec.go b/runsc/cmd/exec.go
index b84a80119..966d2e258 100644
--- a/runsc/cmd/exec.go
+++ b/runsc/cmd/exec.go
@@ -140,16 +140,6 @@ func (ex *Exec) Execute(_ context.Context, f *flag.FlagSet, args ...interface{})
 		}
 	}
 
-	// Get the executable path, which is a bit tricky because we have to
-	// inspect the environment PATH which is relative to the root path.
-	// If the user is overriding environment variables, PATH may have been
-	// overwritten.
-	rootPath := c.Spec.Root.Path
-	e.Filename, err = specutils.GetExecutablePath(e.Argv[0], rootPath, e.Envv)
-	if err != nil {
-		Fatalf("error getting executable path: %v", err)
-	}
-
 	ws, err := c.Execute(e)
 	if err != nil {
 		Fatalf("error getting processes for container: %v", err)
diff --git a/runsc/container/container_test.go b/runsc/container/container_test.go
index e7e53c492..5f452acbf 100644
--- a/runsc/container/container_test.go
+++ b/runsc/container/container_test.go
@@ -521,7 +521,6 @@ func TestExec(t *testing.T) {
 		execArgs := control.ExecArgs{
 			Filename:         "/bin/sleep",
 			Argv:             []string{"sleep", "5"},
-			Envv:             []string{"PATH=" + os.Getenv("PATH")},
 			WorkingDirectory: "/",
 			KUID:             uid,
 		}
@@ -889,7 +888,6 @@ func TestPauseResume(t *testing.T) {
 		execArgs := control.ExecArgs{
 			Filename:         "/bin/bash",
 			Argv:             []string{"bash", "-c", script},
-			Envv:             []string{"PATH=" + os.Getenv("PATH")},
 			WorkingDirectory: "/",
 			KUID:             uid,
 		}
@@ -1070,7 +1068,6 @@ func TestCapabilities(t *testing.T) {
 		execArgs := control.ExecArgs{
 			Filename:         exePath,
 			Argv:             []string{exePath},
-			Envv:             []string{"PATH=" + os.Getenv("PATH")},
 			WorkingDirectory: "/",
 			KUID:             uid,
 			KGID:             gid,
diff --git a/runsc/specutils/specutils.go b/runsc/specutils/specutils.go
index 3234cc088..551718e9a 100644
--- a/runsc/specutils/specutils.go
+++ b/runsc/specutils/specutils.go
@@ -163,37 +163,6 @@ func ReadSpecFromFile(bundleDir string, specFile *os.File) (*specs.Spec, error)
 	return &spec, nil
 }
 
-// GetExecutablePath returns the absolute path to the executable, relative to
-// the root. It searches the environment PATH for the first file that exists
-// with the given name.
-// TODO: Remove this in favor of finding executables via
-// boot.GetExecutablePathInternal.
-func GetExecutablePath(exec, root string, env []string) (string, error) {
-	exec = filepath.Clean(exec)
-
-	// Don't search PATH if exec is a path to a file (absolute or relative).
-	if strings.IndexByte(exec, '/') >= 0 {
-		return exec, nil
-	}
-
-	// Search the PATH for a file whose name matches the one we are looking
-	// for.
-	path := GetPath(env)
-	for _, p := range path {
-		abs := filepath.Join(root, p, exec)
-		// Do not follow symlink link because the target is in the container
-		// root filesystem.
-		if _, err := os.Lstat(abs); err == nil {
-			// We found it!  Return the path relative to the root.
-			return filepath.Join("/", p, exec), nil
-		}
-	}
-
-	// Could not find a suitable path, just return the original string.
-	log.Warningf("could not find executable %s in path %s", exec, path)
-	return exec, nil
-}
-
 // GetPath returns the PATH as a slice of strings given the environemnt
 // variables.
 func GetPath(env []string) []string {