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++: Allocate more FunctionInput and FunctionOutputs #14667

Merged

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Nov 2, 2023

The modelling language we use for specifying C/C++ models allows us to express stuff like "there's flow from the second indirection of a parameter to the second indirection of the return value" using a model like:

override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
  input.isParameterDeref(2) and output.isReturnValueDeref(2)
}

if we add this model to a function foo this should ensure that we get flow in an example such as:

int** foo(int**);

void test(int** buffer) {
  **buffer = source();
  char** buffer2 = foo(buffer);
  sink(**buffer2);
}

However, while the modelling interface allowed us to do this, the modelling implementation was hardcoded to only allow input.isParameterDeref(1) and output.isReturnValueDeref(1) 😱.

So this PR bumps those bounds up to 2. In reality we can probably bump it up arbitrarily large (although we do have to provide some bound to make sure it's all finite), but I didn't want to do that in this PR since I want to get this in quickly to unblock @jketema's work on #14561.

Commit-by-commit review recommended.

…utput's for more indirections. Note that we now mark two output nodes coming out of 'getaddrinfo' as a remote flow source (the first indirection and the second indirection). We'll fix that in the next commit.
@github-actions github-actions bot added the C++ label Nov 2, 2023
@MathiasVP MathiasVP requested a review from jketema November 2, 2023 17:05
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.

One small comment.

Regarding the DCA results: are these actually new results, or are they just duplicating existing results?

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Nov 6, 2023

Regarding the DCA results: are these actually new results, or are they just duplicating existing results?

On Samate this looks like new results 🎉. I'm still checking the other DBs.

Edit: The other ones are also new results 🎉. I have yet to find out exactly why we didn't find these results before, but I'm very happy that we now do find them 😂

@MathiasVP MathiasVP marked this pull request as ready for review November 6, 2023 13:09
@MathiasVP MathiasVP requested a review from a team as a code owner November 6, 2023 13:09
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Nov 6, 2023
@MathiasVP
Copy link
Contributor Author

@jketema in 31c2a3b I've reverted the redefinitions of the single-parameter predicates. I've tested that we preserve the test results even if I keep the old definition of of hasRemoteFlowSource in the recv class (and likewise if I keep the old definition of hasRemoteFlowSink in the Send class). However, I think the more explicit models we have now are better, so I think we should keep them as they are in this PR.

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.

Probably want to re-run DCA, other than that LGTM.

@MathiasVP
Copy link
Contributor Author

Probably want to re-run DCA, other than that LGTM.

Agreed. DCA running now 👍

@MathiasVP
Copy link
Contributor Author

DCA looks fine. The new real-world results are gone, but we still find all the new Samate results. So I'll merge this and we can always investigate why we lost the real-world results again at a later time.

@MathiasVP MathiasVP merged commit a17cd9b into github:main Nov 7, 2023
8 checks passed
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