From 414c9fba1f1b4682d074f83c979881da7ac85a52 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 4 Sep 2024 15:06:51 +0200 Subject: [PATCH 1/3] Pass copy of `dyn.Path` to callback function Some call sites hold on to the `dyn.Path` provided to them by the callback. It must therefore never be mutated after the callback returns, or these mutations leak out into unknown scope. This change means it is no longer possible for this failure mode to happen. --- .../config/validate/unique_resource_keys.go | 6 +--- libs/dyn/visit.go | 2 +- libs/dyn/visit_map.go | 4 +-- libs/dyn/visit_test.go | 35 +++++++++++++++++++ 4 files changed, 39 insertions(+), 8 deletions(-) create mode 100644 libs/dyn/visit_test.go diff --git a/bundle/config/validate/unique_resource_keys.go b/bundle/config/validate/unique_resource_keys.go index d6212b0acf..50295375b6 100644 --- a/bundle/config/validate/unique_resource_keys.go +++ b/bundle/config/validate/unique_resource_keys.go @@ -3,7 +3,6 @@ package validate import ( "context" "fmt" - "slices" "sort" "github.com/databricks/cli/bundle" @@ -66,10 +65,7 @@ func (m *uniqueResourceKeys) Apply(ctx context.Context, b *bundle.Bundle) diag.D } } - // dyn.Path under the hood is a slice. The code that walks the configuration - // tree uses the same underlying slice to track the path as it walks - // the tree. So, we need to clone it here. - m.paths = append(m.paths, slices.Clone(p)) + m.paths = append(m.paths, p) m.locations = append(m.locations, v.Locations()...) resourceMetadata[k] = m diff --git a/libs/dyn/visit.go b/libs/dyn/visit.go index 4d3cf50142..38adec24ff 100644 --- a/libs/dyn/visit.go +++ b/libs/dyn/visit.go @@ -70,7 +70,7 @@ type visitOptions struct { func visit(v Value, prefix Path, suffix Pattern, opts visitOptions) (Value, error) { if len(suffix) == 0 { - return opts.fn(prefix, v) + return opts.fn(slices.Clone(prefix), v) } // Initialize prefix if it is empty. diff --git a/libs/dyn/visit_map.go b/libs/dyn/visit_map.go index cd2cd4831e..3f0cded03b 100644 --- a/libs/dyn/visit_map.go +++ b/libs/dyn/visit_map.go @@ -21,7 +21,7 @@ func Foreach(fn MapFunc) MapFunc { for _, pair := range m.Pairs() { pk := pair.Key pv := pair.Value - nv, err := fn(append(p, Key(pk.MustString())), pv) + nv, err := fn(p.Append(Key(pk.MustString())), pv) if err != nil { return InvalidValue, err } @@ -32,7 +32,7 @@ func Foreach(fn MapFunc) MapFunc { s := slices.Clone(v.MustSequence()) for i, value := range s { var err error - s[i], err = fn(append(p, Index(i)), value) + s[i], err = fn(p.Append(Index(i)), value) if err != nil { return InvalidValue, err } diff --git a/libs/dyn/visit_test.go b/libs/dyn/visit_test.go new file mode 100644 index 0000000000..86b8875fdb --- /dev/null +++ b/libs/dyn/visit_test.go @@ -0,0 +1,35 @@ +package dyn + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestVisitCallbackPathCopy(t *testing.T) { + vin := V(map[string]Value{ + "foo": V(42), + "bar": V(43), + }) + + var paths []Path + + // The callback should receive a copy of the path. + // If the same underlying value is used, all collected paths will be the same. + _, _ = visit(vin, EmptyPath, NewPattern(AnyKey()), visitOptions{ + fn: func(p Path, v Value) (Value, error) { + paths = append(paths, p) + return v, nil + }, + }) + + // Verify that the paths retained their original values. + var strings []string + for _, p := range paths { + strings = append(strings, p.String()) + } + assert.ElementsMatch(t, strings, []string{ + "foo", + "bar", + }) +} From 5a9847b03d05847c7c96668a8f180fc96521f542 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 4 Sep 2024 16:45:47 +0200 Subject: [PATCH 2/3] Update test --- libs/dyn/visit_test.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/libs/dyn/visit_test.go b/libs/dyn/visit_test.go index 86b8875fdb..5b61399be0 100644 --- a/libs/dyn/visit_test.go +++ b/libs/dyn/visit_test.go @@ -1,26 +1,27 @@ -package dyn +package dyn_test import ( "testing" - "github.com/stretchr/testify/assert" + "github.com/databricks/cli/libs/dyn" + assert "github.com/databricks/cli/libs/dyn/dynassert" ) func TestVisitCallbackPathCopy(t *testing.T) { - vin := V(map[string]Value{ - "foo": V(42), - "bar": V(43), + vin := dyn.V(map[string]dyn.Value{ + "foo": dyn.V(42), + "bar": dyn.V(43), }) - var paths []Path + var paths []dyn.Path // The callback should receive a copy of the path. // If the same underlying value is used, all collected paths will be the same. - _, _ = visit(vin, EmptyPath, NewPattern(AnyKey()), visitOptions{ - fn: func(p Path, v Value) (Value, error) { - paths = append(paths, p) - return v, nil - }, + // This test uses `MapByPattern` to collect all paths in the map. + // Visit itself doesn't have public functions and we exclusively use black-box testing for this package. + _, _ = dyn.MapByPattern(vin, dyn.NewPattern(dyn.AnyKey()), func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + paths = append(paths, p) + return v, nil }) // Verify that the paths retained their original values. From 279c19ebc05190ce9b259f5580bcad05975b9042 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 4 Sep 2024 16:46:36 +0200 Subject: [PATCH 3/3] Additional clone call sites --- bundle/artifacts/expand_globs.go | 7 +------ bundle/libraries/expand_glob_references.go | 8 +++----- bundle/libraries/upload.go | 2 +- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/bundle/artifacts/expand_globs.go b/bundle/artifacts/expand_globs.go index 617444054d..cdf3d45900 100644 --- a/bundle/artifacts/expand_globs.go +++ b/bundle/artifacts/expand_globs.go @@ -33,12 +33,7 @@ func createGlobError(v dyn.Value, p dyn.Path, message string) diag.Diagnostic { Severity: diag.Error, Summary: fmt.Sprintf("%s: %s", source, message), Locations: []dyn.Location{v.Location()}, - - Paths: []dyn.Path{ - // Hack to clone the path. This path copy is mutable. - // To be addressed in a later PR. - p.Append(), - }, + Paths: []dyn.Path{p}, } } diff --git a/bundle/libraries/expand_glob_references.go b/bundle/libraries/expand_glob_references.go index 9e90a2a17f..9322a06b83 100644 --- a/bundle/libraries/expand_glob_references.go +++ b/bundle/libraries/expand_glob_references.go @@ -16,12 +16,10 @@ type expand struct { func matchError(p dyn.Path, l []dyn.Location, message string) diag.Diagnostic { return diag.Diagnostic{ - Severity: diag.Error, - Summary: message, - Paths: []dyn.Path{ - p.Append(), - }, + Severity: diag.Error, + Summary: message, Locations: l, + Paths: []dyn.Path{p}, } } diff --git a/bundle/libraries/upload.go b/bundle/libraries/upload.go index be7cc41db5..224e7ab2d9 100644 --- a/bundle/libraries/upload.go +++ b/bundle/libraries/upload.go @@ -76,7 +76,7 @@ func collectLocalLibraries(b *bundle.Bundle) (map[string][]configLocation, error source = filepath.Join(b.RootPath, source) libs[source] = append(libs[source], configLocation{ - configPath: p.Append(), // Hack to get the copy of path + configPath: p, location: v.Location(), })