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

[ahmetb]

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]

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.

[josharian]

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

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]

Paging Mr. @campoy for BigQuery regexps.

[alandonovan]

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]

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]

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]

Yeah, that seems like a lot.

[rsc]

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]

Proposal accepted. If somebody wants to do the work.

[campoy]

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]

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

[campoy]

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

[josharian]

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

[gopherbot]

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

[dmitshur]

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]

On 13 November 2016 at 18:43, Dmitri Shuralyov [email protected]
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.