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.

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

Python: Adopt shared type tracking library #14848

Merged
merged 6 commits into from
Dec 18, 2023
Merged

Python: Adopt shared type tracking library #14848

merged 6 commits into from
Dec 18, 2023

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Nov 20, 2023 •
edited
Loading

This PR adopts the shared type tracking library in Python, just like we recently did for Ruby. Unlike the Ruby PR, I have made no attempt at enabling consistency checks (and fixing inconsistencies), and I had to add a hook to the shared library for using a custom flowsTo predicate, which ideally shouldn't be needed. I'll let it be up to the @github/codeql-python team to investigate both aspects follow-up.

github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 20, 2023
View reviewed changes
python/ql/test/experimental/dataflow/summaries/TestSummaries.qll
@@ -8,7 +8,7 @@
* `getACall` predicate on `SummarizedCallable`.
*/
module RecursionGuard {
private import semmle.python.dataflow.new.internal.TypeTrackerSpecific as TT
private import semmle.python.dataflow.new.internal.TypeTrackingImpl::TypeTrackingInput as TT

Check warning

Code scanning / CodeQL

Names only differing by case Warning test

TT is only different by casing from Tt that is used elsewhere for modules.
python/ql/test/experimental/dataflow/typetracking-summaries/TestSummaries.qll
@@ -8,7 +8,7 @@
* `getACall` predicate on `SummarizedCallable`.
*/
module RecursionGuard {
private import semmle.python.dataflow.new.internal.TypeTrackerSpecific as TT
private import semmle.python.dataflow.new.internal.TypeTrackingImpl::TypeTrackingInput as TT

Check warning

Code scanning / CodeQL

Names only differing by case Warning test

TT is only different by casing from Tt that is used elsewhere for modules.
@hvitved hvitved force-pushed the python/shared-type-tracking branch from 5e804c2 to 83c7959 Compare November 20, 2023 19:01
@hvitved hvitved force-pushed the python/shared-type-tracking branch 5 times, most recently from a9899ec to fd3ea14 Compare December 7, 2023 08:19
@hvitved hvitved marked this pull request as ready for review December 7, 2023 13:05
@hvitved hvitved requested review from a team as code owners December 7, 2023 13:05
@hvitved hvitved force-pushed the python/shared-type-tracking branch from fd3ea14 to 6fc9e61 Compare December 14, 2023 12:25
yoff
yoff reviewed Dec 18, 2023
View reviewed changes
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

I can see why we need a non-standard local-step relation, as we are filtering out steps to module variable nodes. Perhaps we should be doing that in a different place. We are also using a richer local-step relation than the one for type tracking to compute local source nodes. I wonder if that is actually a mistake...

One question about deprecation, and then a few more to satisfy my own curiosity, but this looks generally fine.

shared/typetracking/codeql/typetracking/internal/TypeTrackingImpl.qll
cached
predicate flowsTo(Node localSource, Node dst) {
predicate standardFlowsTo(Node localSource, Node dst) {
not nonStandardFlowsTo(_, _) and
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this logic (no standardFlowsTo if nonStandardFlowsTo is non-empty) would be more clearly visible in the flowsTo predicate. That might be less performant, though, if standardFlowsTo is then computed for no reason. Is that why it is 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.

Indeed, that's why it's there.

python/ql/lib/semmle/python/dataflow/new/internal/LocalSources.qll
Comment on lines +75 to +76
or
this instanceof ParameterNode
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, I am surprised we did not have this before!

I am curious, did you observe test-failures from this?
Given the first case

this instanceof ExprNode and
    not simpleLocalFlowStepForTypetracking(_, this)

this will only add synthetic parameter nodes (I also verified this with a quick query), but there can still be lots of those...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, I am surprised we did not have this before!

Me too.

I am curious, did you observe test-failures from this?

The shared type tracking library (sensibly) assumes parameters to be local source nodes:

codeql/shared/typetracking/codeql/typetracking/TypeTracking.qll

Line 67 in 50b754b

predicate callStep(Node nodeFrom, LocalSourceNode nodeTo);

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah!

python/ql/lib/semmle/python/dataflow/new/internal/LocalSources.qll
@@ -151,7 +153,7 @@ class LocalSourceNode extends Node {
* See `TypeBackTracker` for more details about how to use this.
*/
pragma[inline]
LocalSourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t2 = t.step(result, this) }
LocalSourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t = t2.step(result, this) }
Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes me wonder if the QLDoc for track and backtrack ought to mention t and t2. But I guess now it is at least aligned with other language implementations.

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 don't think the QL doc changed here?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, and even if it had mentioned t and t2 it would not have to change. It just makes it clear that there are two possible conventions here and we have two implementations that made different choices. So we should probably make explicit what role t and t2 are supposed to play. It is tangential to this PR, though.

python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackerSpecific.qll Outdated
or
capturedJumpStep(nodeFrom, nodeTo)
}
predicate simpleLocalFlowStep = TypeTrackingImpl::TypeTrackingInput::simpleLocalSmallStep/2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these not also be deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll deprecate those as well.

yoff
yoff approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

LGTM

@yoff yoff merged commit e0c027f into github:main Dec 18, 2023
26 checks passed
@hvitved hvitved deleted the python/shared-type-tracking branch December 19, 2023 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yoff yoff yoff approved these changes

Assignees

@yoff yoff

Labels
documentation Python Ruby
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hvitved @yoff