go-tools icon indicating copy to clipboard operation
go-tools copied to clipboard

staticcheck: flag subtly-useless/incorrect bounds checks

Open seebs opened this issue 2 years ago • 0 comments

(version not relevant)

if len(x) > 0 && x[1] != '-'

In this code, taken from a real example, the intent is to determine whether x has a [1] before indexing it... But the code is wrong. If len(x) is 1, then the len check passes but the index operation panics.

This is subtly different from #617, in which we know the index is out of bounds. In this case, we don't know that the index is definitely out of bounds -- len(x) could be more than 1, and it'd be fine. We just know that the len check isn't quite doing what it's supposed to be doing.

I think the approximate shape is "someone's using short-circuit operations to make indexing operations contingent on a length check, implying that they think the length check makes the indexing operation safe, but actually it is not safe". Applies to strings, slices, and arrays. (But not to maps.)

Are false positives possible? Yes. If you know for sure that len(x) is always even, for unrelated reasons, then if it's >0, you're safe. For instance, if you have a []byte that's type-punned from a []uint64, so len is always a multiple of 8, then it's safe to access the first 8 members if len is >0. But I don't think that's likely, and even if I did that, I'd probably be fine with making the len check use 7 instead of 0 to make it more clear to the compiler why I think this is safe.

seebs avatar Jul 21 '23 16:07 seebs