fix: prevent data race in setupShell
setupShell would update the shell stored on the `Step` object to a new interpolated value. If the expression evaluation was interpolated with reference to a matrix input to the job, that shell can be written to multiple *different* values for each RunContext, resulting in a data race that causes incorrect shells to be used for each concurrent matrix run.
==================
WARNING: DATA RACE
Write at 0x00c00013e9b0 by goroutine 1470:
code.forgejo.org/forgejo/runner/v9/act/runner.(*stepRun).setupShell()
/.../forgejo-runner/act/runner/step_run.go:210 +0x8f2
code.forgejo.org/forgejo/runner/v9/act/common/git.FindGitRevision()
/.../forgejo-runner/act/common/git/git.go:58 +0xc4
code.forgejo.org/forgejo/runner/v9/act/model.(*GithubContext).SetSha()
/.../forgejo-runner/act/model/github_context.go:161 +0x6b5
code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).getGithubContext()
/.../forgejo-runner/act/runner/run_context.go:1228 +0x26ca
...
Previous write at 0x00c00013e9b0 by goroutine 1469:
code.forgejo.org/forgejo/runner/v9/act/runner.(*stepRun).setupShell()
/.../forgejo-runner/act/runner/step_run.go:210 +0x8f2
code.forgejo.org/forgejo/runner/v9/act/common/git.FindGitRevision()
/.../forgejo-runner/act/common/git/git.go:58 +0xc4
code.forgejo.org/forgejo/runner/v9/act/model.(*GithubContext).SetSha()
/.../forgejo-runner/act/model/github_context.go:161 +0x6b5
code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).getGithubContext()
/.../forgejo-runner/act/runner/run_context.go:1228 +0x26ca
...
==================
This commit is contained in:
Mathieu Fenniak
parent
886bf2a4f3
commit
2a6236f088
4 changed files with 71 additions and 43 deletions
|
|
@ -605,7 +605,7 @@ type Step struct {
|
|||
Uses string `yaml:"uses"`
|
||||
Run string `yaml:"run"`
|
||||
WorkingDirectory string `yaml:"working-directory"`
|
||||
Shell string `yaml:"shell"`
|
||||
RawShell string `yaml:"shell"`
|
||||
Env yaml.Node `yaml:"env"`
|
||||
With map[string]string `yaml:"with"`
|
||||
RawContinueOnError string `yaml:"continue-on-error"`
|
||||
|
|
@ -641,32 +641,6 @@ func (s *Step) GetEnv() map[string]string {
|
|||
return env
|
||||
}
|
||||
|
||||
// ShellCommand returns the command for the shell
|
||||
func (s *Step) ShellCommand() string {
|
||||
var shellCommand string
|
||||
|
||||
// Reference: https://github.com/actions/runner/blob/8109c962f09d9acc473d92c595ff43afceddb347/src/Runner.Worker/Handlers/ScriptHandlerHelpers.cs#L9-L17
|
||||
switch s.Shell {
|
||||
case "", "bash":
|
||||
shellCommand = "bash --noprofile --norc -e -o pipefail {0}"
|
||||
case "pwsh":
|
||||
shellCommand = "pwsh -command . '{0}'"
|
||||
case "python":
|
||||
shellCommand = "python {0}"
|
||||
case "sh":
|
||||
shellCommand = "sh -e {0}"
|
||||
case "cmd":
|
||||
shellCommand = "cmd /D /E:ON /V:OFF /S /C \"CALL \"{0}\"\""
|
||||
case "powershell":
|
||||
shellCommand = "powershell -command . '{0}'"
|
||||
case "node":
|
||||
shellCommand = "node {0}"
|
||||
default:
|
||||
shellCommand = s.Shell
|
||||
}
|
||||
return shellCommand
|
||||
}
|
||||
|
||||
// StepType describes what type of step we are about to run
|
||||
type StepType int
|
||||
|
||||
|
|
|
|||
|
|
@ -283,6 +283,7 @@ func TestRunner_RunEvent(t *testing.T) {
|
|||
{workdir, "matrix", "push", "", platforms, secrets},
|
||||
{workdir, "matrix-include-exclude", "push", "", platforms, secrets},
|
||||
{workdir, "matrix-exitcode", "push", "Job 'test' failed", platforms, secrets},
|
||||
{workdir, "matrix-shell", "push", "", platforms, secrets},
|
||||
{workdir, "commands", "push", "", platforms, secrets},
|
||||
{workdir, "workdir", "push", "", platforms, secrets},
|
||||
{workdir, "defaults-run", "push", "", platforms, secrets},
|
||||
|
|
|
|||
|
|
@ -100,14 +100,33 @@ func getScriptName(rc *RunContext, step *model.Step) string {
|
|||
// OCI runtime exec failed: exec failed: container_linux.go:380: starting container process caused: exec: "${{": executable file not found in $PATH: unknown
|
||||
func (sr *stepRun) setupShellCommand(ctx context.Context) (name, script string, err error) {
|
||||
logger := common.Logger(ctx)
|
||||
sr.setupShell(ctx)
|
||||
shell := sr.interpretShell(ctx)
|
||||
sr.setupWorkingDirectory(ctx)
|
||||
|
||||
step := sr.Step
|
||||
|
||||
script = sr.RunContext.NewStepExpressionEvaluator(ctx, sr).Interpolate(ctx, step.Run)
|
||||
|
||||
scCmd := step.ShellCommand()
|
||||
var shellCommand string
|
||||
// Reference: https://github.com/actions/runner/blob/8109c962f09d9acc473d92c595ff43afceddb347/src/Runner.Worker/Handlers/ScriptHandlerHelpers.cs#L9-L17
|
||||
switch shell {
|
||||
case "", "bash":
|
||||
shellCommand = "bash --noprofile --norc -e -o pipefail {0}"
|
||||
case "pwsh":
|
||||
shellCommand = "pwsh -command . '{0}'"
|
||||
case "python":
|
||||
shellCommand = "python {0}"
|
||||
case "sh":
|
||||
shellCommand = "sh -e {0}"
|
||||
case "cmd":
|
||||
shellCommand = "cmd /D /E:ON /V:OFF /S /C \"CALL \"{0}\"\""
|
||||
case "powershell":
|
||||
shellCommand = "powershell -command . '{0}'"
|
||||
case "node":
|
||||
shellCommand = "node {0}"
|
||||
default:
|
||||
shellCommand = shell
|
||||
}
|
||||
|
||||
name = getScriptName(sr.RunContext, step)
|
||||
|
||||
|
|
@ -115,7 +134,7 @@ func (sr *stepRun) setupShellCommand(ctx context.Context) (name, script string,
|
|||
// Reference: https://github.com/actions/runner/blob/8109c962f09d9acc473d92c595ff43afceddb347/src/Runner.Worker/Handlers/ScriptHandlerHelpers.cs#L19-L27
|
||||
runPrepend := ""
|
||||
runAppend := ""
|
||||
switch step.Shell {
|
||||
switch shell {
|
||||
case "bash", "sh":
|
||||
name += ".sh"
|
||||
case "pwsh", "powershell":
|
||||
|
|
@ -141,7 +160,7 @@ func (sr *stepRun) setupShellCommand(ctx context.Context) (name, script string,
|
|||
|
||||
rc := sr.getRunContext()
|
||||
scriptPath := fmt.Sprintf("%s/%s", rc.JobContainer.GetActPath(), name)
|
||||
sr.cmdline = strings.Replace(scCmd, `{0}`, scriptPath, 1)
|
||||
sr.cmdline = strings.Replace(shellCommand, `{0}`, scriptPath, 1)
|
||||
sr.cmd, err = shellquote.Split(sr.cmdline)
|
||||
|
||||
return name, script, err
|
||||
|
|
@ -163,28 +182,28 @@ func (l *localEnv) Getenv(name string) string {
|
|||
return l.env[name]
|
||||
}
|
||||
|
||||
func (sr *stepRun) setupShell(ctx context.Context) {
|
||||
func (sr *stepRun) interpretShell(ctx context.Context) string {
|
||||
rc := sr.RunContext
|
||||
step := sr.Step
|
||||
shell := sr.Step.RawShell
|
||||
|
||||
if step.Shell == "" {
|
||||
step.Shell = rc.Run.Job().Defaults.Run.Shell
|
||||
if shell == "" {
|
||||
shell = rc.Run.Job().Defaults.Run.Shell
|
||||
}
|
||||
|
||||
step.Shell = rc.NewExpressionEvaluator(ctx).Interpolate(ctx, step.Shell)
|
||||
shell = rc.NewExpressionEvaluator(ctx).Interpolate(ctx, shell)
|
||||
|
||||
if step.Shell == "" {
|
||||
step.Shell = rc.Run.Workflow.Defaults.Run.Shell
|
||||
if shell == "" {
|
||||
shell = rc.Run.Workflow.Defaults.Run.Shell
|
||||
}
|
||||
|
||||
if step.Shell == "" {
|
||||
if shell == "" {
|
||||
if _, ok := rc.JobContainer.(*container.HostEnvironment); ok {
|
||||
shellWithFallback := []string{"bash", "sh"}
|
||||
// Don't use bash on windows by default, if not using a docker container
|
||||
if runtime.GOOS == "windows" {
|
||||
shellWithFallback = []string{"pwsh", "powershell"}
|
||||
}
|
||||
step.Shell = shellWithFallback[0]
|
||||
shell = shellWithFallback[0]
|
||||
lenv := &localEnv{env: map[string]string{}}
|
||||
for k, v := range sr.env {
|
||||
lenv.env[k] = v
|
||||
|
|
@ -192,7 +211,7 @@ func (sr *stepRun) setupShell(ctx context.Context) {
|
|||
sr.getRunContext().ApplyExtraPath(ctx, &lenv.env)
|
||||
_, err := lookpath.LookPath2(shellWithFallback[0], lenv)
|
||||
if err != nil {
|
||||
step.Shell = shellWithFallback[1]
|
||||
shell = shellWithFallback[1]
|
||||
}
|
||||
} else {
|
||||
shellFallback := `
|
||||
|
|
@ -205,11 +224,13 @@ fi
|
|||
stdout, _, err := rc.sh(ctx, shellFallback)
|
||||
if err != nil {
|
||||
common.Logger(ctx).Error("fail to run %q: %v", shellFallback, err)
|
||||
return
|
||||
} else {
|
||||
shell = stdout
|
||||
}
|
||||
step.Shell = stdout
|
||||
}
|
||||
}
|
||||
|
||||
return shell
|
||||
}
|
||||
|
||||
func (sr *stepRun) setupWorkingDirectory(ctx context.Context) {
|
||||
|
|
|
|||
32
act/runner/testdata/matrix-shell/push.yml
vendored
Normal file
32
act/runner/testdata/matrix-shell/push.yml
vendored
Normal file
|
|
@ -0,0 +1,32 @@
|
|||
name: test
|
||||
|
||||
on: push
|
||||
|
||||
jobs:
|
||||
matrix-shell:
|
||||
runs-on: ubuntu-latest
|
||||
strategy:
|
||||
matrix:
|
||||
shell-to-use: ["bash", "cat {0}", "sh"]
|
||||
steps:
|
||||
- shell: ${{ matrix.shell-to-use }}
|
||||
run: |
|
||||
expected=${{ matrix.shell-to-use }}
|
||||
if [ "$expected" = "bash" ]; then
|
||||
if [ -z "$BASH_VERSION" ]; then
|
||||
echo "Expected to execute bash shell, but BASH_VERSION is unset"
|
||||
exit 1
|
||||
else
|
||||
echo "Successfully running in bash ($BASH_VERSION)"
|
||||
fi
|
||||
elif [ "$expected" = "sh" ]; then
|
||||
if [ -n "$BASH_VERSION" ]; then
|
||||
echo "Expected to execute in sh shell, but BASH_VERSION is set ($BASH_VERSION)"
|
||||
exit 1
|
||||
else
|
||||
echo "Probably running in sh"
|
||||
fi
|
||||
else
|
||||
echo "Not sure what's happening; expected shell is $expected ?!"
|
||||
exit 1
|
||||
fi
|
||||
Loading…
Add table
Add a link
Reference in a new issue