From 795a11c44e99eb13a08f77aeaa0a173acaa36adb Mon Sep 17 00:00:00 2001 From: Levi Blackstone Date: Tue, 22 Dec 2020 12:04:49 -0700 Subject: [PATCH] Fix a panic due to unsafe concurrent map access (#5995) State tracking for goals was implemented using a raw map, but this was not safe for concurrent read/write access from multiple goroutines. Switched to using a sync.Map, which is threadsafe. --- CHANGELOG.md | 3 +++ pkg/resource/deploy/deployment.go | 22 +++++++++++++++++++--- pkg/resource/deploy/import.go | 4 ++++ pkg/resource/deploy/step_executor.go | 2 +- pkg/resource/deploy/step_generator.go | 5 +++-- 5 files changed, 30 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ee0aeb62..98f1371fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,9 @@ CHANGELOG ## HEAD (Unreleased) +- Fix a panic due to unsafe concurrent map access. + [#5995](https://github.com/pulumi/pulumi/pull/5995) + - Fix regression in `venv` creation for python policy packs. [#5992](https://github.com/pulumi/pulumi/pull/5992) diff --git a/pkg/resource/deploy/deployment.go b/pkg/resource/deploy/deployment.go index 9e60370ab..b42affe8c 100644 --- a/pkg/resource/deploy/deployment.go +++ b/pkg/resource/deploy/deployment.go @@ -106,6 +106,22 @@ func (p PlanPendingOperationsError) Error() string { return "one or more operations are currently pending" } +type goalMap struct { + m sync.Map +} + +func (m *goalMap) set(urn resource.URN, goal *resource.Goal) { + m.m.Store(urn, goal) +} + +func (m *goalMap) get(urn resource.URN) (*resource.Goal, bool) { + g, ok := m.m.Load(urn) + if !ok { + return nil, false + } + return g.(*resource.Goal), true +} + type resourceMap struct { m sync.Map } @@ -144,7 +160,7 @@ type Deployment struct { preview bool // true if this deployment is to be previewed. depGraph *graph.DependencyGraph // the dependency graph of the old snapshot. providers *providers.Registry // the provider registry for this deployment. - goals map[resource.URN]*resource.Goal // the set of resource goals generated by the deployment. + goals *goalMap // the set of resource goals generated by the deployment. news *resourceMap // the set of new resources generated by the deployment. } @@ -305,8 +321,8 @@ func NewDeployment(ctx *plugin.Context, target *Target, prev *Snapshot, source S // Build the dependency graph for the old resources. depGraph := graph.NewDependencyGraph(oldResources) - // Create a goals map for the deployment. - newGoals := make(map[resource.URN]*resource.Goal) + // Create a goal map for the deployment. + newGoals := &goalMap{} // Create a resource map for the deployment. newResources := &resourceMap{} diff --git a/pkg/resource/deploy/import.go b/pkg/resource/deploy/import.go index 47b0b8cdd..bc4410a83 100644 --- a/pkg/resource/deploy/import.go +++ b/pkg/resource/deploy/import.go @@ -73,6 +73,9 @@ func NewImportDeployment(ctx *plugin.Context, target *Target, projectName tokens return nil, err } + // Create a goal map for the deployment. + newGoals := &goalMap{} + builtins := newBuiltinProvider(nil, nil) // Create a new provider registry. @@ -87,6 +90,7 @@ func NewImportDeployment(ctx *plugin.Context, target *Target, projectName tokens target: target, prev: prev, olds: olds, + goals: newGoals, imports: imports, isImport: true, schemaLoader: schema.NewPluginLoader(ctx.Host), diff --git a/pkg/resource/deploy/step_executor.go b/pkg/resource/deploy/step_executor.go index f4b483d40..fa932d7d4 100644 --- a/pkg/resource/deploy/step_executor.go +++ b/pkg/resource/deploy/step_executor.go @@ -293,7 +293,7 @@ func (se *stepExecutor) executeStep(workerID int, step Step) error { } // If this is not a resource that is managed by Pulumi, then we can ignore it. - if _, hasGoal := se.deployment.goals[newState.URN]; hasGoal { + if _, hasGoal := se.deployment.goals.get(newState.URN); hasGoal { se.deployment.news.set(newState.URN, newState) } } diff --git a/pkg/resource/deploy/step_generator.go b/pkg/resource/deploy/step_generator.go index 2071d4f96..cf323cadf 100644 --- a/pkg/resource/deploy/step_generator.go +++ b/pkg/resource/deploy/step_generator.go @@ -252,7 +252,7 @@ func (sg *stepGenerator) generateSteps(event RegisterResourceEvent) ([]Step, res // Mark the URN/resource as having been seen. So we can run analyzers on all resources seen, as well as // lookup providers for calculating replacement of resources that use the provider. - sg.deployment.goals[urn] = goal + sg.deployment.goals.set(urn, goal) if providers.IsProviderType(goal.Type) { sg.providers[urn] = new } @@ -1286,7 +1286,8 @@ func (sg *stepGenerator) calculateDependentReplacements(root *resource.State) ([ func (sg *stepGenerator) AnalyzeResources() result.Result { var resources []plugin.AnalyzerStackResource sg.deployment.news.mapRange(func(urn resource.URN, v *resource.State) bool { - goal := sg.deployment.goals[urn] + goal, ok := sg.deployment.goals.get(urn) + contract.Assertf(ok, "failed to load goal for %s", urn) resource := plugin.AnalyzerStackResource{ AnalyzerResource: plugin.AnalyzerResource{ URN: v.URN,