Add a new rule to detect integer overflow when converting between integer types

[ccojocar]

fixes #1130

[remyleone]

Could you add documentation about how to fix it and make the linter pass? 🙏

[ccoVeille]

I might have missed something about the implementation, but I would like to suggest this

Suggested change

	fmt.Sprintf("integer overflow conversion %s -> %s", src, dst),
	fmt.Sprintf("possible integer overflow conversion %s -> %s", src, dst),

It might not happen depending on people code.

It's better to report a possible error, than affirming there is one. People would argue.

[ccoVeille]

@ccojocar what do you think about the changes I suggested?

Do you agree with them?

Would you change it? Or do you want me to open a PR for them?

[ccoVeille]

This regexp will be computed each time parseInt will be called, it should be moved out the function

[ccoVeille]

Please add something like this

Suggested change

	it := matches[re.SubexpIndex("type")]
	is := matches[re.SubexpIndex("size")]
	// int64 => int 64, uint8 => uint 8
	it := matches[re.SubexpIndex("type")]
	is := matches[re.SubexpIndex("size")]

[ccoVeille]

This 8 is magical number here

[ccoVeille]

Maybe this

Suggested change

	func isIntOverflow(src string, dst string) bool {
	func couldIntOverflow(src string, dst string) bool {

[ccoVeille]

I'm a bit worried these tests are only focusing on things that could be reported.

I would expect to be added to cover conversion of things that should pass.

They are likely the ones to report false positive if something is invalid in the current implementation, or if a later refactoring screw everything

[FairlySadPanda]

This check has rolled out and ergo suddenly comments on it!

This check seems over-prescriptive. In the builtin functions, len returns type int, despite only returning zero or positive ints. Casting out of len into uint32 is thus more declarative. This is just one example where this change enforces a //nolint flag.

The act of casting is an assertion that the programmer knows what they are doing and are content with the potential for data loss. Whilst I'm sure that a zero-trust environment would want to scrutinize this sort of thing, it's overkill for normal programs.

[moitias]

There is perfectly valid code that can be guaranteed not to overflow that this check catches. For example;

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

As the GoDoc states;

The bitSize argument specifies the integer type that the result must fit into.

Tbh, I'm not sure how one is supposed to cast bigger (or bare) ints/uints (for example, received from a library) into smaller size ints/uints at all with this check? Doing just uint32(v & 0xFFFF) is also flagged even though - as far as I understand - it's not possible for v&0xFFFF to not fit into uint32.

At the very least, it seems to me like this check should be opt-in instead of opt-out.

[ccojocar]

There is perfectly valid code that can be guaranteed not to overflow that this check catches. For example;

@moitias The type of v is still int which on a 64 bit system is 64 bits. Essentially the rule sees a conversion from int64 to int32. It is not so smart to detect the conversion done before using strconv.ParseInt. It think adding these extension can be useful but for now you can make the size explicit by selecting the int32 type for v instead of int. This will make the size explicit into the type.

See more discussions in #1185.

Also if this rule doesn't work for your use case, feel free to exclude it but in some cases integer overflow can lead to security issues and there isn't a protection for this in the go runtime at the moment.

gosec -exclude G115 ./...

[moitias]

@moitias The type of v is still int which on a 64 bit system is 64 bits.

Well, if we're being pedantic, no, the type of v is not int but int64 because ParseInt and ParseUInt return explicit int64s/uint64s but yeah, this is just nitpicking.

It think adding these extension can be useful but for now you can make the size explicit by selecting the int32 type for v instead of int. This will make the size explicit into the type.

Without commit access to Go stdlib (and considering Go 1 compatibility promise), I don't think I can do that.

Essentially the rule sees a conversion from int64 to int32. It is not so smart to detect the conversion done before using strconv.ParseInt.

And that, indeed, is the problem. There are safe ways of doing the conversion (again pointing out the very straightforward v := uint8(x & 0xFF)), but this checker does not allow for them either.

As a sidenote (or, another nit); this also has nothing to do with what happens before strconv.ParseInt but rather that the method makes an explicit guarantee that it will not return values that will not fit into size n values which the linter obviously does not understand.

Also if this rule doesn't work for your use case, feel free to exclude it but in some cases integer overflow can lead to security issues and there isn't a protection for this in the go runtime at the moment.

Yeah, but this checker does not allow the developer to do it safely either, so they will need to disable the checker (locally or globally), making this checker essentially useless for any case that actually needs to do conversions and of course, nothing but lost CPU time for any case that doesn't do conversions. So, I'd argue that the usefulness of this check is dependent on it being 'smart'. If it is not, people will disable it and overflows might then creep in.

[ccojocar]

@moitias @FairlySadPanda @ccoVeille I am looking forward to your contributions to improve this rule. I think is more productive to do this instead of just complaining and using an OSS tool maintained by volunteers ;-).

[moitias]

Look, I appreciate the work you are putting into this (and helping me out) but I'm trying to point out - to help you out in turn - that this rule in its current form seems to be a bit prematurely turned on because it seems to have no use cases at all. This is because if one has it enabled, there is no way of doing the conversion securely while in fact there obviously are.

Could you please, to clarify the reasoning for this rule, at least describe one way of doing a conversion to a smaller bit size safely or are you of the opinion that it is never acceptable/secure to do? I would hope we could at least agree that linter warnings should have a way of fixing them, right?

[ccoVeille]

@moitias @FairlySadPanda @ccoVeille I am looking forward to your contributions to improve this rule. I think is more productive to do this instead of just complaining and using an OSS tool maintained by volunteers ;-).

I can understand your reaction. I could say Touché. But I'm more likely to reply you positively than being affected. I'm a smiling person.

Be sure I have nothing against you or the idea you got. I wish the issues we are facing now were identified before it was merged, but there were not 🤷

You can understand people are reacting when their CI is now a Christmas tree 🎄

I wish I had reviewed your PR earlier. I wish others may have reviewed earlier.
Someone may have raised the consequences of this PR.

There problems was not with the idea or the code, but with the review of your changes suggestions.

That said, sometimes shit happens. Let's try to find a solution.

[ccoVeille]

For anyone interested, the discussion continued here

#1185

Then the remaing issues to be addressed are discussed here #1212