Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
atc0005 opened this issue Aug 27, 2020 · 5 comments
Closed

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

atc0005 opened this issue Aug 27, 2020 · 5 comments

Comments

@atc0005
Copy link

@atc0005 atc0005 commented Aug 27, 2020

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

  1. cd /tmp
  2. git clone https://github.com/atc0005/go-teams-notify
  3. git checkout go-panic-repro
  4. docker run --rm -it -v `pwd`:`pwd` -w `pwd` golang:1.15.0 go test -v ./...
  5. 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

@davecheney
Copy link
Contributor

@davecheney davecheney commented Aug 27, 2020

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
Copy link
Author

@atc0005 atc0005 commented Aug 27, 2020

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?

@atc0005
Copy link
Author

@atc0005 atc0005 commented Aug 27, 2020

@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.

@atc0005 atc0005 closed this Aug 27, 2020
@davecheney
Copy link
Contributor

@davecheney davecheney commented Aug 27, 2020

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

atc0005 added a commit to atc0005/go-teams-notify that referenced this issue Aug 27, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.