github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.59k stars 1.52k forks source link

LGTM.com - false positive (decrementing uint8_t) #4422

Open arvidn opened 4 years ago

arvidn commented 4 years ago

Description of the false positive

lgtm is claiming that a loop variable of type uint8_t will always stay equal to or greater than its initial value, despite the for-loop decrementing it. It's as if lgtm is not modelling unsigned variables or for loops correctly.

URL to the alert on the project page on LGTM.com

https://lgtm.com/projects/g/Chia-Network/chiapos/snapshot/837b9be23d5d21104a41b99fb9b5ad7fd2686e20/files/src/phase2.hpp?sort=name&dir=ASC&mode=heatmap#xb62e750067a5c62f:1

arvidn commented 4 years ago

there's another similar issue as well, involving a for loop and uint8_t where the analysis fails to see that the variable is incremented.

https://lgtm.com/projects/g/Chia-Network/chiapos/snapshot/837b9be23d5d21104a41b99fb9b5ad7fd2686e20/files/src/phase3.hpp?sort=name&dir=ASC&mode=heatmap#x745da1f828d9622b:1

geoffw0 commented 4 years ago

Hi, I mean to look into this, sorry for the delay.

geoffw0 commented 4 years ago

I've had a quick look and I agree this is a false positive. There are some casts in root/src/util.hpp (namely duration_cast and to_time_t) that analysis thinks are non-returning. This leads to it thinking the loop itself doesn't terminate, as if the code were:

for (i = 10; i > 0; i--) // comparison is always true because i == 10
{
    exit(1);
}

I'll keep you updated.

geoffw0 commented 4 years ago

Hi,

We've tracked this down to an extractor bug, which is populating some incorrect data and confusing reachability analysis. We've opened an internal ticket for the underlying issue. Thank you for the report.

Please let us know if you need a workaround until that is fixed (this will probably be either hiding the bad results or hiding that entire query for your project).