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

Fix panic on request failure #20

Merged
merged 2 commits into from
Jul 14, 2019
Merged

Fix panic on request failure #20

merged 2 commits into from
Jul 14, 2019

Conversation

pgaskin
Copy link
Contributor

@pgaskin pgaskin commented Jul 13, 2019

Description

This PR fixes a panic when http.Client.Do returns an error. This issue is caused by trying to close the request body with defer before checking for an error (see golang/go#17780).

I noticed this issue when running dnscontrol (PR StackExchange/dnscontrol#529) on my CI server, and I forgot to install ca-certificates in the image (resulting in the error: Get https://api.vultr.com/v1/account/info: x509: certificate signed by unknown authority).

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x40 pc=0xd277ae]
goroutine 1 [running]:
github.com/StackExchange/dnscontrol/vendor/github.com/vultr/govultr.(*Client).DoWithContext(0xc0006e4680, 0x11d8e60, 0xc0000b4030, 0xc0001c6800, 0xdd0d00, 0xc000295480, 0x0, 0x0)
/home/patrick/go/src/github.com/StackExchange/dnscontrol/vendor/github.com/vultr/govultr/govultr.go:169 +0x12e
github.com/StackExchange/dnscontrol/vendor/github.com/vultr/govultr.(*AccountServiceHandler).GetInfo(0xc0000b8938, 0x11d8e60, 0xc0000b4030, 0xc0006e4680, 0xc0006ef4c8, 0xe9e820)
/home/patrick/go/src/github.com/StackExchange/dnscontrol/vendor/github.com/vultr/govultr/account.go:38 +0xfc
github.com/StackExchange/dnscontrol/providers/vultr.NewProvider(0xc0006d5440, 0x0, 0x0, 0x0, 0xc0000f53d8, 0x1, 0xc0006e6750, 0xeefcc0)
/home/patrick/go/src/github.com/StackExchange/dnscontrol/providers/vultr/vultrProvider.go:64 +0xdc
github.com/StackExchange/dnscontrol/providers.CreateDNSProvider(0xc00003dc35, 0x5, 0xc0006d5440, 0x0, 0x0, 0x0, 0x0, 0x6, 0x63a2e0e200000000, 0xc000627358)
/home/patrick/go/src/github.com/StackExchange/dnscontrol/providers/providers.go:72 +0x92
github.com/StackExchange/dnscontrol/commands.InitializeProviders(0x10076f8, 0xa, 0xc0006c5020, 0x0, 0x11cf480, 0xc000241e20, 0x0, 0x0)
/home/patrick/go/src/github.com/StackExchange/dnscontrol/commands/previewPush.go:219 +0x6c0
github.com/StackExchange/dnscontrol/commands.run(0x100943b, 0xc, 0x0, 0x0, 0x0, 0x0, 0x0, 0x10076f8, 0xa, 0x0, ...)
/home/patrick/go/src/github.com/StackExchange/dnscontrol/commands/previewPush.go:107 +0xf7
github.com/StackExchange/dnscontrol/commands.Preview(...)
/home/patrick/go/src/github.com/StackExchange/dnscontrol/commands/previewPush.go:87
github.com/StackExchange/dnscontrol/commands.glob..func4.1(0xc0000e71e0, 0xc0001a2f00, 0xc0000e71e0)
/home/patrick/go/src/github.com/StackExchange/dnscontrol/commands/previewPush.go:25 +0x97
github.com/StackExchange/dnscontrol/vendor/github.com/urfave/cli.HandleAction(0xe219a0, 0xc0001645d0, 0xc0000e71e0, 0x0, 0xc0001a2f60)
/home/patrick/go/src/github.com/StackExchange/dnscontrol/vendor/github.com/urfave/cli/app.go:501 +0xc8
github.com/StackExchange/dnscontrol/vendor/github.com/urfave/cli.Command.Run(0x1004b97, 0x7, 0x0, 0x0, 0x0, 0x0, 0x0, 0x10334e9, 0x4e, 0x0, ...)
/home/patrick/go/src/github.com/StackExchange/dnscontrol/vendor/github.com/urfave/cli/command.go:165 +0x487
github.com/StackExchange/dnscontrol/vendor/github.com/urfave/cli.(*App).Run(0xc0001728c0, 0xc0000aa020, 0x2, 0x2, 0x0, 0x0)
/home/patrick/go/src/github.com/StackExchange/dnscontrol/vendor/github.com/urfave/cli/app.go:259 +0x6e3
github.com/StackExchange/dnscontrol/commands.Run(0xc000096600, 0x55, 0xc000096058)
/home/patrick/go/src/github.com/StackExchange/dnscontrol/commands/commands.go:58 +0x25d
main.main()
/home/patrick/go/src/github.com/StackExchange/dnscontrol/main.go:18 +0x41 

Checklist:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you linted your code locally prior to submission?
  • Have you successfully ran tests with your changes locally?

@codecov
Copy link

codecov bot commented Jul 13, 2019

Codecov Report

Merging #20 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage    71.7%   71.79%   +0.08%     
==========================================
  Files          22       22              
  Lines        2269     2269              
==========================================
+ Hits         1627     1629       +2     
+ Misses        323      322       -1     
+ Partials      319      318       -1
Impacted Files Coverage Δ
govultr.go 88.23% <100%> (+2.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97c6919...b383c31. Read the comment docs.

@ddymko ddymko self-requested a review July 14, 2019 13:27
Copy link
Contributor

@ddymko ddymko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geek1011

Thanks for submitting this bug fix! We really appreciate the PR & fix....we had just noticed this issue as well and we were going to patch it but you beat us to it!

@ddymko ddymko added the bug Something isn't working label Jul 14, 2019
@ddymko ddymko merged commit 4cfc19d into vultr:master Jul 14, 2019
@ddymko ddymko mentioned this pull request Jul 14, 2019
3 tasks
ddymko added a commit to mamclaughlin/govultr that referenced this pull request Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants