G115 ignores bounds checks

[rittneje]

Summary

G115 reports issues even if we do proper bounds checks. This is similar in spirit to #1185, but would require the linter to be smarter.

Steps to reproduce the behavior

var x []string

if len(x) <= math.MaxUint32 {
    y := uint32(len(x))
    fmt.Println(y)
}

This reports integer overflow conversion int -> uint32 (gosec).

gosec version

I am running via golangci-lint v1.62.0.

Go version (output of 'go version')

n/a

Operating system / Environment

n/a

Expected behavior

The linter should see that there is a bounds check and thus be able to prove to itself that the overflow is impossible.

Actual behavior

The linter does not consider anything about prior bounds checks, leading to false positives that need to be ignored, diminishing the utility of the check.

[ccojocar]

Another case to handle which is safe because the size is checked during parsing:

v,_ := strconv.ParseInt("1",10,32) 
v32 := int32(v) 

[palsivertsen]

This issue was introduced with release v1.60.2 of golangci-lint which bumps gosec version from 5f0084e to 81cda2f in golangci/golangci-lint#4927

[rittneje]

@ccojocar This issue should be re-opened. First, the linked PR did not fix it. The code snippet in my original issue description is still flagged, as is the following example.

func foo(x int) uint32 {
	if x < 0 {
		return 0
	}
	if x > math.MaxUint32 {
		return math.MaxUint32
	}
	return uint32(x)
}

(I tested with commit c52dc0e.)

Furthermore, from reading the code itself, it doesn't seem to care at all what the bounds checks are actually doing, just that they are present. So even if the code did work, the following would have been allowed even though it should be flagged:

func foo(x int) uint32 {
	if x < 0 {
		return 0
	}
	return uint32(x)
}

[ccojocar]

@czechbol It seems that there some more use cases which are not handled in #1189. I would be great if you could also check the bounds. Thanks

[czechbol]

Hi, @ccojocar, thanks for the heads up. I'll take a closer look when I have a bit of time.

[ben-krieger]

@czechbol It seems that there some more use cases which are not handled in #1189. I would be great if you could also check the bounds. Thanks

I may be able to help. First @ccojocar I'd like to confirm something:

The bounds (explicit range) check, as is, appears to be on the converted value. That would mean you're looking for a check on uint32(x) in @rittneje's example rather than a check on x, like he wants. Is there a reason for allowing values when their type converted value is checked?

I also noticed that the tests in g115_samples.go all still pass even with the hasExplicitRangeCheck commented out.

[ben-krieger]

My understanding of SSA is pretty limited, but I think we need to be looking at all the SSA basic blocks within the function. Like this:

diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go
index 1449f94..6c8a35a 100644
--- a/analyzers/conversion_overflow.go
+++ b/analyzers/conversion_overflow.go
@@ -115,13 +115,14 @@ func isConstantInRange(constVal *ssa.Const, dstType string) bool {
 }
 
 func hasExplicitRangeCheck(instr *ssa.Convert) bool {
-	block := instr.Block()
-	for _, i := range block.Instrs {
-		if binOp, ok := i.(*ssa.BinOp); ok {
-			// Check if either operand of the BinOp is the result of the Convert instruction
-			if (binOp.X == instr || binOp.Y == instr) &&
-				(binOp.Op == token.LSS || binOp.Op == token.LEQ || binOp.Op == token.GTR || binOp.Op == token.GEQ) {
-				return true
+	for _, block := range instr.Block().Parent().Blocks {
+		for _, i := range block.Instrs {
+			if binOp, ok := i.(*ssa.BinOp); ok {
+				// Check if either operand of the BinOp is the result of the Convert instruction
+				if (binOp.X == instr.X || binOp.Y == instr.X) &&
+					(binOp.Op == token.LSS || binOp.Op == token.LEQ || binOp.Op == token.GTR || binOp.Op == token.GEQ) {
+					return true
+				}
 			}
 		}
 	}