X Tutup
The Wayback Machine - https://web.archive.org/web/20220103132948/https://github.com/github/codeql/pull/7459
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++: Increase precision of cpp/arithmetic-uncontrolled to high #7459

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

@MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Dec 20, 2021

Fixes github/codeql-c-team#553.

This PR does two things:

  • The first commit removes a bunch of false positives by restricting the sinks to be signed integers in the case of potentially overflowing expressions. This removes a ton of results, and I think this is overall a good thing since (unlike signed-integer overflow) unsigned integer overflow is well-defined.

    Here's a list of 70 random projects that produce alerts on this query, and here is a difference query that shows how many results we remove with this PR.

    A note on Samate:
    On main this query raises 22798 TPs on Samate and after this PR it raises 18012 TPs.
    So we lose 4786 results. Out of those results, 1920 are related to CWE-680 (which we have a seperate query for), and the remaining 2866 results are CWE-190 related that look like:

    /* POTENTIAL FLAW: Use a random value */
    data = (unsigned int)RAND32();
    /* POTENTIAL FLAW: Adding 1 to data could cause an overflow */
    unsigned int result = data + 1;
    printUnsignedLine(result);

    which I'm not convinced is an issue that we want to highlight in real code.

  • The second commit raises the query's precision to high, meaning it'll be part of the code-scanning suite and be displayed by default on LGTM.

Here's the results on our usual list of test projects: https://lgtm.com/query/7783254823209558200/.

I'm leaving this PR as a draft while I run the query on a larger set of LGTM projects.

@MathiasVP MathiasVP added the C++ label Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

1 participant
X Tutup