Sitelet https://web.archive.org/web/20231123090347/https://github.com/github/codeql/pull/14799
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

DataFlow: Add language-specific predicate for ignoring steps in flow-through calculation #14799

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

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Nov 15, 2023 •

TLDR

This PR adds a language-level hook for excluding steps from the flow-through calculation. It's a no-op PR for all other languages than C/C++.

Slightly longer explanation

We have a well-known source of FPs in C/C++ caused by the following pattern:

void modify(int* q) { *q = source(); }

void modify_copy(int* p) {
  int x = *p;
  modify(&x);
}

void test() {
  int x;
  modify_copy(&x);
  sink(x); // we report flow from source to sink here.
}

This happens because we model each indirection of a parameter as a separate SSA variable, so dataflow really sees the above program as

void modify(int* q, int deref_q) { deref_q = source(); }

void modify_copy(int* p, int deref_p) {
  int x = deref_p;
  modify(&x, x);
}

void test() {
  int x;
  modify_copy(&x, x);
  sink(x); // we report flow from source to sink here.
}

and in the above program it's easy to see that there's a ReturnNodeExt generated that updates the parameter deref_p in modify_copy.

The problem is the SSA step generated by int x = *p; which should be treated special when generating flow-throughs since a subsequent update of x should not affect *p (which is currently what the flow-through summaries give us).

@aschackmull I'd like to hear if you have any problems with 748625c? I hope it's not controversial since:

  • it's a no-op for all the other languages
  • it's a very simple fix for a problem that's been observed quite often in C/C++

DCA looks good. I've verified that all the removed results are instances of the test_modify_copy_of_pointer testcase.

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

Successfully merging this pull request may close these issues.

None yet

1 participant