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

cmd/vet: detect "defer resp.Body.Close()"s that will panic #17780

Closed
ahmetb opened this issue Nov 3, 2016 · 17 comments

Comments

Projects
None yet
9 participants
@ahmetb
Copy link

commented Nov 3, 2016

This is a simple newbie mistake I saw in the Go code I wrote many years ago:

resp, err := http.Get(...)
defer resp.Body.Close()
if err != nil {
    return err
}
// read resp.Body or something

This will panic when resp == nil which is iff err != nil. Can go vet have a heuristic for when resp.Body.Close() is invoked in a path where neither of the following are checked:

  • resp != nil
  • err != nil

I don't know the specifics of how cmd/vet works but I was a little surprised when no static analysis tools caught this one so far (obviously points to lack of testing on my end as well, however I feel like static analysis could've found it earlier).

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 3, 2016

Correcting one point: there is one case where http.Get returns a non-nil *Response and non-nil error, but in that case the resp.Body does not need to be closed, so your overall point is still valid.

I feel like this could be a vet rule. There are no false positives I can think of.

@robpike, @alandonovan? Not sure who sets the rules for vet.

@bradfitz bradfitz added the Proposal label Nov 3, 2016

@bradfitz bradfitz changed the title cmd/vet: can it detect "defer resp.Body.Close()"s that will panic? proposal: cmd/vet: can it detect "defer resp.Body.Close()"s that will panic? Nov 3, 2016

@josharian

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

@robpike is the decider.

Seems like a reasonable rule, at least when limited specifically to net/http: No false positives, easy to get wrong, might not be caught during testing because most http requests don't fail, blows up in a bad way when something does go wrong.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

The rules in cmd/vet/readme are:

  1. correctness
  2. frequency
  3. precision

Correctness and precision are covered. Only frequency remains to be demonstrated.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 4, 2016

Paging Mr. @campoy for BigQuery regexps.

@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

Frequency could be covered by generalizing the check: instead of looking for resp.Body specifically, report all guaranteed nil dereferences, and all dead code caused by if x == nil conditions in which x is known to be always nil (or always non-nil). You'd also need to tell it that some functions like http.Get return zero when err != nil, thought it could figure this out for most functions in the same package.

I ran this check across Google's Go code and it found a lot of errors, some quite hard to spot. That implementation used golang.org/x/tools/go/ssa, which needs well-typed input, but it would not be hard to make it build sound SSA output from unsound source input, inserting special "fatal" operators as needed to smooth over the gaps. I'm not sure cmd/vet wants to bundle a copy of x/tools/go/ssa, though.

@campoy

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

Oh, man. @bradfitz likes to ask for complicated analysis!

To be able to answer this question (the general question, not just http.Get) I would cross compile Go programs detecting the pattern (uising go/*) to javascript with gopherjs and using the result to detect rows on BigQuery.

I am pretty sure we discussed something similar to this with @broady.

@campoy

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

For the concrete pattern discussed above:

SELECT COUNT(*) as COUNT
FROM (
  SELECT REGEXP_EXTRACT(content,
       r'(?s)(res, err := http.Get\([^\n]*\)\n[^\n]*defer res.Body\..*)') as match
  FROM [campoy-github:go_files.contents]
  HAVING match is not null
)

28 errors found out of 1609 lines that had res, err := http.Get

SELECT COUNT(*) as COUNT
FROM (
  SELECT REGEXP_EXTRACT(content, r'(?s)(res, err := http.Get\([^\n]*\))') as match
  FROM [campoy-github:go_files.contents]
  HAVING match is not null
)

So ~1.75% of the lines were wrong, which I guess is significant. Would this result be higher if we had a more general algorithm? Who knows

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 4, 2016

Yeah, that seems like a lot.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

It sounds like there's a question of whether this for just http responses or something more general. If more general, I think we need to do a better job. If it's just http.Get and maybe a few other common cases, that seems fine.

@bradfitz bradfitz changed the title proposal: cmd/vet: can it detect "defer resp.Body.Close()"s that will panic? cmd/vet: detect "defer resp.Body.Close()"s that will panic Nov 7, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 7, 2016

Proposal accepted. If somebody wants to do the work.

@campoy

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

I'd be happy to write it, it doesn't seem too hard taking into account we're only writing for http.Get.
Should we also do it for http.Client.Get?
What about http.Client.Do?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 7, 2016

http.<Anyfunc> returning (*Response, error), and any *http.Client receiver returning (*Response, Error), and any *http.Transport or http.RoundTripper returning (*Response, error).

@campoy

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

STGM, I'll send a CL later today (unless anyone wants to take care of this)

@josharian

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

Please add me as a reviewer. (Rob is primary, of course.)

@gopherbot

This comment has been minimized.

Copy link

commented Nov 8, 2016

CL https://golang.org/cl/32911 mentions this issue.

@dmitshur

This comment has been minimized.

Copy link
Member

commented Nov 13, 2016

but I was a little surprised when no static analysis tools caught this one so far

@ahmetalpbalkan, FYI, I know that staticcheck does catch this issue (and similar ones, like with os.Open, etc.):

  • Don't defer rc.Close() before having checked the error returned by Open or similar.

For example, if I run it on the following program:

package main

import (
    "fmt"
    "io"
    "io/ioutil"
    "net/http"
)

func main() {
    resp, err := http.Get("https://www.example.com/")
    defer resp.Body.Close()
    if err != nil {
        fmt.Println(err)
        return
    }
    // read resp.Body or something
    io.Copy(ioutil.Discard, resp.Body)
}

The output is:

$ staticcheck 
main.go:12:2: should check returned error before deferring resp.Body.Close()
@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2016

On 13 November 2016 at 18:43, Dmitri Shuralyov notifications@github.com
wrote:

  • Don't defer rc.Close() before having checked the error returned by
    Open or similar.

That does seem like a sensible generalization of the bug pattern.

@golang golang locked and limited conversation to collaborators Nov 14, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.