Python: Hide points-to imports in python.qll
#20739
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Moves the existing points-to predicates to the newly added class `ControlFlowNodeWithPointsTo` which resides in the `LegacyPointsTo` module. (Existing code that uses these predicates should import this module, and references to `ControlFlowNode` should be changed to `ControlFlowNodeWithPointsTo`.) Also updates all existing points-to based code to do just this.
This had only two uses in our libraries, so I simply inlined the predicate body in both places.
I wasn't entirely sure if this should be classified as `deprecated` or `breaking`, but seeing as these changes technically _could_ break existing queries (requiring a small rewrite), I opted for the latter.
For now, these have just been made into `private` imports. After doing
this, I went through all of the (now not compiling) files and added in
private imports to the modules that they actually depended on.
I also added an explicit import of `LegacyPointsTo` (even though it may
be unnecessary) in cases where the points-to dependency was somewhat
surprising (and one we want to get rid of). This was primarily inside
the various SSA layers.
For modules inside `semmle.python.{types, objects, pointsto}` I did not
bother, as these are fairly clearly related to points-to.
This frees `Class.qll`, `Exprs.qll`, and `Function.qll` from the clutches of points-to. For the somewhat complicated setup with `getLiteralObject` (an abstract method), I opted for a slightly ugly but workable solution of just defining a predicate on `ImmutableLiteral` that inlines each predicate body, special-cased to the specific instance to which it applies.
These methods were in `pointsto.Base` but did not actually interact with the points-to machinery directly, so they were easy to move out.
One might argue that these should be rewritten entirely to use more modern APIs, but for now I'll be content with just having them compile properly.
Mostly just adding `private import LegacyPointsTo`. Sometimes getting rid of other imports that are superceded by that module.
Gets rid of a bunch of predicates relating to reachability (which depended on the modelling of exceptions, which uses points-to), moving them to `LegacyPointsTo`. In the process, we gained a new class `BasicBlockWithPointsTo`.
Turns out the `ImportTime` module (despite living in `semmle.python.types` does not actually depend on points-to, so some of the `LegacyPointsTo` imports could be replaced or removed.
For whatever reason, the CFG node for exceptions and exception groups was placed with the points-to code. (Probably because a lot of the predicates depended on points-to.) However, as it turned out, two of the SSA modules only depended on non-points-to properties of these nodes, and so it was fairly straightforward to remove the imports of `LegacyPointsTo` for those modules. In the process, I moved the aforementioned CFG node types into `Flow.qll`, and changed the classes in the `Exceptions` module to the `...WithPointsTo` form that we introduced elsewhere.
The `Builtins` module is deeply entwined with points-to, so it would be nice to not have this dependence. Happily, the only thing we used `Builtin` for was to get the names of known builtins, and for this we already maintain such a set of names in `dataflow.new.internal.Builtins`.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import semmle.python.pointsto.Context | ||
| import semmle.python.pointsto.PointsTo | ||
| import semmle.python.pointsto.PointsToContext | ||
| import semmle.python.objects.Modules |
Check warning
Code scanning / CodeQL
Redundant import Warning
| @@ -1,4 +1,8 @@ | |||
| import python | |||
| private import LegacyPointsTo | |||
| private import semmle.python.types.FunctionObject | |||
Check warning
Code scanning / CodeQL
Redundant import Warning
| import python | ||
| private import LegacyPointsTo | ||
| private import semmle.python.types.FunctionObject | ||
| private import semmle.python.types.ClassObject |
Check warning
Code scanning / CodeQL
Redundant import Warning
| private import LegacyPointsTo | ||
| private import semmle.python.types.FunctionObject | ||
| private import semmle.python.types.ClassObject | ||
| private import semmle.python.SelfAttribute |
Check warning
Code scanning / CodeQL
Redundant import Warning
| private import semmle.python.types.FunctionObject | ||
| private import semmle.python.types.ClassObject | ||
| private import semmle.python.pointsto.Base | ||
| private import semmle.python.pointsto.Context |
Check warning
Code scanning / CodeQL
Redundant import Warning
| private import semmle.python.types.Builtins | ||
| private import semmle.python.objects.ObjectInternal | ||
| private import semmle.python.types.Object | ||
| private import semmle.python.types.FunctionObject |
Check warning
Code scanning / CodeQL
Redundant import Warning
| private import semmle.python.objects.ObjectInternal | ||
| private import semmle.python.types.Object | ||
| private import semmle.python.types.FunctionObject | ||
| private import semmle.python.SelfAttribute |
Check warning
Code scanning / CodeQL
Redundant import Warning
| private import LegacyPointsTo | ||
| private import semmle.python.types.Builtins | ||
| private import semmle.python.internal.CachedStages | ||
| private import semmle.python.types.Descriptors |
Check warning
Code scanning / CodeQL
Redundant import Warning
| private import semmle.python.types.Builtins | ||
| private import semmle.python.internal.CachedStages | ||
| private import semmle.python.types.Descriptors | ||
| private import semmle.python.pointsto.Base |
Check warning
Code scanning / CodeQL
Redundant import Warning
| not exists(ExprWithPointsTo left, ExprWithPointsTo right, Value val | | ||
| comp.compares(left, op, right) and | ||
| exists(ImmutableLiteral il | il.getLiteralValue() = val) | ||
| exists(ImmutableLiteral il | il = val.(ConstantObjectInternal).getLiteral()) |
Check warning
Code scanning / CodeQL
Expression can be replaced with a cast Warning
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WIP