Go-tools: staticcheck: detect pointer access after non-terminating nil checks

[ainar-g]

I've had code like this:

var foo = foos.Get(fooID)
if foo == nil {
    log.Printf("skipping foo with id %d: not found", fooID)
    // Whoops! Forgot a return.
}

// …

// Boom! foo was nil this time.
var bar = foo.Bars[barID]

I think that when a pointer value is checked against nil, no terminating action like panic, continue, or return is performed, and then the pointer is dereferenced, that probably means that either the developer has forgotten to terminate execution or the nil check here is useless (e.g. was moved up-stack or the pointer is now guaranteed to not be nil).

[dominikh]

This is a valid idea, and something that's been on my mind for a long time. It is essentially one of the kinds of bugs we'll catch once we've worked on #339 or #373 or something similar that enables us to do ~path-sensitive analyses~ backwards dataflow.

[dominikh]

I've finally found the time to get a mostly working implementation of #339, so here are three examples and how it'd help with this and similar checks.

(I'm mostly writing this for myself as a reference and for people following my SSI efforts.)

Example 1:

Input Go:

func fn() {
    x := gen()
    if x == nil {
        println("skipping")
    }
    println(*x)
}

SSI:

func fn():
0:                                                                entry P:0 S:2
        t1 = nil:*int                                                      *int
        t2 = "skipping":string                                           string
        t3 = call gen()                                                    *int
        t4 = t3 == nil:*int                                                bool
        if t4 goto 2 else 3
        t6 = σ(2: t2)                                                   string
1:                                                                 exit P:1 S:0
        return
2:                                                              if.then P:1 S:1
        t8 = call println(t6)                                                ()
        jump 3
3:                                                              if.done P:2 S:1
        t10 = Load <int> t3                                                 int
        t11 = call println(t10)                                              ()
        jump 1

At t3 = call gen(), we know nothing about t3, which we'll denote with unknown.

Upon seeing t4 = t3 == nil:*int we can mark t3 as being maybe nil, because it's being checked for nil. The body of the if branch isn't of much interest, because it doesn't use x nor tells us anything about it; though we would know that in the branch, x is definitely nil, if x were being used.

After the branch, we try to dereference x (t10 = Load <int> t3) and here we can emit a warning, that x may be nil. If x were never checked against nil, t3 would still be unknown and we wouldn't flag it (this is Go, we assume that variables won't be nil).

Example 2

In contrast, consider the fixed code:

Input Go:

func fn() {
    x := gen()
    if x == nil {
        println("skipping")
        return
    }
    println(*x)
}

SSI:

func fn():
0:                                                                entry P:0 S:2
        t1 = nil:*int                                                      *int
        t2 = "skipping":string                                           string
        t3 = call gen()                                                    *int
        t4 = t3 == nil:*int                                                bool
        if t4 goto 2 else 3
        t6 = σ(2: t2)                                                   string
        t7 = σ(3: t3)                                                     *int
        t8 = σ(3: t12)                                                     int
1:                                                                 exit P:2 S:0
        return
2:                                                              if.then P:1 S:1
        t10 = call println(t6)                                               ()
        jump 1
3:                                                              if.done P:1 S:1
        t12 = Load <int> t7                                                 int
        t13 = call println(t8)                                               ()
        jump 1

Again, t3 = call gen() starts out as unknown, and t4 = t3 == nil:*int marks t3 as maybe nil. Additionally, it marks t7 as not nil. t7 is the sigma node created for t3, which is used instead of t3 when taking the control flow rooted at the else branch. Thus, when we dereference x, we'll load from t7, which is known not nil, and no warning is emitted.

Example 3

For completeness sake, consider this program, which is also correct:

Input Go:

func fn() {
    x := gen()
    if x == nil {
        println("skipping")
    }
    if x == nil {
        return
    }
    println(*x)
}

SSI:

func fn():
0:                                                                entry P:0 S:2
        t1 = nil:*int                                                      *int
        t2 = "skipping":string                                           string
        t3 = call gen()                                                    *int
        t4 = t3 == nil:*int                                                bool
        if t4 goto 2 else 3
        t6 = σ(2: t2)                                                   string
1:                                                                 exit P:2 S:0
        return
2:                                                              if.then P:1 S:1
        t8 = call println(t6)                                                ()
        jump 3
3:                                                              if.done P:2 S:2
        t10 = t3 == nil:*int                                               bool
        if t10 goto 1 else 4
        t12 = σ(4: t3)                                                    *int
        t13 = σ(4: t14)                                                    int
4:                                                              if.done P:1 S:1
        t14 = Load <int> t12                                                int
        t15 = call println(t13)                                              ()
        jump 1

t3 starts out as unknown, t4 = t3 == nil:*int marks it as maybe nil. In block 3, we check t3 for nil again, this time creating a sigma node for the not nil branch, so when we dereference x (t12), we again know that it is not nil.

Of course we could've implemented this check without SSI, too. The compiler eliminates unnecessary nil checks, and there exists a vet check similar to this. These are implemented on top of SSA and carry around additional information in extra data structures, often some approach based on stacks and the dominator tree. SSI will provide us with a more generic framework for doing both forwards and backwards data flow analyses, in a pretty intuitive way. Hopefully it will provide the foundation for a number of new checks.

[dominikh]

This is now SA5011.

[ainar-g]

@dominikh That's weird. I've just updated to v0.0.0-20191021081315-c62cfd4e319f, but the code that used to cause troubles is still unreported. --explain shows that the check is there, and I launch it with --checks='*'.

[dominikh]

@ainar-g can you show me the exact piece of code that isn't being reported? It works for the example you showed in your original issue report.

Do note that the check, as all our checks, errs on the side of false negatives, not false positives. There will be possible nil pointer dereferences that won't be caught.

[ainar-g]

@dominikh I've managed to minimise the repro to this:

type device struct{ settings interface{} }

func findDevice() (d *device, err error) { return nil, nil }

func otherAction() (err error) { return nil }

func f() (err error) {
    var d *device
    d, err = findDevice()
    if err != nil {
        return fmt.Errorf("error getting device: %w", err)
    }

    if d == nil {
        log.Printf("no device")
        // N.B.  Missing return.
    }

    err = otherAction()
    if err != nil {
        return fmt.Errorf("other action for device: %w", err)
    }

    _ = d.settings

    return nil
}

Notice the otherAction call. Without it, staticcheck detects the nil pointer.

[dominikh]

Unfortunately that's just one of the false negatives that the check has to live with. Concretely, information that we deduce for a variable (in SSI terms) only holds for its branch of execution. At branch points, we have to discard the information. Your err check after the call to otherAction introduces new branches of execution (the true branch and the implicit else branch). That means that the d in d.settings is separate from the d in d == nil and we don't assume that the d in d.settings can be nil.

If we propagated information across branch points, we'd be prone to false positives because sometimes the (not) nilness of a value is correlated with another variable, so we must assume that _any_ conditional check might affect whether a value can be nil or not. This often takes the form of if function A returns value B, then that implies that C couldn't have been nil.

Long story short is that we're doing a path-insensitive analysis, meaning our information has to hold for all branches of execution at once, and thus we err on the side of false negatives. You can read more about the specifics of the limitations at https://github.com/dominikh/go-tools/blob/c62cfd4e319fb4b628edfef62ca6818a4e24e542/staticcheck/lint.go#L3610

[ainar-g]

I see. That's very unfortunate, since most such bugs will probably occur in situations where the possible nil dereference is at least a couple of paragraphs away from the check.

Should I open a new issue about that with a milestone of “Some day” or are there zero chances that this is ever done?

[dominikh]

I see. That's very unfortunate, since most such bugs will probably occur in situations where the possible nil dereference is at least a couple of paragraphs away from the check.

I wouldn't say _most_. This check has already discovered a fair number of bugs in existing code bases. Yes, the check will also miss a fair bunch, but such is the nature of staticcheck.

Should I open a new issue about that with a milestone of “Some day” or are there zero chances that this is ever done?

I don't think it'd be helpful to have "this could find more bugs" issues, because that'd equal to roughly one issue per analysis we have. Whenever I incorporate new methods that allow me to improve checks, I improve them.

Though in the specific case of flagging potential nil pointer dereferences, I'm not sure there's anything feasible that would improve the rate of true positives without also increasing the rate of false positives.

[kozlovic]

@dominikh staticcheck will now report possible nil pointer dereference in tests where you have a t.FatalX() (which stops its execution by calling runtime.Goexit - which then runs all deferred calls in the current goroutine).

if msg == nil {
    t.Fatal("Message is nil")
}
if msg.Data ... <- this will be reported as a SA5011

Is there anything that can be done or should I go ahead and put returns in some of the tests where I have similar conditions?

[dominikh]

@kozlovic we explicitly handle things like t.Fatal and recognize that it stops control flow, so this shouldn't happen. Do you have a self-contained, reproducible example where this happens, including your invocation of staticcheck?

[kozlovic]

@dominikh Let me know if I should create a separate issue instead. I tried but since I need to provide staticcheck -version and it prints:

$ staticcheck -version
staticcheck (no version)
$ staticcheck -debug.version
staticcheck (no version)

Compiled with Go version: go1.12.9
Built without Go modules

I suspect that I am doing something wrong? I build without go module, but when I try with go module I get:

/Users/ivan/dev/go/src/honnef.co/go/tools
$ GO111MODULE=on go install
can't load package: package honnef.co/go/tools: unknown import path "honnef.co/go/tools": cannot find module providing package honnef.co/go/tools

The repo I am running this from is:
https://github.com/nats-io/nats-streaming-server/blob/master/.travis.yml#L28
And here is the result:
https://travis-ci.org/nats-io/nats-streaming-server/jobs/600663885

Notice also the line:
https://travis-ci.org/nats-io/nats-streaming-server/jobs/600663885#L310

Locally, I have added returns to prevent the explicit reports but still get that line:

-: possible nil pointer dereference (SA5011)

[lupine]

@dominikh I'm experiencing this while running current master staticcheck against https://gitlab.com/gitlab-org/gitlab-workhorse/blob/master/cmd/gitlab-zip-cat/main.go#L62 , regarding the file variable.

staticcheck ./cmd/gitlab-zip-cat/
-: possible nil pointer dereference (SA5011)

This patch (against that file) matches the problem go away:

diff --git a/cmd/gitlab-zip-cat/main.go b/cmd/gitlab-zip-cat/main.go
index 52e5fc5..cdf0ff4 100644
--- a/cmd/gitlab-zip-cat/main.go
+++ b/cmd/gitlab-zip-cat/main.go
@@ -60,6 +60,7 @@ func main() {
        file := findFileInZip(fileName, archive)
        if file == nil {
                notFoundError(fmt.Errorf("find %q in %q: not found", fileName, scrubbedArchivePath))
+               os.Exit(1)
        }
        // Start decompressing the file
        reader, err := file.Open()

notFoundError(...) is a local function that unconditionally calls os.Exit(), so this seems like an unambiguous false positive.

I appreciate it's not the most minimal of repros!

A couple of other notes -

The error message is painful. I had to resort to commenting out chunks of the file to work out where the failure was.

Using //lint:disable or //line:disable-file both fail to make the error go away.

Let me know if I can be of further assistance here.

[dominikh]

@kozlovic you aren't calling (*testing.T).Fatal, but stackFatalf, which itself calls tLogger.Fatalf, where tLogger is an interface. We can't look through that interface and thus have no idea that stackFatalf calls runtime.Goexit. You can work around that by adding a panic("unreachable") at the end of stackFatalf (or a runtime.Goexit(), whichever you think is more obvious to readers of your code base). Do note that you have more than one helper called stackFatalf.

I'll have to look into why we're reporting an issue for -, but it does go away once you adjust stackFatalf, so it's probably not as critical an issue.

as for staticcheck -version and staticcheck -debug.version: when not building from a release branch, -version correctly reports the lack of a version. -debug.version only contains additional information when building in module mode, in which case it tells us about the exact revision used, as well as dependencies. It's fine for either or both information to be missing.

As for installing in module mode: you need to install honnef.co/go/tools/cmd/staticcheck, not honnef.co/go/tools

@lupine I can reproduce the problem, but haven't yet figured out why it happens. I'll keep you updated. Note that the error _isn't supposed to_ be without position information. But also note that neither //lint:disable nor //line:disable-file (nor /lint:disable-file) are instructions understood by the staticcheck runner.

[kozlovic]

@dominikh As I said, I had put a return after those reported SA0511 (following stackfatal) or even started to replace with t.TestingTB (but that is an interface too), and I still get the single - report even after fixing them all (by adding returns). Will try again. Thanks!

[kozlovic]

FYI: The runtime.Goexit() trick does not work for me, has to be "panic"...

[kozlovic]

@dominikh Since with the panic() code in those stackfatalf the - error disappears, there is indeed no urgency on my end. Thanks for the prompt reply!

[lupine]

Sorry, I meant lint:file-ignore and lint:ignore, per http://staticcheck.io/docs/#line-based-linter-directives. I guess that's just another sympton of the lack of position info, though.

[dominikh]

@kozlovic runtime.Goexit should work, but you may have run into another bug that was fixed in d69af9c0, which might've made the behavior of SA5011 flaky.

@lupine please try with master; d69af9c0 and cf5cff5d fix the false positive, as well one instance of reporting -.

[lupine]

Thanks @dominikh , that fixes my false positive!