Python: Modernize 4 queries for missing/multiple calls to init/del methods #19932
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,221 @@ | ||||||||||||
| /** Definitions for reasoning about multiple or missing calls to superclass methods. */ | ||||||||||||
|
|
||||||||||||
| import python | ||||||||||||
| import semmle.python.ApiGraphs | ||||||||||||
| import semmle.python.dataflow.new.internal.DataFlowDispatch | ||||||||||||
| import codeql.util.Option | ||||||||||||
|
|
||||||||||||
| /** Holds if `meth` is a method named `name` that transitively calls `calledMulti` of the same name via the calls `call1` and `call2`. */ | ||||||||||||
| predicate multipleCallsToSuperclassMethod( | ||||||||||||
| Function meth, Function calledMulti, DataFlow::MethodCallNode call1, | ||||||||||||
| DataFlow::MethodCallNode call2, string name | ||||||||||||
| ) { | ||||||||||||
| exists(Class cls | | ||||||||||||
| meth.getName() = name and | ||||||||||||
| meth.getScope() = cls and | ||||||||||||
| locationBefore(call1.getLocation(), call2.getLocation()) and | ||||||||||||
| calledMulti = getASuperCallTargetFromCall(cls, meth, call1, name) and | ||||||||||||
| calledMulti = getASuperCallTargetFromCall(cls, meth, call2, name) and | ||||||||||||
| nonTrivial(calledMulti) | ||||||||||||
| ) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** Holds if l1 comes before l2, assuming they're in the same file. */ | ||||||||||||
| private predicate locationBefore(Location l1, Location l2) { | ||||||||||||
| l1.getStartLine() < l2.getStartLine() | ||||||||||||
| or | ||||||||||||
| l1.getStartLine() = l2.getStartLine() and | ||||||||||||
| l1.getStartColumn() < l2.getStartColumn() | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** Gets a method transitively called by `meth` named `name` with `call` that it overrides, with `mroBase` as the type determining the MRO to search. */ | ||||||||||||
| Function getASuperCallTargetFromCall( | ||||||||||||
| Class mroBase, Function meth, DataFlow::MethodCallNode call, string name | ||||||||||||
| ) { | ||||||||||||
| exists(Function target | target = getDirectSuperCallTargetFromCall(mroBase, meth, call, name) | | ||||||||||||
| result = target | ||||||||||||
| or | ||||||||||||
| result = getASuperCallTargetFromCall(mroBase, target, _, name) | ||||||||||||
| ) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** Gets the method called by `meth` named `name` with `call`, with `mroBase` as the type determining the MRO to search. */ | ||||||||||||
| Function getDirectSuperCallTargetFromCall( | ||||||||||||
| Class mroBase, Function meth, DataFlow::MethodCallNode call, string name | ||||||||||||
| ) { | ||||||||||||
| meth = call.getScope() and | ||||||||||||
| getADirectSuperclass*(mroBase) = meth.getScope() and | ||||||||||||
| meth.getName() = name and | ||||||||||||
| call.calls(_, name) and | ||||||||||||
| mroBase = getADirectSubclass*(meth.getScope()) and | ||||||||||||
| exists(Class targetCls | | ||||||||||||
| // the differences between 0-arg and 2-arg super is not considered; we assume each super uses the mro of the instance `self` | ||||||||||||
| superCall(call, _) and | ||||||||||||
| targetCls = getNextClassInMroKnownStartingClass(meth.getScope(), mroBase) and | ||||||||||||
| result = findFunctionAccordingToMroKnownStartingClass(targetCls, mroBase, name) | ||||||||||||
| or | ||||||||||||
| // targetCls is the mro base for this lookup. | ||||||||||||
| // note however that if the call we find uses super(), that still uses the mro of the instance `self` will sill be used | ||||||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||||||
| // assuming it's 0-arg or is 2-arg with `self` as second arg. | ||||||||||||
| callsMethodOnClassWithSelf(meth, call, targetCls, _) and | ||||||||||||
| result = findFunctionAccordingToMroKnownStartingClass(targetCls, targetCls, name) | ||||||||||||
| ) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** Gets a method that is transitively called by a call to `cls.<name>`, with `mroBase` as the type determining the MRO to search. */ | ||||||||||||
| Function getASuperCallTargetFromClass(Class mroBase, Class cls, string name) { | ||||||||||||
| exists(Function target | | ||||||||||||
| target = findFunctionAccordingToMroKnownStartingClass(cls, mroBase, name) and | ||||||||||||
| ( | ||||||||||||
| result = target | ||||||||||||
| or | ||||||||||||
| result = getASuperCallTargetFromCall(mroBase, target, _, name) | ||||||||||||
| ) | ||||||||||||
| ) | ||||||||||||
| } | ||||||||||||
|
Comment on lines
+66
to
+75
There was a problem hiding this comment. This MRO business is a bit awkward. I wonder if we could clean it up by creating a new IPA type representing "the MRO starting at a particular base class", and thus avoid having to thread This is just some musing on my part -- not necessarily an immediately actionable suggestion. |
||||||||||||
|
|
||||||||||||
| /** Holds if `meth` does something besides calling a superclass method. */ | ||||||||||||
| predicate nonTrivial(Function meth) { | ||||||||||||
| exists(Stmt s | s = meth.getAStmt() | | ||||||||||||
| not s instanceof Pass and | ||||||||||||
| not exists(DataFlow::Node call | call.asExpr() = s.(ExprStmt).getValue() | | ||||||||||||
| superCall(call, meth.getName()) | ||||||||||||
| or | ||||||||||||
| callsMethodOnClassWithSelf(meth, call, _, meth.getName()) | ||||||||||||
| ) | ||||||||||||
| ) and | ||||||||||||
| exists(meth.getANormalExit()) // doesn't always raise an exception | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** Holds if `call` is a call to `super().<name>`. No distinction is made between 0- and 2- arg super calls. */ | ||||||||||||
| predicate superCall(DataFlow::MethodCallNode call, string name) { | ||||||||||||
| exists(DataFlow::Node sup | | ||||||||||||
| call.calls(sup, name) and | ||||||||||||
| sup = API::builtin("super").getACall() | ||||||||||||
| ) | ||||||||||||
|
Comment on lines
+92
to
+95
There was a problem hiding this comment.
Suggested change
|
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** Holds if `meth` calls `super().<name>` where `name` is the name of the method. */ | ||||||||||||
|
There was a problem hiding this comment. Comment needs updating? I don't see |
||||||||||||
| predicate callsSuper(Function meth) { | ||||||||||||
| exists(DataFlow::MethodCallNode call | | ||||||||||||
| call.getScope() = meth and | ||||||||||||
| superCall(call, meth.getName()) | ||||||||||||
| ) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** Holds if `meth` calls `target.<name>(self, ...)` with the call `call`. */ | ||||||||||||
| predicate callsMethodOnClassWithSelf( | ||||||||||||
| Function meth, DataFlow::MethodCallNode call, Class target, string name | ||||||||||||
| ) { | ||||||||||||
| exists(DataFlow::Node callTarget, DataFlow::ParameterNode self | | ||||||||||||
| call.calls(callTarget, name) and | ||||||||||||
| self.getParameter() = meth.getArg(0) and | ||||||||||||
| self.(DataFlow::LocalSourceNode).flowsTo(call.getArg(0)) and | ||||||||||||
| callTarget = classTracker(target) | ||||||||||||
| ) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** Holds if `meth` calls a method named `name` passing its `self` argument as its first parameter, but the class it refers to is unknown. */ | ||||||||||||
| predicate callsMethodOnUnknownClassWithSelf(Function meth, string name) { | ||||||||||||
| exists(DataFlow::MethodCallNode call, DataFlow::Node callTarget, DataFlow::ParameterNode self | | ||||||||||||
| call.calls(callTarget, name) and | ||||||||||||
| self.getParameter() = meth.getArg(0) and | ||||||||||||
| self.(DataFlow::LocalSourceNode).flowsTo(call.getArg(0)) and | ||||||||||||
| not exists(Class target | callTarget = classTracker(target)) | ||||||||||||
|
||||||||||||
| not exists(Class target | callTarget = classTracker(target)) | |
| not callTarget = classTracker(any(Class target)) |
Could also write it in this way.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
| <overview> | ||
| <p> | ||
| Python, unlike some other object-oriented languages such as Java, allows the developer complete freedom in | ||
| when and how superclass finalizers are called during object finalization. | ||
| However, the developer has responsibility for ensuring that objects are properly cleaned up, and that all superclass <code>__del__</code> | ||
| methods are called. | ||
| </p> | ||
| <p> | ||
| Classes with a <code>__del__</code> method (a finalizer) typically hold some resource such as a file handle that needs to be cleaned up. | ||
| If the <code>__del__</code> method of a superclass is not called during object finalization, it is likely that | ||
| resources may be leaked. | ||
| </p> | ||
|
|
||
| <p>A call to the <code>__init__</code> method of a superclass during object initialization may be unintentionally skipped: | ||
| </p> | ||
| <ul> | ||
| <li>If a subclass calls the <code>__del__</code> method of the wrong class.</li> | ||
| <li>If a call to the <code>__del__</code> method of one its base classes is omitted.</li> | ||
| <li>If a call to <code>super().__del__</code> is used, but not all <code>__del__</code> methods in the Method Resolution Order (MRO) | ||
| chain themselves call <code>super()</code>. This in particular arises more often in cases of multiple inheritance. </li> | ||
| </ul> | ||
|
|
||
|
|
||
| </overview> | ||
| <recommendation> | ||
| <p>Ensure that all superclass <code>__del__</code> methods are properly called. | ||
| Either each base class's finalize method should be explicitly called, or <code>super()</code> calls | ||
| should be consistently used throughout the inheritance hierarchy.</p> | ||
|
|
||
|
|
||
| </recommendation> | ||
| <example> | ||
| <p>In the following example, explicit calls to <code>__del__</code> are used, but <code>SportsCar</code> erroneously calls | ||
| <code>Vehicle.__del__</code>. This is fixed in <code>FixedSportsCar</code> by calling <code>Car.__del__</code>. | ||
| </p> | ||
|
|
||
| <sample src="examples/MissingCallToDel.py" /> | ||
|
|
||
| </example> | ||
| <references> | ||
|
|
||
| <li>Python Reference: <a href="https://docs.python.org/3/reference/datamodel.html#object.__del__">__del__</a>.</li> | ||
| <li>Python Standard Library: <a href="https://docs.python.org/3/library/functions.html#super">super</a>.</li> | ||
| <li>Python Glossary: <a href="https://docs.python.org/3/glossary.html#term-method-resolution-order">Method resolution order</a>.</li> | ||
|
|
||
| </references> | ||
| </qhelp> |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like either
or
is redundant.