From ff1894d55a9d3e053a9fbdb448a5e6778a53b055 Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Thu, 12 Jun 2025 11:10:49 +0200 Subject: [PATCH 1/6] reproduce panic --- .../httptrace/otelhttptrace/clienttracetest_test.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/instrumentation/net/http/httptrace/otelhttptrace/clienttracetest_test.go b/instrumentation/net/http/httptrace/otelhttptrace/clienttracetest_test.go index 4afe48d4f4d..2a6a27e2269 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/clienttracetest_test.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/clienttracetest_test.go @@ -15,11 +15,12 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" + + "go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace" ) func getSpanFromRecorder(sr *tracetest.SpanRecorder, name string) (trace.ReadOnlySpan, bool) { @@ -266,6 +267,15 @@ func TestEndBeforeStartWithoutSubSpansDoesNotPanic(t *testing.T) { require.Empty(t, sr.Ended()) } +func TestGot100ContinueInvokeBeforeFirstByte(t *testing.T) { + // It is possible that Got100Continue is called before GotFirstResponseByte. + // Also as there is no guarantee provided in the ClientTrace docs that GotFirstResponseByte should be called before + // Got100Continue this edge case should be covered. + + clienTrace := otelhttptrace.NewClientTrace(context.Background()) + clienTrace.Got100Continue() +} + type clientTraceTestFixture struct { Address string URL string From a3e8d2702c5d6cd8f679335762574d9d6b2ae63b Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Thu, 12 Jun 2025 11:11:33 +0200 Subject: [PATCH 2/6] check for `nil` span to avoid panic --- .../httptrace/otelhttptrace/clienttrace.go | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go index 222b35871fa..7b50563b0e9 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go @@ -11,11 +11,12 @@ import ( "strings" "sync" - "go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace/internal/semconv" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/trace" + + "go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace/internal/semconv" ) // ScopeName is the instrumentation scope name. @@ -360,7 +361,10 @@ func (ct *clientTracer) got100Continue() { if ct.useSpans { span = ct.span("http.receive") } - span.AddEvent("GOT 100 - Continue") + // It's possible that Got100Continue is called before GotFirstResponseByte at which point span can be `nil`. + if span != nil { + span.AddEvent("GOT 100 - Continue") + } } func (ct *clientTracer) wait100Continue() { @@ -368,7 +372,9 @@ func (ct *clientTracer) wait100Continue() { if ct.useSpans { span = ct.span("http.send") } - span.AddEvent("GOT 100 - Wait") + if span != nil { + span.AddEvent("GOT 100 - Wait") + } } func (ct *clientTracer) got1xxResponse(code int, header textproto.MIMEHeader) error { @@ -376,10 +382,12 @@ func (ct *clientTracer) got1xxResponse(code int, header textproto.MIMEHeader) er if ct.useSpans { span = ct.span("http.receive") } - span.AddEvent("GOT 1xx", trace.WithAttributes( - HTTPStatus.Int(code), - HTTPHeaderMIME.String(sm2s(header)), - )) + if span != nil { + span.AddEvent("GOT 1xx", trace.WithAttributes( + HTTPStatus.Int(code), + HTTPHeaderMIME.String(sm2s(header)), + )) + } return nil } From e7265338bab2b3efb0314d6b22c3b763cb2b0ee4 Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Thu, 12 Jun 2025 11:31:56 +0200 Subject: [PATCH 3/6] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83a3512d30f..1dc37a48021 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed - Fix data race when writing log entries with `context.Context` fields in `go.opentelemetry.io/contrib/bridges/otelzap`. (#7368) +- Fix nil pointer dereference when `ClientTracer` did not have a span in `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace`. (#7464) From 858e40318c78256d2e92849cb44ddc40de44019a Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Fri, 13 Jun 2025 10:38:15 +0200 Subject: [PATCH 4/6] improve nil pointer tests --- .../otelhttptrace/clienttracetest_test.go | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/instrumentation/net/http/httptrace/otelhttptrace/clienttracetest_test.go b/instrumentation/net/http/httptrace/otelhttptrace/clienttracetest_test.go index 2a6a27e2269..6aad09b65fb 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/clienttracetest_test.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/clienttracetest_test.go @@ -267,13 +267,27 @@ func TestEndBeforeStartWithoutSubSpansDoesNotPanic(t *testing.T) { require.Empty(t, sr.Ended()) } -func TestGot100ContinueInvokeBeforeFirstByte(t *testing.T) { - // It is possible that Got100Continue is called before GotFirstResponseByte. - // Also as there is no guarantee provided in the ClientTrace docs that GotFirstResponseByte should be called before - // Got100Continue this edge case should be covered. - - clienTrace := otelhttptrace.NewClientTrace(context.Background()) - clienTrace.Got100Continue() +func TestNoClientTraceCallGuarantee(t *testing.T) { + t.Run("Got100Continue", func(t *testing.T) { + // It is possible that Got100Continue is called before GotFirstResponseByte. + // Also as there is no guarantee provided in the ClientTrace docs that GotFirstResponseByte should be called before + // Got100Continue this edge case should be covered. + require.NotPanics(t, func() { + clientTrace := otelhttptrace.NewClientTrace(context.Background()) + clientTrace.Got100Continue() + }) + }) + t.Run("Got1xxResponse", func(t *testing.T) { + clientTrace := otelhttptrace.NewClientTrace(context.Background()) + err := clientTrace.Got1xxResponse(http.StatusNoContent, nil) + require.NoError(t, err) + }) + t.Run("Wait100Continue", func(t *testing.T) { + require.NotPanics(t, func() { + clientTrace := otelhttptrace.NewClientTrace(context.Background()) + clientTrace.Wait100Continue() + }) + }) } type clientTraceTestFixture struct { From 462ca116599d72e7d7aae4d4f1e41dc1b1bce557 Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Fri, 13 Jun 2025 10:41:02 +0200 Subject: [PATCH 5/6] add comments to got1xxResponse and wait100Continue --- instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go index 7b50563b0e9..942566e6550 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go @@ -372,6 +372,7 @@ func (ct *clientTracer) wait100Continue() { if ct.useSpans { span = ct.span("http.send") } + // It's possible that Wait100Continue is called before GotFirstResponseByte at which point span can be `nil`. if span != nil { span.AddEvent("GOT 100 - Wait") } @@ -382,6 +383,7 @@ func (ct *clientTracer) got1xxResponse(code int, header textproto.MIMEHeader) er if ct.useSpans { span = ct.span("http.receive") } + // It's possible that Got1xxResponse is called before GotFirstResponseByte at which point span can be `nil`. if span != nil { span.AddEvent("GOT 1xx", trace.WithAttributes( HTTPStatus.Int(code), From 1787d28dae9f8d5e0ba0c9c72c31f260dd6b6275 Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Mon, 16 Jun 2025 10:38:49 +0200 Subject: [PATCH 6/6] use assert instead of require --- .../http/httptrace/otelhttptrace/clienttracetest_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/instrumentation/net/http/httptrace/otelhttptrace/clienttracetest_test.go b/instrumentation/net/http/httptrace/otelhttptrace/clienttracetest_test.go index 6aad09b65fb..f498c7ca640 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/clienttracetest_test.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/clienttracetest_test.go @@ -272,7 +272,7 @@ func TestNoClientTraceCallGuarantee(t *testing.T) { // It is possible that Got100Continue is called before GotFirstResponseByte. // Also as there is no guarantee provided in the ClientTrace docs that GotFirstResponseByte should be called before // Got100Continue this edge case should be covered. - require.NotPanics(t, func() { + assert.NotPanics(t, func() { clientTrace := otelhttptrace.NewClientTrace(context.Background()) clientTrace.Got100Continue() }) @@ -280,10 +280,10 @@ func TestNoClientTraceCallGuarantee(t *testing.T) { t.Run("Got1xxResponse", func(t *testing.T) { clientTrace := otelhttptrace.NewClientTrace(context.Background()) err := clientTrace.Got1xxResponse(http.StatusNoContent, nil) - require.NoError(t, err) + assert.NoError(t, err) }) t.Run("Wait100Continue", func(t *testing.T) { - require.NotPanics(t, func() { + assert.NotPanics(t, func() { clientTrace := otelhttptrace.NewClientTrace(context.Background()) clientTrace.Wait100Continue() })