Skip to content

C++: Block summary flow through strdup and friends #15504

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

Merged

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Feb 1, 2024

This PR fixes a TODO that I left in as part of #14799.

In the above PR we removed false flow in code like:

void modify_copy(int* p) {
  int x = *p; // (1)
  taint(&x);
}

void test() {
  int x;
  modify_copy(&x);
  sink(x);
}

by teaching the C/C++ dataflow library to rule out certain flow steps that shouldn't be used when calculating summaries (i.e., there shouldn't be a summary from the indirection of the input argument to modify_copy to the output indirection of modify_copy). The terminology I used was that the step from *p to x at // (1) isn't "identity preserving".

This PR extends this set of exclusion rules to also block flow when a modelled function isn't identity preserving. A common example that we've seen in many FPs is strdup(str).

To verify that a functions input -> output relation is identity preserving, we check that *input -> *output and input -> output both holds (i.e., not only does the function has flow from the indirection of the input to the indirection of the output, but it also has flow from the input pointer to the output pointer).

@github-actions github-actions bot added the C++ label Feb 1, 2024
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Feb 1, 2024
@MathiasVP MathiasVP force-pushed the block-summary-flow-out-of-strdup-and-friends branch from cb4bfa6 to 6f5ed9a Compare February 1, 2024 20:15
@MathiasVP MathiasVP marked this pull request as ready for review February 2, 2024 09:31
@MathiasVP MathiasVP requested a review from a team as a code owner February 2, 2024 09:31
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

LGTM if the DCA result changes make sense.

One question: This seems to work for cases where the modification occurs at indirection index 1. If it would occur at a higher indirection index, this is not covered, is that a correct observation? If so, this is probably not super relevant, as we likely don't model functions that behave in this way and have more than 1 indirection.

@MathiasVP
Copy link
Contributor Author

One question: This seems to work for cases where the modification occurs at indirection index 1. If it would occur at a higher indirection index, this is not covered, is that a correct observation? If so, this is probably not super relevant, as we likely don't model functions that behave in this way and have more than 1 indirection.

It actually does work perfectly fine for any indirection level. You're right in that we don't actually model any such strange function that modifies something two indirections away, but that should still Just Work. To test this out, I've:

  1. added some strange versions of strdup that takes an char** parameter and returns a char** value
  2. Added a test model for various variations of how flow could travel between the parameter and the return value

The tests aren't pretty, but hopefully it should demonstrate that this isn't only working for indirection index 1 🤞

@MathiasVP MathiasVP force-pushed the block-summary-flow-out-of-strdup-and-friends branch from 68007a0 to 439d3d2 Compare February 2, 2024 12:09
@MathiasVP
Copy link
Contributor Author

I've looked at the lost results and they're all FPs which are now removed for the exact right reason 🎉

@jketema jketema merged commit 6b13a8c into github:main Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants