Skip to content

Ruby: TypeTracker: add smallstep for functions that return their arguments #8302

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 7, 2022
Merged

Ruby: TypeTracker: add smallstep for functions that return their arguments #8302

merged 3 commits into from
Mar 7, 2022

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Mar 1, 2022 •
edited
Loading

Depends on #8317

@aibaars aibaars requested a review from a team as a code owner March 1, 2022 16:42
@aibaars aibaars changed the title Ruby: TypeTracker: add smallstep for functions that return their argu… Ruby: TypeTracker: add smallstep for functions that return their arguments Mar 1, 2022
@github-actions github-actions bot added the Ruby label Mar 1, 2022
asgerf
asgerf reviewed Mar 1, 2022
View reviewed changes
ruby/ql/lib/codeql/ruby/typetracking/TypeTrackerSpecific.qll Outdated
@@ -13,7 +13,18 @@ class TypeTrackingNode = DataFlowPublic::LocalSourceNode;

predicate simpleLocalFlowStep = DataFlowPrivate::localFlowStepTypeTracker/2;

predicate jumpStep = DataFlowPrivate::jumpStep/2;
predicate jumpStep(Node nodeFrom, TypeTrackingNode nodeTo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be a jump step, it should be local.

Please correct me if I'm wrong, but as I understand it, a "jump" step is one that resets the calling context (allowing flow out of a different caller than where it came in). Any reason to allow that here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just (ab)used the jumpStep predicate because that has a LevelStep summary in smallstepNoCall. Perhaps it would be better to introduce a new predicate (localStep ?) in the shared TypeTracking library.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that is should be part of simpleLocalFlowStep above instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it should perhaps be part of localFlowStepTypeTracker instead, since then it will work recursively (i.e., a method that returns one of its arguments via a call to another method that also does). This means such summarized steps will also be included in LocalSourceNode::flowsTo, which I guess we want?

Note that if we do this, we need to add a new disjunct to isLocalSourceNode which includes all nodes that correspond to a ExprNodes::CallCfgNode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hvitved I added the new step to simpleLocalFlowStep. Somehow this changes the expected output back to the previous version. Not sure if that is desirable or not. I am not sure if/how this can be added to localFlowStepTypeTracker instead. I ran into some non-monotonic recursion errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I have given this some more thought, and I now understand why @aibaars used jumpStep, namely because it uses LevelStep. So unless we change the interface to the shared type tracking library, I think that is the best we can do for now. I will revert my changes and go back to using jumpStep.

Copy link
Contributor

Choose a reason for hiding this comment

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

With #8317 we should change it to using levelStep instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hvitved Shall we close this pull request, or rebase it onto your work in #8317 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rebase once that PR is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aibaars : That PR has now been merged, so let's rebase this PR and use the newly introduced levelStep.

hvitved
hvitved requested changes Mar 2, 2022
View reviewed changes
ruby/ql/lib/codeql/ruby/typetracking/TypeTrackerSpecific.qll Outdated
@@ -13,7 +13,18 @@ class TypeTrackingNode = DataFlowPublic::LocalSourceNode;

predicate simpleLocalFlowStep = DataFlowPrivate::localFlowStepTypeTracker/2;

predicate jumpStep = DataFlowPrivate::jumpStep/2;
predicate jumpStep(Node nodeFrom, TypeTrackingNode nodeTo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that is should be part of simpleLocalFlowStep above instead.

ruby/ql/lib/codeql/ruby/typetracking/TypeTrackerSpecific.qll Outdated
@@ -13,7 +13,18 @@ class TypeTrackingNode = DataFlowPublic::LocalSourceNode;

predicate simpleLocalFlowStep = DataFlowPrivate::localFlowStepTypeTracker/2;

predicate jumpStep = DataFlowPrivate::jumpStep/2;
predicate jumpStep(Node nodeFrom, TypeTrackingNode nodeTo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it should perhaps be part of localFlowStepTypeTracker instead, since then it will work recursively (i.e., a method that returns one of its arguments via a call to another method that also does). This means such summarized steps will also be included in LocalSourceNode::flowsTo, which I guess we want?

Note that if we do this, we need to add a new disjunct to isLocalSourceNode which includes all nodes that correspond to a ExprNodes::CallCfgNode.

hvitved
hvitved reviewed Mar 2, 2022
View reviewed changes
@aibaars aibaars added the no-change-note-required This PR does not need a change note label Mar 2, 2022
@aibaars aibaars marked this pull request as draft March 3, 2022 13:44
@aibaars aibaars force-pushed the type-tracking-smallstep branch from 4c742cc to f5784a7 Compare March 3, 2022 13:45
@github-actions github-actions bot added the Python label Mar 3, 2022
@aibaars aibaars force-pushed the type-tracking-smallstep branch 2 times, most recently from 8f25750 to e55dd64 Compare March 3, 2022 17:05
@aibaars aibaars force-pushed the type-tracking-smallstep branch from e55dd64 to 200a965 Compare March 7, 2022 10:52
@aibaars aibaars marked this pull request as ready for review March 7, 2022 10:52
hvitved
hvitved approved these changes Mar 7, 2022
View reviewed changes
Copy link
Contributor

@hvitved hvitved 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 let's wait for DCA to confirm that performance is not worsened.

@hvitved hvitved merged commit 6aad8d6 into github:main Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asgerf asgerf asgerf left review comments

@hvitved hvitved hvitved approved these changes

Assignees
No one assigned
Labels
no-change-note-required This PR does not need a change note Python Ruby
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aibaars @asgerf @hvitved