Discovering Goroutine Leaks with Semgrep

https://www.trailofbits.com/post/discovering-goroutine-leaks-with-semgrep

by rbanffy • 5 years ago

81 10 5 years ago

eis• 5 years ago

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.

fuzzybear3965• 5 years ago

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

JoshMandel• 5 years ago

GH keyboard shortcut: When viewing a file in GitHub, type "y" to update the URL so it explicitly pins to a commit (e.g., turning https://github.com/trailofbits/semgrep-rules/blob/main/hangi... into https://github.com/trailofbits/semgrep-rules/blob/385c42a/go...).

This has become part of my muscle memory when copying links for posterity, but it makes sense not to be the default, because sometimes you want to share an evergreen link (i.e., always pointing to the latest content) rather than an immutable link.

trentdotexe• 5 years ago

Sorry about that. Should be fixed now.

jrockway• 5 years ago

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.

jidiculous• 5 years ago

Yeah, that default ruleset is quite sparse. It would be nice to see Go's Code Review Comments (https://github.com/golang/go/wiki/CodeReviewComments) added as a basic ruleset.

leetrout• 5 years ago

That is a really great idea. I love that wiki page. Some of them are harder to follow in a team that prefers "expressiveness". Quotes because I think it's easier to follow Go's prescription especially for always returning concrete types instead of interfaces.

return a concrete type and let the consumer mock the producer implementation

komuW• 5 years ago

I usually use Damian Gryski's: https://github.com/dgryski/semgrep-go

StandardFuture• 5 years ago

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.

ptspts• 5 years ago

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

Discovering Goroutine Leaks with Semgrep

Related Stories