C++: Multiplication overflow not possible due to type width

[MathiasVP]

This PR fixes the false positive reported in #2561.

Differences query: https://lgtm.com/query/3029067271222385694/

I have included 3 tests (from @jbj), all of which are currently not detected due to excluding values of "small types":

predicate likelySmall(Expr e) {
  ...
  or
  e.getType().getSize() <= 1
  or
  ...
}

However, it might be worth discussing whether we should still exclude such values from the analysis now that we're using SimpleRangeAnalysis.

[jbj]

I think this approach introduces too many false negatives, so I hope we can do better.

[geoffw0]

I was surprised to find that all four cases are necessary (e.g. multiplying a number in the range [-10 .. -5] by a number in the range [5 .. 10] has a maximum result of -25).

[geoffw0]

Looks promising.

Any idea why ffmpeg.c line 187 is no longer reported?

[MathiasVP]

Looks promising.

Any idea why ffmpeg.c line 187 is no longer reported?

Hmmm... It seems like VarAnalyzableExpr fails for accesses of the form a->b (and probably also a.b, although I haven't checked it), causing the analysis to discard multiplications that involve computing upper/lower bounds for such expressions. I'll dig into this!

I fear that this might make the results less impressive. 👎I'll update the PR when I'm at the bottom of this!

Edit: I've updated the PR and produced a new differences query.

[geoffw0]

Changes and new query differences LGTM. We have lost the bulk of the differences, but perhaps that's to be expected.

Performance looks to be fine as well - the predicates in this file take negligible time compared to computing SimpleRangeAnalysis, the CFG and other things (tested Qt, wxWidgets, wireshark).

In the long run (not in this PR), support for multiplication should probably be moved into a library file. I have a feeling support it was deliberately left out for the benefit of some uses cases, but perhaps we could provide an extension library?

[MathiasVP]

In the long run (not in this PR), support for multiplication should probably be moved into a library file. I have a feeling support it was deliberately left out for the benefit of some uses cases, but perhaps we could provide an extension library?

AFAIK support for multiplication was attempted in https://git.semmle.com/Semmle/code/pull/17898, but was abandoned for performance reasons.

I do agree that it would be a useful extension library to be used when the number of query candidates has been reduced significantly (as it was in this case by the original predicate).

[geoffw0]

LGTM. I'm going to merge this.

I think this approach introduces too many false negatives, so I hope we can do better.

@jbj I think we've made considerable improvements since you posted this, but please do raise any concerns you still have.

["small types"] it might be worth discussing whether we should still exclude such values from the analysis now that we're using SimpleRangeAnalysis.

This might still be worth exploring as follow-up.