mirror of
https://code.forgejo.org/forgejo/runner.git
synced 2025-09-05 18:40:59 +00:00
chore: fix 'false positive' data race detection in Id/Number default init (#867)
A step's `Id` & `Number` are potentially initialized in different goroutines on matrix evaluations; this change ensures they're initialized before execution fans out to multiple goroutines. There doesn't seem to be any functional impact of this data race for end-users.
Where `Number` was previously initialized, a runtime error was added to ensure that the behavior is the same.
`ID` data race:
```
==================
WARNING: DATA RACE
Read at 0x00c0001ff348 by goroutine 77:
code.forgejo.org/forgejo/runner/v9/act/runner.newJobExecutor()
/.../forgejo-runner/act/runner/job_executor.go:64 +0x424
code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).Executor()
/.../forgejo-runner/act/runner/run_context.go:931 +0x2c6
code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.1()
/.../forgejo-runner/act/runner/runner.go:218 +0x150
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 0x00c0001ff348 by goroutine 76:
code.forgejo.org/forgejo/runner/v9/act/runner.newJobExecutor()
/.../forgejo-runner/act/runner/job_executor.go:65 +0x4cc
code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).Executor()
/.../forgejo-runner/act/runner/run_context.go:931 +0x2c6
code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.1()
/.../forgejo-runner/act/runner/runner.go:218 +0x150
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 77 (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 76 (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
==================
```
`Number` data race:
```
==================
WARNING: DATA RACE
Write at 0x00c0001ff340 by goroutine 77:
code.forgejo.org/forgejo/runner/v9/act/runner.newJobExecutor()
/.../forgejo-runner/act/runner/job_executor.go:67 +0x536
code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).Executor()
/.../forgejo-runner/act/runner/run_context.go:931 +0x2c6
code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.1()
/.../forgejo-runner/act/runner/runner.go:218 +0x150
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 0x00c0001ff340 by goroutine 76:
code.forgejo.org/forgejo/runner/v9/act/runner.newJobExecutor()
/.../forgejo-runner/act/runner/job_executor.go:67 +0x536
code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).Executor()
/.../forgejo-runner/act/runner/run_context.go:931 +0x2c6
code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.1()
/.../forgejo-runner/act/runner/runner.go:218 +0x150
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 77 (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 76 (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
==================
```
<!--start release-notes-assistant-->
<!--URL:https://code.forgejo.org/forgejo/runner-->
- other
- [PR](https://code.forgejo.org/forgejo/runner/pulls/867): <!--number 867 --><!--line 0 --><!--description Y2hvcmU6IGZpeCAnZmFsc2UgcG9zaXRpdmUnIGRhdGEgcmFjZSBkZXRlY3Rpb24gaW4gSWQvTnVtYmVyIGRlZmF1bHQgaW5pdA==-->chore: fix 'false positive' data race detection in Id/Number default init<!--description-->
<!--end release-notes-assistant-->
Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/867
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
This commit is contained in:
Mathieu Fenniak
earl-warren
parent
91e7940947
commit
ce6502e7b6
5 changed files with 51 additions and 19 deletions
|
|
@ -72,6 +72,22 @@ type ActionRuns struct {
|
||||||
Steps []Step `yaml:"steps"`
|
Steps []Step `yaml:"steps"`
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func (actionRuns *ActionRuns) UnmarshalYAML(value *yaml.Node) error {
|
||||||
|
type ActionRunsDefault ActionRuns
|
||||||
|
if err := value.Decode((*ActionRunsDefault)(actionRuns)); err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
for i := range actionRuns.Steps {
|
||||||
|
step := &actionRuns.Steps[i]
|
||||||
|
// Set `Number` and `ID` on each step based upon their position in the steps array:
|
||||||
|
if step.ID == "" {
|
||||||
|
step.ID = fmt.Sprintf("%d", i)
|
||||||
|
}
|
||||||
|
step.Number = i
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
// Action describes a metadata file for GitHub actions. The metadata filename must be either action.yml or action.yaml. The data in the metadata file defines the inputs, outputs and main entrypoint for your action.
|
// Action describes a metadata file for GitHub actions. The metadata filename must be either action.yml or action.yaml. The data in the metadata file defines the inputs, outputs and main entrypoint for your action.
|
||||||
type Action struct {
|
type Action struct {
|
||||||
Name string `yaml:"name"`
|
Name string `yaml:"name"`
|
||||||
|
|
|
||||||
|
|
@ -594,6 +594,24 @@ func (j *Job) Type() (JobType, error) {
|
||||||
return JobTypeDefault, nil
|
return JobTypeDefault, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func (j *Job) UnmarshalYAML(value *yaml.Node) error {
|
||||||
|
type JobDefault Job
|
||||||
|
if err := value.Decode((*JobDefault)(j)); err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
for i, step := range j.Steps {
|
||||||
|
if step == nil {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
// Set `Number` and `ID` on each step based upon their position in the steps array:
|
||||||
|
if step.ID == "" {
|
||||||
|
step.ID = fmt.Sprintf("%d", i)
|
||||||
|
}
|
||||||
|
step.Number = i
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
// ContainerSpec is the specification of the container to use for the job
|
// ContainerSpec is the specification of the container to use for the job
|
||||||
type ContainerSpec struct {
|
type ContainerSpec struct {
|
||||||
Image string `yaml:"image"`
|
Image string `yaml:"image"`
|
||||||
|
|
|
||||||
|
|
@ -136,17 +136,16 @@ func (rc *RunContext) compositeExecutor(action *model.Action) *compositeSteps {
|
||||||
|
|
||||||
sf := &stepFactoryImpl{}
|
sf := &stepFactoryImpl{}
|
||||||
|
|
||||||
for i, step := range action.Runs.Steps {
|
for i, stepModel := range action.Runs.Steps {
|
||||||
if step.ID == "" {
|
if stepModel.Number != i {
|
||||||
step.ID = fmt.Sprintf("%d", i)
|
return &compositeSteps{
|
||||||
|
pre: func(ctx context.Context) error { return nil },
|
||||||
|
main: common.NewErrorExecutor(fmt.Errorf("internal error: invalid Step: Number expected %v, was actually %v", i, stepModel.Number)),
|
||||||
|
post: func(ctx context.Context) error { return nil },
|
||||||
|
}
|
||||||
}
|
}
|
||||||
step.Number = i
|
|
||||||
|
|
||||||
// create a copy of the step, since this composite action could
|
step, err := sf.newStep(&stepModel, rc)
|
||||||
// run multiple times and we might modify the instance
|
|
||||||
stepcopy := step
|
|
||||||
|
|
||||||
step, err := sf.newStep(&stepcopy, rc)
|
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return &compositeSteps{
|
return &compositeSteps{
|
||||||
main: common.NewErrorExecutor(err),
|
main: common.NewErrorExecutor(err),
|
||||||
|
|
|
||||||
|
|
@ -57,14 +57,10 @@ func newJobExecutor(info jobInfo, sf stepFactory, rc *RunContext) common.Executo
|
||||||
|
|
||||||
for i, stepModel := range infoSteps {
|
for i, stepModel := range infoSteps {
|
||||||
if stepModel == nil {
|
if stepModel == nil {
|
||||||
return func(ctx context.Context) error {
|
return common.NewErrorExecutor(fmt.Errorf("invalid Step %v: missing run or uses key", i))
|
||||||
return fmt.Errorf("invalid Step %v: missing run or uses key", i)
|
} else if stepModel.Number != i {
|
||||||
}
|
return common.NewErrorExecutor(fmt.Errorf("internal error: invalid Step: Number expected %v, was actually %v", i, stepModel.Number))
|
||||||
}
|
}
|
||||||
if stepModel.ID == "" {
|
|
||||||
stepModel.ID = fmt.Sprintf("%d", i)
|
|
||||||
}
|
|
||||||
stepModel.Number = i
|
|
||||||
|
|
||||||
step, err := sf.newStep(stepModel, rc)
|
step, err := sf.newStep(stepModel, rc)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|
|
||||||
|
|
@ -209,11 +209,14 @@ func TestJobExecutorNewJobExecutor(t *testing.T) {
|
||||||
{
|
{
|
||||||
name: "stepsWithPreAndPost",
|
name: "stepsWithPreAndPost",
|
||||||
steps: []*model.Step{{
|
steps: []*model.Step{{
|
||||||
ID: "1",
|
ID: "1",
|
||||||
|
Number: 0,
|
||||||
}, {
|
}, {
|
||||||
ID: "2",
|
ID: "2",
|
||||||
|
Number: 1,
|
||||||
}, {
|
}, {
|
||||||
ID: "3",
|
ID: "3",
|
||||||
|
Number: 2,
|
||||||
}},
|
}},
|
||||||
preSteps: []bool{true, false, true},
|
preSteps: []bool{true, false, true},
|
||||||
postSteps: []bool{false, true, true},
|
postSteps: []bool{false, true, true},
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue