From 6ace60b95005e16c93d973d6dac0d73e841bc17e Mon Sep 17 00:00:00 2001 From: Grant Murphy Date: Fri, 2 Dec 2016 10:20:23 -0800 Subject: [PATCH] Address unhandled error conditions Closes #95 --- core/analyzer.go | 6 ++++-- core/helpers.go | 6 ++++++ filelist.go | 7 ++++++- main.go | 2 ++ rules/fileperms.go | 6 +++++- rules/sql.go | 4 ++-- rules/tempfiles.go | 2 +- tools.go | 46 +++++++++++++++++++++++++++++++++++++++++++--- 8 files changed, 69 insertions(+), 10 deletions(-) diff --git a/core/analyzer.go b/core/analyzer.go index 5b0b720..8661732 100644 --- a/core/analyzer.go +++ b/core/analyzer.go @@ -23,6 +23,7 @@ import ( "go/types" "log" "os" + "path" "reflect" "strings" ) @@ -222,8 +223,9 @@ func (gas *Analyzer) Visit(n ast.Node) ast.Visitor { for _, rule := range val { ret, err := rule.Match(n, &gas.context) if err != nil { - // will want to give more info than this ... - gas.logger.Println("internal error running rule:", err) + file, line := GetLocation(n, &gas.context) + file = path.Base(file) + gas.logger.Printf("Rule error: %v => %s (%s:%d)\n", reflect.TypeOf(rule), err, file, line) } if ret != nil { gas.Issues = append(gas.Issues, *ret) diff --git a/core/helpers.go b/core/helpers.go index d42ceca..8ccfbbf 100644 --- a/core/helpers.go +++ b/core/helpers.go @@ -212,3 +212,9 @@ func GetImportPath(name string, ctx *Context) (string, bool) { } return "", false } + +// GetLocation returns the filename and line number of an ast.Node +func GetLocation(n ast.Node, ctx *Context) (string, int) { + fobj := ctx.FileSet.File(n.Pos()) + return fobj.Name(), fobj.Line(n.Pos()) +} diff --git a/filelist.go b/filelist.go index f52fcb8..86e144c 100644 --- a/filelist.go +++ b/filelist.go @@ -15,6 +15,8 @@ package main import ( + "fmt" + "os" "path/filepath" "strings" ) @@ -32,7 +34,10 @@ func newFileList(paths ...string) *filelist { } for _, path := range paths { - f.Set(path) + if e := f.Set(path); e != nil { + // #nosec + fmt.Fprintf(os.Stderr, "Unable to add %s to filelist: %s\n", path, e) + } } return f } diff --git a/main.go b/main.go index 451b563..248f548 100644 --- a/main.go +++ b/main.go @@ -117,7 +117,9 @@ func buildConfig(incRules string, excRules string) map[string]interface{} { return config } +// #nosec func usage() { + fmt.Fprintln(os.Stderr, usageText) fmt.Fprint(os.Stderr, "OPTIONS:\n\n") flag.PrintDefaults() diff --git a/rules/fileperms.go b/rules/fileperms.go index 9a812cb..101c7e2 100644 --- a/rules/fileperms.go +++ b/rules/fileperms.go @@ -36,7 +36,11 @@ func getConfiguredMode(conf map[string]interface{}, configKey string, defaultMod case int64: mode = value.(int64) case string: - mode, _ = strconv.ParseInt(value.(string), 0, 64) + if m, e := strconv.ParseInt(value.(string), 0, 64); e != nil { + mode = defaultMode + } else { + mode = m + } } } return mode diff --git a/rules/sql.go b/rules/sql.go index 10db0a2..9b8b79f 100644 --- a/rules/sql.go +++ b/rules/sql.go @@ -42,7 +42,7 @@ func (s *SqlStrConcat) checkObject(n *ast.Ident) bool { func (s *SqlStrConcat) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) { if node, ok := n.(*ast.BinaryExpr); ok { if start, ok := node.X.(*ast.BasicLit); ok { - if str, _ := gas.GetString(start); s.pattern.MatchString(str) { + if str, e := gas.GetString(start); s.pattern.MatchString(str) && e == nil { if _, ok := node.Y.(*ast.BasicLit); ok { return nil, nil // string cat OK } @@ -77,7 +77,7 @@ type SqlStrFormat struct { // Looks for "fmt.Sprintf("SELECT * FROM foo where '%s', userInput)" func (s *SqlStrFormat) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) { if node := gas.MatchCall(n, s.call); node != nil { - if arg, _ := gas.GetString(node.Args[0]); s.pattern.MatchString(arg) { + if arg, e := gas.GetString(node.Args[0]); s.pattern.MatchString(arg) && e == nil { return gas.NewIssue(c, n, s.What, s.Severity, s.Confidence), nil } } diff --git a/rules/tempfiles.go b/rules/tempfiles.go index 3d2f49e..8cbd55a 100644 --- a/rules/tempfiles.go +++ b/rules/tempfiles.go @@ -29,7 +29,7 @@ type BadTempFile struct { func (t *BadTempFile) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) { if node := gas.MatchCall(n, t.call); node != nil { - if arg, _ := gas.GetString(node.Args[0]); t.args.MatchString(arg) { + if arg, e := gas.GetString(node.Args[0]); t.args.MatchString(arg) && e == nil { return gas.NewIssue(c, n, t.What, t.Severity, t.Confidence), nil } } diff --git a/tools.go b/tools.go index 9bf03f3..c265819 100644 --- a/tools.go +++ b/tools.go @@ -74,10 +74,12 @@ func (u *utilities) run(args ...string) { func shouldSkip(path string) bool { st, e := os.Stat(path) if e != nil { + // #nosec fmt.Fprintf(os.Stderr, "Skipping: %s - %s\n", path, e) return true } if st.IsDir() { + // #nosec fmt.Fprintf(os.Stderr, "Skipping: %s - directory\n", path) return true } @@ -95,11 +97,12 @@ func dumpAst(files ...string) { fset := token.NewFileSet() // positions are relative to fset f, err := parser.ParseFile(fset, arg, nil, 0) if err != nil { + // #nosec fmt.Fprintf(os.Stderr, "Unable to parse file %s\n", err) continue } - // Print the AST. + // Print the AST. #nosec ast.Print(fset, f) } } @@ -115,7 +118,12 @@ type context struct { func createContext(filename string) *context { fileset := token.NewFileSet() - root, _ := parser.ParseFile(fileset, filename, nil, parser.ParseComments) + root, e := parser.ParseFile(fileset, filename, nil, parser.ParseComments) + if e != nil { + // #nosec + fmt.Fprintf(os.Stderr, "Unable to parse file: %s. Reason: %s\n", filename, e) + return nil + } comments := ast.NewCommentMap(fileset, root, root.Comments) info := &types.Info{ Types: make(map[ast.Expr]types.TypeAndValue), @@ -126,7 +134,12 @@ func createContext(filename string) *context { Implicits: make(map[ast.Node]types.Object), } config := types.Config{Importer: importer.Default()} - pkg, _ := config.Check("main.go", fileset, []*ast.File{root}, info) + pkg, e := config.Check("main.go", fileset, []*ast.File{root}, info) + if e != nil { + // #nosec + fmt.Fprintf(os.Stderr, "Type check failed for file: %s. Reason: %s\n", filename, e) + return nil + } return &context{fileset, comments, info, pkg, &config, root} } @@ -147,6 +160,15 @@ func printObject(obj types.Object) { fmt.Printf(" Id = %v\n", obj.Id()) } +func checkContext(ctx *context, file string) bool { + // #nosec + if ctx == nil { + fmt.Fprintln(os.Stderr, "Failed to create context for file: ", file) + return false + } + return true +} + func dumpCallObj(files ...string) { for _, file := range files { @@ -154,6 +176,9 @@ func dumpCallObj(files ...string) { continue } context := createContext(file) + if !checkContext(context, file) { + return + } ast.Inspect(context.root, func(n ast.Node) bool { var obj types.Object switch node := n.(type) { @@ -178,6 +203,9 @@ func dumpUses(files ...string) { continue } context := createContext(file) + if !checkContext(context, file) { + return + } for ident, obj := range context.info.Uses { fmt.Printf("IDENT: %v, OBJECT: %v\n", ident, obj) } @@ -190,6 +218,9 @@ func dumpTypes(files ...string) { continue } context := createContext(file) + if !checkContext(context, file) { + return + } for expr, tv := range context.info.Types { fmt.Printf("EXPR: %v, TYPE: %v\n", expr, tv) } @@ -202,6 +233,9 @@ func dumpDefs(files ...string) { continue } context := createContext(file) + if !checkContext(context, file) { + return + } for ident, obj := range context.info.Defs { fmt.Printf("IDENT: %v, OBJ: %v\n", ident, obj) } @@ -214,6 +248,9 @@ func dumpComments(files ...string) { continue } context := createContext(file) + if !checkContext(context, file) { + return + } for _, group := range context.comments.Comments() { fmt.Println(group.Text()) } @@ -226,6 +263,9 @@ func dumpImports(files ...string) { continue } context := createContext(file) + if !checkContext(context, file) { + return + } for _, pkg := range context.pkg.Imports() { fmt.Println(pkg.Path(), pkg.Name()) for _, name := range pkg.Scope().Names() {