Skip to content

Commit 1f3bdd9

Browse files
czechbolczechbol
authored
G115 Struct Attribute Checks (#1221)
* allow struct attributes checks * fix explicit check results
1 parent 5f3194b commit 1f3bdd9

File tree

2 files changed

+151
-28
lines changed

2 files changed

+151
-28
lines changed

‎analyzers/conversion_overflow.go

+58-28
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ type integer struct {
4040
type rangeResult struct {
4141
minValue int
4242
maxValue uint
43-
explixitPositiveVals []uint
43+
explicitPositiveVals []uint
4444
explicitNegativeVals []int
4545
isRangeCheck bool
4646
convertFound bool
@@ -271,7 +271,7 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool {
271271
if result.isRangeCheck {
272272
minValue = max(minValue, &result.minValue)
273273
maxValue = min(maxValue, &result.maxValue)
274-
explicitPositiveVals = append(explicitPositiveVals, result.explixitPositiveVals...)
274+
explicitPositiveVals = append(explicitPositiveVals, result.explicitPositiveVals...)
275275
explicitNegativeVals = append(explicitNegativeVals, result.explicitNegativeVals...)
276276
}
277277
case *ssa.Call:
@@ -325,16 +325,17 @@ func getResultRange(ifInstr *ssa.If, instr *ssa.Convert, visitedIfs map[*ssa.If]
325325
result.convertFound = true
326326
result.minValue = max(result.minValue, thenBounds.minValue)
327327
result.maxValue = min(result.maxValue, thenBounds.maxValue)
328-
result.explixitPositiveVals = append(result.explixitPositiveVals, thenBounds.explixitPositiveVals...)
329-
result.explicitNegativeVals = append(result.explicitNegativeVals, thenBounds.explicitNegativeVals...)
330328
} else if elseBounds.convertFound {
331329
result.convertFound = true
332330
result.minValue = max(result.minValue, elseBounds.minValue)
333331
result.maxValue = min(result.maxValue, elseBounds.maxValue)
334-
result.explixitPositiveVals = append(result.explixitPositiveVals, elseBounds.explixitPositiveVals...)
335-
result.explicitNegativeVals = append(result.explicitNegativeVals, elseBounds.explicitNegativeVals...)
336332
}
337333

334+
result.explicitPositiveVals = append(result.explicitPositiveVals, thenBounds.explixitPositiveVals...)
335+
result.explicitNegativeVals = append(result.explicitNegativeVals, thenBounds.explicitNegativeVals...)
336+
result.explicitPositiveVals = append(result.explicitPositiveVals, elseBounds.explixitPositiveVals...)
337+
result.explicitNegativeVals = append(result.explicitNegativeVals, elseBounds.explicitNegativeVals...)
338+
338339
return result
339340
}
340341

@@ -344,8 +345,15 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con
344345
operandsFlipped := false
345346

346347
compareVal, op := getRealValueFromOperation(instr.X)
347-
if x != compareVal {
348-
y, operandsFlipped = x, true
348+
349+
// Handle FieldAddr
350+
if fieldAddr, ok := compareVal.(*ssa.FieldAddr); ok {
351+
compareVal = fieldAddr
352+
}
353+
354+
if !isSameOrRelated(x, compareVal) {
355+
y = x
356+
operandsFlipped = true
349357
}
350358

351359
constVal, ok := y.(*ssa.Const)
@@ -362,25 +370,12 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con
362370
if !successPathConvert {
363371
break
364372
}
365-
366-
// Determine if the constant value is positive or negative.
367-
if strings.Contains(constVal.String(), "-") {
368-
result.explicitNegativeVals = append(result.explicitNegativeVals, int(constVal.Int64()))
369-
} else {
370-
result.explixitPositiveVals = append(result.explixitPositiveVals, uint(constVal.Uint64()))
371-
}
372-
373+
updateExplicitValues(result, constVal)
373374
case token.NEQ:
374375
if successPathConvert {
375376
break
376377
}
377-
378-
// Determine if the constant value is positive or negative.
379-
if strings.Contains(constVal.String(), "-") {
380-
result.explicitNegativeVals = append(result.explicitNegativeVals, int(constVal.Int64()))
381-
} else {
382-
result.explixitPositiveVals = append(result.explixitPositiveVals, uint(constVal.Uint64()))
383-
}
378+
updateExplicitValues(result, constVal)
384379
}
385380

386381
if op == "neg" {
@@ -391,11 +386,19 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con
391386
result.maxValue = uint(min)
392387
}
393388
if max <= math.MaxInt {
394-
result.minValue = int(max) //nolint:gosec
389+
result.minValue = int(max)
395390
}
396391
}
397392
}
398393

394+
func updateExplicitValues(result *rangeResult, constVal *ssa.Const) {
395+
if strings.Contains(constVal.String(), "-") {
396+
result.explicitNegativeVals = append(result.explicitNegativeVals, int(constVal.Int64()))
397+
} else {
398+
result.explicitPositiveVals = append(result.explicitPositiveVals, uint(constVal.Uint64()))
399+
}
400+
}
401+
399402
func updateMinMaxForLessOrEqual(result *rangeResult, constVal *ssa.Const, op token.Token, operandsFlipped bool, successPathConvert bool) {
400403
// If the success path has a conversion and the operands are not flipped, then the constant value is the maximum value.
401404
if successPathConvert && !operandsFlipped {
@@ -439,6 +442,8 @@ func walkBranchForConvert(block *ssa.BasicBlock, instr *ssa.Convert, visitedIfs
439442
if result.isRangeCheck {
440443
bounds.minValue = toPtr(max(result.minValue, bounds.minValue))
441444
bounds.maxValue = toPtr(min(result.maxValue, bounds.maxValue))
445+
bounds.explixitPositiveVals = append(bounds.explixitPositiveVals, result.explicitPositiveVals...)
446+
bounds.explicitNegativeVals = append(bounds.explicitNegativeVals, result.explicitNegativeVals...)
442447
}
443448
case *ssa.Call:
444449
if v == instr.X {
@@ -463,9 +468,10 @@ func isRangeCheck(v ssa.Value, x ssa.Value) bool {
463468
switch op := v.(type) {
464469
case *ssa.BinOp:
465470
switch op.Op {
466-
case token.LSS, token.LEQ, token.GTR, token.GEQ,
467-
token.EQL, token.NEQ:
468-
return op.X == compareVal || op.Y == compareVal
471+
case token.LSS, token.LEQ, token.GTR, token.GEQ, token.EQL, token.NEQ:
472+
leftMatch := isSameOrRelated(op.X, compareVal)
473+
rightMatch := isSameOrRelated(op.Y, compareVal)
474+
return leftMatch || rightMatch
469475
}
470476
}
471477
return false
@@ -475,12 +481,36 @@ func getRealValueFromOperation(v ssa.Value) (ssa.Value, string) {
475481
switch v := v.(type) {
476482
case *ssa.UnOp:
477483
if v.Op == token.SUB {
478-
return v.X, "neg"
484+
val, _ := getRealValueFromOperation(v.X)
485+
return val, "neg"
479486
}
487+
return getRealValueFromOperation(v.X)
488+
case *ssa.FieldAddr:
489+
return v, "field"
490+
case *ssa.Alloc:
491+
return v, "alloc"
480492
}
481493
return v, ""
482494
}
483495

496+
func isSameOrRelated(a, b ssa.Value) bool {
497+
aVal, _ := getRealValueFromOperation(a)
498+
bVal, _ := getRealValueFromOperation(b)
499+
500+
if aVal == bVal {
501+
return true
502+
}
503+
504+
// Check if both are FieldAddr operations referring to the same field of the same struct
505+
if aField, aOk := aVal.(*ssa.FieldAddr); aOk {
506+
if bField, bOk := bVal.(*ssa.FieldAddr); bOk {
507+
return aField.X == bField.X && aField.Field == bField.Field
508+
}
509+
}
510+
511+
return false
512+
}
513+
484514
func explicitValsInRange(explicitPosVals []uint, explicitNegVals []int, dstInt integer) bool {
485515
if len(explicitPosVals) == 0 && len(explicitNegVals) == 0 {
486516
return false

‎testutils/g115_samples.go

+93
Original file line numberDiff line numberDiff line change
@@ -716,4 +716,97 @@ func main() {
716716
}
717717
`,
718718
}, 0, gosec.NewConfig()},
719+
{[]string{
720+
`
721+
package main
722+
723+
import (
724+
"fmt"
725+
"math"
726+
)
727+
728+
type CustomStruct struct {
729+
Value int
730+
}
731+
732+
func main() {
733+
results := CustomStruct{Value: 0}
734+
if results.Value < math.MinInt32 || results.Value > math.MaxInt32 {
735+
panic("value out of range for int32")
736+
}
737+
convertedValue := int32(results.Value)
738+
739+
fmt.Println(convertedValue)
740+
}
741+
`,
742+
}, 0, gosec.NewConfig()},
743+
{[]string{
744+
`
745+
package main
746+
747+
import (
748+
"fmt"
749+
"math"
750+
)
751+
752+
type CustomStruct struct {
753+
Value int
754+
}
755+
756+
func main() {
757+
results := CustomStruct{Value: 0}
758+
if results.Value >= math.MinInt32 && results.Value <= math.MaxInt32 {
759+
convertedValue := int32(results.Value)
760+
fmt.Println(convertedValue)
761+
}
762+
panic("value out of range for int32")
763+
}
764+
`,
765+
}, 0, gosec.NewConfig()},
766+
{[]string{
767+
`
768+
package main
769+
770+
import (
771+
"fmt"
772+
"math"
773+
)
774+
775+
type CustomStruct struct {
776+
Value int
777+
}
778+
779+
func main() {
780+
results := CustomStruct{Value: 0}
781+
if results.Value < math.MinInt32 || results.Value > math.MaxInt32 {
782+
panic("value out of range for int32")
783+
}
784+
// checked value is decremented by 1 before conversion which is unsafe
785+
convertedValue := int32(results.Value-1)
786+
787+
fmt.Println(convertedValue)
788+
}
789+
`,
790+
}, 1, gosec.NewConfig()},
791+
{[]string{
792+
`
793+
package main
794+
795+
import (
796+
"fmt"
797+
"math"
798+
"math/rand"
799+
)
800+
801+
func main() {
802+
a := rand.Int63()
803+
if a < math.MinInt32 || a > math.MaxInt32 {
804+
panic("out of range")
805+
}
806+
// checked value is incremented by 1 before conversion which is unsafe
807+
b := int32(a+1)
808+
fmt.Printf("%d\n", b)
809+
}
810+
`,
811+
}, 1, gosec.NewConfig()},
719812
}

0 commit comments

Comments
 (0)