G115: integer overflow conversion uint8 -> int64

[ldemailly]

Summary

uint8 -> int64 has no overflow

Steps to reproduce the behavior

package main

import (
	"fmt"
)

func main() {
	str := "A\xFF"
	i := int64(str[0])
	fmt.Printf("%d\n", i)
}

gosec version

81cda2f

Go version (output of 'go version')

go version go1.22.6 darwin/arm64

Operating system / Environment

n/a

Expected behavior

no complaint

Actual behavior

complains
G115: integer overflow conversion uint8 -> int64:

main.go:9:12: G115: integer overflow conversion uint8 -> int64 (gosec)
	i := int64(str[0])
	          ^

[r--w]

Agree, lots of alerts from yesterday in our CI/CD pipeline

[remyleone]

Yes same here, could you add documentation about how to make G115 pass? Otherwise we are going to ignore all alerts :( We've tried to add bound checks and nothing works

[FairlySadPanda]

Broken in this change I guess #1130

This seems like a seriously under-cooked change which is currently mandating a lot of //nolint flags for us.

Example of another goof -> foo := []int{1,2,3}; bar := uint32(len(foo)) cannot possibly cause data loss yet now fails.

[ccoVeille]

I was also surprised to see #1149 being merged so fast.

While the idea is good, why not after all. But it should have been reviewed by testing the behavior of the rules on large codebase.

Or make this rule optional at first

[ccojocar]

The rule can be simply excluded from the scanning if is causing too many issues on your code base:

gosec -exclude=G115 ./...

[FairlySadPanda]

For my toolchain I've opted for adding //nolint:gosec to lines where casting is required and otherwise reviewed casting, so I suppose it's a useful exercise for code tidiness...

[ccojocar]

Example of another goof -> foo := []int{1,2,3}; bar := uint32(len(foo)) cannot possibly cause data loss yet now fails.

@FairlySadPanda the len function returns an int which is variable based on which architecture are you running. So in a 64 bit arch which is the most common these days, the value is actually int64. In the end, you are converting from int64 to uin32 this is a clear overflow.

[ccojocar]

@ldemailly the false positive from byte to int64 conversion is not reproducible, see the attached tests in #1186.

[FairlySadPanda]

Example of another goof -> foo := []int{1,2,3}; bar := uint32(len(foo)) cannot possibly cause data loss yet now fails.

@FairlySadPanda the len function returns an int which is variable based on which architecture are you running. So in a 64 bit arch which is the most common these days, the value is actually int64. In the end, you are converting from int64 to uin32 this is a clear overflow.

I actually had not considered this, and it's a good point, but this doesn't sit completely right still. Casting is ultimately the executive decision of the programmer; having to specifically disable the rule or add a flag to skip specific casts as known-good means the value of the test itself becomes questionable.

Int and the other core builtins being variable in size would be good to communicate as part of the failure string when doing this sort of check, regardless - both for this and 109. Whilst 64-bit is the default these days, the specification itself is written to cope with 32 bit, and it's easy to get led astray by the documentation.

[ccojocar]

@FairlySadPanda You are free to skip this rule if you don't find it useful for your use case. Nobody is dictating its usage, but it some cases, integer overflow can lead to security issues. Unfortunately go runtime doesn't protect against this.

[ldemailly]

@ldemailly the false positive from byte to int64 conversion is not reproducible, see the attached tests in #1186.

I don't see that test and I definitely see the error. unless it's been fixed since the version golanglint-ci picked in 1.60.2 - on phone but will link ci output as well as repro in a bit

[pierrre]

I can reproduce the "uint8 -> int" overflow false positive on my side.
See the screenshot.

FYI I'm using golangci-lint v1.60.2 (I didn't try gosec itself)