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++: Make swap member functions data-flow functions #17351

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

paldepind
Copy link
Contributor

@paldepind paldepind commented Sep 3, 2024

This PR makes swap member functions DataFlowFunctions instead of TaintFunctions.

This causes some good results in the AST based taint tracking to disappear. We'll have to do some more investigation to figure out why that is the case. I've not updated the tests to expect this regression, so CI will fail.

@github-actions github-actions bot added the C++ label Sep 3, 2024
@@ -68,7 +68,7 @@ void test_pair()
i.swap(j);
k.swap(l);
sink(i.first);
sink(i.second); // $ MISSING: ast,ir
sink(i.second); // $ ir, MISSING: ast
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shows that the change is doing what we want for the IR based taint tracking.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

LGTM, but it looks like you still need to accept changes to some other tests to get CI passing.

@paldepind
Copy link
Contributor Author

I've figured out why we loose some of the edges in the AST-based taint tracking. It turns out that some cases related to member functions are handled for TaintFunctions but not for DataFlowFunctions. For instance, the disjunct here allows for this to flow to an argument for a TaintFunction, but a similar case does not exist earlier in the predicate for DataFlowFunctions. A similar asymmetry exist in other predicates in that file.

I've quickly tried adding some of these extra cases for DataFlowFunctions and this does fix the missing AST-based flow. But, given that the library is deprecated and subject for removal it doesn't seem like this is worth fixing (a fix that could have other ramifications perhaps). So now that we know the reason I think it's ok to keep the PR as is and accept the lost edges.

@paldepind paldepind force-pushed the swap-member-data-flow branch from ff236f1 to 9ae16be Compare September 4, 2024 10:47
@paldepind paldepind marked this pull request as ready for review September 4, 2024 11:53
@paldepind paldepind requested a review from a team as a code owner September 4, 2024 11:53
@paldepind paldepind force-pushed the swap-member-data-flow branch from 9ae16be to f066f21 Compare September 4, 2024 11:55
@paldepind paldepind requested a review from geoffw0 September 4, 2024 11:57
@geoffw0
Copy link
Contributor

geoffw0 commented Sep 4, 2024

a fix that could have other ramifications perhaps

Since the library is deprecated we should no longer use it in any of our queries, so there should be no ramifications on results in our query suites and I think no ramifications on performance either. It could well affect users who are still using the deprecated libraries though. We generally expect users to move away from deprecated libraries within 1 year at which point we delete them.

I agree it probably isn't worth fixing, though I'm not strongly against it if you want to.

@paldepind
Copy link
Contributor Author

Since the library is deprecated we should no longer use it in any of our queries, so there should be no ramifications on results in our query suites and I think no ramifications on performance either. It could well affect users who are still using the deprecated libraries though. We generally expect users to move away from deprecated libraries within 1 year at which point we delete them.

Makes sense.

I agree it probably isn't worth fixing, though I'm not strongly against it if you want to.

Let's keep it as is then 👍

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

The force-push made this a little harder to re-review than it should have been, but LGTM.

@paldepind
Copy link
Contributor Author

Thanks for the review and sorry about force pushing. I was thinking that as the PR was previously marked as a draft I could get away with it. But it makes sense to not do that given that you'd already reviewed 👍

@paldepind paldepind merged commit 5950af3 into github:main Sep 5, 2024
15 checks passed
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.

2 participants