Detect defer calls skipped by os.Exit

[fionera]

The following code is not executing its defer function as log.Fatal calls syscall.Exit. This can prevent cleanup functions to run and could be unexpected to some developers.

package main

import (
    "log"
)

func main() {
    defer func() {
        println("no worries I will help you, too bad this doesn't execute")
    }()

    log.Fatal("oh no I slipped")
}

[arp242]

This has definitely taken me by surprise on a few occasions; the question is: how often is it not a problem that the defer doesn't run? I fear such a check might be very noisy.

Below is a quick (incomplete) patch; I ran it on my set of test code and it didn't flag anything, but that's mostly libraries rather than applications, which shouldn't call os.Exit() or log.Fatal() in the first place. Also not 100% sure my patch below will actually catch all cases.

Someone should probably run this on a whole bunch of package main applications and see what comes rolling out.

Patch

commit e87a51b6 Author: Martin Tournoij Date: Sun May 26 23:04:30 2024 +0100 SA9010: check for functions with defer and calls to os.Exit() diff --git a/staticcheck/analysis.go b/staticcheck/analysis.go index cce61975..b682a3aa 100644 --- a/staticcheck/analysis.go +++ b/staticcheck/analysis.go @@ -97,6 +97,7 @@ import ( "honnef.co/go/tools/staticcheck/sa9006" "honnef.co/go/tools/staticcheck/sa9007" "honnef.co/go/tools/staticcheck/sa9008" + "honnef.co/go/tools/staticcheck/sa9010" ) var Analyzers = []*lint.Analyzer{ @@ -193,4 +194,5 @@ var Analyzers = []*lint.Analyzer{ sa9006.SCAnalyzer, sa9007.SCAnalyzer, sa9008.SCAnalyzer, + sa9010.SCAnalyzer, } diff --git a/staticcheck/sa9010/sa9010.go b/staticcheck/sa9010/sa9010.go new file mode 100644 index 00000000..e5b381b3 --- /dev/null +++ b/staticcheck/sa9010/sa9010.go @@ -0,0 +1,93 @@ +package sa9010 + +import ( + "fmt" + "go/types" + + "golang.org/x/tools/go/analysis" + "honnef.co/go/tools/analysis/lint" + "honnef.co/go/tools/analysis/report" + "honnef.co/go/tools/go/ir" + "honnef.co/go/tools/go/ir/irutil" + "honnef.co/go/tools/internal/passes/buildir" +) + +var SCAnalyzer = lint.InitializeAnalyzer(&lint.Analyzer{ + Analyzer: &analysis.Analyzer{ + Name: "SA9010", + Run: run, + Requires: []*analysis.Analyzer{buildir.Analyzer}, + }, + Doc: &lint.Documentation{ + Title: ``, + Since: "Unreleased", + Severity: lint.SeverityWarning, + MergeIf: lint.MergeIfAny, + }, +}) + +var Analyzer = SCAnalyzer.Analyzer + +func run(pass *analysis.Pass) (interface{}, error) { + for _, fn := range pass.ResultOf[buildir.Analyzer].(*buildir.IR).SrcFuncs { + for _, block := range fn.Blocks { + instrs := irutil.FilterDebug(block.Instrs) + if len(instrs) < 2 { + continue + } + var hasDefer, hasExit bool + for _, ins := range instrs[:len(instrs)-1] { + switch v := ins.(type) { + case (*ir.Call): + hasExit = hasExit || irutil.IsCallToAny(v.Common(), "os.Exit", "log.Fatal", "log.Fatalf", "log.Fatalln") + case (*ir.Defer): + hasDefer = true + } + } + // TODO: should only trigger if defer is before exit. + if hasDefer && hasExit { + //n := fn.Pkg.Pkg.Path() + "." + fn.Name() + n := fn.Name() + report.Report(pass, fn, fmt.Sprintf("function %q has defer and exit", n)) + } + + // nins, ok := instrs[i+1].(*ir.Defer) + // if !ok { + // continue + // } + // if !irutil.IsCallToAny(&nins.Call, "(*sync.Mutex).Lock", "(*sync.RWMutex).RLock") { + // continue + // } + // if call.Common().Args[0] != nins.Call.Args[0] { + // continue + // } + // name := shortCallName(call.Common()) + // alt := "" + // switch name { + // case "Lock": + // alt = "Unlock" + // case "RLock": + // alt = "RUnlock" + // } + //report.Report(pass, nins, fmt.Sprintf("defer and exit", name, alt)) + } + } + return nil, nil +} + +func shortCallName(call *ir.CallCommon) string { + if call.IsInvoke() { + return "" + } + switch v := call.Value.(type) { + case *ir.Function: + fn, ok := v.Object().(*types.Func) + if !ok { + return "" + } + return fn.Name() + case *ir.Builtin: + return v.Name() + } + return "" +} diff --git a/staticcheck/sa9010/sa9010_test.go b/staticcheck/sa9010/sa9010_test.go new file mode 100644 index 00000000..6fdd9661 --- /dev/null +++ b/staticcheck/sa9010/sa9010_test.go @@ -0,0 +1,13 @@ +// Code generated by generate.go. DO NOT EDIT. + +package sa9010 + +import ( + "testing" + + "honnef.co/go/tools/analysis/lint/testutil" +) + +func TestTestdata(t *testing.T) { + testutil.Run(t, SCAnalyzer) +} diff --git a/staticcheck/sa9010/testdata/src/example.com/CheckDeferAndExit/a.go b/staticcheck/sa9010/testdata/src/example.com/CheckDeferAndExit/a.go new file mode 100644 index 00000000..e4a8178a --- /dev/null +++ b/staticcheck/sa9010/testdata/src/example.com/CheckDeferAndExit/a.go @@ -0,0 +1,29 @@ +package x + +import ( + "fmt" + "log" + "os" +) + +func x() { //@diag(`defer and exit`) + defer y() + fmt.Println() + os.Exit(1) +} + +func y() { + fmt.Println() + os.Exit(1) +} + +func z() { + defer y() + fmt.Println() +} + +func aa() { //@diag(`defer and exit`) + defer y() + fmt.Println() + log.Fatal("") +}

[arp242]

Actually I ran it wrong >_<; it does find three cases:

• https://github.com/anacrolix/missinggo/blob/master/cmd/http-file-server/main.go#L48-L51

  Doesn't matter as the listener is automatically closed on exit anyway.

• https://github.com/performancecopilot/speed/blob/main/examples/http_counter/server.go#L33-L44

  Not entirely sure what that MustStop does; might be a bug here.

• https://github.com/census-instrumentation/opencensus-go/blob/master/examples/http/helloworld_server/main.go#L54

  Might be a bug.

IMO it's right to flag all three of these.

Still needs more testing on applications though.

[dominikh]

I don't think we can flag this particular one as a definite mistake. The defer may be used solely to handle panics before an eventual, intended call to os.Exit. For example a CLI command function that intends to exit when it is done, but still wants to close files it opened in case the function panics before it can exit.

We might consider this in the context of #1102.

[fionera]

Could this may be added as check that is disabled by default? I will try to run it against our codebase (https://github.com/monogon-dev/monogon/) next week and report if it found some actual things.

[dominikh]

We only have non-default checks for stylistic checks because they're a matter of taste and a large number of people wanted them, coming from golint. I don't want to add checks that are non-default because they have significant false positives. People are prone to blindly enabling all checks, and while a debatable stylistic warning is harmless, incorrectly pointing out a bug is not.

[fionera]

I tested your patch in our codebase and noticed that it doesn't complain at all since it doesn't build a full graph of the execution. I know it's not possible to do completely since interface types exist but at least for "normal" functions it would be nice to have