Skip to content

Commit 01029f0

Browse files
kondratevKondratev Pavel
kondratev
and
Kondratev Pavel
authored
check nil slices, partially check bounds (#1396)
* 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 <kondratev_pa@magnit.ru>
1 parent 34db3de commit 01029f0

File tree

2 files changed

+200
-12
lines changed

2 files changed

+200
-12
lines changed

‎analyzers/slice_bounds.go‎

Lines changed: 125 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -81,29 +81,64 @@ func runSliceBounds(pass *analysis.Pass) (interface{}, error) {
8181
for _, s := range violations {
8282
switch s := s.(type) {
8383
case *ssa.Slice:
84-
issue := newIssue(
84+
issues[s] = newIssue(
8585
pass.Analyzer.Name,
8686
"slice bounds out of range",
8787
pass.Fset,
8888
s.Pos(),
8989
issue.Low,
9090
issue.High)
91-
issues[s] = issue
9291
case *ssa.IndexAddr:
93-
issue := newIssue(
92+
issues[s] = newIssue(
9493
pass.Analyzer.Name,
9594
"slice index out of range",
9695
pass.Fset,
9796
s.Pos(),
9897
issue.Low,
9998
issue.High)
100-
issues[s] = issue
10199
}
102100
}
103101
}
104102
}
105103
}
106104
}
105+
case *ssa.IndexAddr:
106+
switch indexInstr := instr.X.(type) {
107+
case *ssa.Const:
108+
if indexInstr.Type().String()[:2] == "[]" {
109+
if indexInstr.Value == nil {
110+
issues[instr] = newIssue(
111+
pass.Analyzer.Name,
112+
"slice index out of range",
113+
pass.Fset,
114+
instr.Pos(),
115+
issue.Low,
116+
issue.High)
117+
118+
break
119+
}
120+
}
121+
case *ssa.Alloc:
122+
if instr.Pos() > 0 {
123+
typeStr := indexInstr.Type().String()
124+
arrayLen, err := extractArrayAllocValue(typeStr) // preallocated array
125+
if err != nil {
126+
break
127+
}
128+
129+
_, err = extractIntValueIndexAddr(instr, arrayLen)
130+
if err != nil {
131+
break
132+
}
133+
issues[instr] = newIssue(
134+
pass.Analyzer.Name,
135+
"slice index out of range",
136+
pass.Fset,
137+
instr.Pos(),
138+
issue.Low,
139+
issue.High)
140+
}
141+
}
107142
}
108143
}
109144
}
@@ -143,7 +178,7 @@ func runSliceBounds(pass *analysis.Pass) (interface{}, error) {
143178
if err != nil {
144179
break
145180
}
146-
if isSliceIndexInsideBounds(0, value, indexValue) {
181+
if isSliceIndexInsideBounds(value, indexValue) {
147182
delete(issues, instr)
148183
}
149184
}
@@ -161,8 +196,8 @@ func runSliceBounds(pass *analysis.Pass) (interface{}, error) {
161196
}
162197

163198
foundIssues := []*issue.Issue{}
164-
for _, issue := range issues {
165-
foundIssues = append(foundIssues, issue)
199+
for _, v := range issues {
200+
foundIssues = append(foundIssues, v)
166201
}
167202
if len(foundIssues) > 0 {
168203
return foundIssues, nil
@@ -192,7 +227,11 @@ func trackSliceBounds(depth int, sliceCap int, slice ssa.Node, violations *[]ssa
192227
}
193228
case *ssa.IndexAddr:
194229
indexValue, err := extractIntValue(refinstr.Index.String())
195-
if err == nil && !isSliceIndexInsideBounds(0, sliceCap, indexValue) {
230+
if err == nil && !isSliceIndexInsideBounds(sliceCap, indexValue) {
231+
*violations = append(*violations, refinstr)
232+
}
233+
indexValue, err = extractIntValueIndexAddr(refinstr, sliceCap)
234+
if err == nil && !isSliceIndexInsideBounds(sliceCap, indexValue) {
196235
*violations = append(*violations, refinstr)
197236
}
198237
case *ssa.Call:
@@ -217,6 +256,32 @@ func trackSliceBounds(depth int, sliceCap int, slice ssa.Node, violations *[]ssa
217256
}
218257
}
219258

259+
func extractIntValueIndexAddr(refinstr *ssa.IndexAddr, sliceCap int) (int, error) {
260+
var indexIncr, sliceIncr int
261+
262+
for _, block := range refinstr.Block().Preds {
263+
for _, instr := range block.Instrs {
264+
switch instr := instr.(type) {
265+
case *ssa.BinOp:
266+
_, index, err := extractBinOpBound(instr)
267+
if err != nil {
268+
return 0, err
269+
}
270+
switch instr.Op {
271+
case token.LSS:
272+
indexIncr--
273+
}
274+
275+
if !isSliceIndexInsideBounds(sliceCap+sliceIncr, index+indexIncr) {
276+
return index, nil
277+
}
278+
}
279+
}
280+
}
281+
282+
return 0, errors.New("no found")
283+
}
284+
220285
func checkAllSlicesBounds(depth int, sliceCap int, slice *ssa.Slice, violations *[]ssa.Instruction, ifs map[ssa.If]*ssa.BinOp) {
221286
if depth == maxDepth {
222287
return
@@ -303,9 +368,14 @@ func invBound(bound bound) bound {
303368
}
304369
}
305370

371+
var errExtractBinOp = fmt.Errorf("unable to extract constant from binop")
372+
306373
func extractBinOpBound(binop *ssa.BinOp) (bound, int, error) {
307374
if binop.X != nil {
308375
if x, ok := binop.X.(*ssa.Const); ok {
376+
if x.Value == nil {
377+
return lowerUnbounded, 0, errExtractBinOp
378+
}
309379
value, err := strconv.Atoi(x.Value.String())
310380
if err != nil {
311381
return lowerUnbounded, value, err
@@ -324,6 +394,9 @@ func extractBinOpBound(binop *ssa.BinOp) (bound, int, error) {
324394
}
325395
if binop.Y != nil {
326396
if y, ok := binop.Y.(*ssa.Const); ok {
397+
if y.Value == nil {
398+
return lowerUnbounded, 0, errExtractBinOp
399+
}
327400
value, err := strconv.Atoi(y.Value.String())
328401
if err != nil {
329402
return lowerUnbounded, value, err
@@ -340,11 +413,11 @@ func extractBinOpBound(binop *ssa.BinOp) (bound, int, error) {
340413
}
341414
}
342415
}
343-
return lowerUnbounded, 0, fmt.Errorf("unable to extract constant from binop")
416+
return lowerUnbounded, 0, errExtractBinOp
344417
}
345418

346-
func isSliceIndexInsideBounds(l, h int, index int) bool {
347-
return (l <= index && index < h)
419+
func isSliceIndexInsideBounds(h int, index int) bool {
420+
return (0 <= index && index < h)
348421
}
349422

350423
func isSliceInsideBounds(l, h int, cl, ch int) bool {
@@ -370,6 +443,10 @@ func extractSliceBounds(slice *ssa.Slice) (int, int) {
370443
}
371444

372445
func extractIntValue(value string) (int, error) {
446+
if i, err := extractIntValuePhi(value); err == nil {
447+
return i, nil
448+
}
449+
373450
parts := strings.Split(value, ":")
374451
if len(parts) != 2 {
375452
return 0, fmt.Errorf("invalid value: %s", value)
@@ -381,7 +458,7 @@ func extractIntValue(value string) (int, error) {
381458
}
382459

383460
func extractSliceCapFromAlloc(instr string) (int, error) {
384-
re := regexp.MustCompile(`new \[(\d+)\]*`)
461+
re := regexp.MustCompile(`new \[(\d+)\].*`)
385462
var sliceCap int
386463
matches := re.FindAllStringSubmatch(instr, -1)
387464
if matches == nil {
@@ -397,3 +474,39 @@ func extractSliceCapFromAlloc(instr string) (int, error) {
397474

398475
return 0, errors.New("no slice cap found")
399476
}
477+
478+
func extractIntValuePhi(value string) (int, error) {
479+
re := regexp.MustCompile(`phi \[.+: (\d+):.+, .*\].*`)
480+
var sliceCap int
481+
matches := re.FindAllStringSubmatch(value, -1)
482+
if matches == nil {
483+
return sliceCap, fmt.Errorf("invalid value: %s", value)
484+
}
485+
486+
if len(matches) > 0 {
487+
m := matches[0]
488+
if len(m) > 1 {
489+
return strconv.Atoi(m[1])
490+
}
491+
}
492+
493+
return 0, fmt.Errorf("invalid value: %s", value)
494+
}
495+
496+
func extractArrayAllocValue(value string) (int, error) {
497+
re := regexp.MustCompile(`.*\[(\d+)\].*`)
498+
var sliceCap int
499+
matches := re.FindAllStringSubmatch(value, -1)
500+
if matches == nil {
501+
return sliceCap, fmt.Errorf("invalid value: %s", value)
502+
}
503+
504+
if len(matches) > 0 {
505+
m := matches[0]
506+
if len(m) > 1 {
507+
return strconv.Atoi(m[1])
508+
}
509+
}
510+
511+
return 0, fmt.Errorf("invalid value: %s", value)
512+
}

‎testutils/g602_samples.go‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,4 +338,79 @@ func main() {
338338
}
339339
340340
`}, 0, gosec.NewConfig()},
341+
{[]string{`
342+
package main
343+
344+
func main() {
345+
s := make([]int, 16)
346+
for i := 10; i < 17; i++ {
347+
s[i]=i
348+
}
349+
}
350+
351+
`}, 1, gosec.NewConfig()},
352+
{[]string{`
353+
package main
354+
355+
func main() {
356+
var s []int
357+
for i := 10; i < 17; i++ {
358+
s[i]=i
359+
}
360+
}
361+
362+
`}, 1, gosec.NewConfig()},
363+
{[]string{`
364+
package main
365+
366+
func main() {
367+
s := make([]int,5, 16)
368+
for i := 1; i < 6; i++ {
369+
s[i]=i
370+
}
371+
}
372+
373+
`}, 1, gosec.NewConfig()},
374+
{[]string{`
375+
package main
376+
377+
func main() {
378+
var s [20]int
379+
for i := 10; i < 17; i++ {
380+
s[i]=i
381+
}
382+
}`}, 0, gosec.NewConfig()},
383+
{[]string{`
384+
package main
385+
386+
func main() {
387+
var s [20]int
388+
for i := 1; i < len(s); i++ {
389+
s[i]=i
390+
}
391+
}
392+
393+
`}, 0, gosec.NewConfig()},
394+
{[]string{`
395+
package main
396+
397+
func main() {
398+
var s [20]int
399+
for i := 1; i <= len(s); i++ {
400+
s[i]=i
401+
}
402+
}
403+
404+
`}, 1, gosec.NewConfig()},
405+
{[]string{`
406+
package main
407+
408+
func main() {
409+
var s [20]int
410+
for i := 18; i <= 22; i++ {
411+
s[i]=i
412+
}
413+
}
414+
415+
`}, 1, gosec.NewConfig()},
341416
}

0 commit comments

Comments
 (0)