Bug fix: Generic Worker: Process absolute paths in task payload correctly

changelog/issue-6689.md

@@ -0,0 +1,17 @@
+audience: users
+level: major
+reference: issue 6689
+---
+Breaking change: Generic Worker now evaluates absolute paths inside `mounts`
+(properties `directory` and `file`) and artifacts (property `path`) correctly.
+Previously Generic Worker would effectively strip leading path separators and
+treat them as relative paths inside the task directory. For example, `/tmp`
+would be resolved as the relative path `tmp` from inside the task directory.
+
+Although this is technically a bug fix, it does change the behaviour of Generic
+Worker when absolute paths are specified in task payloads. We have examined
+production tasks on both the Community and Firefox CI deployments of
+taskcluster, and are reasonably confident that this change should not have
+adverse affects on existing tasks. However we are bumping the major version
+number of the taskcluster release, in recognition of the backward
+incompatibility.

taskcluster/kinds/generic-worker/kind.yml

@@ -171,7 +171,7 @@ tasks:
  cp "${{TASK_USER_CREDENTIALS}}" next-task-user.json
  GORACE=history_size=7 CGO_ENABLED=1 go test -tags "${{ENGINE}}" -failfast -timeout 20m -ldflags "-X github.com/taskcluster/taskcluster/v59/workers/generic-worker.revision=${{GITHUB_SHA}}" ${{RACE}} ${{VET}} ./...
  else
- GORACE=history_size=7 CGO_ENABLED=${{CGO_ENABLED_TESTS}} go test -tags "${{ENGINE}}" -failfast -timeout 20m -ldflags "-X github.com/taskcluster/taskcluster/v59/workers/generic-worker.revision=${{GITHUB_SHA}}" ${{RACE}} ${{VET}} ./...
+ GORACE=history_size=7 CGO_ENABLED=${{CGO_ENABLED_TESTS}} go test -v -run TestMounts -tags "${{ENGINE}}" -failfast -timeout 20m -ldflags "-X github.com/taskcluster/taskcluster/v59/workers/generic-worker.revision=${{GITHUB_SHA}}" ${{RACE}} ${{VET}} ./...
  fi
  ../../../golangci-lint/golangci-lint-$GOLANGCI_LINT_VERSION-*/golangci-lint run --build-tags "${{ENGINE}}" --timeout=5m
  build/test-multiuser-current-user-ubuntu-22.04-amd64:
@@ -365,7 +365,7 @@ tasks:
  - set CGO_ENABLED=%CGO_ENABLED_TESTS%
  - set GORACE=history_size=7
  - copy "%TASK_USER_CREDENTIALS%" "%CD%\next-task-user.json"
- - go test -tags "%ENGINE%" -failfast -timeout 20m -ldflags "-X github.com/taskcluster/taskcluster/v59/workers/generic-worker.revision=%GITHUB_SHA%" %RACE% %VET% ./...
+ - go test -v -run TestMounts -tags "%ENGINE%" -failfast -timeout 20m -ldflags "-X github.com/taskcluster/taskcluster/v59/workers/generic-worker.revision=%GITHUB_SHA%" %RACE% %VET% ./...
  - |
  :: assumption here is that if something inside the if fails, we'll get a non zero exit code
  :: i've also made it an if/else so that one of them has to run, as there should always be a

workers/generic-worker/artifacts.go

@@ -19,6 +19,7 @@ import (
  tcclient "github.com/taskcluster/taskcluster/v59/clients/client-go"
  "github.com/taskcluster/taskcluster/v59/clients/client-go/tcqueue"
  "github.com/taskcluster/taskcluster/v59/workers/generic-worker/artifacts"
+ "github.com/taskcluster/taskcluster/v59/workers/generic-worker/fileutil"
 )
 
 var (
@@ -83,7 +84,7 @@ func (task *TaskRun) PayloadArtifacts() []artifacts.TaskArtifact {
  // cause the task to fail, and the cause to be preserved in the
  // error artifact.
  case incomingErr != nil:
- fullPath := filepath.Join(taskContext.TaskDir, subPath)
+ fullPath := fileutil.AbsFrom(taskContext.TaskDir, subPath)
  payloadArtifacts = append(
  payloadArtifacts,
  &artifacts.ErrorArtifact{
@@ -104,7 +105,7 @@ func (task *TaskRun) PayloadArtifacts() []artifacts.TaskArtifact {
  }
  // Any error returned here should already have been handled by
  // walkFn, so should be safe to ignore.
- _ = filepath.Walk(filepath.Join(taskContext.TaskDir, basePath), walkFn)
+ _ = filepath.Walk(fileutil.AbsFrom(taskContext.TaskDir, basePath), walkFn)
  }
  }
  return payloadArtifacts
@@ -120,7 +121,7 @@ func (task *TaskRun) PayloadArtifacts() []artifacts.TaskArtifact {
 // "invalid-resource-on-worker" ErrorArtifact
 // TODO: need to also handle "too-large-file-on-worker"
 func resolve(base *artifacts.BaseArtifact, artifactType string, path string, contentType string, contentEncoding string) artifacts.TaskArtifact {
- fullPath := filepath.Join(taskContext.TaskDir, path)
+ fullPath := fileutil.AbsFrom(taskContext.TaskDir, path)
  fileReader, err := os.Open(fullPath)
  if err != nil {
  // cannot read file/dir, create an error artifact

workers/generic-worker/artifacts_test.go

@@ -1032,3 +1032,32 @@ func TestObjectArtifact(t *testing.T) {
  td := testTask(t)
  _ = submitAndAssert(t, td, payload, "completed", "completed")
 }
+
+func TestFileArtifactWithAbsolutePath(t *testing.T) {
+
+ setup(t)
+ validateArtifacts(t,
+
+ // what appears in task payload
+ []Artifact{
+ {
+ Expires: inAnHour,
+ Path: filepath.Join(cwd, filepath.Join("testdata", "SampleArtifacts", "b", "c", "d.jpg")),
+ Type: "file",
+ Name: "public/build/firefox.exe",
+ },
+ },
+
+ // what we expect to discover on file system
+ []artifacts.TaskArtifact{
+ &artifacts.S3Artifact{
+ BaseArtifact: &artifacts.BaseArtifact{
+ Name: "public/build/firefox.exe",
+ Expires: inAnHour,
+ },
+ ContentType: "image/jpeg",
+ ContentEncoding: "identity",
+ Path: filepath.Join(cwd, filepath.Join("testdata", "SampleArtifacts", "b", "c", "d.jpg")),
+ },
+ })
+}

workers/generic-worker/chain_of_trust.go

@@ -127,15 +127,15 @@ func (feature *ChainOfTrustTaskFeature) Stop(err *ExecutionErrors) {
  if feature.disabled {
  return
  }
- logFile := filepath.Join(taskContext.TaskDir, logPath)
- certifiedLogFile := filepath.Join(taskContext.TaskDir, certifiedLogPath)
- unsignedCert := filepath.Join(taskContext.TaskDir, unsignedCertPath)
- ed25519SignedCert := filepath.Join(taskContext.TaskDir, ed25519SignedCertPath)
+ logFile := fileutil.AbsFrom(taskContext.TaskDir, logPath)
+ certifiedLogFile := fileutil.AbsFrom(taskContext.TaskDir, certifiedLogPath)
+ unsignedCert := fileutil.AbsFrom(taskContext.TaskDir, unsignedCertPath)
+ ed25519SignedCert := fileutil.AbsFrom(taskContext.TaskDir, ed25519SignedCertPath)
  copyErr := copyFileContents(logFile, certifiedLogFile)
  if copyErr != nil {
  panic(copyErr)
  }
- err.add(feature.task.uploadLog(certifiedLogName, filepath.Join(taskContext.TaskDir, certifiedLogPath)))
+ err.add(feature.task.uploadLog(certifiedLogName, fileutil.AbsFrom(taskContext.TaskDir, certifiedLogPath)))
  artifactHashes := map[string]ArtifactHash{}
  for _, artifact := range feature.task.Artifacts {
  // make sure SHA256 is calculated
@@ -188,7 +188,7 @@ func (feature *ChainOfTrustTaskFeature) Stop(err *ExecutionErrors) {
  if e != nil {
  panic(e)
  }
- err.add(feature.task.uploadLog(unsignedCertName, filepath.Join(taskContext.TaskDir, unsignedCertPath)))
+ err.add(feature.task.uploadLog(unsignedCertName, fileutil.AbsFrom(taskContext.TaskDir, unsignedCertPath)))
 
  // create detached ed25519 chain-of-trust.json.sig
  sig := ed25519.Sign(feature.ed25519PrivKey, certBytes)
@@ -202,8 +202,8 @@ func (feature *ChainOfTrustTaskFeature) Stop(err *ExecutionErrors) {
  Name: ed25519SignedCertName,
  Expires: feature.task.Definition.Expires,
  },
- filepath.Join(taskContext.TaskDir, ed25519SignedCertPath),
- filepath.Join(taskContext.TaskDir, ed25519SignedCertPath),
+ fileutil.AbsFrom(taskContext.TaskDir, ed25519SignedCertPath),
+ fileutil.AbsFrom(taskContext.TaskDir, ed25519SignedCertPath),
  "application/octet-stream",
  "gzip",
  ),

workers/generic-worker/fileutil/filepath.go

@@ -0,0 +1,15 @@
+package fileutil
+
+import (
+ "path/filepath"
+)
+
+// AbsFrom returns path if an absolute path, otherwise the result of joining it
+// to the parent path. This mimics filepath.Abs(path) without restricting
+// parent to the current working directory.
+func AbsFrom(parent string, path string) string {
+ if filepath.IsAbs(path) {
+ return path
+ }
+ return filepath.Join(parent, path)
+}

workers/generic-worker/helper_test.go

@@ -97,6 +97,7 @@ func scheduleNamedTask[P GenericWorkerPayload | dockerworker.DockerWorkerPayload
  t.Fatalf("Could not submit task: %v", err)
  }
  t.Logf("Scheduled task %v", taskID)
+ t.Logf("%v", string(td.Payload))
 }
 
 func execute(t *testing.T, expectedExitCode ExitCode) {
@@ -162,18 +163,18 @@ func ensureResolution(t *testing.T, taskID, state, reason string) {
  if err != nil {
  t.Fatal("Error retrieving status from queue")
  }
+ t.Log("Task logs:")
+ // This extra space is *super-useful* for breaking up the output since
+ // this shows a task log embedded inside a different task log
+ t.Log("")
+ t.Log("")
+ t.Log("")
+ t.Log(LogText(t))
+ t.Log("")
+ t.Log("")
+ t.Log("")
  if status.Status.Runs[0].State != state || status.Status.Runs[0].ReasonResolved != reason {
- t.Logf("Expected task %v to resolve as '%v/%v' but resolved as '%v/%v'", taskID, state, reason, status.Status.Runs[0].State, status.Status.Runs[0].ReasonResolved)
- t.Log("Task logs:")
- // This extra space is *super-useful* for breaking up the output since
- // this shows a task log embedded inside a different task log
- t.Log("")
- t.Log("")
- t.Log("")
- t.Fatal(LogText(t))
- t.Log("")
- t.Log("")
- t.Log("")
+ t.Fatalf("Expected task %v to resolve as '%v/%v' but resolved as '%v/%v'", taskID, state, reason, status.Status.Runs[0].State, status.Status.Runs[0].ReasonResolved)
  } else {
  t.Logf("Task %v resolved as %v/%v as required.", taskID, status.Status.Runs[0].State, status.Status.Runs[0].ReasonResolved)
  }
@@ -193,7 +194,7 @@ func ensureDirContainsNFiles(t *testing.T, dir string, n int) {
 
 func LogText(t *testing.T) string {
  t.Helper()
- bytes, err := os.ReadFile(filepath.Join(taskContext.TaskDir, logPath))
+ bytes, err := os.ReadFile(fileutil.AbsFrom(taskContext.TaskDir, logPath))
  if err != nil {
  t.Fatalf("Error when trying to read log file: %v", err)
  }

workers/generic-worker/main.go

@@ -922,7 +922,7 @@ func (task *TaskRun) kill() {
 }
 
 func (task *TaskRun) createLogFile() *os.File {
- absLogFile := filepath.Join(taskContext.TaskDir, logPath)
+ absLogFile := fileutil.AbsFrom(taskContext.TaskDir, logPath)
  logFileHandle, err := os.Create(absLogFile)
  if err != nil {
  panic(err)
@@ -979,10 +979,10 @@ func (task *TaskRun) Run() (err *ExecutionErrors) {
  }
  task.closeLog(logHandle)
  if task.Payload.Features.BackingLog {
- err.add(task.uploadLog(task.Payload.Logs.Backing, filepath.Join(taskContext.TaskDir, logPath)))
+ err.add(task.uploadLog(task.Payload.Logs.Backing, fileutil.AbsFrom(taskContext.TaskDir, logPath)))
  }
  if config.CleanUpTaskDirs {
- _ = os.Remove(filepath.Join(taskContext.TaskDir, logPath))
+ _ = os.Remove(fileutil.AbsFrom(taskContext.TaskDir, logPath))
  }
  }()
 
@@ -1183,7 +1183,7 @@ func PrepareTaskEnvironment() (reboot bool) {
  if PlatformTaskEnvironmentSetup(taskDirName) {
  return true
  }
- logDir := filepath.Join(taskContext.TaskDir, filepath.Dir(logPath))
+ logDir := fileutil.AbsFrom(taskContext.TaskDir, filepath.Dir(logPath))
  err := os.MkdirAll(logDir, 0700)
  if err != nil {
  panic(err)

workers/generic-worker/main_test.go

@@ -13,6 +13,7 @@ import (
  "github.com/mcuadros/go-defaults"
  "github.com/stretchr/testify/require"
  "github.com/taskcluster/slugid-go/slugid"
+ "github.com/taskcluster/taskcluster/v59/workers/generic-worker/fileutil"
 )
 
 // Test failure should resolve as "failed"
@@ -152,7 +153,7 @@ func TestExecutionErrorsText(t *testing.T) {
 func TestNonExecutableBinaryFailsTask(t *testing.T) {
  setup(t)
  commands := copyTestdataFile("ed25519_public_key")
- commands = append(commands, singleCommandNoArgs(filepath.Join(taskContext.TaskDir, "ed25519_public_key"))...)
+ commands = append(commands, singleCommandNoArgs(fileutil.AbsFrom(taskContext.TaskDir, "ed25519_public_key"))...)
  payload := GenericWorkerPayload{
  Command: commands,
  MaxRunTime: 10,

workers/generic-worker/mounts.go

@@ -386,9 +386,11 @@ func garbageCollection() error {
 
 // called when a task starts
 func (taskMount *TaskMount) Start() *CommandExecutionError {
+ taskMount.Info("In Start()")
  if taskMount.payloadError != nil {
  return MalformedPayloadError(taskMount.payloadError)
  }
+ taskMount.Info("No payload error")
  // Check if any caches need to be purged. See:
  // https://docs.taskcluster.net/docs/reference/core/purge-cache
  err := taskMount.purgeCaches()
@@ -406,6 +408,7 @@ func (taskMount *TaskMount) Start() *CommandExecutionError {
  }
  // loop through all mounts described in payload
  for _, mount := range taskMount.mounts {
+ taskMount.Infof("Found mount %v", mount)
  err = mount.Mount(taskMount)
  // An error is returned if it is a task problem, such as an invalid url
  // to download content, or a downloaded archive cannot be extracted.
@@ -497,7 +500,7 @@ func (f *FileMount) FSContent() (FSContent, error) {
 }
 
 func (w *WritableDirectoryCache) Mount(taskMount *TaskMount) error {
- target := filepath.Join(taskContext.TaskDir, w.Directory)
+ target := fileutil.AbsFrom(taskContext.TaskDir, w.Directory)
  // cache already there?
  if _, dirCacheExists := directoryCaches[w.CacheName]; dirCacheExists {
  // bump counter
@@ -559,7 +562,7 @@ func (w *WritableDirectoryCache) Mount(taskMount *TaskMount) error {
 func (w *WritableDirectoryCache) Unmount(taskMount *TaskMount) error {
  cache := directoryCaches[w.CacheName]
  cacheDir := cache.Location
- taskCacheDir := filepath.Join(taskContext.TaskDir, w.Directory)
+ taskCacheDir := fileutil.AbsFrom(taskContext.TaskDir, w.Directory)
  taskMount.Infof("Preserving cache: Moving %q to %q", taskCacheDir, cacheDir)
  err := RenameCrossDevice(taskCacheDir, cacheDir)
  if err != nil {
@@ -611,7 +614,7 @@ func (r *ReadOnlyDirectory) Mount(taskMount *TaskMount) error {
  if err != nil {
  return fmt.Errorf("Not able to retrieve FSContent: %v", err)
  }
- dir := filepath.Join(taskContext.TaskDir, r.Directory)
+ dir := fileutil.AbsFrom(taskContext.TaskDir, r.Directory)
  err = extract(c, r.Format, dir, taskMount)
  if err != nil {
  return err
@@ -630,7 +633,7 @@ func (f *FileMount) Mount(taskMount *TaskMount) error {
  return err
  }
 
- file := filepath.Join(taskContext.TaskDir, f.File)
+ file := fileutil.AbsFrom(taskContext.TaskDir, f.File)
  if info, err := os.Stat(file); err == nil && info.IsDir() {
  return fmt.Errorf("Cannot mount file at path %v since it already exists as a directory", file)
  }

workers/generic-worker/mounts_test.go

@@ -656,7 +656,8 @@ func TestMounts(t *testing.T) {
  }`),
  },
  &FileMount{
- File: filepath.Join("preloaded", "shasums"),
+ // use absolute path of file
+ File: filepath.Join(taskContext.TaskDir, "preloaded", "shasums"),
  Content: json.RawMessage(`{
  "url": "https://raw.githubusercontent.com/taskcluster/testrepo/db12070fc7ea6e5d21797bf943c0b9466fb4d65e/generic-worker/shasums"
  }`),
@@ -715,9 +716,9 @@ func TestMounts(t *testing.T) {
  Format: "zip",
  },
 
- // read only directory from url
+ // read only directory from url with absolute path for directory
  &ReadOnlyDirectory{
- Directory: filepath.Join("my-task-caches", "package"),
+ Directory: filepath.Join(taskContext.TaskDir, "my-task-caches", "package"),
  Content: json.RawMessage(`{
  "url": "https://github.com/taskcluster/logserver/raw/53134a5b9cbece05752c0ecc1a6c6d7c2fbf6580/node_modules/express/node_modules/connect/node_modules/multiparty/test/fixture/file/binaryfile.tar.gz"
  }`),

workers/generic-worker/multiuser_windows.go

@@ -13,6 +13,7 @@ import (
  "strings"
  "syscall"
 
+ "github.com/taskcluster/taskcluster/v59/workers/generic-worker/fileutil"
  "github.com/taskcluster/taskcluster/v59/workers/generic-worker/host"
  "github.com/taskcluster/taskcluster/v59/workers/generic-worker/process"
  "github.com/taskcluster/taskcluster/v59/workers/generic-worker/runtime"
@@ -59,7 +60,7 @@ func deleteDir(path string) error {
 
 func (task *TaskRun) generateCommand(index int) error {
  commandName := fmt.Sprintf("command_%06d", index)
- wrapper := filepath.Join(taskContext.TaskDir, commandName+"_wrapper.bat")
+ wrapper := fileutil.AbsFrom(taskContext.TaskDir, commandName+"_wrapper.bat")
  log.Printf("Creating wrapper script: %v", wrapper)
  command, err := process.NewCommand([]string{wrapper}, taskContext.TaskDir, nil, taskContext.pd)
  if err != nil {
@@ -75,11 +76,11 @@ func (task *TaskRun) generateCommand(index int) error {
 func (task *TaskRun) prepareCommand(index int) *CommandExecutionError {
  // In order that capturing of log files works, create a custom .bat file
  // for the task which redirects output to a log file...
- env := filepath.Join(taskContext.TaskDir, "env.txt")
- dir := filepath.Join(taskContext.TaskDir, "dir.txt")
+ env := fileutil.AbsFrom(taskContext.TaskDir, "env.txt")
+ dir := fileutil.AbsFrom(taskContext.TaskDir, "dir.txt")
  commandName := fmt.Sprintf("command_%06d", index)
- wrapper := filepath.Join(taskContext.TaskDir, commandName+"_wrapper.bat")
- script := filepath.Join(taskContext.TaskDir, commandName+".bat")
+ wrapper := fileutil.AbsFrom(taskContext.TaskDir, commandName+"_wrapper.bat")
+ script := fileutil.AbsFrom(taskContext.TaskDir, commandName+".bat")
  contents := ":: This script runs command " + strconv.Itoa(index) + " defined in TaskId " + task.TaskID + "..." + "\r\n"
  contents += "@echo off\r\n"
 

workers/generic-worker/rdp_windows.go

@@ -83,7 +83,7 @@ func (l *RDPTask) createRDPArtifact() {
  Username: taskContext.User.Name,
  Password: taskContext.User.Password,
  }
- rdpInfoFile := filepath.Join(taskContext.TaskDir, rdpInfoPath)
+ rdpInfoFile := fileutil.AbsFrom(taskContext.TaskDir, rdpInfoPath)
  err := fileutil.WriteToFileAsJSON(l.info, rdpInfoFile)
  // if we can't write this, something seriously wrong, so cause worker to
  // report an internal-error to sentry and crash!
@@ -100,8 +100,8 @@ func (l *RDPTask) uploadRDPArtifact() *CommandExecutionError {
  // RDP info expires one day after task
  Expires: tcclient.Time(time.Now().Add(time.Hour * 24)),
  },
- filepath.Join(taskContext.TaskDir, rdpInfoPath),
- filepath.Join(taskContext.TaskDir, rdpInfoPath),
+ fileutil.AbsFrom(taskContext.TaskDir, rdpInfoPath),
+ fileutil.AbsFrom(taskContext.TaskDir, rdpInfoPath),
  "application/json",
  "gzip",
  ),