From b4b20d7d3f970a2eec359d3da189c4e86d3863e3 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 4 Jul 2025 10:30:20 +0100 Subject: [PATCH] Update multiple calls queries to include call targets in alert message --- .../CallsToInitDel/MethodCallOrder.qll | 38 +++++++++++++++---- .../SuperclassDelCalledMultipleTimes.ql | 33 ++++++++++++---- .../SuperclassInitCalledMultipleTimes.ql | 33 ++++++++++++---- 3 files changed, 81 insertions(+), 23 deletions(-) diff --git a/python/ql/src/Classes/CallsToInitDel/MethodCallOrder.qll b/python/ql/src/Classes/CallsToInitDel/MethodCallOrder.qll index 191243e0332..02d60ca420e 100644 --- a/python/ql/src/Classes/CallsToInitDel/MethodCallOrder.qll +++ b/python/ql/src/Classes/CallsToInitDel/MethodCallOrder.qll @@ -5,11 +5,14 @@ import semmle.python.ApiGraphs import semmle.python.dataflow.new.internal.DataFlowDispatch import codeql.util.Option -predicate multipleCallsToSuperclassMethod(Function meth, Function calledMulti, string name) { - exists(DataFlow::MethodCallNode call1, DataFlow::MethodCallNode call2, Class cls | +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 - call1.asExpr() != call2.asExpr() and + call1.getLocation().toString() < call2.getLocation().toString() and calledMulti = getASuperCallTargetFromCall(cls, meth, call1, name) and calledMulti = getASuperCallTargetFromCall(cls, meth, call2, name) and nonTrivial(calledMulti) @@ -18,23 +21,44 @@ predicate multipleCallsToSuperclassMethod(Function meth, Function calledMulti, s 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) + ) +} + +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 - exists(Class targetCls | result = getASuperCallTargetFromClass(mroBase, targetCls, name) | + 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) + targetCls = getNextClassInMroKnownStartingClass(meth.getScope(), mroBase) and + result = findFunctionAccordingToMroKnownStartingClass(targetCls, mroBase, name) or - callsMethodOnClassWithSelf(meth, call, targetCls, _) + // 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 + // assuming it's 0-arg or is 2-arg with `self` as second arg. + callsMethodOnClassWithSelf(meth, call, targetCls, _) and + result = findFunctionAccordingToMroKnownStartingClass(targetCls, targetCls, name) ) } 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)) + ( + result = target + or + result = getASuperCallTargetFromCall(mroBase, target, _, name) + ) ) } diff --git a/python/ql/src/Classes/CallsToInitDel/SuperclassDelCalledMultipleTimes.ql b/python/ql/src/Classes/CallsToInitDel/SuperclassDelCalledMultipleTimes.ql index 7aca3dee189..7772aa15373 100644 --- a/python/ql/src/Classes/CallsToInitDel/SuperclassDelCalledMultipleTimes.ql +++ b/python/ql/src/Classes/CallsToInitDel/SuperclassDelCalledMultipleTimes.ql @@ -14,17 +14,34 @@ import python import MethodCallOrder -predicate multipleCallsToSuperclassDel(Function meth, Function calledMulti) { - multipleCallsToSuperclassMethod(meth, calledMulti, "__del__") +predicate multipleCallsToSuperclassDel( + Function meth, Function calledMulti, DataFlow::MethodCallNode call1, + DataFlow::MethodCallNode call2 +) { + multipleCallsToSuperclassMethod(meth, calledMulti, call1, call2, "__del__") } -from Function meth, Function calledMulti +from + Function meth, Function calledMulti, DataFlow::MethodCallNode call1, + DataFlow::MethodCallNode call2, Function target1, Function target2, string msg where - multipleCallsToSuperclassDel(meth, calledMulti) and - // Don't alert for multiple calls to a superclass del when a subclass will do. + multipleCallsToSuperclassDel(meth, calledMulti, call1, call2) and + // Only alert for the lowest method in the hierarchy that both calls will call. not exists(Function subMulti | - multipleCallsToSuperclassDel(meth, subMulti) and + multipleCallsToSuperclassDel(meth, subMulti, _, _) and calledMulti.getScope() = getADirectSuperclass+(subMulti.getScope()) + ) and + target1 = getDirectSuperCallTargetFromCall(meth.getScope(), meth, call1, _) and + target2 = getDirectSuperCallTargetFromCall(meth.getScope(), meth, call2, _) and + ( + target1 != target2 and + msg = + "This deletion method calls $@ multiple times, via $@ and $@, resolving to $@ and $@ respectively." + or + target1 = target2 and + // The targets themselves are called multiple times (either is calledMulti, or something earlier in the MRO) + // Mentioning them again would be redundant. + msg = "This deletion method calls $@ multiple times, via $@ and $@." ) -select meth, "This delete method calls $@ multiple times.", calledMulti, - calledMulti.getQualifiedName() +select meth, msg, calledMulti, calledMulti.getQualifiedName(), call1, "this call", call2, + "this call", target1, target1.getQualifiedName(), target2, target2.getQualifiedName() diff --git a/python/ql/src/Classes/CallsToInitDel/SuperclassInitCalledMultipleTimes.ql b/python/ql/src/Classes/CallsToInitDel/SuperclassInitCalledMultipleTimes.ql index 4f577dc4a76..04c226aa195 100644 --- a/python/ql/src/Classes/CallsToInitDel/SuperclassInitCalledMultipleTimes.ql +++ b/python/ql/src/Classes/CallsToInitDel/SuperclassInitCalledMultipleTimes.ql @@ -14,17 +14,34 @@ import python import MethodCallOrder -predicate multipleCallsToSuperclassInit(Function meth, Function calledMulti) { - multipleCallsToSuperclassMethod(meth, calledMulti, "__init__") +predicate multipleCallsToSuperclassInit( + Function meth, Function calledMulti, DataFlow::MethodCallNode call1, + DataFlow::MethodCallNode call2 +) { + multipleCallsToSuperclassMethod(meth, calledMulti, call1, call2, "__init__") } -from Function meth, Function calledMulti +from + Function meth, Function calledMulti, DataFlow::MethodCallNode call1, + DataFlow::MethodCallNode call2, Function target1, Function target2, string msg where - multipleCallsToSuperclassInit(meth, calledMulti) and - // Don't alert for multiple calls to a superclass init when a subclass will do. + multipleCallsToSuperclassInit(meth, calledMulti, call1, call2) and + // Only alert for the lowest method in the hierarchy that both calls will call. not exists(Function subMulti | - multipleCallsToSuperclassInit(meth, subMulti) and + multipleCallsToSuperclassInit(meth, subMulti, _, _) and calledMulti.getScope() = getADirectSuperclass+(subMulti.getScope()) + ) and + target1 = getDirectSuperCallTargetFromCall(meth.getScope(), meth, call1, _) and + target2 = getDirectSuperCallTargetFromCall(meth.getScope(), meth, call2, _) and + ( + target1 != target2 and + msg = + "This initialization method calls $@ multiple times, via $@ and $@, resolving to $@ and $@ respectively." + or + target1 = target2 and + // The targets themselves are called multiple times (either is calledMulti, or something earlier in the MRO) + // Mentioning them again would be redundant. + msg = "This initialization method calls $@ multiple times, via $@ and $@." ) -select meth, "This initialization method calls $@ multiple times.", calledMulti, - calledMulti.getQualifiedName() +select meth, msg, calledMulti, calledMulti.getQualifiedName(), call1, "this call", call2, + "this call", target1, target1.getQualifiedName(), target2, target2.getQualifiedName()