check nil slices, partially check bounds (#1396)

kondratev · Kondratev Pavel · web-flow · commit 01029f0a7416 · 2025-10-03T10:41:33.000+02:00
* check nil slices, partially check bounds

* add tests, cleanup, add fixed array

* cleanup

* lint

* looks like go bug, add second check

* ohh

* check instruction position

---------

Co-authored-by: Kondratev Pavel &lt;kondratev_pa@magnit.ru&gt;
diff --git 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
@@ -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()},
 }
