Improve the G307 rule

* Add G307 sample code.
The sample should reflect a defered close that leads to data loss.
Due to IDE auto-complete people tend at least log errors, but not
really care about handling.

* Add more G307 sample code. Propose a way to implement

* Remove unused code. Add example that should not return an error but does

* Remove test for synced closed file for now.
Will add this later

Co-authored-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>
This commit is contained in:
Lars 2021-07-31 23:03:09 +02:00 committed by GitHub
parent 8b90c95c07
commit d4dc2d2df5
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 118 additions and 12 deletions

View file

@ -38,10 +38,11 @@ func contains(methods []string, method string) bool {
func (r *badDefer) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) { func (r *badDefer) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) {
if deferStmt, ok := n.(*ast.DeferStmt); ok { if deferStmt, ok := n.(*ast.DeferStmt); ok {
for _, deferTyp := range r.types { for _, deferTyp := range r.types {
if typ, method, err := gosec.GetCallInfo(deferStmt.Call, c); err == nil { if issue := r.checkChild(n, c, deferStmt.Call, deferTyp); issue != nil {
if normalize(typ) == deferTyp.typ && contains(deferTyp.methods, method) { return issue, nil
return gosec.NewIssue(c, n, r.ID(), fmt.Sprintf(r.What, method, typ), r.Severity, r.Confidence), nil
} }
if issue := r.checkFunction(n, c, deferStmt, deferTyp); issue != nil {
return issue, nil
} }
} }
} }
@ -49,6 +50,42 @@ func (r *badDefer) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) {
return nil, nil return nil, nil
} }
func (r *badDefer) checkChild(n ast.Node, c *gosec.Context, callExp *ast.CallExpr, deferTyp deferType) *gosec.Issue {
if typ, method, err := gosec.GetCallInfo(callExp, c); err == nil {
if normalize(typ) == deferTyp.typ && contains(deferTyp.methods, method) {
return gosec.NewIssue(c, n, r.ID(), fmt.Sprintf(r.What, method, typ), r.Severity, r.Confidence)
}
}
return nil
}
func (r *badDefer) checkFunction(n ast.Node, c *gosec.Context, deferStmt *ast.DeferStmt, deferTyp deferType) *gosec.Issue {
if anonFunc, isAnonFunc := deferStmt.Call.Fun.(*ast.FuncLit); isAnonFunc {
for _, subElem := range anonFunc.Body.List {
if issue := r.checkStmt(n, c, subElem, deferTyp); issue != nil {
return issue
}
}
}
return nil
}
func (r *badDefer) checkStmt(n ast.Node, c *gosec.Context, subElem ast.Stmt, deferTyp deferType) *gosec.Issue {
switch stmt := subElem.(type) {
case *ast.AssignStmt:
for _, rh := range stmt.Rhs {
if e, isCallExp := rh.(*ast.CallExpr); isCallExp {
return r.checkChild(n, c, e, deferTyp)
}
}
case *ast.IfStmt:
if s, is := stmt.Init.(*ast.AssignStmt); is {
return r.checkStmt(n, c, s, deferTyp)
}
}
return nil
}
// NewDeferredClosing detects unsafe defer of error returning methods // NewDeferredClosing detects unsafe defer of error returning methods
func NewDeferredClosing(id string, conf gosec.Config) (gosec.Rule, []ast.Node) { func NewDeferredClosing(id string, conf gosec.Config) (gosec.Rule, []ast.Node) {
return &badDefer{ return &badDefer{

View file

@ -1984,7 +1984,6 @@ func main() {
{[]string{`package main {[]string{`package main
import ( import (
"bufio"
"fmt" "fmt"
"io/ioutil" "io/ioutil"
"os" "os"
@ -2016,16 +2015,86 @@ func main() {
defer check(err) defer check(err)
fmt.Printf("wrote %d bytes\n", n2) fmt.Printf("wrote %d bytes\n", n2)
n3, err := f.WriteString("writes\n") }`}, 1, gosec.NewConfig()},
fmt.Printf("wrote %d bytes\n", n3) {[]string{`package main
f.Sync() import (
"fmt"
"io/ioutil"
"log"
"os"
)
w := bufio.NewWriter(f) func check(e error) {
n4, err := w.WriteString("buffered\n") if e != nil {
fmt.Printf("wrote %d bytes\n", n4) panic(e)
}
}
w.Flush() func main() {
d1 := []byte("hello\ngo\n")
err := ioutil.WriteFile("/tmp/dat1", d1, 0744)
check(err)
allowed := ioutil.WriteFile("/tmp/dat1", d1, 0600)
check(allowed)
f, err := os.Create("/tmp/dat2")
check(err)
defer func() {
if err := f.Close(); err != nil {
log.Println(err)
}
}()
d2 := []byte{115, 111, 109, 101, 10}
n2, err := f.Write(d2)
defer check(err)
fmt.Printf("wrote %d bytes\n", n2)
}`}, 1, gosec.NewConfig()},
{[]string{`package main
import (
"fmt"
"io/ioutil"
"log"
"os"
)
func check(e error) {
if e != nil {
panic(e)
}
}
func main() {
d1 := []byte("hello\ngo\n")
err := ioutil.WriteFile("/tmp/dat1", d1, 0744)
check(err)
allowed := ioutil.WriteFile("/tmp/dat1", d1, 0600)
check(allowed)
f, err := os.Create("/tmp/dat2")
check(err)
defer func() {
err := f.Close()
if err != nil {
log.Println(err)
}
}()
d2 := []byte{115, 111, 109, 101, 10}
n2, err := f.Write(d2)
defer check(err)
fmt.Printf("wrote %d bytes\n", n2)
}`}, 1, gosec.NewConfig()}, }`}, 1, gosec.NewConfig()},
} }