Skip to content

C++: Fix models and simplify taint flow #11946

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
merged 2 commits into from
Jan 24, 2023

Conversation

MathiasVP
Copy link
Contributor

The taint-model interpretation code had a weird case that implemented something along the lines of:

If the model says that there's incoming flow from a dereference, then I'm also going to assume that there's incoming flow from the pointer.

(and similarly for the outgoing flow).

As far as I can see, these rules only existed to fix some models that were missing flow into arguments that didn't have indirections (for example, because the function actually took a non-pointer like argument), but for some reason used input.asParameterDeref(...) in their models.

This PR fixes those models and simplifies the taint-model interpretation code.

@MathiasVP MathiasVP requested a review from a team as a code owner January 21, 2023 01:29
@github-actions github-actions bot added the C++ label Jan 21, 2023
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Jan 21, 2023
@jketema
Copy link
Contributor

jketema commented Jan 23, 2023

I hadn't thought about this before, but since we opted for an opt-in model for use-use dataflow, I think the model changes should go directly to main. This allows us to check that we don't break anything later for the AST and use-def dataflow. Looking a the feature branch we might have a similar "problem" with the changes to FlowSources.qll that are on the feature branch only.

@MathiasVP
Copy link
Contributor Author

I hadn't thought about this before, but since we opted for an opt-in model for use-use dataflow, I think the model changes should go directly to main. This allows us to check that we don't break anything later for the AST and use-def dataflow. Looking a the feature branch we might have a similar "problem" with the changes to FlowSources.qll that are on the feature branch only.

That's a good point. This entire PR might actually be able to target the main branch since main also has this weird model deref -> model pointer conflation (see here). I'll try it out on main and see what happens 🤞.

@MathiasVP MathiasVP marked this pull request as draft January 23, 2023 10:02
@MathiasVP MathiasVP changed the base branch from mathiasvp/replace-ast-with-ir-use-usedataflow to main January 23, 2023 10:10
@MathiasVP MathiasVP marked this pull request as ready for review January 23, 2023 10:11
@jketema
Copy link
Contributor

jketema commented Jan 23, 2023

CI doesn't seem to have triggered. Closing and re-opening this PR might do the trick? I'd also like to see some DCA results for this rebased branch 😄

@MathiasVP
Copy link
Contributor Author

CI doesn't seem to have triggered. Closing and re-opening this PR might do the trick? I'd also like to see some DCA results for this rebased branch 😄

Will try that! I'm actually seeing one regression on set.cpp that I wasn't seeing on the use-use flow branch 😭.

@MathiasVP MathiasVP closed this Jan 23, 2023
@MathiasVP MathiasVP reopened this Jan 23, 2023
@MathiasVP
Copy link
Contributor Author

470abfd fixes the lost results. I've started another DCA run as well.

@jketema
Copy link
Contributor

jketema commented Jan 23, 2023

470abfd fixes the lost results. I've started another DCA run as well.

Is this only a problem with the set model, or can this happen with other models (and are we missing tests for it). I guess DCA might partially help figuring this out.

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Jan 23, 2023

Is this only a problem with the set model, or can this happen with other models (and are we missing tests for it). I guess DCA might partially help figuring this out.

Yeah, it could be a problem for other instances of iterator-based model flow if we're missing tests.

Edit: DCA didn't show any result changes, though 🎉.

@MathiasVP MathiasVP merged commit ca5916f into github:main Jan 24, 2023
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