From 414c9fba1f1b4682d074f83c979881da7ac85a52 Mon Sep 17 00:00:00 2001
From: Pieter Noordhuis <pieter.noordhuis@databricks.com>
Date: Wed, 4 Sep 2024 15:06:51 +0200
Subject: [PATCH] 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 d6212b0ac..50295375b 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 4d3cf5014..38adec24f 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 cd2cd4831..3f0cded03 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 000000000..86b8875fd
--- /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",
+	})
+}