Discovering Goroutine Leaks with Semgrep

While using the described pattern analysis can be an interesting tool to find bugs in other peoples software (what Trail of Bits are doing) or maybe for integrating it into a static code analysis tool like "go vet" to prevent the offending code even getting into production, discovering goroutine leaks in your own program once they happen is not that difficult in most cases from my experience. You just dump the stack of all goroutines via for example runtime.GoroutineProfile() or using pprof and it becomes obvious very quickly where goroutines are leaking. And this way you are not limited to concurrency bugs caused by communicating over channels.

I wish blog posts would pin links to commits on repos - and check their links.

404: https://github.com/trailofbits/semgrep-rules/blob/main/hangi... [1]

invalid URL: http://nd%20basic%20cross-function%20analysis/ [2]

[1]: https://github.com/trailofbits/semgrep-rules/blob/385c42a/go...

[2]: https://github.com/returntocorp/semgrep/pull/2913 from https://github.com/returntocorp/semgrep/issues/2787

The leak is from the unclosed channel. In the particular example in the article, the leaked goroutine is attempting to write to the unclosed channel which is a good example for using the `context` package. [0]

But, if the leaked goroutine was blocking while attempting to read from an unclosed channel, then you could simply `close(channel)` and this would cause the goroutine to return. [1]

[0] https://play.golang.org/p/FDfZZfdXgMC [1] https://play.golang.org/p/cr7JzIdRDjR

EDIT: I am making this comment to emphasize that goroutines pretty much only leak because of unclosed channels. So, it might be a little difficult to justify manual (or even automated) static analysis for leaked goroutines unless you have channel spaghetti. But, channel spaghetti is a sign of faulty design.

Why is there a difference of goroutine leak behavior between channels with or without a length?

It sort of works. If you refactor their example to use contexts, the rule misses the leak:

    func requestFromSlowServer(ctx context.Context) string {
        select {
        case <-ctx.Done():
            return ctx.Err().Error()
        case <-time.After(time.Second):
        }
        return "very important data"
     }

     func requestData(timeout time.Duration) string { 
         dataChan := make(chan string)
    
         go func() {
             ctx := context.Background()
             newData := requestFromSlowServer(ctx)
             select {
             case <-ctx.Done():
             case dataChan <- newData:
             }
        }()
        select {
        case result := <- dataChan:
            fmt.Printf("[+] request returned: %s", result)
            return result
        case <- time.After(timeout):
            // Hitting this case doesn't cancel the context, so you're still leaking goroutines.
            fmt.Println("[!] request timeout!")
            return ""
        }
    }

You can try this example in their playground (https://semgrep.dev/s/Q8Bo/) and note that it basically does the exact same thing as before (context.Background() never gates the channel sends or receives, but it does convince the linter that you aren't leaking). I got here by using context.WithTimeout(...) first, which was correctly marked as not leaking. But it's faked out by any gating, not by actually useful gating. So it just pushes the problem to more subtle cases that are even easier for code reviewers to miss. "foo <- bar" is always something a code reviewer is going to worry about, but creating a context ten levels up and still leaking goroutines is much more subtle and would be just the thing that static analysis could make the reviewer aware of. (Basically saying, "hey, you did this right, but there are some implementation details that prevent it from working".)

Faking out the linter easily is pretty scary. I ran into a big memory leak with Sentry's client library where they weren't closing http.Response.Body. They had a lint rule to make sure they were closing the body, but it didn't fire because they managed to fake out the linter (by passing the response to a helper function, which didn't close the body).

Overall I think this is too simplistic to benefit from unless you have found a particular pattern used by a particular author in a particular codebase, and want to get a list of places to fix. Certainly, a lot of new programmers to Go forget that channel writes block, and don't gate the write with a timeout condition. But fixing this is kind of the second thing you learn in Go ;)

I thought "do I need to integrate semgrep with my CI", and was underwhelmed by the default Go ruleset: https://semgrep.dev/p/golang. Making sure that people don't connect to SSH without verifying the server's host key is not in my top 15 lint rules (no complaints about including that, but it's very situational), but that's the sort of thing they provide out of the box. This particular rule doesn't even make it into their default list of rules.