C++: Recognize any non-overflowing arithmetic expression as a barrier for cpp/uncontrolled-arithmetic

[MathiasVP]

This should be one of the final low-hanging fruits for this query that's motivated by real-world FPs.

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

[geoffw0]

This is similar to something we did on another query recently, right?

[MathiasVP]

This is similar to something we did on another query recently, right?

Yes. We did this in #5903. Once this PR is merged I'll pull the bounded predicate into a shared file and use it in both cpp/uncontrolled-allocation-size, cpp/uncontrolled-arithmetic and cpp/tainted-arithmetic.

[geoffw0]

Once this PR is merged I'll pull the bounded predicate into a shared file and use it in both cpp/uncontrolled-allocation-size, cpp/uncontrolled-arithmetic and cpp/tainted-arithmetic.

Sounds good.

Can you point me to some LGTM projects for which this query has results?

[MathiasVP]

Can you point me to some LGTM projects for which this query has results?

Our list of usual suspect has some results: https://lgtm.com/query/6937375284332555889/.

In terms of where this PR makes a difference, you can see an instance of this on the python-pillow/Pillow project if you compare the query without this change to the query after this change.

The other projects in that run are similar cases which sadly need a more syntactic barrier which I haven't added yet.

[geoffw0]

A few comments.

Can we get rid of any of the boundedDiv cases in bounded now?

[geoffw0]

If I'm understanding correctly, this can in certain cases fall short of the specification "greatly reduces the range of possible values" - to give an extreme example the following is now a barrier:

x = x + 0; // this can't overflow

However this is pretty silly, in practice we expect the overwhelming majority of arithmetic either can cause an overflow (x = x + 1), or greatly narrows the range of values (x = x / RAND_MAX) ... or is working on an already constrained value (hopefully safely).

If my above understanding is accurate, this is a somewhat heuristic approach, but I think I'm happy with it.

[MathiasVP]

Indeed. This whole predicate uses a bunch of heuristics.

[MathiasVP]

Can we get rid of any of the boundedDiv cases in bounded now?

Unfortunately not. SimpleRangeAnalysis only handles division and right shifts when one of the operations is a constant. Whereas boundedDiv removes them regardless of whether or not they are constants.

[MathiasVP]

@geoffw0 I believe all your concerns have been addressed.

[geoffw0]

Yep, thanks for the fixes, merging...