Skip to content

Commit 869b7e0

Browse files
Merge pull request #19932 from joefarebrother/python-qual-init-del-calls
Python: Modernize 4 queries for missing/multiple calls to init/del methods
2 parents 8c34b7e + cd6a151 commit 869b7e0

File tree

49 files changed

+955
-607
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

49 files changed

+955
-607
lines changed

python/ql/integration-tests/query-suite/python-code-quality-extended.qls.expected

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
1+
ql/python/ql/src/Classes/CallsToInitDel/MissingCallToDel.ql
2+
ql/python/ql/src/Classes/CallsToInitDel/MissingCallToInit.ql
3+
ql/python/ql/src/Classes/CallsToInitDel/SuperclassDelCalledMultipleTimes.ql
4+
ql/python/ql/src/Classes/CallsToInitDel/SuperclassInitCalledMultipleTimes.ql
15
ql/python/ql/src/Classes/Comparisons/EqualsOrHash.ql
26
ql/python/ql/src/Classes/Comparisons/EqualsOrNotEquals.ql
37
ql/python/ql/src/Classes/Comparisons/IncompleteOrdering.ql
48
ql/python/ql/src/Classes/ConflictingAttributesInBaseClasses.ql
59
ql/python/ql/src/Classes/DefineEqualsWhenAddingAttributes.ql
610
ql/python/ql/src/Classes/InconsistentMRO.ql
711
ql/python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql
8-
ql/python/ql/src/Classes/MissingCallToDel.ql
9-
ql/python/ql/src/Classes/MissingCallToInit.ql
1012
ql/python/ql/src/Classes/MutatingDescriptor.ql
1113
ql/python/ql/src/Classes/SubclassShadowing/SubclassShadowing.ql
12-
ql/python/ql/src/Classes/SuperclassDelCalledMultipleTimes.ql
13-
ql/python/ql/src/Classes/SuperclassInitCalledMultipleTimes.ql
1414
ql/python/ql/src/Classes/WrongNameForArgumentInClassInstantiation.ql
1515
ql/python/ql/src/Classes/WrongNumberArgumentsInClassInstantiation.ql
1616
ql/python/ql/src/Exceptions/CatchingBaseException.ql

python/ql/integration-tests/query-suite/python-code-quality.qls.expected

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
1+
ql/python/ql/src/Classes/CallsToInitDel/MissingCallToDel.ql
2+
ql/python/ql/src/Classes/CallsToInitDel/MissingCallToInit.ql
3+
ql/python/ql/src/Classes/CallsToInitDel/SuperclassDelCalledMultipleTimes.ql
4+
ql/python/ql/src/Classes/CallsToInitDel/SuperclassInitCalledMultipleTimes.ql
15
ql/python/ql/src/Classes/Comparisons/EqualsOrHash.ql
26
ql/python/ql/src/Classes/Comparisons/EqualsOrNotEquals.ql
37
ql/python/ql/src/Classes/Comparisons/IncompleteOrdering.ql
48
ql/python/ql/src/Classes/ConflictingAttributesInBaseClasses.ql
59
ql/python/ql/src/Classes/DefineEqualsWhenAddingAttributes.ql
610
ql/python/ql/src/Classes/InconsistentMRO.ql
711
ql/python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql
8-
ql/python/ql/src/Classes/MissingCallToDel.ql
9-
ql/python/ql/src/Classes/MissingCallToInit.ql
1012
ql/python/ql/src/Classes/MutatingDescriptor.ql
1113
ql/python/ql/src/Classes/SubclassShadowing/SubclassShadowing.ql
12-
ql/python/ql/src/Classes/SuperclassDelCalledMultipleTimes.ql
13-
ql/python/ql/src/Classes/SuperclassInitCalledMultipleTimes.ql
1414
ql/python/ql/src/Classes/WrongNameForArgumentInClassInstantiation.ql
1515
ql/python/ql/src/Classes/WrongNumberArgumentsInClassInstantiation.ql
1616
ql/python/ql/src/Exceptions/CatchingBaseException.ql

python/ql/integration-tests/query-suite/python-security-and-quality.qls.expected

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
1+
ql/python/ql/src/Classes/CallsToInitDel/MissingCallToDel.ql
2+
ql/python/ql/src/Classes/CallsToInitDel/MissingCallToInit.ql
3+
ql/python/ql/src/Classes/CallsToInitDel/SuperclassDelCalledMultipleTimes.ql
4+
ql/python/ql/src/Classes/CallsToInitDel/SuperclassInitCalledMultipleTimes.ql
15
ql/python/ql/src/Classes/Comparisons/EqualsOrHash.ql
26
ql/python/ql/src/Classes/Comparisons/EqualsOrNotEquals.ql
37
ql/python/ql/src/Classes/Comparisons/IncompleteOrdering.ql
48
ql/python/ql/src/Classes/ConflictingAttributesInBaseClasses.ql
59
ql/python/ql/src/Classes/DefineEqualsWhenAddingAttributes.ql
610
ql/python/ql/src/Classes/InconsistentMRO.ql
711
ql/python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql
8-
ql/python/ql/src/Classes/MissingCallToDel.ql
9-
ql/python/ql/src/Classes/MissingCallToInit.ql
1012
ql/python/ql/src/Classes/MutatingDescriptor.ql
1113
ql/python/ql/src/Classes/OverwritingAttributeInSuperClass.ql
1214
ql/python/ql/src/Classes/PropertyInOldStyleClass.ql
1315
ql/python/ql/src/Classes/SlotsInOldStyleClass.ql
1416
ql/python/ql/src/Classes/SubclassShadowing/SubclassShadowing.ql
1517
ql/python/ql/src/Classes/SuperInOldStyleClass.ql
16-
ql/python/ql/src/Classes/SuperclassDelCalledMultipleTimes.ql
17-
ql/python/ql/src/Classes/SuperclassInitCalledMultipleTimes.ql
1818
ql/python/ql/src/Classes/WrongNameForArgumentInClassInstantiation.ql
1919
ql/python/ql/src/Classes/WrongNumberArgumentsInClassInstantiation.ql
2020
ql/python/ql/src/Diagnostics/ExtractedFiles.ql

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -856,9 +856,14 @@ Class getNextClassInMroKnownStartingClass(Class cls, Class startingClass) {
856856
)
857857
}
858858

859-
private Function findFunctionAccordingToMroKnownStartingClass(
860-
Class cls, Class startingClass, string name
861-
) {
859+
/**
860+
* Gets a potential definition of the function `name` of the class `cls` according to our approximation of
861+
* MRO for the class `startingCls` (see `getNextClassInMroKnownStartingClass` for more information).
862+
*
863+
* Note: this is almost the same as `findFunctionAccordingToMro`, except we know the
864+
* `startingClass`, which can give slightly more precise results.
865+
*/
866+
Function findFunctionAccordingToMroKnownStartingClass(Class cls, Class startingClass, string name) {
862867
result = cls.getAMethod() and
863868
result.getName() = name and
864869
cls = getADirectSuperclass*(startingClass)
@@ -871,7 +876,7 @@ private Function findFunctionAccordingToMroKnownStartingClass(
871876

872877
/**
873878
* Gets a potential definition of the function `name` according to our approximation of
874-
* MRO for the class `cls` (see `getNextClassInMroKnownStartingClass` for more information).
879+
* MRO for the class `startingCls` (see `getNextClassInMroKnownStartingClass` for more information).
875880
*
876881
* Note: this is almost the same as `findFunctionAccordingToMro`, except we know the
877882
* `startingClass`, which can give slightly more precise results.
Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,207 @@
1+
/** Definitions for reasoning about multiple or missing calls to superclass methods. */
2+
3+
import python
4+
import semmle.python.ApiGraphs
5+
import semmle.python.dataflow.new.internal.DataFlowDispatch
6+
import codeql.util.Option
7+
8+
/** Holds if `meth` is a method named `name` that transitively calls `calledMulti` of the same name via the calls `call1` and `call2`. */
9+
predicate multipleCallsToSuperclassMethod(
10+
Function meth, Function calledMulti, DataFlow::MethodCallNode call1,
11+
DataFlow::MethodCallNode call2, string name
12+
) {
13+
exists(Class cls |
14+
meth.getName() = name and
15+
meth.getScope() = cls and
16+
locationBefore(call1.getLocation(), call2.getLocation()) and
17+
rankedSuperCallByLocation(1, cls, meth, call1, name, calledMulti) and
18+
rankedSuperCallByLocation(2, cls, meth, call2, name, calledMulti) and
19+
nonTrivial(calledMulti)
20+
)
21+
}
22+
23+
predicate rankedSuperCallByLocation(
24+
int i, Class mroBase, Function meth, DataFlow::MethodCallNode call, string name, Function target
25+
) {
26+
call =
27+
rank[i](DataFlow::MethodCallNode calli |
28+
target = getASuperCallTargetFromCall(mroBase, meth, calli, name)
29+
|
30+
calli order by calli.getLocation().getStartLine(), calli.getLocation().getStartColumn()
31+
)
32+
}
33+
34+
/** Holds if l1 comes before l2, assuming they're in the same file. */
35+
pragma[inline]
36+
private predicate locationBefore(Location l1, Location l2) {
37+
l1.getStartLine() < l2.getStartLine()
38+
or
39+
l1.getStartLine() = l2.getStartLine() and
40+
l1.getStartColumn() < l2.getStartColumn()
41+
}
42+
43+
/** Gets a method transitively called by `meth` named `name` with `call` that it overrides, with `mroBase` as the type determining the MRO to search. */
44+
Function getASuperCallTargetFromCall(
45+
Class mroBase, Function meth, DataFlow::MethodCallNode call, string name
46+
) {
47+
exists(Function target | target = getDirectSuperCallTargetFromCall(mroBase, meth, call, name) |
48+
result = target
49+
or
50+
result = getASuperCallTargetFromCall(mroBase, target, _, name)
51+
)
52+
}
53+
54+
/** Gets the method called by `meth` named `name` with `call`, with `mroBase` as the type determining the MRO to search. */
55+
Function getDirectSuperCallTargetFromCall(
56+
Class mroBase, Function meth, DataFlow::MethodCallNode call, string name
57+
) {
58+
meth = call.getScope() and
59+
meth.getName() = name and
60+
call.calls(_, name) and
61+
mroBase = getADirectSubclass*(meth.getScope()) and
62+
exists(Class targetCls |
63+
// the differences between 0-arg and 2-arg super is not considered; we assume each super uses the mro of the instance `self`
64+
superCall(call, _) and
65+
targetCls = getNextClassInMroKnownStartingClass(meth.getScope(), mroBase) and
66+
result = findFunctionAccordingToMroKnownStartingClass(targetCls, mroBase, name)
67+
or
68+
// targetCls is the mro base for this lookup.
69+
// note however that if the call we find uses super(), that still uses the mro of the instance `self`
70+
// assuming it's 0-arg or is 2-arg with `self` as second arg.
71+
callsMethodOnClassWithSelf(meth, call, targetCls, _) and
72+
result = findFunctionAccordingToMroKnownStartingClass(targetCls, targetCls, name)
73+
)
74+
}
75+
76+
/** Gets a method that is transitively called by a call to `cls.<name>`, with `mroBase` as the type determining the MRO to search. */
77+
Function getASuperCallTargetFromClass(Class mroBase, Class cls, string name) {
78+
exists(Function target |
79+
target = findFunctionAccordingToMroKnownStartingClass(cls, mroBase, name) and
80+
(
81+
result = target
82+
or
83+
result = getASuperCallTargetFromCall(mroBase, target, _, name)
84+
)
85+
)
86+
}
87+
88+
/** Holds if `meth` does something besides calling a superclass method. */
89+
predicate nonTrivial(Function meth) {
90+
exists(Stmt s | s = meth.getAStmt() |
91+
not s instanceof Pass and
92+
not exists(DataFlow::Node call | call.asExpr() = s.(ExprStmt).getValue() |
93+
superCall(call, meth.getName())
94+
or
95+
callsMethodOnClassWithSelf(meth, call, _, meth.getName())
96+
)
97+
) and
98+
exists(meth.getANormalExit()) // doesn't always raise an exception
99+
}
100+
101+
/** Holds if `call` is a call to `super().<name>`. No distinction is made between 0- and 2- arg super calls. */
102+
predicate superCall(DataFlow::MethodCallNode call, string name) {
103+
exists(DataFlow::Node sup |
104+
call.calls(sup, name) and
105+
sup = API::builtin("super").getACall()
106+
)
107+
}
108+
109+
/** Holds if `meth` calls a `super()` method of the same name. */
110+
predicate callsSuper(Function meth) {
111+
exists(DataFlow::MethodCallNode call |
112+
call.getScope() = meth and
113+
superCall(call, meth.getName())
114+
)
115+
}
116+
117+
/** Holds if `meth` calls `target.<name>(self, ...)` with the call `call`. */
118+
predicate callsMethodOnClassWithSelf(
119+
Function meth, DataFlow::MethodCallNode call, Class target, string name
120+
) {
121+
exists(DataFlow::Node callTarget, DataFlow::ParameterNode self |
122+
call.calls(callTarget, name) and
123+
self.getParameter() = meth.getArg(0) and
124+
self.(DataFlow::LocalSourceNode).flowsTo(call.getArg(0)) and
125+
callTarget = classTracker(target)
126+
)
127+
}
128+
129+
/** Holds if `meth` calls a method named `name` passing its `self` argument as its first parameter, but the class it refers to is unknown. */
130+
predicate callsMethodOnUnknownClassWithSelf(Function meth, string name) {
131+
exists(DataFlow::MethodCallNode call, DataFlow::Node callTarget, DataFlow::ParameterNode self |
132+
call.calls(callTarget, name) and
133+
self.getParameter() = meth.getArg(0) and
134+
self.(DataFlow::LocalSourceNode).flowsTo(call.getArg(0)) and
135+
not callTarget = classTracker(any(Class target))
136+
)
137+
}
138+
139+
/** Holds if `base` does not call a superclass method `shouldCall` named `name` when it appears it should. */
140+
predicate missingCallToSuperclassMethod(Class base, Function shouldCall, string name) {
141+
shouldCall.getName() = name and
142+
shouldCall.getScope() = getADirectSuperclass+(base) and
143+
not shouldCall = getASuperCallTargetFromClass(base, base, name) and
144+
nonTrivial(shouldCall) and
145+
// "Benefit of the doubt" - if somewhere in the chain we call an unknown superclass, assume all the necessary parent methods are called from it
146+
not callsMethodOnUnknownClassWithSelf(getASuperCallTargetFromClass(base, base, name), name)
147+
}
148+
149+
/**
150+
* Holds if `base` does not call a superclass method `shouldCall` named `name` when it appears it should.
151+
* Results are restricted to hold only for the highest `base` class and the lowest `shouldCall` method in the hierarchy for which this applies.
152+
*/
153+
predicate missingCallToSuperclassMethodRestricted(Class base, Function shouldCall, string name) {
154+
missingCallToSuperclassMethod(base, shouldCall, name) and
155+
not exists(Class superBase |
156+
// Alert only on the highest base class that has the issue
157+
superBase = getADirectSuperclass+(base) and
158+
missingCallToSuperclassMethod(superBase, shouldCall, name)
159+
) and
160+
not exists(Function subShouldCall |
161+
// Mention in the alert only the lowest method we're missing the call to
162+
subShouldCall.getScope() = getADirectSubclass+(shouldCall.getScope()) and
163+
missingCallToSuperclassMethod(base, subShouldCall, name)
164+
)
165+
}
166+
167+
/**
168+
* If `base` contains a `super()` call, gets a method in the inheritance hierarchy of `name` in the MRO of `base`
169+
* that does not contain a `super()` call, but would call `shouldCall` if it did, which does not otherwise get called
170+
* during a call to `base.<name>`.
171+
*/
172+
Function getPossibleMissingSuper(Class base, Function shouldCall, string name) {
173+
missingCallToSuperclassMethod(base, shouldCall, name) and
174+
exists(Function baseMethod |
175+
baseMethod.getScope() = base and
176+
baseMethod.getName() = name and
177+
// the base method calls super, so is presumably expecting every method called in the MRO chain to do so
178+
callsSuper(baseMethod) and
179+
// result is something that does get called in the chain
180+
result = getASuperCallTargetFromClass(base, base, name) and
181+
// it doesn't call super
182+
not callsSuper(result) and
183+
// if it did call super, it would resolve to the missing method
184+
shouldCall =
185+
findFunctionAccordingToMroKnownStartingClass(getNextClassInMroKnownStartingClass(result
186+
.getScope(), base), base, name)
187+
)
188+
}
189+
190+
/** An optional `Function`. */
191+
class FunctionOption extends LocatableOption<Location, Function>::Option {
192+
/** Gets the qualified name of this function, or the empty string if it is None. */
193+
string getQualifiedName() {
194+
this.isNone() and result = ""
195+
or
196+
result = this.asSome().getQualifiedName()
197+
}
198+
}
199+
200+
/** Gets the result of `getPossibleMissingSuper`, or None if none exists. */
201+
bindingset[name]
202+
FunctionOption getPossibleMissingSuperOption(Class base, Function shouldCall, string name) {
203+
result.asSome() = getPossibleMissingSuper(base, shouldCall, name)
204+
or
205+
not exists(getPossibleMissingSuper(base, shouldCall, name)) and
206+
result.isNone()
207+
}
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Python, unlike some other object-oriented languages such as Java, allows the developer complete freedom in
9+
when and how superclass finalizers are called during object finalization.
10+
However, the developer has responsibility for ensuring that objects are properly cleaned up, and that all superclass <code>__del__</code>
11+
methods are called.
12+
</p>
13+
<p>
14+
Classes with a <code>__del__</code> method (a finalizer) typically hold some resource such as a file handle that needs to be cleaned up.
15+
If the <code>__del__</code> method of a superclass is not called during object finalization, it is likely that
16+
resources may be leaked.
17+
</p>
18+
19+
<p>A call to the <code>__del__</code> method of a superclass during object initialization may be unintentionally skipped:
20+
</p>
21+
<ul>
22+
<li>If a subclass calls the <code>__del__</code> method of the wrong class.</li>
23+
<li>If a call to the <code>__del__</code> method of one its base classes is omitted.</li>
24+
<li>If a call to <code>super().__del__</code> is used, but not all <code>__del__</code> methods in the Method Resolution Order (MRO)
25+
chain themselves call <code>super()</code>. This in particular arises more often in cases of multiple inheritance. </li>
26+
</ul>
27+
28+
29+
</overview>
30+
<recommendation>
31+
<p>Ensure that all superclass <code>__del__</code> methods are properly called.
32+
Either each base class's finalize method should be explicitly called, or <code>super()</code> calls
33+
should be consistently used throughout the inheritance hierarchy.</p>
34+
35+
36+
</recommendation>
37+
<example>
38+
<p>In the following example, explicit calls to <code>__del__</code> are used, but <code>SportsCar</code> erroneously calls
39+
<code>Vehicle.__del__</code>. This is fixed in <code>FixedSportsCar</code> by calling <code>Car.__del__</code>.
40+
</p>
41+
42+
<sample src="examples/MissingCallToDel.py" />
43+
44+
</example>
45+
<references>
46+
47+
<li>Python Reference: <a href="https://docs.python.org/3/reference/datamodel.html#object.__del__">__del__</a>.</li>
48+
<li>Python Standard Library: <a href="https://docs.python.org/3/library/functions.html#super">super</a>.</li>
49+
<li>Python Glossary: <a href="https://docs.python.org/3/glossary.html#term-method-resolution-order">Method resolution order</a>.</li>
50+
51+
</references>
52+
</qhelp>

0 commit comments

Comments
 (0)