From 815e7aed0485e12b40e1cb1e474714f32e0fed66 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Fri, 15 Aug 2025 09:11:15 +0000 Subject: [PATCH] chore: prevent "false positive" data race detection w/ ContainerDaemonSocket (#859) `ContainerDaemonSocket` is stored on a shared struct and was mutated to a default value when empty, which trips the data race detector as a mutation of shared state without any synchronization. However as all codepaths would be setting it to the same value in the mutation, here's no functional bug. This commit prevents the "false positive", but it also centralizes the default value for a slightly better programming practice. ``` ================== WARNING: DATA RACE Read at 0x00c00027f9e0 by goroutine 1104: code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).GetBindsAndMounts() /.../forgejo-runner/act/runner/run_context.go:130 +0x87 code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).prepareJobContainer() /.../forgejo-runner/act/runner/run_context.go:449 +0xad1 code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).startContainer.func1.(*RunContext).startJobContainer.2() /.../forgejo-runner/act/runner/run_context.go:587 +0x5e code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).startContainer.func1() /.../forgejo-runner/act/runner/run_context.go:836 +0xf3 code.forgejo.org/forgejo/runner/v9/act/runner.newJobExecutor.NewPipelineExecutor.Executor.Then.func21() /.../forgejo-runner/act/common/executor.go:136 +0x57 code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).Executor.func1() /.../forgejo-runner/act/runner/run_context.go:929 +0x68 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.1() /.../forgejo-runner/act/runner/runner.go:218 +0x271 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2.1() /.../forgejo-runner/act/common/executor.go:107 +0x61 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2.gowrap1() /.../forgejo-runner/act/common/executor.go:109 +0x4f Previous write at 0x00c00027f9e0 by goroutine 1103: code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).GetBindsAndMounts() /.../forgejo-runner/act/runner/run_context.go:131 +0xc7 code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).prepareJobContainer() /.../forgejo-runner/act/runner/run_context.go:449 +0xad1 code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).startContainer.func1.(*RunContext).startJobContainer.2() /.../forgejo-runner/act/runner/run_context.go:587 +0x5e code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).startContainer.func1() /.../forgejo-runner/act/runner/run_context.go:836 +0xf3 code.forgejo.org/forgejo/runner/v9/act/runner.newJobExecutor.NewPipelineExecutor.Executor.Then.func21() /.../forgejo-runner/act/common/executor.go:136 +0x57 code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).Executor.func1() /.../forgejo-runner/act/runner/run_context.go:929 +0x68 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.1() /.../forgejo-runner/act/runner/runner.go:218 +0x271 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2.1() /.../forgejo-runner/act/common/executor.go:107 +0x61 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2.gowrap1() /.../forgejo-runner/act/common/executor.go:109 +0x4f Goroutine 1104 (running) created at: code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2() /.../forgejo-runner/act/common/executor.go:105 +0x144 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.3.1() /.../forgejo-runner/act/common/executor.go:107 +0x61 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.3.gowrap1() /.../forgejo-runner/act/common/executor.go:109 +0x4f Goroutine 1103 (running) created at: code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2() /.../forgejo-runner/act/common/executor.go:105 +0x144 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.3.1() /.../forgejo-runner/act/common/executor.go:107 +0x61 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.3.gowrap1() /.../forgejo-runner/act/common/executor.go:109 +0x4f ================== ``` - other - [PR](https://code.forgejo.org/forgejo/runner/pulls/859): chore: prevent "false positive" data race detection w/ ContainerDaemonSocket Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/859 Reviewed-by: Gusted Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- act/runner/run_context.go | 20 ++++++++------------ act/runner/runner.go | 7 +++++++ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/act/runner/run_context.go b/act/runner/run_context.go index 2d40f56a..5ed2bb35 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -138,13 +138,11 @@ func (rc *RunContext) getInternalVolumeEnv(ctx context.Context) string { // Returns the binds and mounts for the container, resolving paths as appopriate func (rc *RunContext) GetBindsAndMounts(ctx context.Context) ([]string, map[string]string, []string) { - if rc.Config.ContainerDaemonSocket == "" { - rc.Config.ContainerDaemonSocket = "/var/run/docker.sock" - } - binds := []string{} - if rc.Config.ContainerDaemonSocket != "-" { - daemonPath := getDockerDaemonSocketMountPath(rc.Config.ContainerDaemonSocket) + + containerDaemonSocket := rc.Config.GetContainerDaemonSocket() + if containerDaemonSocket != "-" { + daemonPath := getDockerDaemonSocketMountPath(containerDaemonSocket) binds = append(binds, fmt.Sprintf("%s:%s", daemonPath, "/var/run/docker.sock")) } @@ -182,7 +180,7 @@ func (rc *RunContext) GetBindsAndMounts(ctx context.Context) ([]string, map[stri mounts[rc.getInternalVolumeWorkdir(ctx)] = ext.ToContainerPath(rc.Config.Workdir) } - validVolumes := append(rc.getInternalVolumeNames(ctx), getDockerDaemonSocketMountPath(rc.Config.ContainerDaemonSocket)) + validVolumes := append(rc.getInternalVolumeNames(ctx), getDockerDaemonSocketMountPath(containerDaemonSocket)) validVolumes = append(validVolumes, rc.Config.ValidVolumes...) return binds, mounts, validVolumes } @@ -1445,12 +1443,10 @@ func (rc *RunContext) handleServiceCredentials(ctx context.Context, creds map[st // GetServiceBindsAndMounts returns the binds and mounts for the service container, resolving paths as appopriate func (rc *RunContext) GetServiceBindsAndMounts(svcVolumes []string) ([]string, map[string]string) { - if rc.Config.ContainerDaemonSocket == "" { - rc.Config.ContainerDaemonSocket = "/var/run/docker.sock" - } + containerDaemonSocket := rc.Config.GetContainerDaemonSocket() binds := []string{} - if rc.Config.ContainerDaemonSocket != "-" { - daemonPath := getDockerDaemonSocketMountPath(rc.Config.ContainerDaemonSocket) + if containerDaemonSocket != "-" { + daemonPath := getDockerDaemonSocketMountPath(containerDaemonSocket) binds = append(binds, fmt.Sprintf("%s:%s", daemonPath, "/var/run/docker.sock")) } diff --git a/act/runner/runner.go b/act/runner/runner.go index 0bf95264..7fd5139e 100644 --- a/act/runner/runner.go +++ b/act/runner/runner.go @@ -85,6 +85,13 @@ func (c Config) GetToken() string { return token } +func (c *Config) GetContainerDaemonSocket() string { + if c.ContainerDaemonSocket == "" { + return "/var/run/docker.sock" + } + return c.ContainerDaemonSocket +} + type caller struct { runContext *RunContext }