-
Notifications
You must be signed in to change notification settings - Fork 1.8k
C++: Add range analysis to cpp/static-buffer-overflow
#5923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C++: Add range analysis to cpp/static-buffer-overflow
#5923
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There's a PR check failure asking for a change note. |
Thanks for the heads up. Fixed in 6103aab. |
// result in this case we pick the minimum value obtainable from dataflow and range analysis. | ||
result = | ||
upperBound(statedSizeExpr()) | ||
.minimum(any(Expr statedSizeSrc | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using the min
aggregate here, rather than int::minimum
and any
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Fixed in 741eed9.
… min aggregate further down since it's no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, merging...
(Part of https://github.com/github/codeql-c-analysis-team/issues/191.)
This PR uses range analysis to filter out false positives in 2 out of the 3 kinds of problems identified by
cpp/static-buffer-overflow
.It does remove a couple of false positives on our usual list of LGTM projects: https://lgtm.com/query/3164577302384476236/. Hopefully, once this PR is merged, we can see what the next class of low-hanging fruit is after the next LGTM upgrade.