Address PR comments

workers/generic-worker/README.md

@@ -46,8 +46,9 @@ and reports back results to the queue.
                                             to stdout. Intended for internal use.
     create-file                             This will create a file at the specified path.
                                             Intended for internal use.
-    create-dir                              This will create a directory (and any subdirectories)
-                                            at the specified path. Intended for internal use.
+    create-dir                              This will create a directory (including missing
+                                            parent directories) at the specified path.
+                                            Intended for internal use.
 
   Options:
     --config CONFIG-FILE                    Json configuration file to use. See

workers/generic-worker/mounts.go

@@ -144,13 +144,6 @@ func MkdirAll(taskMount *TaskMount, dir string) error {
 	return MkdirAllTaskUser(dir)
 }
 
-func MkdirAllOrDie(taskMount *TaskMount, dir string) {
-	err := MkdirAll(taskMount, dir)
-	if err != nil {
-		panic(fmt.Errorf("[mounts] Not able to create directory %v: %v", dir, err))
-	}
-}
-
 func (cm *CacheMap) LoadFromFile(stateFile string, cacheDir string) {
 	_, err := os.Stat(stateFile)
 	if err != nil {
@@ -513,8 +506,11 @@ func (w *WritableDirectoryCache) Mount(taskMount *TaskMount) error {
 		src := directoryCaches[w.CacheName].Location
 		parentDir := filepath.Dir(target)
 		taskMount.Infof("Moving existing writable directory cache %v from %v to %v", w.CacheName, src, target)
-		MkdirAllOrDie(taskMount, parentDir)
-		err := RenameCrossDevice(src, target)
+		err := MkdirAll(taskMount, parentDir)
+		if err != nil {
+			return fmt.Errorf("[mounts] Not able to create directory %v: %v", parentDir, err)
+		}
+		err = RenameCrossDevice(src, target)
 		if err != nil {
 			panic(fmt.Errorf("[mounts] Not able to rename dir %v as %v: %v", src, target, err))
 		}
@@ -542,7 +538,10 @@ func (w *WritableDirectoryCache) Mount(taskMount *TaskMount) error {
 			}
 		} else {
 			// no preloaded content => just create dir in place
-			MkdirAllOrDie(taskMount, target)
+			err := MkdirAll(taskMount, target)
+			if err != nil {
+				return fmt.Errorf("[mounts] Not able to create directory %v: %v", target, err)
+			}
 		}
 	}
 	// Regardless of whether we are running as current user, grant task user access
@@ -633,7 +632,7 @@ func (f *FileMount) Mount(taskMount *TaskMount) error {
 
 	file := filepath.Join(taskContext.TaskDir, f.File)
 	if info, err := os.Stat(file); err == nil && info.IsDir() {
-		return fmt.Errorf("Cannot mount file %v since it is a directory", file)
+		return fmt.Errorf("Cannot mount file at path %v since it already exists as a directory", file)
 	}
 	err = decompress(fsContent, f.Format, file, taskMount)
 	if err != nil {

workers/generic-worker/mounts_multiuser_test.go

@@ -0,0 +1,50 @@
+//go:build multiuser
+
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/mcuadros/go-defaults"
+	"github.com/taskcluster/taskcluster/v59/workers/generic-worker/fileutil"
+)
+
+func TestTaskUserCannotMountInPrivilegedLocation(t *testing.T) {
+	setup(t)
+
+	if config.RunTasksAsCurrentUser {
+		t.Skip("This test is only relevant when running as task user")
+	}
+
+	dir, err := os.MkdirTemp(taskContext.TaskDir, t.Name())
+	if err != nil {
+		t.Fatalf("Failed to create temporary directory: %v", err)
+	}
+	defer os.RemoveAll(dir)
+
+	err = fileutil.SecureFiles(dir)
+	if err != nil {
+		t.Fatalf("Failed to secure temporary directory: %v", err)
+	}
+
+	mounts := []MountEntry{
+		&WritableDirectoryCache{
+			CacheName: "banana-cache",
+			Directory: filepath.Join("../../../", filepath.Base(dir)),
+		},
+	}
+
+	payload := GenericWorkerPayload{
+		Mounts:     toMountArray(t, &mounts),
+		Command:    helloGoodbye(),
+		MaxRunTime: 180,
+	}
+	defaults.SetDefaults(&payload)
+
+	td := testTask(t)
+	td.Scopes = append(td.Scopes, "generic-worker:cache:banana-cache")
+
+	_ = submitAndAssert(t, td, payload, "failed", "failed")
+}

workers/generic-worker/mounts_test.go

@@ -520,7 +520,7 @@ func TestMountFileAtCWD(t *testing.T) {
 			TaskRunReasonResolved:  "failed",
 			PerTaskRunLogExcerpts: [][]string{
 				{
-					`Cannot mount file .* since it is a directory`,
+					"Cannot mount file at path .* since it already exists as a directory",
 				},
 			},
 		},

workers/generic-worker/multiuser.go

@@ -237,7 +237,11 @@ func MkdirAllTaskUser(dir string) error {
 		if err != nil {
 			return err
 		}
-		return file.Close()
+		err = file.Close()
+		if err != nil {
+			return err
+		}
+		return os.Remove(file.Name())
 	}
 
 	cmd, err := process.NewCommand([]string{gwruntime.GenericWorkerBinary(), "create-dir", "--create-dir", dir}, taskContext.TaskDir, []string{}, taskContext.pd)

workers/generic-worker/usage.go

@@ -62,8 +62,9 @@ and reports back results to the queue.
                                             to stdout. Intended for internal use.
     create-file                             This will create a file at the specified path.
                                             Intended for internal use.
-    create-dir                              This will create a directory (and any subdirectories)
-                                            at the specified path. Intended for internal use.
+    create-dir                              This will create a directory (including missing
+                                            parent directories) at the specified path.
+                                            Intended for internal use.
 
   Options:
     --config CONFIG-FILE                    Json configuration file to use. See