X Tutup
The Wayback Machine - https://web.archive.org/web/20251227030002/https://github.com/github/codeql/pull/6760
Skip to content

Conversation

@andersfugmann
Copy link
Contributor

@andersfugmann andersfugmann commented Sep 27, 2021

As this changes the precision on static-buffer-overflow query, please do not merge before GHES 3.3 has been branched.

This PR includes:

  • Further relax predicate memberMaybeVarsized to include all members of size 0 or 1
  • Remove warnings for static buffer overflows in when fields are indexed as an argument to offsetof
  • Do not warn on static buffer overflows if upper bound analysis used widening.
  • Weaken wording on overflow static alert text (is accessed -> may be accessed)

Statistics from running OverflowStatic on all LGTM projects:

Total Alerts 206
Projects with alerts 91
False positives 16
Unknown* 6

[*] Unknown alerts denotes alerts, where I cannot judge if the alerts is correct. These usually includes nested macro expansions.

The false positives can be grouped into:

  • Copying multiple fields (7 FP's)
  • Limitations in the range analysis (1 FP)
    • 1 reasonably trivial
    • 3 are non trivial, and would require a deeper invariant analysis.
  • Dead code (2 FP's)
  • Wrong assumption of page_size (evaluated to 0) (1 FP)
  • Variable sized member of size 4 (char data[sizeof(int)]) (2 FP)

Detailed results and the actual analysis can be found here:
https://gist.github.com/andersfugmann/5091e7c63cd4f6ae94d3b86e426efd77

Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes LGTM, but I've suggested some expansion of the docs.

This PR removes many good results in the tests, but I think those results were on contrived code.

andersfugmann and others added 2 commits September 28, 2021 09:40
Co-authored-by: Jonas Jensen <jbj@github.com>
Co-authored-by: Jonas Jensen <jbj@github.com>
@andersfugmann
Copy link
Contributor Author

Test fails on /home/runner/work/semmle-code/semmle-code/semmlecode-cpp-tests/DO_NOT_DISTRIBUTE/security-tests/CWE-120/CWE/examples/OverrunWrite.qlref. Any suggestions on how I can resolve this?

@MathiasVP
Copy link
Contributor

MathiasVP commented Sep 28, 2021

Test fails on /home/runner/work/semmle-code/semmle-code/semmlecode-cpp-tests/DO_NOT_DISTRIBUTE/security-tests/CWE-120/CWE/examples/OverrunWrite.qlref. Any suggestions on how I can resolve this?

These needs to be resolved in the internal repo. You need to create a PR in the internal repo with the changes and bump the QL submodule in the PR to point to the HEAD of this PR. I'll happily go over this on a call if you want. It takes a couple of tries to get used to :)

@jbj
Copy link
Contributor

jbj commented Sep 28, 2021

I'm around to help in the office.

Co-authored-by: Jonas Jensen <jbj@github.com>
@andersfugmann andersfugmann added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Sep 28, 2021
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@MathiasVP MathiasVP merged commit 8dcf792 into github:main Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

X Tutup