Improvement the int conversion overflow logic to handle bound checks #1194
Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1194 +/- ##
==========================================
+ Coverage 67.84% 68.48% +0.64%
==========================================
Files 75 75
Lines 4186 4360 +174
==========================================
+ Hits 2840 2986 +146
- Misses 1210 1226 +16
- Partials 136 148 +12 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I left some comments on how I might make the code a bit more clear. Feel free to completely ignore.
I did test this out on my codebase locally and it removed the same false negatives as my PR #1193. Therefore, I will be closing my PR in favor of yours.
|
@czechbol Unfortunately, I've never worked with the Go parser/AST before, so I won't be much help. |
|
I apologise for the email spam. Got my github accounts mixed up. Everything should now be accounted for apart from this comment - #1194 (comment). I'm still thinking through how to tackle it. |
@czechbol Thanks for addressing the review comments. Please just let me know when this is ready for final review. |
|
I think this requires a rebase, since there are some conflicts in |
|
Unfortunately with the latest changes, I've seen two regressions running gosec on my private codebase. First, is a case of: xbLen := len(xb)
if xbLen > math.MaxUint16 {
return nil, fmt.Errorf("x contains too many bytes")
}
b = binary.BigEndian.AppendUint16(b, uint16(xbLen))Second is a case of: numEntries := len(ov.Entries)
if numEntries > math.MaxUint8 {
return nil, fmt.Errorf(...)
}
_ = uint8(numEntries)I'm guessing this is because int -> uintX doesn't guarantee that the value is > 0. However, |
sure, that is not an issue
I'll look into it too after I figure out the conditional stuff. |
czechbol
force-pushed
the
improvement/int-overflow-logic
branch
from
ae6860c
to
529f6b4
Compare
|
This code is definitely not final. It will most likely need some refactoring after the following cases are handled as well. I currently have some way to handle AND and OR conditions in the if statements. Since the SSA does not give me a nice way to check the entire if statement and breaks it up into component instructions, I need to look through them to determine their relationship. If you have suggestions how to do it differently, please let me know. Currently unhandled cases that I'm aware of:
I hope to be able to solve these in the coming days. Some help is also welcome if anyone wants to get this done sooner. |
|
@czechbol I am currently getting a panic on the following input: package main
import "math"
func foo(x []string) int16 {
if len(x) < math.MaxInt16 {
return int16(len(x))
}
return 0
}
func main() {
}
|
|
@czechbol I haven't thought through it much, but I'm wondering if it would be better to keep track of the known minimum and maximum bounds themselves, instead of just flags. That way we have a good foundation for adding support for more interesting cases in the future. As a rough idea: func foo(x int64) int16 {
// Initially our "context" is that x is in the range [MinInt64, MaxInt64].
if x > math.MaxInt16 {
return math.MaxInt16
}
// Now because of the return, we can negate the if check and "apply" it to the context.
// Thus x is in the range [MinInt64, MaxInt16].
if x < math.MinInt16 {
return math.MinInt16
}
// Again because of the return, we can negate the if check and "apply" it to the context.
// Thus x is in the range [MinInt16, MaxInt16].
// We know that x must be in range, so this conversion is safe.
return int16(x)
}In the future, I think this bookkeeping could help with situations involving arithmetic: func foo(x uint64) uint16 {
if x > math.MaxUint16 - 1 {
return math.MaxUint16
}
return uint16(x + 1)
} |
Putting the panic aside, we actually won't be able to allow this conversion. SSA IR will have each I imagine that this case will frustrate a lot of people with gosec, but there's nothing we can do. Realizing that both len calls will have the same value is a compiler optimization at best, since I'm pretty sure you can mutate a slice's length using reflection. |
@rittneje Yes, I know there's panics, that's why I said I "have some way" of handling conditions, that Converts inside if blocks don't work and that it's not ready to be merged yet. There still is work to do, but I'm slowly covering more test cases and want to share the changes with you. If you find any other curious test cases, feel free to suggest them or open a PR to my branch. I'll be honest, I definitely didn't fully understand what I was getting myself into when I opened #1189 but I'm in too deep now and I still want to fix all the false positives I can. If what @ben-krieger says is true then it's a shame but what can we do. In the meantime I'm switching this PR to a Draft. |
@czechbol I also feel like I jumped into something out of my depth. I was absolutely nerd-sniped. But you've done a great job! In fact, I'm quite impressed with what 3 unprepared devs could do to significantly tackle these false positives. In my opinion, the main thing holding us back is not even our experience with SSA or language analysis, but the fact that this probably really requires complex inspection of the abstract syntax tree to understand things like "does a failed bounds check result in conversion code being unreachable" and "does a possibly complex boolean expression check either bound, none, or both?" This is no doubt difficult even for developers of compilers! So I think we should just take a few more good looks at what we can do here, document the most common false positives (and the couple false negatives we will introduce), and ship it. I know this PR has merit, because it saves me from 22 obviously false positives in just one library I maintain. This is good work! |
@czechbol I think from I would focus to cover the most common uses cases at the function level. Validation outside of the function block can be addressed later. To simplify, I would track the bounds and keep their values through different if checks similar with @rittneje suggested in #1194 (comment). The check can be then performed only once at the conversion time. I would approach something like:
|
|
Now this is an interesting corner I've gotten myself into. I've involuntarily created a situation where the range check is an insufficient solution. If someone wants to explicitly check a value with an equals operator, I currently have no way of handling that. @ccojocar What do you think? Should I consider this out of scope of this PR? |
czechbol
force-pushed
the
improvement/int-overflow-logic
branch
from
749ac90
to
ade882a
Compare
It sounds good to me to handle it later since the current implementation in converting quite a good chunk of use cases. A good solution is better than a perfect solution which is not implemented. |
Signed-off-by: czechbol <[email protected]>
Co-authored-by: Ben Krieger <[email protected]>
Signed-off-by: czechbol <[email protected]>
Signed-off-by: czechbol <[email protected]>
Signed-off-by: czechbol <[email protected]>
Signed-off-by: czechbol <[email protected]>
Signed-off-by: czechbol <[email protected]>
Signed-off-by: czechbol <[email protected]>
Signed-off-by: czechbol <[email protected]>
Signed-off-by: czechbol <[email protected]>
Signed-off-by: czechbol <[email protected]>
Change-Id: I8da6495eaaf25b1739389aa98492bd7df338085b Signed-off-by: Cosmin Cojocar <[email protected]>
Signed-off-by: czechbol <[email protected]>
Signed-off-by: czechbol <[email protected]>
Change-Id: I0db56cb0a5f9ab6e815e2480ec0b66d7061b23d3 Signed-off-by: Cosmin Cojocar <[email protected]>
ccojocar
force-pushed
the
improvement/int-overflow-logic
branch
from
b131cc1
to
9f0427a
Compare
I ignored the warning for now. |
|
Does this also allow us to signal intentional truncation without a bounds check, e.g. |
No. Someone would need to take up a significant effort in order to support bounds checks that involve 1) arithmetic binary operations, 2) boolean logic operations, or 3) the min and max builtins from Go 1.21. Perhaps we can create a ticket to track this? But I'm not sure who will sign up for that level of effort, so maybe it should be implemented across multiple PRs. |
|
@stephenc I'm sorry I claimed that it has been resolved I only noticed the sentence about truncation after you commented here. That being said I currently don't have the time capacity to continue working on this feature, I've already sank too much time into it. I hope you understand that. I've pushed this PR to a point where most people can at least work around the issue somehow and the most common examples that we could come up with should be covered. |
|
@czechbol fixing the ranges is great. As you closed my issue perhaps you could just create a placeholder about adding support for bitmasks to signal intentional truncation so that it does not get lost. No rush to implement, I can live with |
I see those as all different cases. I think intentional truncation is always going to be of a fixed pattern... a mask that fits into the target type and a bitwise and... and for it to be intentional I would pair that with both being inside the cast... such a cast, for the all 1's case, should be optimized as simple truncation by the compiler and thus it is clearly signalling intent to truncate. as things currently stand I don't see how I can signal intentional truncation other than by disabling the rule on the line which could mask an unintended truncation |
The (ugly) way to get gosec to accept the conversion would be something like: x = x&0xffff
if x < 0 || x > math.MaxUint16 {
panic("unreachable")
}
_ = uint16(x)I'm no compiler dev, but I expect that the entire if-block with panic would be pruned during compiling and not even have a runtime effect. |
|
@ben-krieger, @stephenc, @rittneje, @ccojocar FYI I opened #1212 to compile all the false positives we can find in a single issue. |
As per discussion in #1187, the implementation was lacking. This PR extends the bounds check logic to validate that the integer size is checked within the bounds of the destination type.
I'd like to hear @rittneje's opinion on the changes as they reported the issues with it and @ben-krieger's too since he is already looking into it as well.
fixes #1187
fixes #1202