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: flag simple nil pointer dereferences for struct field usage and nil map assignments #39462

Open
odeke-em opened this issue Jun 8, 2020 · 5 comments
Milestone

Comments

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jun 8, 2020

Coming here from an experience report narrated to me offline by @kastiglione and @indragiek.

The code below is biting yet can be trivially flagged by cmd/vet

Usage of field in an obvious nil struct

https://play.golang.org/p/i_a3kGE61UZ
724716BA-8467-4A68-8D9D-3F30840DC6FB

Assignment to a nil map

https://play.golang.org/p/KlN7AY1U4lo
5FA92B7F-78A6-4410-B9F1-8FDE1F551504

@robpike
Copy link
Contributor

@robpike robpike commented Jun 8, 2020

That's just a bug that will trigger the first time the program is run. There's nothing subtle about it, and I don't think vet should bother with it.

@odeke-em
Copy link
Member Author

@odeke-em odeke-em commented Jun 9, 2020

That's just a bug that will trigger the first time the program is run. There's nothing subtle about it, and I don't think vet should bother with it.

@robpike thanks for the response! While I made the code intentionally trivial to clearly illustrate the problem, I've encountered such failures in deeply nested code e.g in logic nested in HTTP handlers e.g.

func handleIt(w http.ResponseWriter, r *http.Request) {
     ....
     var hdr http.Header
     hdr.Set("cookie", "cookie-value")
     ...
     ...
     req.Header = hdr
     res, err := client.Do(req)
}

where url.Values' concrete type is map[string][]string. In a code review, it is hard for reviewers to easily flag where hdr.Set will panic or not, unless if hdr's type is clearly map[string][]string or if unless they did

req, err := http.NewRequest("POST", url, body)
...
req.Header.Set("cookie", "cookie-value")
req.Header....

Sorry to preach to you, the choir, but perhaps we could consider this to be an engineering productivity boost to catch subtle bugs in deeply nested code. I don't have concrete data of misuse in the wild and bug reports from the report patterns. However, perhaps @rsc or @sqs can help out with with scanning within SourceGraph's body of knowledge to show misuses and bugs that are waiting to happen, or reported bugs.

Searching on Google or Github for "panic: assignment to entry in nil map" or "invalid memory address or nil pointer dereference" yield way too many hits/noise for me to sift through, but a few I could pull up straight off the top of my head, in which panics happen later on in code and could trivially have been flagged:

Source Link
StackOverflow https://stackoverflow.com/questions/35379378/go-assignment-to-entry-in-nil-map?rq=1
gocookbook kevinyan815/gocookbook#7
Twitter plugin (uses url.Values and then .Set(“key”, “value”) not yet discovered! https://github.com/chrissexton/alepale/blob/5ba37511597357d45e77492a5d2c64f00b94ebb8/plugins/twitter.go#L124-L125
Stackoverflow https://stackoverflow.com/questions/50668817/runtime-error-panic-assignment-to-entry-in-nil-map

as well as from the private reports by @kastiglione and @indragiek tagged.

Considerations

a) It is trivial for us to statically analyze such cases and can prevent disasters later on, I myself have encountered these problems in production from bug reports I've received from other people's programs
b) we can consider this our preventative measure for flagging and mitigating a little on the nil dereference problem
c) variants of type aliases and type declarations impose cognitive loads on reviewers, yet vet can flag the offending patterns directly

Thank you!

@toothrot toothrot added this to the Backlog milestone Jun 12, 2020
@toothrot
Copy link
Contributor

@toothrot toothrot commented Jun 12, 2020

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Jun 12, 2020

a) It is trivial for us to statically analyze such cases and can prevent disasters later on

It may appear trivial, but it is in fact not at all simple. To prove that this program would crash, an analysis would need to inline two nested function calls (hdr.Set calls textproto.MIMEHeader.Set, which contains the actual map update operation). This requires interprocedural analysis with a context-sensitive model of the heap. There's tons of literature on how to do this, but it is computationally expensive, prone to inaccurate conclusions, and demands a level of analytical sophistication far beyond what we aim for in vet checks.

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Jun 12, 2020

Related: a seemingly simpler version of the problem is that of inferring which Go methods on *T may be safely called with a nil receiver, so that the existing nilness analysis could additionally report errors of the form "this call of x.f() has a nil receiver x and will panic". But even that inference is difficult: if we define a nil-unsafe function as one that will definitely panic if passed nil, then, this is a nil-unsafe function:

// f(x) does something. x must be non-nil.
func f(x *T) {
      ...
      *x = ...
      ...
  }

only if the first '...' doesn't contain a return statement. Consider this:

// f(x, y) does something. x must be non-nil; y must be even.
func f(x *T, y int) error {
      if odd(y) { return fmt.Errorf("y is not even"); }
      ...
      *x = ...
      ...
 }

Now this function should still never be passed a nil value for x, but the check for y makes the function nil-safe for x, according to our definition---though only for bad values of y.

It turns out that a great many Go functions resemble this one.

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
4 participants
You can’t perform that action at this time.