From 85c05b23b90e60b5df825c4b3003a8b9fdc75475 Mon Sep 17 00:00:00 2001
From: Nick Ripley <97066770+nsrip-dd@users.noreply.github.com>
Date: Thu, 21 Jul 2022 10:51:40 -0400
Subject: [PATCH] profiler: make configured tags read-only (#1383)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* profiler: make configured tags read-only

After being bitten by a bug which was in part caused by implicitly
relying on append() creating a slice backed by new memory rather than
sharing the same allocation, we should regard the configured tags as
read-only. See PR #1377.

This commit adds a small immutable package with a StringSlice type which
wraps the slice and provides fresh copies on any access or modification,
and uses the package for the configured tags.

Co-authored-by: Felix Geisendörfer <felix@datadoghq.com>
---
 profiler/internal/immutable/stringslice.go    | 33 ++++++++++
 .../internal/immutable/stringslice_test.go    | 61 +++++++++++++++++++
 profiler/options.go                           |  9 +--
 profiler/options_test.go                      | 33 +++++-----
 profiler/profile.go                           |  8 +--
 profiler/profiler.go                          |  7 +--
 profiler/upload.go                            |  4 +-
 7 files changed, 123 insertions(+), 32 deletions(-)
 create mode 100644 profiler/internal/immutable/stringslice.go
 create mode 100644 profiler/internal/immutable/stringslice_test.go

diff --git a/profiler/internal/immutable/stringslice.go b/profiler/internal/immutable/stringslice.go
new file mode 100644
index 0000000000..bce454708b
--- /dev/null
+++ b/profiler/internal/immutable/stringslice.go
@@ -0,0 +1,33 @@
+// Unless explicitly stated otherwise all files in this repository are licensed
+// under the Apache License Version 2.0.
+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2016 Datadog, Inc.
+
+// Package immutable provides read-only types
+package immutable
+
+// StringSlice holds a slice which cannot be modified and must be copied to
+// access. The zero value for a StringSlice is an empty slice (not nil). All StringSlice
+// methods are safe to call from multiple goroutines concurrently.
+type StringSlice struct {
+	s []string
+}
+
+// NewStringSlice creates a StringSlice from a copy of the input slice
+func NewStringSlice(s []string) StringSlice {
+	return StringSlice{s: append([]string{}, s...)}
+}
+
+// Slice returns a copy of the slice held by s
+func (s StringSlice) Slice() []string {
+	return append([]string{}, s.s...)
+}
+
+// Append creates a new StringSlice by concatenating the given strings to a copy
+// of the slice held by s.
+func (s StringSlice) Append(strings ...string) StringSlice {
+	dup := make([]string, len(s.s)+len(strings))
+	copy(dup, s.s)
+	copy(dup[len(s.s):], strings)
+	return StringSlice{s: dup}
+}
diff --git a/profiler/internal/immutable/stringslice_test.go b/profiler/internal/immutable/stringslice_test.go
new file mode 100644
index 0000000000..f4416d5023
--- /dev/null
+++ b/profiler/internal/immutable/stringslice_test.go
@@ -0,0 +1,61 @@
+// Unless explicitly stated otherwise all files in this repository are licensed
+// under the Apache License Version 2.0.
+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2016 Datadog, Inc.
+
+package immutable_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"gopkg.in/DataDog/dd-trace-go.v1/profiler/internal/immutable"
+)
+
+func TestStringSlice(t *testing.T) {
+	tags := []string{"service:foo", "env:bar", "ggthingy:baz"}
+	f := immutable.NewStringSlice(tags)
+	assert.Equal(t, tags, f.Slice())
+}
+
+func TestStringSliceModify(t *testing.T) {
+	t.Run("modify-original", func(t *testing.T) {
+		tags := []string{"service:foo", "env:bar", "thingy:baz"}
+		f := immutable.NewStringSlice(tags)
+		tags[0] = "service:different"
+		assert.Equal(t, "service:foo", f.Slice()[0])
+	})
+
+	t.Run("modify-copy", func(t *testing.T) {
+		tags := []string{"service:foo", "env:bar", "thingy:baz"}
+		f := immutable.NewStringSlice(tags)
+		dup := f.Slice()
+		dup[0] = "service:different"
+		assert.Equal(t, "service:foo", tags[0])
+	})
+
+	t.Run("modify-2-copies", func(t *testing.T) {
+		tags := []string{"service:foo", "env:bar", "thingy:baz"}
+		f := immutable.NewStringSlice(tags)
+		dup := f.Slice()
+		dup[0] = "service:different"
+		dup2 := f.Slice()
+		dup2[0] = "service:alsodifferent"
+		assert.Equal(t, "service:foo", tags[0])
+		assert.Equal(t, "service:different", dup[0])
+		assert.Equal(t, "service:alsodifferent", dup2[0])
+	})
+
+	t.Run("append-duplicates", func(t *testing.T) {
+		var f immutable.StringSlice
+		before := f.Slice()
+		g := f.Append("foo:bar")
+		h := f.Append("other:tag")
+		after := g.Slice()
+		after2 := h.Slice()
+		assert.NotEqual(t, before, after)
+		assert.NotEqual(t, before, after2)
+		assert.NotEqual(t, after, after2)
+	})
+}
diff --git a/profiler/options.go b/profiler/options.go
index 1168fa7f38..900f192500 100644
--- a/profiler/options.go
+++ b/profiler/options.go
@@ -26,6 +26,7 @@ import (
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/osinfo"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/version"
 	"gopkg.in/DataDog/dd-trace-go.v1/profiler/internal/extensions"
+	"gopkg.in/DataDog/dd-trace-go.v1/profiler/internal/immutable"
 
 	"github.com/DataDog/datadog-go/v5/statsd"
 )
@@ -95,7 +96,7 @@ type config struct {
 	hostname          string
 	statsd            StatsdClient
 	httpClient        *http.Client
-	tags              []string
+	tags              immutable.StringSlice
 	types             map[ProfileType]struct{}
 	period            time.Duration
 	cpuDuration       time.Duration
@@ -150,7 +151,7 @@ func logStartup(c *config) {
 		Env:                  c.env,
 		TargetURL:            c.targetURL,
 		Agentless:            c.agentless,
-		Tags:                 c.tags,
+		Tags:                 c.tags.Slice(),
 		ProfilePeriod:        c.period.String(),
 		CPUDuration:          c.cpuDuration.String(),
 		CPUProfileRate:       c.cpuProfileRate,
@@ -211,12 +212,12 @@ func defaultConfig() (*config, error) {
 		mutexFraction:     DefaultMutexFraction,
 		uploadTimeout:     DefaultUploadTimeout,
 		maxGoroutinesWait: 1000, // arbitrary value, should limit STW to ~30ms
-		tags:              []string{fmt.Sprintf("process_id:%d", os.Getpid())},
 		deltaProfiles:     internal.BoolEnv("DD_PROFILING_DELTA", true),
 		logStartup:        internal.BoolEnv("DD_TRACE_STARTUP_LOGS", true),
 		cmemprofEnabled:   false,
 		cmemprofRate:      extensions.DefaultCAllocationSamplingRate,
 	}
+	c.tags = c.tags.Append(fmt.Sprintf("process_id:%d", os.Getpid()))
 	for _, t := range defaultProfileTypes {
 		c.addProfileType(t)
 	}
@@ -441,7 +442,7 @@ func WithVersion(version string) Option {
 // filter the profiling view based on various information.
 func WithTags(tags ...string) Option {
 	return func(cfg *config) {
-		cfg.tags = append(cfg.tags, tags...)
+		cfg.tags = cfg.tags.Append(tags...)
 	}
 }
 
diff --git a/profiler/options_test.go b/profiler/options_test.go
index 9d867fdc9a..22e0ae9bd1 100644
--- a/profiler/options_test.go
+++ b/profiler/options_test.go
@@ -180,7 +180,7 @@ func TestOptions(t *testing.T) {
 	t.Run("WithVersion", func(t *testing.T) {
 		var cfg config
 		WithVersion("1.2.3")(&cfg)
-		assert.Contains(t, cfg.tags, "version:1.2.3")
+		assert.Contains(t, cfg.tags.Slice(), "version:1.2.3")
 	})
 
 	t.Run("WithVersion/override", func(t *testing.T) {
@@ -189,15 +189,16 @@ func TestOptions(t *testing.T) {
 		cfg, err := defaultConfig()
 		require.NoError(t, err)
 		WithVersion("1.2.3")(cfg)
-		assert.Contains(t, cfg.tags, "version:1.2.3")
+		assert.Contains(t, cfg.tags.Slice(), "version:1.2.3")
 	})
 
 	t.Run("WithTags", func(t *testing.T) {
 		var cfg config
 		WithTags("a:1", "b:2", "c:3")(&cfg)
-		assert.Contains(t, cfg.tags, "a:1")
-		assert.Contains(t, cfg.tags, "b:2")
-		assert.Contains(t, cfg.tags, "c:3")
+		tags := cfg.tags.Slice()
+		assert.Contains(t, tags, "a:1")
+		assert.Contains(t, tags, "b:2")
+		assert.Contains(t, tags, "c:3")
 	})
 
 	t.Run("WithTags/override", func(t *testing.T) {
@@ -206,11 +207,12 @@ func TestOptions(t *testing.T) {
 		cfg, err := defaultConfig()
 		require.NoError(t, err)
 		WithTags("a:1", "b:2", "c:3")(cfg)
-		assert.Contains(t, cfg.tags, "a:1")
-		assert.Contains(t, cfg.tags, "b:2")
-		assert.Contains(t, cfg.tags, "c:3")
-		assert.Contains(t, cfg.tags, "env1:tag1")
-		assert.Contains(t, cfg.tags, "env2:tag2")
+		tags := cfg.tags.Slice()
+		assert.Contains(t, tags, "a:1")
+		assert.Contains(t, tags, "b:2")
+		assert.Contains(t, tags, "c:3")
+		assert.Contains(t, tags, "env1:tag1")
+		assert.Contains(t, tags, "env2:tag2")
 	})
 
 	t.Run("WithDeltaProfiles", func(t *testing.T) {
@@ -294,7 +296,7 @@ func TestEnvVars(t *testing.T) {
 		defer os.Unsetenv("DD_VERSION")
 		cfg, err := defaultConfig()
 		require.NoError(t, err)
-		assert.Contains(t, cfg.tags, "version:1.2.3")
+		assert.Contains(t, cfg.tags.Slice(), "version:1.2.3")
 	})
 
 	t.Run("DD_TAGS", func(t *testing.T) {
@@ -302,9 +304,10 @@ func TestEnvVars(t *testing.T) {
 		defer os.Unsetenv("DD_TAGS")
 		cfg, err := defaultConfig()
 		require.NoError(t, err)
-		assert.Contains(t, cfg.tags, "a:1")
-		assert.Contains(t, cfg.tags, "b:2")
-		assert.Contains(t, cfg.tags, "c:3")
+		tags := cfg.tags.Slice()
+		assert.Contains(t, tags, "a:1")
+		assert.Contains(t, tags, "b:2")
+		assert.Contains(t, tags, "c:3")
 	})
 
 	t.Run("DD_PROFILING_DELTA", func(t *testing.T) {
@@ -338,7 +341,7 @@ func TestDefaultConfig(t *testing.T) {
 		assert.Equal(0, cfg.cpuProfileRate)
 		assert.Equal(DefaultMutexFraction, cfg.mutexFraction)
 		assert.Equal(DefaultBlockRate, cfg.blockRate)
-		assert.Contains(cfg.tags, "runtime-id:"+globalconfig.RuntimeID())
+		assert.Contains(cfg.tags.Slice(), "runtime-id:"+globalconfig.RuntimeID())
 		assert.Equal(true, cfg.deltaProfiles)
 	})
 }
diff --git a/profiler/profile.go b/profiler/profile.go
index 1067bc94d9..73a08e3956 100644
--- a/profiler/profile.go
+++ b/profiler/profile.go
@@ -195,9 +195,7 @@ func collectGenericProfile(name string, delta *pprofutils.Delta) func(p *profile
 
 		start := time.Now()
 		delta, err := p.deltaProfile(name, delta, data, extra...)
-		tags := make([]string, len(p.cfg.tags), len(p.cfg.tags)+1)
-		copy(tags, p.cfg.tags)
-		tags = append(tags, fmt.Sprintf("profile_type:%s", name))
+		tags := append(p.cfg.tags.Slice(), fmt.Sprintf("profile_type:%s", name))
 		p.cfg.statsd.Timing("datadog.profiling.go.delta_time", time.Since(start), tags, 1)
 		if err != nil {
 			return nil, fmt.Errorf("delta profile error: %s", err)
@@ -266,9 +264,7 @@ func (p *profiler) runProfile(pt ProfileType) ([]*profile, error) {
 		return nil, err
 	}
 	end := now()
-	tags := make([]string, len(p.cfg.tags), len(p.cfg.tags)+1)
-	copy(tags, p.cfg.tags)
-	tags = append(tags, pt.Tag())
+	tags := append(p.cfg.tags.Slice(), pt.Tag())
 	filename := t.Filename
 	// TODO(fg): Consider making Collect() return the filename.
 	if p.cfg.deltaProfiles && t.SupportsDelta {
diff --git a/profiler/profiler.go b/profiler/profiler.go
index 790134eef9..11b9fe8331 100644
--- a/profiler/profiler.go
+++ b/profiler/profiler.go
@@ -242,9 +242,8 @@ func (p *profiler) collect(ticker <-chan time.Time) {
 					profs, err := p.runProfile(t)
 					if err != nil {
 						log.Error("Error getting %s profile: %v; skipping.", t, err)
-						tags := make([]string, len(p.cfg.tags), len(p.cfg.tags)+1)
-						copy(tags, p.cfg.tags)
-						p.cfg.statsd.Count("datadog.profiling.go.collect_error", 1, append(tags, t.Tag()), 1)
+						tags := append(p.cfg.tags.Slice(), t.Tag())
+						p.cfg.statsd.Count("datadog.profiling.go.collect_error", 1, tags, 1)
 					}
 					mu.Lock()
 					defer mu.Unlock()
@@ -297,7 +296,7 @@ func (p *profiler) enqueueUpload(bat batch) {
 			// queue is full; evict oldest
 			select {
 			case <-p.out:
-				p.cfg.statsd.Count("datadog.profiling.go.queue_full", 1, p.cfg.tags, 1)
+				p.cfg.statsd.Count("datadog.profiling.go.queue_full", 1, p.cfg.tags.Slice(), 1)
 				log.Warn("Evicting one profile batch from the upload queue to make room.")
 			default:
 				// this case should be almost impossible to trigger, it would require a
diff --git a/profiler/upload.go b/profiler/upload.go
index 8e6f3e872f..ae6779698d 100644
--- a/profiler/upload.go
+++ b/profiler/upload.go
@@ -68,9 +68,7 @@ func (e retriableError) Error() string { return e.err.Error() }
 // doRequest makes an HTTP POST request to the Datadog Profiling API with the
 // given profile.
 func (p *profiler) doRequest(bat batch) error {
-	tags := make([]string, len(p.cfg.tags))
-	copy(tags, p.cfg.tags)
-	tags = append(tags,
+	tags := append(p.cfg.tags.Slice(),
 		fmt.Sprintf("service:%s", p.cfg.service),
 		fmt.Sprintf("env:%s", p.cfg.env),
 		// The profile_seq tag can be used to identify the first profile