Repository: open-telemetry/opentelemetry-go-contrib
Pull request #2164 (2022-04-06 17:56:37)

otelhttp: nil check wrappedBody.Close
We're experimenting with open-telemetry and ran into a panic where `*wrappedBody.Close()` can panic. This seems to be code from 2+ years ago, so I'm surprised we ran into it. 

I'm happy to sign the CLA if this change will be accepted. 

linux-foundation-easycla[bot]:  <a href="https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/9522512/242202166/2164/#/?version=2"><img src="https://s3.amazonaws.com/cla-project-logo-prod/cla-not-signed.svg" alt="CLA Not Signed" align="left" height="28" width="328"></a><br/><br /><ul><li><a href='https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/9522512/242202166/2164/#/?version=2' target='_blank'>:x:</a> - login: @adamdecaf / name: Adam Shannon . The commit (75693e7cfe597c95cf0ac4f5cbf32e762609080e) is not authorized under a signed CLA. <a href='https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/9522512/242202166/2164/#/?version=2' target='_blank'>Please click here to be authorized</a>. For further assistance with EasyCLA, <a href='https://jira.linuxfoundation.org/servicedesk/customer/portal/4' target='_blank'>please submit a support request ticket</a>.</li></ul>
adamdecaf: @Aneurysm9 added, thanks! 
codecov[bot]: # [Codecov](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2164?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) Report
> Merging [#2164](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2164?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) (75693e7) into [main](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/commit/8cf7954c58eab45e3fef799742c3e798c2174a59?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) (8cf7954) will **increase** coverage by `0.0%`.
> The diff coverage is `100.0%`.

> :exclamation: Current head 75693e7 differs from pull request most recent head 78437aa. Consider uploading reports for the commit 78437aa to get more accurate results

[![Impacted file tree graph](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2164/graphs/tree.svg?width=650&height=150&src=pr&token=P6F3W9WA7Q&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)

```diff
@@          Coverage Diff          @@
##            main   #2164   +/-   ##
=====================================
  Coverage   69.4%   69.4%           
=====================================
  Files        135     135           
  Lines       6113    6116    +3     
=====================================
+ Hits        4243    4246    +3     
  Misses      1742    1742           
  Partials     128     128           
```


| [Impacted Files](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | |
|---|---|---|
| [instrumentation/net/http/otelhttp/transport.go](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2164/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-aW5zdHJ1bWVudGF0aW9uL25ldC9odHRwL290ZWxodHRwL3RyYW5zcG9ydC5nbw==) | `94.7% <100.0%> (+0.1%)` | :arrow_up: |

adamdecaf: I got a notification about an error with the GRPC instrument, but that's not part of my changes.

https://github.com/open-telemetry/opentelemetry-go-contrib/blob/f06b204c74976660004392462bed1a8c6b8171ca/instrumentation/google.golang.org/grpc/otelgrpc/test/interceptor_test.go#L396 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1d0f27439..58b89ce17 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -22,6 +22,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Upgraded all dependencies on stable modules from `go.opentelemetry.io/otel` from v1.5.0 to v1.6.1. (#2134)
 - Upgraded all dependencies on metric modules from `go.opentelemetry.io/otel` from v0.27.0 to v0.28.0. (#1977)
 
+### Fixed
+
+- otelhttp: Avoid panic by adding nil check to `wrappedBody.Close` (#2164)
+
 ## [1.5.0/0.30.0/0.1.0] - 2022-03-16
 
 ### Added
diff --git a/instrumentation/net/http/otelhttp/transport.go b/instrumentation/net/http/otelhttp/transport.go
index b436f6a2a..6c32bb117 100644
--- a/instrumentation/net/http/otelhttp/transport.go
+++ b/instrumentation/net/http/otelhttp/transport.go
@@ -186,5 +186,8 @@ func (wb *wrappedBody) Read(b []byte) (int, error) {
 
 func (wb *wrappedBody) Close() error {
 	wb.span.End()
-	return wb.body.Close()
+	if wb.body != nil {
+		return wb.body.Close()
+	}
+	return nil
 }


Test patch:
diff --git a/instrumentation/net/http/otelhttp/transport_test.go b/instrumentation/net/http/otelhttp/transport_test.go
index e155d3e08..af532fa88 100644
--- a/instrumentation/net/http/otelhttp/transport_test.go
+++ b/instrumentation/net/http/otelhttp/transport_test.go
@@ -281,6 +281,13 @@ func TestWrappedBodyClose(t *testing.T) {
 	s.assert(t, true, nil, codes.Unset, "")
 }
 
+func TestWrappedBodyClosePanic(t *testing.T) {
+	s := new(span)
+	var body io.ReadCloser
+	wb := newWrappedBody(s, body)
+	assert.NotPanics(t, func() { wb.Close() }, "nil body should not panic on close")
+}
+
 func TestWrappedBodyCloseError(t *testing.T) {
 	s := new(span)
 	expectedErr := errors.New("test")