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 <martin@arp242.net>
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.