Skip to content

Conversation

MathiasVP
Copy link
Contributor

(Part of https://github.com/github/codeql-c-team/issues/272.)

This PR does a couple of things to reduce the number of false positives on cpp/uncontrolled-arithmetic:

  1. It moves the division sanitizer into the local dataflow configuration.
  2. It marks % and & as sanitizers.

This reduces the number of false positives quite a bit when running our favorite projects on LGTM (compare the one from this PR with the one on main). This doesn't affect any of the results on SAMATE.

@MathiasVP MathiasVP added the C++ label May 12, 2021
@MathiasVP MathiasVP requested a review from a team as a code owner May 12, 2021 14:05
* Holds if `fc` is a part of the left operand of a binary operation that greatly reduces the range
* of possible values.
*/
predicate bounded(Expr e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the kind of awkward code that the IR is supposed to avoid - would it help here, or would mapping back into the AST to use SimpleRangeAnalysis be equally awkward?

Copy link
Contributor Author

@MathiasVP MathiasVP May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mapping back to expressions will make this solution equally awkward, yeah. Since RemInstruction.getUnconvertedResultExpr() won't have a result if the original expression was a compound assignment, I think we'd need a disjunct for both % and %= anyway, unfortunately.

I like the idea, though. An easy-to-use mapping like that would certainly be helpful for predicates like this. I guess this is thematically related to https://github.com/github/codeql-c-team/issues/147.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants