-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Python: Modernize 4 queries for missing/multiple calls to init/del methods #19932
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.
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
Open
joefarebrother
wants to merge
28
commits into
github:main
Choose a base branch
from
joefarebrother:python-qual-init-del-calls
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+884
−551
Open
Changes from 3 commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
271f32e
Move missing/multiple calls to init/del queries to folder
joefarebrother a2fc14a
Update missing call to init
joefarebrother 6f9983a
Add missing call to del
joefarebrother adcfdf1
Modernize multple calls to init/del
joefarebrother caddec4
Update alert messages
joefarebrother 71d1179
Fix FPs and typo
joefarebrother 085df26
Move tests and add inline expectation postprocessing
joefarebrother 2faf67d
Update test outputs + fix semantics
joefarebrother 1b4e2fe
Change implenetation of missing calls to use getASuperCallTarget, and…
joefarebrother 16b90a1
Fixes
joefarebrother b3056fc
Update tests for calls to init + fixes
joefarebrother 73057d3
Add additional test case + update missing del tests
joefarebrother 2e6f35b
Remove case excluding classes with a __new__ method; as it doesn't ma…
joefarebrother c5b79fa
Update multiple calls queries to include call targets in alert message
joefarebrother 804b9ef
Update tests and add an additional test
joefarebrother 6ca4f32
qhelp: move examples to subfolder
joefarebrother 2e5f470
Update qhelp + alert messages
joefarebrother d2c68de
Update integration test output
joefarebrother f1026e4
Add change note
joefarebrother c47e6e3
Add qldoc
joefarebrother 4b49ac3
Fix changenote formatting
joefarebrother d163bdf
Fix typos
joefarebrother e8a65b8
Update integration test outout and fix qhelp
joefarebrother f5066c7
Remove tostring
joefarebrother 2c93e2c
Inline locationBefore
joefarebrother 7dad89f
Adress review suggestions - cleanups
joefarebrother d2a8e5d
Fix typo in example
joefarebrother b33a1c2
Fix doc typo
joefarebrother File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
114 changes: 114 additions & 0 deletions
114
python/ql/src/Classes/CallsToInitDel/MethodCallOrder.qll
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,114 @@ | ||
/** Definitions for reasoning about multiple or missing calls to superclass methods. */ | ||
|
||
import python | ||
import semmle.python.ApiGraphs | ||
import semmle.python.dataflow.new.internal.DataFlowDispatch | ||
|
||
// Helper predicates for multiple call to __init__/__del__ queries. | ||
pragma[noinline] | ||
private predicate multiple_invocation_paths_helper( | ||
FunctionInvocation top, FunctionInvocation i1, FunctionInvocation i2, FunctionObject multi | ||
) { | ||
i1 != i2 and | ||
i1 = top.getACallee+() and | ||
i2 = top.getACallee+() and | ||
i1.getFunction() = multi | ||
} | ||
|
||
pragma[noinline] | ||
private predicate multiple_invocation_paths( | ||
FunctionInvocation top, FunctionInvocation i1, FunctionInvocation i2, FunctionObject multi | ||
) { | ||
multiple_invocation_paths_helper(top, i1, i2, multi) and | ||
i2.getFunction() = multi | ||
} | ||
|
||
/** Holds if `self.name` calls `multi` by multiple paths, and thus calls it more than once. */ | ||
predicate multiple_calls_to_superclass_method(ClassObject self, FunctionObject multi, string name) { | ||
exists(FunctionInvocation top, FunctionInvocation i1, FunctionInvocation i2 | | ||
multiple_invocation_paths(top, i1, i2, multi) and | ||
top.runtime(self.declaredAttribute(name)) and | ||
self.getASuperType().declaredAttribute(name) = multi | ||
| | ||
// Only called twice if called from different functions, | ||
// or if one call-site can reach the other. | ||
i1.getCall().getScope() != i2.getCall().getScope() | ||
or | ||
i1.getCall().strictlyReaches(i2.getCall()) | ||
) | ||
} | ||
|
||
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 | ||
} | ||
|
||
predicate superCall(DataFlow::MethodCallNode call, string name) { | ||
exists(DataFlow::Node sup | | ||
call.calls(sup, name) and | ||
sup = API::builtin("super").getACall() | ||
) | ||
} | ||
|
||
predicate callsSuper(Function meth) { | ||
exists(DataFlow::MethodCallNode call | | ||
call.getScope() = meth and | ||
superCall(call, meth.getName()) | ||
) | ||
} | ||
|
||
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) | ||
) | ||
} | ||
|
||
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)) | ||
|
||
) | ||
} | ||
|
||
predicate mayProceedInMro(Class a, Class b, Class mroStart) { | ||
b = getNextClassInMroKnownStartingClass(a, mroStart) | ||
or | ||
exists(Class mid | | ||
mid = getNextClassInMroKnownStartingClass(a, mroStart) and | ||
mayProceedInMro(mid, b, mroStart) | ||
) | ||
} | ||
|
||
predicate missingCallToSuperclassMethod( | ||
Function base, Function shouldCall, Class mroStart, string name | ||
) { | ||
base.getName() = name and | ||
shouldCall.getName() = name and | ||
not callsSuper(base) and | ||
not callsMethodOnUnknownClassWithSelf(base, name) and | ||
nonTrivial(shouldCall) and | ||
base.getScope() = getADirectSuperclass*(mroStart) and | ||
mayProceedInMro(base.getScope(), shouldCall.getScope(), mroStart) and | ||
not exists(Class called | | ||
( | ||
callsMethodOnClassWithSelf(base, _, called, name) | ||
or | ||
callsMethodOnClassWithSelf(findFunctionAccordingToMro(mroStart, name), _, called, name) | ||
) and | ||
shouldCall.getScope() = getADirectSuperclass*(called) | ||
) | ||
} |
File renamed without changes.
File renamed without changes.
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,35 @@ | ||
/** | ||
* @name Missing call to superclass `__del__` during object destruction | ||
* @description An omitted call to a superclass `__del__` method may lead to class instances not being cleaned up properly. | ||
* @kind problem | ||
* @tags quality | ||
* reliability | ||
* correctness | ||
* performance | ||
* @problem.severity error | ||
* @sub-severity low | ||
* @precision high | ||
* @id py/missing-call-to-delete | ||
*/ | ||
|
||
import python | ||
import MethodCallOrder | ||
|
||
predicate missingCallToSuperclassDel(Function base, Function shouldCall, Class mroStart) { | ||
missingCallToSuperclassMethod(base, shouldCall, mroStart, "__del__") | ||
} | ||
|
||
from Function base, Function shouldCall, Class mroStart, string msg | ||
where | ||
missingCallToSuperclassDel(base, shouldCall, mroStart) and | ||
( | ||
// Simple case: the method that should be called is directly overridden | ||
mroStart = base.getScope() and | ||
msg = "This deletion method does not call $@, which may leave $@ not properly cleaned up." | ||
or | ||
// Only alert for a different mro base if there are no alerts for direct overrides | ||
not missingCallToSuperclassDel(base, _, base.getScope()) and | ||
msg = | ||
"This deletion method does not call $@, which follows it in the MRO of $@, leaving it not properly cleaned up." | ||
) | ||
select base, msg, shouldCall, shouldCall.getQualifiedName(), mroStart, mroStart.getName() |
File renamed without changes.
File renamed without changes.
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,34 @@ | ||
/** | ||
* @name Missing call to superclass `__init__` during object initialization | ||
* @description An omitted call to a superclass `__init__` method may lead to objects of this class not being fully initialized. | ||
* @kind problem | ||
* @tags quality | ||
* reliability | ||
* correctness | ||
* @problem.severity error | ||
* @sub-severity low | ||
* @precision high | ||
* @id py/missing-call-to-init | ||
*/ | ||
|
||
import python | ||
import MethodCallOrder | ||
|
||
predicate missingCallToSuperclassInit(Function base, Function shouldCall, Class mroStart) { | ||
missingCallToSuperclassMethod(base, shouldCall, mroStart, "__init__") | ||
} | ||
|
||
from Function base, Function shouldCall, Class mroStart, string msg | ||
where | ||
missingCallToSuperclassInit(base, shouldCall, mroStart) and | ||
( | ||
// Simple case: the method that should be called is directly overridden | ||
mroStart = base.getScope() and | ||
msg = "This initialization method does not call $@, which may leave $@ partially initialized." | ||
or | ||
// Only alert for a different mro base if there are no alerts for direct overrides | ||
not missingCallToSuperclassInit(base, _, base.getScope()) and | ||
msg = | ||
"This initialization method does not call $@, which follows it in the MRO of $@, leaving it partially initialized." | ||
) | ||
select base, msg, shouldCall, shouldCall.getQualifiedName(), mroStart, mroStart.getName() |
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
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 |
---|---|---|
@@ -1,3 +1,5 @@ | ||
deprecated module; | ||
|
||
import python | ||
|
||
// Helper predicates for multiple call to __init__/__del__ queries. | ||
|
This file was deleted.
Oops, something went wrong.
This file was deleted.
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.
Uh oh!
There was an error while loading. Please reload this page.