From 3a5a051992e47e48812c661d5e19bf46d84ddcc7 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Tue, 1 Dec 2020 13:34:30 +0700 Subject: [PATCH] go/analysis/passes: add sigchanyzer Analyzer This change brings in a static analyzer that was created by Orijtech, Inc. to detect the use of unbuffered channels with signal.Notify, which is a bug that's documented but not enforced. We've found it even in official examples and in the main Go repository as per https://go-review.googlesource.com/c/go/+/274332. We've found that this bug is very common in lots of Go code, hence the reason why we are directly donating it, so that all Go developers running go test, using gopls can use it. Updates golang/go#9399 Change-Id: Ief6d7238dc80bc9fd5f11a585e41387704457276 Reviewed-on: https://go-review.googlesource.com/c/tools/+/274352 gopls-CI: kokoro TryBot-Result: Go Bot Trust: Rebecca Stambler Trust: Cuong Manh Le Trust: Emmanuel Odeke Run-TryBot: Rebecca Stambler Reviewed-by: Emmanuel Odeke --- go/analysis/passes/sigchanyzer/sigchanyzer.go | 129 ++++++++++++++++++ .../passes/sigchanyzer/sigchanyzer_test.go | 17 +++ .../passes/sigchanyzer/testdata/src/a/a.go | 38 ++++++ .../sigchanyzer/testdata/src/a/a.go.golden | 38 ++++++ 4 files changed, 222 insertions(+) create mode 100644 go/analysis/passes/sigchanyzer/sigchanyzer.go create mode 100644 go/analysis/passes/sigchanyzer/sigchanyzer_test.go create mode 100644 go/analysis/passes/sigchanyzer/testdata/src/a/a.go create mode 100644 go/analysis/passes/sigchanyzer/testdata/src/a/a.go.golden diff --git a/go/analysis/passes/sigchanyzer/sigchanyzer.go b/go/analysis/passes/sigchanyzer/sigchanyzer.go new file mode 100644 index 000000000..3d89061d1 --- /dev/null +++ b/go/analysis/passes/sigchanyzer/sigchanyzer.go @@ -0,0 +1,129 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package sigchanyzer defines an Analyzer that detects +// misuse of unbuffered signal as argument to signal.Notify. +package sigchanyzer + +import ( + "bytes" + "go/ast" + "go/format" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +const Doc = `check for unbuffered channel of os.Signal + +This checker reports call expression of the form signal.Notify(c <-chan os.Signal, sig ...os.Signal), +where c is an unbuffered channel, which can be at risk of missing the signal.` + +// Analyzer describes sigchanyzer analysis function detector. +var Analyzer = &analysis.Analyzer{ + Name: "sigchanyzer", + Doc: Doc, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{ + (*ast.CallExpr)(nil), + } + inspect.Preorder(nodeFilter, func(n ast.Node) { + call := n.(*ast.CallExpr) + if !isSignalNotify(pass.TypesInfo, call) { + return + } + var chanDecl *ast.CallExpr + switch arg := call.Args[0].(type) { + case *ast.Ident: + if decl, ok := findDecl(arg).(*ast.CallExpr); ok { + chanDecl = decl + } + case *ast.CallExpr: + chanDecl = arg + } + if chanDecl == nil || len(chanDecl.Args) != 1 { + return + } + chanDecl.Args = append(chanDecl.Args, &ast.BasicLit{ + Kind: token.INT, + Value: "1", + }) + var buf bytes.Buffer + if err := format.Node(&buf, token.NewFileSet(), chanDecl); err != nil { + return + } + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: "misuse of unbuffered os.Signal channel as argument to signal.Notify", + SuggestedFixes: []analysis.SuggestedFix{{ + Message: "Change to buffer channel", + TextEdits: []analysis.TextEdit{{ + Pos: chanDecl.Pos(), + End: chanDecl.End(), + NewText: buf.Bytes(), + }}, + }}, + }) + }) + return nil, nil +} + +func isSignalNotify(info *types.Info, call *ast.CallExpr) bool { + check := func(id *ast.Ident) bool { + obj := info.ObjectOf(id) + return obj.Name() == "Notify" && obj.Pkg().Path() == "os/signal" + } + switch fun := call.Fun.(type) { + case *ast.SelectorExpr: + return check(fun.Sel) + case *ast.Ident: + if fun, ok := findDecl(fun).(*ast.SelectorExpr); ok { + return check(fun.Sel) + } + return false + default: + return false + } +} + +func findDecl(arg *ast.Ident) ast.Node { + if arg.Obj == nil { + return nil + } + switch as := arg.Obj.Decl.(type) { + case *ast.AssignStmt: + if len(as.Lhs) != len(as.Rhs) { + return nil + } + for i, lhs := range as.Lhs { + lid, ok := lhs.(*ast.Ident) + if !ok { + continue + } + if lid.Obj == arg.Obj { + return as.Rhs[i] + } + } + case *ast.ValueSpec: + if len(as.Names) != len(as.Values) { + return nil + } + for i, name := range as.Names { + if name.Obj == arg.Obj { + return as.Values[i] + } + } + } + return nil +} diff --git a/go/analysis/passes/sigchanyzer/sigchanyzer_test.go b/go/analysis/passes/sigchanyzer/sigchanyzer_test.go new file mode 100644 index 000000000..50b3f4b7e --- /dev/null +++ b/go/analysis/passes/sigchanyzer/sigchanyzer_test.go @@ -0,0 +1,17 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package sigchanyzer_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/sigchanyzer" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + analysistest.RunWithSuggestedFixes(t, testdata, sigchanyzer.Analyzer, "a") +} diff --git a/go/analysis/passes/sigchanyzer/testdata/src/a/a.go b/go/analysis/passes/sigchanyzer/testdata/src/a/a.go new file mode 100644 index 000000000..277bf2054 --- /dev/null +++ b/go/analysis/passes/sigchanyzer/testdata/src/a/a.go @@ -0,0 +1,38 @@ +package p + +import ( + "os" + ao "os" + "os/signal" +) + +var c = make(chan os.Signal) +var d = make(chan os.Signal) + +func f() { + c := make(chan os.Signal, 1) + signal.Notify(c, os.Interrupt) // ok + _ = <-c +} + +func g() { + c := make(chan os.Signal) + signal.Notify(c, os.Interrupt) // want "misuse of unbuffered os.Signal channel as argument to signal.Notify" + _ = <-c +} + +func h() { + c := make(chan ao.Signal) + signal.Notify(c, os.Interrupt) // want "misuse of unbuffered os.Signal channel as argument to signal.Notify" + _ = <-c +} + +func i() { + signal.Notify(d, os.Interrupt) // want "misuse of unbuffered os.Signal channel as argument to signal.Notify" +} + +func j() { + c := make(chan os.Signal) + f := signal.Notify + f(c, os.Interrupt) // want "misuse of unbuffered os.Signal channel as argument to signal.Notify" +} diff --git a/go/analysis/passes/sigchanyzer/testdata/src/a/a.go.golden b/go/analysis/passes/sigchanyzer/testdata/src/a/a.go.golden new file mode 100644 index 000000000..e3702d72f --- /dev/null +++ b/go/analysis/passes/sigchanyzer/testdata/src/a/a.go.golden @@ -0,0 +1,38 @@ +package p + +import ( + "os" + ao "os" + "os/signal" +) + +var c = make(chan os.Signal) +var d = make(chan os.Signal, 1) + +func f() { + c := make(chan os.Signal, 1) + signal.Notify(c, os.Interrupt) // ok + _ = <-c +} + +func g() { + c := make(chan os.Signal, 1) + signal.Notify(c, os.Interrupt) // want "misuse of unbuffered os.Signal channel as argument to signal.Notify" + _ = <-c +} + +func h() { + c := make(chan ao.Signal, 1) + signal.Notify(c, os.Interrupt) // want "misuse of unbuffered os.Signal channel as argument to signal.Notify" + _ = <-c +} + +func i() { + signal.Notify(d, os.Interrupt) // want "misuse of unbuffered os.Signal channel as argument to signal.Notify" +} + +func j() { + c := make(chan os.Signal, 1) + f := signal.Notify + f(c, os.Interrupt) // want "misuse of unbuffered os.Signal channel as argument to signal.Notify" +}