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++: Exclusion rules for system macros #6723

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

Conversation

@jbj
Copy link
Contributor

@jbj jbj commented Sep 21, 2021

Unwanted results were reported for our JPL Rule 24 queries. Inclusion of system headers with complex macros could lead to unpredictable alerts from these rules.

Fixes #6522.

It's hard to write unit tests for these changes, like it was in #5085, because there are no system headers in qltests. I tested manually on a vim/vim snapshot and found some examples:

Unwanted results were reported for our JPL Rule 24 queries. Including
system headers with complex macros could lead to unpredictable alerts
from these rules.
@jbj jbj added the C++ label Sep 21, 2021
@jbj jbj requested a review from as a code owner Sep 21, 2021
Copy link
Contributor

@geoffw0 geoffw0 left a comment

It's hard to write unit tests for these changes, like it was in #5085, because there are no system headers in qltests

ql\cpp\ql\test\query-tests\Security\CWE\CWE-190\semmle\tainted implements a system header in a test, though the method is a bit of a fiddle.

*
* If the system macro is invoked through a non-system macro, then this
* predicate does not hold. That's a limitation of how macros are represented
* in the database.
Copy link
Contributor

@geoffw0 geoffw0 Sep 21, 2021

Could you share an example? (I'd have thought between getAnExpandedElement() and MacroInvocation.getParentInvocation() we can work this out, I could be wrong)

Copy link
Contributor Author

@jbj jbj Sep 30, 2021

I've pushed a commit that removes the claim about db limitations.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Approved, because neither of my comments needs to block merging this.

jbj added 2 commits Sep 30, 2021
A review comment pointed out that this claim might not be correct.
@jbj jbj marked this pull request as draft Sep 30, 2021
@jbj
Copy link
Contributor Author

@jbj jbj commented Sep 30, 2021

I've converted this PR to a draft because we're trying to find out which customers (and which teams) would be affected by this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants