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
Comments
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. |
@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. |
The rules in cmd/vet/readme are:
Correctness and precision are covered. Only frequency remains to be demonstrated. |
Paging Mr. @campoy for BigQuery regexps. |
Frequency could be covered by generalizing the check: instead of looking for 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. |
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. |
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 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 |
Yeah, that seems like a lot. |
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. |
Proposal accepted. If somebody wants to do the work. |
I'd be happy to write it, it doesn't seem too hard taking into account we're only writing for |
|
STGM, I'll send a CL later today (unless anyone wants to take care of this) |
Please add me as a reviewer. (Rob is primary, of course.) |
CL https://golang.org/cl/32911 mentions this issue. |
@ahmetalpbalkan, FYI, I know that
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:
|
On 13 November 2016 at 18:43, Dmitri Shuralyov notifications@github.com
|
This is a simple newbie mistake I saw in the Go code I wrote many years ago:
This will panic when
resp == nil
which is ifferr != nil
. Cango vet
have a heuristic for whenresp.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).
The text was updated successfully, but these errors were encountered: