From 52345f40d7425cbf843528eb653069c587acfcb9 Mon Sep 17 00:00:00 2001
From: Gregory NISBET <gregorynisbet@google.com>
Date: Fri, 31 May 2024 18:40:39 -0700
Subject: [PATCH] Fix and detect race condition in clienttrace.go

ct.root can be updated without holding the mutex on ct. Also, addEvent
can be called on a nil ct.root.

Fix both of these issues by moving the mutex acquisition logic so that
ct.root is only touched while the mutex is held.
---
 .../httptrace/otelhttptrace/clienttrace.go    | 18 ++++++---
 .../otelhttptrace/clienttrace_test.go         | 39 +++++++++++++++++++
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go
index 67e03f24810..bb1f63a3bde 100644
--- a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go
+++ b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go
@@ -186,6 +186,9 @@ func NewClientTrace(ctx context.Context, opts ...ClientTraceOption) *httptrace.C
 }
 
 func (ct *clientTracer) start(hook, spanName string, attrs ...attribute.KeyValue) {
+	ct.mtx.Lock()
+	defer ct.mtx.Unlock()
+
 	if !ct.useSpans {
 		if ct.root == nil {
 			ct.root = trace.SpanFromContext(ct.Context)
@@ -194,9 +197,6 @@ func (ct *clientTracer) start(hook, spanName string, attrs ...attribute.KeyValue
 		return
 	}
 
-	ct.mtx.Lock()
-	defer ct.mtx.Unlock()
-
 	if hookCtx, found := ct.activeHooks[hook]; !found {
 		var sp trace.Span
 		ct.activeHooks[hook], sp = ct.tr.Start(ct.getParentContext(hook), spanName, trace.WithAttributes(attrs...), trace.WithSpanKind(trace.SpanKindClient))
@@ -214,6 +214,13 @@ func (ct *clientTracer) start(hook, spanName string, attrs ...attribute.KeyValue
 }
 
 func (ct *clientTracer) end(hook string, err error, attrs ...attribute.KeyValue) {
+	ct.mtx.Lock()
+	defer ct.mtx.Unlock()
+
+	if ct.root == nil {
+		return
+	}
+
 	if !ct.useSpans {
 		if err != nil {
 			attrs = append(attrs, attribute.String(hook+".error", err.Error()))
@@ -222,8 +229,6 @@ func (ct *clientTracer) end(hook string, err error, attrs ...attribute.KeyValue)
 		return
 	}
 
-	ct.mtx.Lock()
-	defer ct.mtx.Unlock()
 	if ctx, ok := ct.activeHooks[hook]; ok {
 		span := trace.SpanFromContext(ctx)
 		if err != nil {
@@ -321,6 +326,9 @@ func (ct *clientTracer) tlsHandshakeDone(_ tls.ConnectionState, err error) {
 }
 
 func (ct *clientTracer) wroteHeaderField(k string, v []string) {
+	if ct.root == nil {
+		return
+	}
 	if ct.useSpans && ct.span("http.headers") == nil {
 		ct.start("http.headers", "http.headers")
 	}
diff --git a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go
index 966473dd0c3..8405ed451ef 100644
--- a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go
+++ b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go
@@ -7,7 +7,10 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"net/http/httptest"
 	"net/http/httptrace"
+	"sync"
+	"testing"
 
 	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
 )
@@ -32,3 +35,39 @@ func ExampleNewClientTrace() {
 
 	fmt.Println(resp.Status)
 }
+
+// TestNewClientParallelismWithoutSubspans tests running many Gets on a client simultaneously,
+// which would trigger a race condition if root were not protected by a mutex.
+func TestNewClientParallelismWithoutSubspans(t *testing.T) {
+	t.Parallel()
+
+	makeClientTrace := func(ctx context.Context) *httptrace.ClientTrace {
+		return NewClientTrace(ctx, WithoutSubSpans())
+	}
+
+	server := httptest.NewServer(nil)
+
+	client := http.Client{
+		Transport: otelhttp.NewTransport(
+			server.Client().Transport,
+			otelhttp.WithClientTrace(makeClientTrace),
+		),
+	}
+
+	var wg sync.WaitGroup
+
+	for i := 1; i < 10000; i++ {
+		wg.Add(1)
+		go func() {
+			resp, err := client.Get("https://example.com")
+			if err != nil {
+				t.Error(err)
+				return
+			}
+			resp.Body.Close()
+			wg.Done()
+		}()
+	}
+
+	wg.Wait()
+}