From 513eb10f0b5832e8889eb255e99d8f4f453258bd Mon Sep 17 00:00:00 2001
From: Anton Ovchinnikov <anton@tonyo.info>
Date: Wed, 14 Jun 2023 10:33:06 +0200
Subject: [PATCH] fix: Clone scope.Context in multiple places to avoid
 concurrent reads/writes (#638)

---
 _examples/basic/main.go           |  6 ++---
 _examples/recover-repanic/main.go |  2 +-
 scope.go                          | 17 ++++++++++--
 scope_test.go                     | 22 ++++++++++++++++
 tracing.go                        |  2 +-
 tracing_test.go                   | 44 +++++++++++++++++++++++++++++++
 6 files changed, 86 insertions(+), 7 deletions(-)

diff --git a/_examples/basic/main.go b/_examples/basic/main.go
index 9833cf456..bb1c51da6 100644
--- a/_examples/basic/main.go
+++ b/_examples/basic/main.go
@@ -3,9 +3,9 @@
 //
 // Try it by running:
 //
-// 	go run main.go
-// 	go run main.go https://sentry.io
-// 	go run main.go bad-url
+//	go run main.go
+//	go run main.go https://sentry.io
+//	go run main.go bad-url
 //
 // To actually report events to Sentry, set the DSN either by editing the
 // appropriate line below or setting the environment variable SENTRY_DSN to
diff --git a/_examples/recover-repanic/main.go b/_examples/recover-repanic/main.go
index 5344f0933..a35bc7482 100644
--- a/_examples/recover-repanic/main.go
+++ b/_examples/recover-repanic/main.go
@@ -5,7 +5,7 @@
 //
 // Try it by running:
 //
-//  go run main.go
+//	go run main.go
 //
 // To actually report events to Sentry, set the DSN either by editing the
 // appropriate line below or setting the environment variable SENTRY_DSN to
diff --git a/scope.go b/scope.go
index 03602bd65..52cb76192 100644
--- a/scope.go
+++ b/scope.go
@@ -287,7 +287,7 @@ func (scope *Scope) Clone() *Scope {
 		clone.tags[key] = value
 	}
 	for key, value := range scope.contexts {
-		clone.contexts[key] = value
+		clone.contexts[key] = cloneContext(value)
 	}
 	for key, value := range scope.extra {
 		clone.extra[key] = value
@@ -350,7 +350,7 @@ func (scope *Scope) ApplyToEvent(event *Event, hint *EventHint) *Event {
 
 			// Ensure we are not overwriting event fields
 			if _, ok := event.Contexts[key]; !ok {
-				event.Contexts[key] = value
+				event.Contexts[key] = cloneContext(value)
 			}
 		}
 	}
@@ -404,3 +404,16 @@ func (scope *Scope) ApplyToEvent(event *Event, hint *EventHint) *Event {
 
 	return event
 }
+
+// cloneContext returns a new context with keys and values copied from the passed one.
+//
+// Note: a new Context (map) is returned, but the function does NOT do
+// a proper deep copy: if some context values are pointer types (e.g. maps),
+// they won't be properly copied.
+func cloneContext(c Context) Context {
+	res := Context{}
+	for k, v := range c {
+		res[k] = v
+	}
+	return res
+}
diff --git a/scope_test.go b/scope_test.go
index eb295598b..b2ca37465 100644
--- a/scope_test.go
+++ b/scope_test.go
@@ -618,3 +618,25 @@ func TestEventProcessorsAddEventProcessor(t *testing.T) {
 		t.Error("event should be dropped")
 	}
 }
+
+func TestCloneContext(t *testing.T) {
+	context := Context{
+		"key1": "value1",
+		"key2": []string{"s1", "s2"},
+	}
+
+	clone := cloneContext(context)
+
+	// Value-wise they should be identical
+	assertEqual(t, context, clone)
+	// ..but it shouldn't be the same map
+	if &context == &clone {
+		t.Error("original and cloned context should be different objects")
+	}
+
+	sliceOriginal := context["key2"].([]string)
+	sliceClone := clone["key2"].([]string)
+	if &sliceOriginal[0] != &sliceClone[0] {
+		t.Error("complex values are not supposed to be copied")
+	}
+}
diff --git a/tracing.go b/tracing.go
index 32aea821b..8466bacca 100644
--- a/tracing.go
+++ b/tracing.go
@@ -535,7 +535,7 @@ func (s *Span) toEvent() *Event {
 
 	contexts := map[string]Context{}
 	for k, v := range s.contexts {
-		contexts[k] = v
+		contexts[k] = cloneContext(v)
 	}
 	contexts["trace"] = s.traceContext().Map()
 
diff --git a/tracing_test.go b/tracing_test.go
index ca3762a1d..dc7fd9d75 100644
--- a/tracing_test.go
+++ b/tracing_test.go
@@ -10,6 +10,7 @@ import (
 	"math"
 	"net/http"
 	"reflect"
+	"sync"
 	"testing"
 	"time"
 
@@ -891,6 +892,49 @@ func TestDeprecatedSpanOptionTransctionSource(t *testing.T) {
 	StartSpan(context.Background(), "op", TransctionSource("src"))
 }
 
+// This test checks that there are no concurrent reads/writes to
+// substructures in scope.contexts.
+// See https://github.com/getsentry/sentry-go/issues/570 for more details.
+func TestConcurrentContextAccess(t *testing.T) {
+	ctx := NewTestContext(ClientOptions{
+		EnableTracing:    true,
+		TracesSampleRate: 1,
+	})
+	hub := GetHubFromContext(ctx)
+
+	const writersNum = 200
+
+	// Unbuffered channel, writing to it will be block if nobody reads
+	c := make(chan *Span)
+
+	// Start writers
+	for i := 0; i < writersNum; i++ {
+		go func() {
+			transaction := StartTransaction(ctx, "test")
+			c <- transaction
+			hub.Scope().SetContext("device", Context{"test": "bla"})
+		}()
+	}
+
+	var wg sync.WaitGroup
+	wg.Add(writersNum)
+
+	// Start readers
+	go func() {
+		for transaction := range c {
+			transaction := transaction
+			go func() {
+				defer wg.Done()
+				// While finalizing every transaction, scope.Contexts and Event.Contexts fields
+				// will be accessed, e.g. in environmentIntegration.processor()
+				transaction.Finish()
+			}()
+		}
+	}()
+
+	wg.Wait()
+}
+
 func TestAdjustingTransactionSourceBeforeSending(t *testing.T) {
 	tests := []struct {
 		name                   string