Panic with Go versions < 1.15.0 when reading (http.Response).Body #41071

atc0005 · 2020-08-27T10:24:24Z

Overview

The steps below reproduce a panic with Go versions earlier than 1.15beta1. If Go 1.15 has the correct behavior, should Go 1.14.x also behave the same?

Go versions tested

Panics:

1.13.14
1.13.15
1.14.0
1.14.1
1.14.2
1.14.3
1.14.4
1.14.5
1.14.6
1.14.7

Does not panic:

1.15beta1
1.15rc1
1.15rc2

Instructions

cd /tmp
git clone https://github.com/atc0005/go-teams-notify
git checkout go-panic-repro
docker run --rm -it -v `pwd`:`pwd` -w `pwd` golang:1.15.0 go test -v ./...
docker run --rm -it -v `pwd`:`pwd` -w `pwd` golang:1.14.7 go test -v ./...

Output

Passing

=== RUN   TestTeamsClientSend
    send_test.go:100: OK: test 0; test.error is of type *fmt.wrapError, err is of type *fmt.wrapError
    send_test.go:100: OK: test 1; test.error is of type *errors.errorString, err is of type *errors.errorString
2020/08/27 09:18:25 RoundTripper returned a response & error; ignoring response
    send_test.go:100: OK: test 2; test.error is of type *url.Error, err is of type *url.Error
2020/08/27 09:18:25 RoundTripper returned a response & error; ignoring response
    send_test.go:100: OK: test 3; test.error is of type *url.Error, err is of type *url.Error
    send_test.go:100: OK: test 4; test.error is of type *errors.errorString, err is of type *errors.errorString
--- PASS: TestTeamsClientSend (0.00s)
PASS
ok      github.com/atc0005/go-teams-notify/v2   0.005s

Failing

=== RUN   TestTeamsClientSend
    send_test.go:100: OK: test 0; test.error is of type *fmt.wrapError, err is of type *fmt.wrapError
    send_test.go:100: OK: test 1; test.error is of type *errors.errorString, err is of type *errors.errorString
2020/08/27 09:18:39 RoundTripper returned a response & error; ignoring response
    send_test.go:100: OK: test 2; test.error is of type *url.Error, err is of type *url.Error
2020/08/27 09:18:39 RoundTripper returned a response & error; ignoring response
    send_test.go:100: OK: test 3; test.error is of type *url.Error, err is of type *url.Error
--- FAIL: TestTeamsClientSend (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x4d68bd]

goroutine 19 [running]:
testing.tRunner.func1.1(0x6e28e0, 0x99af10)
        /usr/local/go/src/testing/testing.go:988 +0x30d
testing.tRunner.func1(0xc0000ca480)
        /usr/local/go/src/testing/testing.go:991 +0x3f9
panic(0x6e28e0, 0x99af10)
        /usr/local/go/src/runtime/panic.go:969 +0x166
io/ioutil.readAll.func1(0xc00010b6c0)
        /usr/local/go/src/io/ioutil/ioutil.go:30 +0x101
panic(0x6e28e0, 0x99af10)
        /usr/local/go/src/runtime/panic.go:969 +0x166
bytes.(*Buffer).ReadFrom(0xc00010b648, 0x0, 0x0, 0xc0000932b0, 0x0, 0x0)
        /usr/local/go/src/bytes/buffer.go:204 +0x7d
io/ioutil.readAll(0x0, 0x0, 0x200, 0x0, 0x0, 0x0, 0x0, 0x0)
        /usr/local/go/src/io/ioutil/ioutil.go:36 +0xe3
io/ioutil.ReadAll(...)
        /usr/local/go/src/io/ioutil/ioutil.go:45
github.com/atc0005/go-teams-notify/v2.teamsClient.Send(0xc000097650, 0x74596e, 0x26, 0x73b510, 0xb, 0x741bdb, 0x1d, 0x0, 0x0, 0x0, ...)
        /tmp/go-teams-notify/send.go:70 +0x2ab
github.com/atc0005/go-teams-notify/v2.TestTeamsClientSend(0xc0000ca480)
        /tmp/go-teams-notify/send_test.go:89 +0x55e
testing.tRunner(0xc0000ca480, 0x7526e8)
        /usr/local/go/src/testing/testing.go:1039 +0xdc
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:1090 +0x372
FAIL    github.com/atc0005/go-teams-notify/v2   0.014s
FAIL

References

Changes that triggered the panic behavior
- atc0005/go-teams-notify@6db6217
Apparent "fix" for the panic
- atc0005/go-teams-notify@414b97a
  - specifically line 123
- note: this does not appear to be strictly required for Go 1.15beta1, rc1, rc2 or the 1.15.0 release

The text was updated successfully, but these errors were encountered:

davecheney · 2020-08-27T12:56:14Z

There is a bug in your test

2020/08/27 09:18:25 RoundTripper returned a response & error; ignoring response

This is the fix

(~/devel/go-teams-notify) % git diff
diff --git a/send_test.go b/send_test.go
index 6a1d6d1..8824f67 100644
--- a/send_test.go
+++ b/send_test.go
@@ -116,6 +116,9 @@ func TestTeamsClientSend(t *testing.T) {
                client := NewTestClient(func(req *http.Request) (*http.Response, error) {
                        // Test request parameters
                        assert.Equal(t, req.URL.String(), test.reqURL)
+                       if test.resError != nil {
+                               return nil, test.resError
+                       }
                        return &http.Response{
                                StatusCode: test.resStatus,

@@ -124,7 +127,7 @@ func TestTeamsClientSend(t *testing.T) {

                                // Must be set to non-nil value or it panics
                                Header: make(http.Header),
-                       }, test.resError
+                       }, nil
                })
                c := &teamsClient{httpClient: client}

atc0005 · 2020-08-27T13:30:32Z

Thank you @davecheney, I'll file that and review the changes.

The behavior you note precedes me (confirmed all the way back to v1.2.0) and I had yet to dig into it deep enough to understand all aspects of the test. I'm not sure I would have caught this specific issue for some time (I am still very new to Go).

Regarding the issue I opened here and the difference in behavior between Go 1.14 and 1.15: do you know if Go 1.15's handling of the code within the go-panic-repro branch is intentionally different than Go 1.14?

davecheney · 2020-08-27T13:46:14Z

Use the source, Luke. https://github.com/golang/go/commits/7fbd8c75c6c57e713069a3a405e5cde26cfae090/src/net/http/client.go

atc0005 · 2020-08-27T14:18:24Z

@davecheney: Use the source, Luke. https://github.com/golang/go/commits/7fbd8c75c6c57e713069a3a405e5cde26cfae090/src/net/http/client.go

Fair enough, and thank you for the pointer.

I didn't realize it was that easy to learn the answer (#38095, 2d77d33). I also failed to realize the scope of your involvement with the project! It appears that I also misunderstood the backporting policy (lots of learning today).

Based on what you've noted, it doesn't appear that the changes made in 12d02e7 and 2d77d33 for #38095 will be backported to Go 1.14.

That said, thank you for your time, the pointers and your suggested fixes to the current faulty test. I'll go ahead and close this out.

davecheney · 2020-08-27T15:00:45Z

No worries, glad you’ve been able to solve the problem.

@davecheney

CHANGES Apply fix provided by @davecheney in order to properly implement the RoundTripper interface's expected behavior: - return response and nil error OR - return nil and a non-nil error to explain failures to obtain a response I likely *over* explained this with doc comments, but I am still very much in "learning" mode here. REFERENCES - GH-46 - 2ce144f - 6db6217 - http://hassansin.github.io/Unit-Testing-http-client-in-Go - golang/go#41071 - golang/go#38095 - golang/go@2d77d33 - golang/go@12d02e7 - https://godoc.org/net/http#RoundTripper - https://godoc.org/net/http#Response - https://godoc.org/net/http#Response.Body

atc0005 mentioned this issue Aug 27, 2020

Report: bug in TestTeamsClientSend re handling of response error atc0005/go-teams-notify#46

Closed

atc0005 closed this as completed Aug 27, 2020

atc0005 mentioned this issue Aug 27, 2020

Fix invalid test RoundTripper implementation atc0005/go-teams-notify#47

Merged

golang locked and limited conversation to collaborators Aug 27, 2021

gopherbot added the FrozenDueToAge label Aug 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Panic with Go versions < 1.15.0 when reading (http.Response).Body #41071

Panic with Go versions < 1.15.0 when reading (http.Response).Body #41071

atc0005 commented Aug 27, 2020

davecheney commented Aug 27, 2020

atc0005 commented Aug 27, 2020

davecheney commented Aug 27, 2020

atc0005 commented Aug 27, 2020

davecheney commented Aug 27, 2020

Panic with Go versions < 1.15.0 when reading (http.Response).Body #41071

Panic with Go versions < 1.15.0 when reading (http.Response).Body #41071

Comments

atc0005 commented Aug 27, 2020

Overview

Go versions tested

Instructions

Output

Passing

Failing

References

davecheney commented Aug 27, 2020

atc0005 commented Aug 27, 2020

davecheney commented Aug 27, 2020

atc0005 commented Aug 27, 2020

davecheney commented Aug 27, 2020