From 79faf5e55df7a3b312fb4888b7405a58e01f567c Mon Sep 17 00:00:00 2001 From: bircni Date: Sat, 7 Nov 2026 04:33:19 +0000 Subject: [PATCH] fix: empty root-owned bind workspaces for non-root runners (#1399) A job running as root leaves root-owned files in the bind-mounted workspace. A non-root runner on Linux cannot remove them, so workspaces pile up with only a warning. Such a runner now empties the workspace as root from inside the job container, right before removing the container. Root runners and Docker Desktop already remove these files themselves and skip this. The workspace is only emptied when it is exactly the runner's own mountpoint with no mount below it. Otherwise, or when the command fails, the existing removal and warning apply. Jobs of a reusable workflow share the workspace, so they are not covered and still end in the existing warning. Fixes: https://gitea.com/gitea/runner/issues/879 Co-authored-by: silverwind <2021+silverwind@noreply.gitea.com> Co-authored-by: silverwind Reviewed-on: https://gitea.com/gitea/runner/pulls/1399 Reviewed-by: silverwind <2021+silverwind@noreply.gitea.com> Co-authored-by: bircni --- act/container/container_types.go | 8 ++ act/container/docker_run.go | 11 +-- act/runner/action.go | 2 +- act/runner/container_mock_test.go | 5 ++ act/runner/run_context.go | 57 +++++++++++-- act/runner/run_context_test.go | 133 ++++++++++++++++++++++++++++-- docs/post-task-script.md | 2 +- 7 files changed, 193 insertions(+), 25 deletions(-) diff --git a/act/container/container_types.go b/act/container/container_types.go index fe1abb9d..4fa25896 100644 --- a/act/container/container_types.go +++ b/act/container/container_types.go @@ -15,6 +15,7 @@ import ( "strings" "sync" "sync/atomic" + "time" "gitea.com/gitea/runner/act/common" @@ -24,6 +25,13 @@ import ( var hostOS = runtime.GOOS // replaced in tests +// DrainGracePeriod bounds how long we wait for an output-copy goroutine to +// finish draining a container's output before returning, so that neither a +// cancellation (waitForCommand) nor a normal container exit (wait) truncates +// the tail of the log. It is a safety bound: in the common case the stream +// reaches EOF and the goroutine returns well before this elapses. +const DrainGracePeriod = 2 * time.Second + // ExitCodeError reports a non-zero process exit code from a container command. type ExitCodeError int diff --git a/act/container/docker_run.go b/act/container/docker_run.go index 7f56956d..b2266ee8 100644 --- a/act/container/docker_run.go +++ b/act/container/docker_run.go @@ -41,13 +41,6 @@ import ( specs "github.com/opencontainers/image-spec/specs-go/v1" ) -// drainGracePeriod bounds how long we wait for an output-copy goroutine to -// finish draining a container's output before returning, so that neither a -// cancellation (waitForCommand) nor a normal container exit (wait) truncates -// the tail of the log. It is a safety bound: in the common case the stream -// reaches EOF and the goroutine returns well before this elapses. -const drainGracePeriod = 2 * time.Second - // NewContainer creates a reference to a container func NewContainer(input *NewContainerInput) ExecutionsEnvironment { // Resolved up front because the image pull runs before the container is created. @@ -947,7 +940,7 @@ func (cr *containerReference) waitForCommand(ctx context.Context, resp client.Hi if err != nil { logger.Debugf("Failed to drain command output after cancellation: %v", err) } - case <-time.After(drainGracePeriod): + case <-time.After(DrainGracePeriod): logger.Warn("Timed out draining command output after cancellation") } @@ -1122,7 +1115,7 @@ func (cr *containerReference) wait() common.Executor { if cr.attachDone != nil { select { case <-cr.attachDone: - case <-time.After(drainGracePeriod): + case <-time.After(DrainGracePeriod): logger.Warn("Timed out draining container output") } cr.attachDone = nil diff --git a/act/runner/action.go b/act/runner/action.go index bf801b3a..05781287 100644 --- a/act/runner/action.go +++ b/act/runner/action.go @@ -472,7 +472,7 @@ func newStepContainer(ctx context.Context, step step, image string, cmd, entrypo envList = append(envList, rc.runnerEnv(ctx)...) - binds, mounts := rc.GetBindsAndMounts(ctx) + binds, mounts, _ := rc.GetBindsAndMounts(ctx) networkMode := "container:" + rc.jobContainerName() if rc.IsHostEnv() { networkMode = "default" diff --git a/act/runner/container_mock_test.go b/act/runner/container_mock_test.go index 30317c40..73f8001d 100644 --- a/act/runner/container_mock_test.go +++ b/act/runner/container_mock_test.go @@ -92,3 +92,8 @@ func (cm *containerMock) Inspect(ctx context.Context) (*container.Info, error) { func (cm *containerMock) CopyTarStream(ctx context.Context, destPath string, write func(*container.TarWriter) error) error { return cm.Called(ctx, destPath, write).Error(0) } + +func (cm *containerMock) ReplaceLogWriter(stdout, stderr io.Writer) (io.Writer, io.Writer) { + args := cm.Called(stdout, stderr) + return args.Get(0).(io.Writer), args.Get(1).(io.Writer) +} diff --git a/act/runner/run_context.go b/act/runner/run_context.go index e038ab4c..383e012c 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -30,6 +30,7 @@ import ( "time" "gitea.com/gitea/runner/act/common" + "gitea.com/gitea/runner/act/common/logging" "gitea.com/gitea/runner/act/container" "gitea.com/gitea/runner/act/ghcontext" "gitea.com/gitea/runner/internal/pkg/lock" @@ -75,6 +76,7 @@ type RunContext struct { actionInputs map[string]any // inputs of the composite action this runs, nil for a job Masks []string cleanUpJobContainer common.Executor + taskWorkspace string deferVolumeCleanup func(common.Executor) caller *caller // job calling this RunContext (reusable workflows) workflowCallInputs map[string]any // the caller's with:, resolved once by resolveWorkflowCall @@ -378,8 +380,8 @@ func splitVolumes(specs []string) ([]string, map[string]string, map[string]bool) return binds, mounts, targets } -// Returns the binds and mounts for the container, resolving paths as appopriate -func (rc *RunContext) GetBindsAndMounts(ctx context.Context) ([]string, map[string]string) { +// Returns the binds and mounts for the container, resolving paths as appropriate, and the task workspace the runner binds +func (rc *RunContext) GetBindsAndMounts(ctx context.Context) ([]string, map[string]string, string) { name := rc.jobContainerName() ext := container.LinuxContainerEnvironmentExtensions{} @@ -399,12 +401,16 @@ func (rc *RunContext) GetBindsAndMounts(ctx context.Context) ([]string, map[stri } mounts[name+"-env"] = ext.GetActPath() // runner-internal, never overridable + taskWorkspace := "" if workdir := ext.ToContainerPath(rc.Config.Workdir); !claimed[workdir] { source := rc.workdirMountRoot() target := ext.ToContainerPath(source) if claimed[target] { source, target = rc.Config.Workdir, workdir } + if rc.Config.BindWorkdir && rc.Config.PresetGitHubContext != nil { + taskWorkspace = target + } if rc.Config.BindWorkdir { bindModifiers := "" if runtime.GOOS == "darwin" { @@ -419,7 +425,7 @@ func (rc *RunContext) GetBindsAndMounts(ctx context.Context) ([]string, map[stri } } - return binds, mounts + return binds, mounts, taskWorkspace } func (rc *RunContext) startHostEnvironment() common.Executor { @@ -672,7 +678,8 @@ func (rc *RunContext) startJobContainer() common.Executor { return err } rc.startDockerProxy(ctx) - containerInput.Binds, containerInput.Mounts = rc.GetBindsAndMounts(ctx) + var taskWorkspace string + containerInput.Binds, containerInput.Mounts, taskWorkspace = rc.GetBindsAndMounts(ctx) containerInput.ValidVolumes = rc.validVolumes() rc.jobNetworkName = networkName @@ -687,6 +694,7 @@ func (rc *RunContext) startJobContainer() common.Executor { func(ctx context.Context) error { return rc.createJobVolumes(ctx, containerInput.Mounts) }, rc.JobContainer.Create(rc.Config.ContainerCapAdd, rc.Config.ContainerCapDrop), rc.JobContainer.Start(false), + func(context.Context) error { rc.taskWorkspace = taskWorkspace; return nil }, rc.captureJobContainerInfo(), rc.copyWorkflowFiles(), rc.waitForServiceContainers(), @@ -801,7 +809,10 @@ func (rc *RunContext) cleanupJobResources(networkName string, createAndDeleteNet errs := []error{rc.closeDockerProxy(ctx)} var containers []common.Executor if rc.JobContainer != nil { - containers = append(containers, rc.JobContainer.Remove()) + containers = append(containers, func(ctx context.Context) error { + rc.emptyTaskWorkspace(ctx) + return rc.JobContainer.Remove()(ctx) + }) } if len(rc.serviceContainers) > 0 { logger.Infof("Cleaning up services for job %s", rc.JobName) @@ -831,6 +842,42 @@ func (rc *RunContext) cleanupJobResources(networkName string, createAndDeleteNet } } +// exits 3 unless $1 is a mountpoint in mountinfo $2, at least two levels deep, with no mount below it for rm to cross into +const emptyTaskWorkspaceScript = `case "$1" in /?*/?*) ;; *) exit 3 ;; esac +mounted= +while read -r _ _ _ _ point _; do + case "$point" in "$1") mounted=1 ;; "$1"/*) exit 3 ;; esac +done <"$2" +[ "$mounted" ] || exit 3 +rm -rf "$1"/* "$1"/.[!.]* "$1"/..?*` + +var runnerCanRemoveJobFiles = runtime.GOOS != "linux" || os.Geteuid() == 0 // root, or a desktop VM mapping container files to the user, replaced in tests + +// empties the workspace as root, on root-owned PATH dirs only, for a runner that may not own what the job wrote, called workflows share it +func (rc *RunContext) emptyTaskWorkspace(ctx context.Context) { + if rc.taskWorkspace == "" || rc.caller != nil || runnerCanRemoveJobFiles { + return + } + budget := 30 * time.Second + if deadline, ok := ctx.Deadline(); ok { + budget = min(budget, time.Until(deadline)/2-container.DrainGracePeriod) + } + ctx, cancel := context.WithTimeout(ctx, budget) + defer cancel() + stdout, stderr := rc.JobContainer.ReplaceLogWriter(io.Discard, io.Discard) + defer rc.JobContainer.ReplaceLogWriter(stdout, stderr) + err := rc.JobContainer.Exec( + []string{"sh", "-c", emptyTaskWorkspaceScript, "sh", rc.taskWorkspace, "/proc/self/mountinfo"}, + map[string]string{"PATH": "/usr/sbin:/usr/bin:/sbin:/bin"}, "0", "/", + )(common.WithLogger(ctx, logging.New(logging.DiscardHandler))) + switch { + case errors.Is(err, container.ExitCodeError(3)): + logging.Infof("not emptying workspace %s of %s: under two levels deep, not a mountpoint as given, or a mount below it", rc.taskWorkspace, rc.jobContainerName()) + case err != nil: + logging.Infof("could not empty workspace %s of %s: %v", rc.taskWorkspace, rc.jobContainerName(), err) + } +} + func runAll(ctx context.Context, executors []common.Executor) error { errs := make([]error, len(executors)) var wg sync.WaitGroup diff --git a/act/runner/run_context_test.go b/act/runner/run_context_test.go index c81b3ec9..6c5248cf 100644 --- a/act/runner/run_context_test.go +++ b/act/runner/run_context_test.go @@ -16,6 +16,7 @@ import ( "net/http" "net/http/httptest" "os" + "os/exec" "path/filepath" "runtime" "slices" @@ -455,7 +456,7 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { t.Run(testcase.name+testBindSuffix, func(t *testing.T) { rctemplate.Config.Workdir = testcase.name rctemplate.Config.BindWorkdir = bindWorkDir - gotbind, gotmount := rctemplate.GetBindsAndMounts(t.Context()) + gotbind, gotmount, _ := rctemplate.GetBindsAndMounts(t.Context()) if bindWorkDir { fullBind := testcase.name + ":" + testcase.want @@ -507,7 +508,7 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { rc.ExprEval = rc.NewExpressionEvaluator(t.Context()) require.NoError(t, rc.resolvePlatformImage(t.Context())) - gotbind, gotmount := rc.GetBindsAndMounts(t.Context()) + gotbind, gotmount, _ := rc.GetBindsAndMounts(t.Context()) assert.Contains(t, gotbind, "/host/mame/roms:/root/.mame/roms:ro") assert.NotContains(t, gotbind, "${{ secrets.MAME }}") assert.NotContains(t, gotmount, "${{ secrets.MAME }}") @@ -522,7 +523,7 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { rc.containerSpec.Volumes = testcase.volumes rc.Run.JobID = "job1" - gotbind, gotmount := rc.GetBindsAndMounts(t.Context()) + gotbind, gotmount, _ := rc.GetBindsAndMounts(t.Context()) if len(testcase.wantbind) > 0 { assert.Contains(t, gotbind, testcase.wantbind) @@ -551,12 +552,12 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { t.Run("ToolCacheMount", func(t *testing.T) { rc := newRunContext(&Config{}) - _, gotmount := rc.GetBindsAndMounts(t.Context()) + _, gotmount, _ := rc.GetBindsAndMounts(t.Context()) assert.NotContains(t, gotmount, sharedToolCacheVolume) rc.Config.SharedToolCache = true rc.containerSpec.Volumes = []string{"/unapproved:" + container.DefaultToolCache} - _, gotmount = rc.GetBindsAndMounts(t.Context()) + _, gotmount, _ = rc.GetBindsAndMounts(t.Context()) assert.Equal(t, container.DefaultToolCache, gotmount[sharedToolCacheVolume]) }) @@ -569,16 +570,26 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { Config: &Config{BindWorkdir: true, Workdir: "/workspace/1/owner/repo", PresetGitHubContext: &model.GithubContext{}, ValidVolumes: []string{"claimed"}}, } - gotbind, _ := rc.GetBindsAndMounts(t.Context()) + gotbind, _, workspace := rc.GetBindsAndMounts(t.Context()) assert.True(t, slices.ContainsFunc(gotbind, func(bind string) bool { return strings.HasPrefix(bind, "/workspace/1:/workspace/1") }), gotbind) + assert.Equal(t, "/workspace/1", workspace) rc.Config.BindWorkdir = false - _, gotmount := rc.GetBindsAndMounts(t.Context()) + _, gotmount, workspace := rc.GetBindsAndMounts(t.Context()) assert.Equal(t, "/workspace/1", gotmount[rc.jobContainerName()]) + assert.Empty(t, workspace) rc.containerSpec.Volumes = []string{"claimed:/workspace/1"} - _, gotmount = rc.GetBindsAndMounts(t.Context()) + _, gotmount, _ = rc.GetBindsAndMounts(t.Context()) assert.Equal(t, "/workspace/1/owner/repo", gotmount[rc.jobContainerName()]) + rc.Config.BindWorkdir = true + _, _, workspace = rc.GetBindsAndMounts(t.Context()) + assert.Equal(t, "/workspace/1/owner/repo", workspace) + + rc.containerSpec.Volumes = nil + rc.Config.PresetGitHubContext = nil + _, _, workspace = rc.GetBindsAndMounts(t.Context()) + assert.Empty(t, workspace) }) } @@ -611,7 +622,7 @@ func TestRunContextValidVolumes(t *testing.T) { rc.JobContainer = hostEnv rc.Config.BindWorkdir = false rc.Config.SharedToolCache = true - binds, mounts := rc.GetBindsAndMounts(t.Context()) + binds, mounts, _ := rc.GetBindsAndMounts(t.Context()) assert.Subset(t, binds, []string{hostEnv.Path + ":" + hostEnv.Path, hostEnv.GetActPath() + ":" + hostEnv.GetActPath()}) assert.Contains(t, mounts, sharedToolCacheVolume) assert.Equal(t, binds, container.SanitizeBinds(t.Context(), rc.validVolumes(), binds)) @@ -683,6 +694,110 @@ func TestCleanupJobVolumesReapsAbandonedDeferredCleanup(t *testing.T) { assert.ElementsMatch(t, []string{"became-active", "remove-failed", "foreign", "fresh", "unknown-age"}, slices.Collect(maps.Keys(volumes))) } +func TestCleanupJobResourcesEmptiesTaskWorkspace(t *testing.T) { + defer func(previous bool) { runnerCanRemoveJobFiles = previous }(runnerCanRemoveJobFiles) + for _, tt := range []struct { + name, workspace string + caller *caller + runnerCanRemove bool + execErr error + log string + }{ + {name: "refusal is logged", workspace: "/workspace/1", execErr: container.ExitCodeError(3), log: "not emptying workspace /workspace/1"}, + {name: "failure does not fail the job", workspace: "/workspace/1", execErr: errors.New("no rm"), log: "could not empty workspace /workspace/1"}, + {name: "no runner bind"}, + {name: "called workflow shares it", workspace: "/workspace/1", caller: &caller{}}, + {name: "runner removes it itself", workspace: "/workspace/1", runnerCanRemove: true}, + } { + t.Run(tt.name, func(t *testing.T) { + runnerCanRemoveJobFiles = tt.runnerCanRemove + processLog := logtest.SetDefault(t) + var jobLog bytes.Buffer + var calls []string + job := &containerMock{} + if tt.log != "" { + job.On("ReplaceLogWriter", io.Discard, io.Discard).Return(&jobLog, &jobLog).Once() + job.On("ReplaceLogWriter", &jobLog, &jobLog).Return(io.Discard, io.Discard).Once() + job.On("Exec", mock.MatchedBy(func(command []string) bool { return command[4] == tt.workspace }), map[string]string{"PATH": "/usr/sbin:/usr/bin:/sbin:/bin"}, "0", "/").Return(func(ctx context.Context) error { + calls = append(calls, "exec") + deadline, ok := ctx.Deadline() + assert.True(t, ok) + assert.LessOrEqual(t, time.Until(deadline), 10*time.Second-container.DrainGracePeriod) + common.Logger(ctx).Infof("workspace script output") + return tt.execErr + }) + } + job.On("Remove").Return(func(context.Context) error { + calls = append(calls, "remove") + return nil + }) + rc := &RunContext{ + Config: &Config{}, + Run: &model.Run{Workflow: &model.Workflow{Name: "workflow"}, JobID: "job"}, + JobContainer: job, + taskWorkspace: tt.workspace, + caller: tt.caller, + deferVolumeCleanup: func(common.Executor) {}, + } + ctx, cancel := context.WithTimeout(t.Context(), 20*time.Second) + defer cancel() + ctx = common.WithLogger(ctx, logging.New(logging.NewLevelHandler(newJobLogHandler(&jobLog, cyan), logging.LevelInfo))) + require.NoError(t, rc.cleanupJobResources("", false, false)(ctx)) + job.AssertExpectations(t) + assert.NotContains(t, jobLog.String(), "workspace") + if tt.log == "" { + assert.Equal(t, []string{"remove"}, calls) + return + } + assert.Equal(t, []string{"exec", "remove"}, calls) + assert.True(t, slices.ContainsFunc(processLog.AllRecords(), func(record logging.Record) bool { + return strings.HasPrefix(record.Message, tt.log) + }), processLog.AllRecords()) + }) + } +} + +func TestEmptyTaskWorkspaceScript(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the script runs in the linux job container") + } + run := func(t *testing.T, workspace string, mountpoints ...string) error { + t.Helper() + var mountinfo strings.Builder + for _, mountpoint := range mountpoints { + fmt.Fprintf(&mountinfo, "36 35 98:0 /src %s rw - ext4 /dev/sda1 rw\n", mountpoint) + } + info := filepath.Join(t.TempDir(), "mountinfo") + require.NoError(t, os.WriteFile(info, []byte(mountinfo.String()), 0o600)) + return exec.Command("sh", "-c", emptyTaskWorkspaceScript, "sh", workspace, info).Run() + } + + workspace := t.TempDir() + for _, file := range []string{"owner/repo/file", ".hidden", "..odd"} { + require.NoError(t, os.MkdirAll(filepath.Dir(filepath.Join(workspace, file)), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(workspace, file), nil, 0o600)) + } + require.NoError(t, run(t, workspace, workspace)) + entries, err := os.ReadDir(workspace) + require.NoError(t, err) + assert.Empty(t, entries) + + for _, tt := range []struct { + name, workspace string + mountpoints []string + }{ + {"a mount below it", "/nonexistent/1", []string{"/nonexistent/1", "/nonexistent/1/owner/repo/cache"}}, + {"a workspace that is not a mountpoint", "/nonexistent/1", []string{"/nonexistent/2"}}, + {"a top-level directory", "/nonexistent", []string{"/nonexistent"}}, + } { + t.Run("refuses "+tt.name, func(t *testing.T) { + var exitErr *exec.ExitError + require.ErrorAs(t, run(t, tt.workspace, tt.mountpoints...), &exitErr) + assert.Equal(t, 3, exitErr.ExitCode()) + }) + } +} + func TestRemoveDockerActionImagesByTag(t *testing.T) { var removed []string fakeDockerDaemon(t, func(writer http.ResponseWriter, request *http.Request) { diff --git a/docs/post-task-script.md b/docs/post-task-script.md index cca3f4bb..b40fb071 100644 --- a/docs/post-task-script.md +++ b/docs/post-task-script.md @@ -9,7 +9,7 @@ It is configured under `runner.post_task_script` in the runner YAML config (see For each task, execution order is: 1. Workflow runs (steps, actions, containers). -2. In-job cleanup (action `post:` steps, container stop/remove). +2. In-job cleanup (action `post:` steps, emptying a bind-mounted workspace from inside the job container when a non-root runner on Linux may not own its files, container stop/remove). 3. Job outputs are reported to Gitea. 4. Bind-workdir workspace removal, when `container.bind_workdir` is enabled. 5. **Post-task script** (this hook).