False positive: Multiplication result converted to larger type

[robn]

Description of the false positive

Monocypher implements, among other things, the Poly1305 MAC. CodeQL takes issue with a carefully-constructed sequence of multiplications, tripping cpp/integer-multiplication-cast-to-long.

I asked the author about it in LoupVaillant/Monocypher#245, and you should check there for an analysis. I understand the short version to be:

• CodeQL is correct
• The sequence is carefully constructed to never overflow, and the author has a proof
• There are additional invariants enforced earlier that stop the overflow from being possible, that can't be expressed in C's type system
• Adding an explicit cast silences the warnings, but results in a measurable drop in performance (it converts a 64x32b multiplication to a 64x64b)

Ideally if there was a suppression mechanism, I would use it. As it is, I will likely simply leave a comment.

Code samples or links to source code

https://github.com/LoupVaillant/Monocypher/blob/master/src/monocypher.c#L352

URL to the alert on GitHub code scanning (optional)

The Security tab appears to have no alerts in it; the reports appear in openzfs/zfs#14249.

[ryao]

The Security tab appears to have no alerts in it; the reports appear in openzfs/zfs#14249.

That is issue #11021. Github does not give projects a way to stop hiding those results without giving everyone commit access. I am #4 by commit count in openzfs/zfs and even I do not have access to it. Ironically, I am the one who proposed using CodeQL in the first place. :/

We can still see general code scanning results in our own forked repositories, which makes hiding them pointless.

[hvitved]

Hi

Just to be clear: Are you asking whether the query can be updated, to not report the claimed false-positive, or that we introduce some form of suppression mechanism (e.g. a comment)? Are you aware, that alerts can be dismissed in the UI?

[robn]

I'm not asking for anything; I'm just letting you know that this happened so you can think about it and if its interesting, put it in your plans somewhere.

I'd prefer an inline method to silence the warnings but I know you're not interested in that (#9298) and I can't dismiss them in the UI as I do not have commit access to the repo (#11021).

[hvitved]

I'm not asking for anything; I'm just letting you know that this happened so you can think about it and if its interesting, put it in your plans somewhere.

👍

@github/codeql-c I'll let you decide, whether you want to track this internally.