G115 is reporting false positives (a summary)

[czechbol]

Summary

G115 is still reporting false positives even after #1189 and #1194.

Known false positives

I'm writing these down without much investigation whether it is possible to implement fixes for them.

• any kind of arithmetic done on the checked variable after the check:

func foo(a int) uint {
    if a != 3 || a != 4 {
        panic("not supported")
    }
    // the value being passed here is different than the one checked 
    // so it is tricky to determine if the check is valid
    //
    // this will be difficult to implement without introducing false negatives
    return uint(a-1)  // false positive 
}

• explicit value checks are lacking:

func foo(a int) uint {
    if a == 3 || a != 4 {
        // see that sneaky NEQ there?
        return uint(a)  // false negative 
    }
    panic("not supported")
}

• binary truncation (reported by @stephenc):

func foo(a int) uint16 {
    return uint16(a &0xffff) // false positive 
}

• builtin min and max functions (reported by @ben-krieger):

func foo() uint16 {
    a, b := 1234, 2345
    result := min(a,b)

    return uint(result) // false positive 
}

• loop indices (reported by @PlasmaPower):

func foo(myArr []string) {
    // these seem to have a similar implementation difficulty to the arithmetic example
    for i, _ := range myArr {
        _ = uint64(i) // false positive 
    }

    for i := 0; i < 10; i++ {
        _ = uint64(i) // false positive 
    }

    for i := randIntMightBeNegativeEven(); i >= 0; i-- {
        _ = uint64(i) // false positive 
    }
}

Notes:

I recommend opening multiple small PRs that fix one issue at a time.

[ccoVeille]

At least these use cases are less frequent than the previous ones.

[deerbone]

I'd say that people will hit the first one pretty frequently, and it also seems to me it's the most difficult to tackle without bringing in any false negatives.

[Victoremepunto]

I think I may be hitting this: https://github.com/RedHatInsights/frontend-operator/pull/188/files#diff-e8b328ffb5ced885894c80b942846601597e05020f831c2b1442d8fb8a654c8dR120

[czechbol]

@Victoremepunto you're getting that warning because golangci-lint has not released a new version with the changes from #1194 yet. The code you have there looks sound so once we have a new golangci-lint release, it should work.

@ccoVeille do you know and can share at least approximately when we can expect it?

[ccoVeille]

I'm just a random gopher. I'm pretty active, but I'm not part of golangci-lint release.

@ldez might be a better candidate to reply that question

[ldez]

do you know and can share at least approximately when we can expect it?

I have no precise release date for now but soon, mainly because of G115 false positives.

[PlasmaPower]

I'm not sure if I should open a separate issue for this, but another false positive is casting the index produced by iterating over a range to an unsigned int. We do this a lot in our codebase. E.g.:

myArr := []string{"hello", "world"}

for i := range len(myArr) {
    _ = uint64(i) // shouldn't error but does with G115
}

for i, _ := range myArr {
    _ = uint64(i) // shouldn't error but does with G115
}

Ideally it'd be able to look at bounds in for loops as well, but I get that's more complicated:

for i := 0; i < 10; i++ {
    _ = uint64(i) // shouldn't error but does with G115
}

for i := randIntMightBeNegativeEven(); i >= 0; i-- {
    _ = uint64(i) // shouldn't error but does with G115
}

[ccoVeille]

Quick follow up, golangci-lint 1.61.0 was shipped with gosec 2.21.2 💪

So everything that was made in gosec to improve G115 detection are now released 🎉

[czechbol]

I'm not sure if I should open a separate issue for this, but another false positive is casting the index produced by iterating over a range to an unsigned int. We do this a lot in our codebase. E.g.:

@PlasmaPower You are correct, this seems like another issue. I'll get back to it a bit later and update my summary.

[Victoremepunto]

@Victoremepunto you're getting that warning because golangci-lint has not released a new version with the changes from #1194 yet. The code you have there looks sound so once we have a new golangci-lint release, it should work.

@ccoVeille do you know and can share at least approximately when we can expect it?

Hi! unfortunately, using golangci-lint 1.60.1 the problem is still there - might worth trying to get a simpler reproducer and add it to this issue?

check this: https://github.com/RedHatInsights/frontend-operator/actions/runs/10835444044/job/30066989278?pr=188#step:5:26

[ldez]

Hi! unfortunately, using golangci-lint 1.60.1 the problem is still there

Maybe it's because the fix is inside golangci-lint v1.61.0 and not v1.60.1 😉

[Victoremepunto]

Hi! unfortunately, using golangci-lint 1.60.1 the problem is still there

Maybe it's because the fix is inside golangci-lint v1.61.0 and not v1.60.1 😉

lol, I'm sorry the version I used was v1.61.0, I just mistyped it. Here's an excerpt of the GHA logs:

run golangci-lint
  Running [/home/runner/golangci-lint-1.61.0-linux-amd64/golangci-lint run] in [/home/runner/work/frontend-operator/frontend-operator] ...
  Error: controllers/status.go:120:44: G115: integer overflow conversion int -> int32 (gosec)

[ben-krieger]

Hi! unfortunately, using golangci-lint 1.60.1 the problem is still there

Maybe it's because the fix is inside golangci-lint v1.61.0 and not v1.60.1 😉

lol, I'm sorry the version I used was v1.61.0, I just mistyped it. Here's an excerpt of the GHA logs:

run golangci-lint
  Running [/home/runner/golangci-lint-1.61.0-linux-amd64/golangci-lint run] in [/home/runner/work/frontend-operator/frontend-operator] ...
  Error: controllers/status.go:120:44: G115: integer overflow conversion int -> int32 (gosec)

In order to keep this thread a useful source of truth, let me fill in some details.

@Victoremepunto's issue was that accessing a struct field is considered a different value every time in SSA. This is because without deeper analysis (than SSA), you can't rule out a concurrent routine modifying that value between accesses.

Therefore, the issue could have been resolved without any changes to gosec by simply changing

if results.Managed < math.MinInt32 || results.Managed > math.MaxInt32 {
	return crd.FrontendDeployments{}, "", errors.NewClowderError("value out of range for int32")
}
deploymentStats.ManagedDeployments = int32(results.Managed)

to

managed := results.Managed
if managed < math.MinInt32 || managed > math.MaxInt32 {
	return crd.FrontendDeployments{}, "", errors.NewClowderError("value out of range for int32")
}
deploymentStats.ManagedDeployments = int32(managed)

However, since struct field access is a common use case and modifying the value at the same time as reading it is a race condition (which is usually considered a bug), #1221 treats field accesses as not being modified between time-of-check and time-of-use (TOCTOU).

And yes, TOCTOU is its own type of potential security vulnerability. Maybe a future linter can better look for this.

[czechbol]

Thanks @ben-krieger for the explanation. I purposefully decided to ignore TOCTOU in that PR.

The proposal of a separate lint rule for it is a very good idea since a different lint message would explain the issue better.

[ldemailly]

Contributing a real world example of false positive with min:

p1.R = uint8(min(255, uint16(p1.R)+uint16(p2.R)))

[ccojocar]

On this, I think we need to start the Obfuscated Go Integer Overflow Contest. 😄