diff --git a/analyzers/slice_bounds.go b/analyzers/slice_bounds.go index 968102f268..2347af07ea 100644 --- a/analyzers/slice_bounds.go +++ b/analyzers/slice_bounds.go @@ -81,29 +81,64 @@ func runSliceBounds(pass *analysis.Pass) (interface{}, error) { for _, s := range violations { switch s := s.(type) { case *ssa.Slice: - issue := newIssue( + issues[s] = newIssue( pass.Analyzer.Name, "slice bounds out of range", pass.Fset, s.Pos(), issue.Low, issue.High) - issues[s] = issue case *ssa.IndexAddr: - issue := newIssue( + issues[s] = newIssue( pass.Analyzer.Name, "slice index out of range", pass.Fset, s.Pos(), issue.Low, issue.High) - issues[s] = issue } } } } } } + case *ssa.IndexAddr: + switch indexInstr := instr.X.(type) { + case *ssa.Const: + if indexInstr.Type().String()[:2] == "[]" { + if indexInstr.Value == nil { + issues[instr] = newIssue( + pass.Analyzer.Name, + "slice index out of range", + pass.Fset, + instr.Pos(), + issue.Low, + issue.High) + + break + } + } + case *ssa.Alloc: + if instr.Pos() > 0 { + typeStr := indexInstr.Type().String() + arrayLen, err := extractArrayAllocValue(typeStr) // preallocated array + if err != nil { + break + } + + _, err = extractIntValueIndexAddr(instr, arrayLen) + if err != nil { + break + } + issues[instr] = newIssue( + pass.Analyzer.Name, + "slice index out of range", + pass.Fset, + instr.Pos(), + issue.Low, + issue.High) + } + } } } } @@ -143,7 +178,7 @@ func runSliceBounds(pass *analysis.Pass) (interface{}, error) { if err != nil { break } - if isSliceIndexInsideBounds(0, value, indexValue) { + if isSliceIndexInsideBounds(value, indexValue) { delete(issues, instr) } } @@ -161,8 +196,8 @@ func runSliceBounds(pass *analysis.Pass) (interface{}, error) { } foundIssues := []*issue.Issue{} - for _, issue := range issues { - foundIssues = append(foundIssues, issue) + for _, v := range issues { + foundIssues = append(foundIssues, v) } if len(foundIssues) > 0 { return foundIssues, nil @@ -192,7 +227,11 @@ func trackSliceBounds(depth int, sliceCap int, slice ssa.Node, violations *[]ssa } case *ssa.IndexAddr: indexValue, err := extractIntValue(refinstr.Index.String()) - if err == nil && !isSliceIndexInsideBounds(0, sliceCap, indexValue) { + if err == nil && !isSliceIndexInsideBounds(sliceCap, indexValue) { + *violations = append(*violations, refinstr) + } + indexValue, err = extractIntValueIndexAddr(refinstr, sliceCap) + if err == nil && !isSliceIndexInsideBounds(sliceCap, indexValue) { *violations = append(*violations, refinstr) } case *ssa.Call: @@ -217,6 +256,32 @@ func trackSliceBounds(depth int, sliceCap int, slice ssa.Node, violations *[]ssa } } +func extractIntValueIndexAddr(refinstr *ssa.IndexAddr, sliceCap int) (int, error) { + var indexIncr, sliceIncr int + + for _, block := range refinstr.Block().Preds { + for _, instr := range block.Instrs { + switch instr := instr.(type) { + case *ssa.BinOp: + _, index, err := extractBinOpBound(instr) + if err != nil { + return 0, err + } + switch instr.Op { + case token.LSS: + indexIncr-- + } + + if !isSliceIndexInsideBounds(sliceCap+sliceIncr, index+indexIncr) { + return index, nil + } + } + } + } + + return 0, errors.New("no found") +} + func checkAllSlicesBounds(depth int, sliceCap int, slice *ssa.Slice, violations *[]ssa.Instruction, ifs map[ssa.If]*ssa.BinOp) { if depth == maxDepth { return @@ -303,9 +368,14 @@ func invBound(bound bound) bound { } } +var errExtractBinOp = fmt.Errorf("unable to extract constant from binop") + func extractBinOpBound(binop *ssa.BinOp) (bound, int, error) { if binop.X != nil { if x, ok := binop.X.(*ssa.Const); ok { + if x.Value == nil { + return lowerUnbounded, 0, errExtractBinOp + } value, err := strconv.Atoi(x.Value.String()) if err != nil { return lowerUnbounded, value, err @@ -324,6 +394,9 @@ func extractBinOpBound(binop *ssa.BinOp) (bound, int, error) { } if binop.Y != nil { if y, ok := binop.Y.(*ssa.Const); ok { + if y.Value == nil { + return lowerUnbounded, 0, errExtractBinOp + } value, err := strconv.Atoi(y.Value.String()) if err != nil { return lowerUnbounded, value, err @@ -340,11 +413,11 @@ func extractBinOpBound(binop *ssa.BinOp) (bound, int, error) { } } } - return lowerUnbounded, 0, fmt.Errorf("unable to extract constant from binop") + return lowerUnbounded, 0, errExtractBinOp } -func isSliceIndexInsideBounds(l, h int, index int) bool { - return (l <= index && index < h) +func isSliceIndexInsideBounds(h int, index int) bool { + return (0 <= index && index < h) } func isSliceInsideBounds(l, h int, cl, ch int) bool { @@ -370,6 +443,10 @@ func extractSliceBounds(slice *ssa.Slice) (int, int) { } func extractIntValue(value string) (int, error) { + if i, err := extractIntValuePhi(value); err == nil { + return i, nil + } + parts := strings.Split(value, ":") if len(parts) != 2 { return 0, fmt.Errorf("invalid value: %s", value) @@ -381,7 +458,7 @@ func extractIntValue(value string) (int, error) { } func extractSliceCapFromAlloc(instr string) (int, error) { - re := regexp.MustCompile(`new \[(\d+)\]*`) + re := regexp.MustCompile(`new \[(\d+)\].*`) var sliceCap int matches := re.FindAllStringSubmatch(instr, -1) if matches == nil { @@ -397,3 +474,39 @@ func extractSliceCapFromAlloc(instr string) (int, error) { return 0, errors.New("no slice cap found") } + +func extractIntValuePhi(value string) (int, error) { + re := regexp.MustCompile(`phi \[.+: (\d+):.+, .*\].*`) + var sliceCap int + matches := re.FindAllStringSubmatch(value, -1) + if matches == nil { + return sliceCap, fmt.Errorf("invalid value: %s", value) + } + + if len(matches) > 0 { + m := matches[0] + if len(m) > 1 { + return strconv.Atoi(m[1]) + } + } + + return 0, fmt.Errorf("invalid value: %s", value) +} + +func extractArrayAllocValue(value string) (int, error) { + re := regexp.MustCompile(`.*\[(\d+)\].*`) + var sliceCap int + matches := re.FindAllStringSubmatch(value, -1) + if matches == nil { + return sliceCap, fmt.Errorf("invalid value: %s", value) + } + + if len(matches) > 0 { + m := matches[0] + if len(m) > 1 { + return strconv.Atoi(m[1]) + } + } + + return 0, fmt.Errorf("invalid value: %s", value) +} diff --git a/testutils/g602_samples.go b/testutils/g602_samples.go index c0fee62949..3f42c41ca1 100644 --- a/testutils/g602_samples.go +++ b/testutils/g602_samples.go @@ -338,4 +338,79 @@ func main() { } `}, 0, gosec.NewConfig()}, + {[]string{` +package main + +func main() { + s := make([]int, 16) + for i := 10; i < 17; i++ { + s[i]=i + } +} + +`}, 1, gosec.NewConfig()}, + {[]string{` +package main + +func main() { + var s []int + for i := 10; i < 17; i++ { + s[i]=i + } +} + +`}, 1, gosec.NewConfig()}, + {[]string{` +package main + +func main() { + s := make([]int,5, 16) + for i := 1; i < 6; i++ { + s[i]=i + } +} + +`}, 1, gosec.NewConfig()}, + {[]string{` +package main + +func main() { + var s [20]int + for i := 10; i < 17; i++ { + s[i]=i + } +}`}, 0, gosec.NewConfig()}, + {[]string{` +package main + +func main() { + var s [20]int + for i := 1; i < len(s); i++ { + s[i]=i + } +} + +`}, 0, gosec.NewConfig()}, + {[]string{` +package main + +func main() { + var s [20]int + for i := 1; i <= len(s); i++ { + s[i]=i + } +} + +`}, 1, gosec.NewConfig()}, + {[]string{` +package main + +func main() { + var s [20]int + for i := 18; i <= 22; i++ { + s[i]=i + } +} + +`}, 1, gosec.NewConfig()}, }