diff --git a/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll b/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll index ff68540d346..2e231383522 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll @@ -1066,20 +1066,46 @@ class ClassNode extends DataFlow::SourceNode instanceof ClassNode::Range { result = this.getAnInstanceReference(DataFlow::TypeTracker::end()) } + pragma[nomagic] + private DataFlow::PropRead getAnOwnInstanceMemberAccess(string name, DataFlow::TypeTracker t) { + result = this.getAnInstanceReference(t.continue()).getAPropertyRead(name) + } + + pragma[nomagic] + private DataFlow::PropRead getAnInstanceMemberAccessOnSubClass( + string name, DataFlow::TypeTracker t + ) { + exists(DataFlow::ClassNode subclass | + subclass = this.getADirectSubClass() and + not exists(subclass.getInstanceMember(name, _)) + | + result = subclass.getAnOwnInstanceMemberAccess(name, t) + or + result = subclass.getAnInstanceMemberAccessOnSubClass(name, t) + ) + } + + pragma[nomagic] + private DataFlow::PropRead getAnInstanceMemberAccessOnSuperClass(string name) { + result = this.getADirectSuperClass().getAReceiverNode().getAPropertyRead(name) + or + result = this.getADirectSuperClass().getAnInstanceMemberAccessOnSuperClass(name) + } + /** * Gets a property read that accesses the property `name` on an instance of this class. * - * Concretely, this holds when the base is an instance of this class or a subclass thereof. + * This includes accesses on subclasses (if the member is not overridden) and accesses in a base class + * (only if accessed on `this`). */ pragma[nomagic] DataFlow::PropRead getAnInstanceMemberAccess(string name, DataFlow::TypeTracker t) { - result = this.getAnInstanceReference(t.continue()).getAPropertyRead(name) + result = this.getAnOwnInstanceMemberAccess(name, t) or - exists(DataFlow::ClassNode subclass | - result = subclass.getAnInstanceMemberAccess(name, t) and - not exists(subclass.getInstanceMember(name, _)) and - this = subclass.getADirectSuperClass() - ) + result = this.getAnInstanceMemberAccessOnSubClass(name, t) + or + t.start() and + result = this.getAnInstanceMemberAccessOnSuperClass(name) } /** diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll index e25f93bca3f..d170b8a3c1e 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll @@ -558,15 +558,19 @@ DataFlowCallable nodeGetEnclosingCallable(Node node) { newtype TDataFlowType = TFunctionType(Function f) or + TInstanceType(DataFlow::ClassNode cls) or TAnyType() class DataFlowType extends TDataFlowType { string toDebugString() { - this instanceof TFunctionType and result = "TFunctionType(" + this.asFunction().toString() + ") at line " + this.asFunction().getLocation().getStartLine() or + result = + "TInstanceType(" + this.asInstanceOfClass().toString() + ") at line " + + this.asInstanceOfClass().getLocation().getStartLine() + or this instanceof TAnyType and result = "TAnyType" } @@ -575,13 +579,20 @@ class DataFlowType extends TDataFlowType { } Function asFunction() { this = TFunctionType(result) } + + DataFlow::ClassNode asInstanceOfClass() { this = TInstanceType(result) } } /** * Holds if `t1` is strictly stronger than `t2`. */ predicate typeStrongerThan(DataFlowType t1, DataFlowType t2) { - t1 instanceof TFunctionType and t2 = TAnyType() + // 't1' is a subclass of 't2' + t1.asInstanceOfClass() = t2.asInstanceOfClass().getADirectSubClass+() + or + // Ensure all types are stronger than 'any' + not t1 = TAnyType() and + t2 = TAnyType() } private DataFlowType getPreciseType(Node node) { @@ -590,6 +601,9 @@ private DataFlowType getPreciseType(Node node) { result = TFunctionType(f) ) or + result.asInstanceOfClass() = + unique(DataFlow::ClassNode cls | cls.getAnInstanceReference().getALocalUse() = node) + or result = getPreciseType(node.getImmediatePredecessor()) or result = getPreciseType(node.(PostUpdateNode).getPreUpdateNode()) @@ -683,18 +697,27 @@ predicate neverSkipInPathGraph(Node node) { string ppReprType(DataFlowType t) { none() } pragma[inline] -private predicate compatibleTypesNonSymRefl(DataFlowType t1, DataFlowType t2) { +private predicate compatibleTypesWithAny(DataFlowType t1, DataFlowType t2) { t1 != TAnyType() and t2 = TAnyType() } +pragma[nomagic] +private predicate compatibleTypes1(DataFlowType t1, DataFlowType t2) { + t1.asInstanceOfClass().getADirectSubClass+() = t2.asInstanceOfClass() +} + pragma[inline] predicate compatibleTypes(DataFlowType t1, DataFlowType t2) { t1 = t2 or - compatibleTypesNonSymRefl(t1, t2) + compatibleTypesWithAny(t1, t2) or - compatibleTypesNonSymRefl(t2, t1) + compatibleTypesWithAny(t2, t1) + or + compatibleTypes1(t1, t2) + or + compatibleTypes1(t2, t1) } predicate forceHighPrecision(Content c) { none() } @@ -1061,17 +1084,54 @@ DataFlowCallable viableCallable(DataFlowCall node) { result.asSourceCallableNotExterns() = node.asImpliedLambdaCall() } +private DataFlowCall getACallOnThis(DataFlow::ClassNode cls) { + result.asOrdinaryCall() = cls.getAReceiverNode().getAPropertyRead().getACall() + or + result.asAccessorCall() = cls.getAReceiverNode().getAPropertyRead() + or + result.asPartialCall().getACallbackNode() = cls.getAReceiverNode().getAPropertyRead() +} + +private predicate downwardCall(DataFlowCall call) { + exists(DataFlow::ClassNode cls | + call = getACallOnThis(cls) and + viableCallable(call).asSourceCallable() = + cls.getADirectSubClass+().getAnInstanceMember().getFunction() + ) +} + /** * Holds if the set of viable implementations that can be called by `call` * might be improved by knowing the call context. */ -predicate mayBenefitFromCallContext(DataFlowCall call) { none() } +predicate mayBenefitFromCallContext(DataFlowCall call) { downwardCall(call) } + +/** Gets the type of the receiver of `call`. */ +private DataFlowType getThisArgumentType(DataFlowCall call) { + exists(DataFlow::Node node | + isArgumentNodeImpl(node, call, MkThisParameter()) and + result = getNodeType(node) + ) +} + +/** Gets the type of the 'this' parameter of `call`. */ +private DataFlowType getThisParameterType(DataFlowCallable callable) { + exists(DataFlow::Node node | + isParameterNodeImpl(node, callable, MkThisParameter()) and + result = getNodeType(node) + ) +} /** * Gets a viable dispatch target of `call` in the context `ctx`. This is * restricted to those `call`s for which a context might make a difference. */ -DataFlowCallable viableImplInCallContext(DataFlowCall call, DataFlowCall ctx) { none() } +DataFlowCallable viableImplInCallContext(DataFlowCall call, DataFlowCall ctx) { + mayBenefitFromCallContext(call) and + result = viableCallable(call) and + viableCallable(ctx) = call.getEnclosingCallable() and + compatibleTypes(getThisArgumentType(ctx), getThisParameterType(result)) +} bindingset[node, fun] pragma[inline_late] diff --git a/javascript/ql/src/change-notes/2025-02-17-downward-calls.md b/javascript/ql/src/change-notes/2025-02-17-downward-calls.md new file mode 100644 index 00000000000..84bde9dbde8 --- /dev/null +++ b/javascript/ql/src/change-notes/2025-02-17-downward-calls.md @@ -0,0 +1,6 @@ +--- +category: majorAnalysis +--- +* Improved call resolution logic to better handle calls resolving "downwards", targeting + a method declared in a subclass of the enclosing class. Data flow analysis + has also improved to avoid spurious flow between unrelated classes in the class hierarchy. diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected index 8182d017414..0abd563b419 100644 --- a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected @@ -12,4 +12,4 @@ accessorCall | accessors.js:44:1:44:9 | new D().f | accessors.js:37:8:37:13 | (x) {} | | accessors.js:48:1:48:5 | obj.f | accessors.js:5:8:5:12 | () {} | | accessors.js:51:1:51:3 | C.f | accessors.js:19:15:19:19 | () {} | -| accessors.js:54:1:54:9 | new D().f | accessors.js:34:8:34:12 | () {} | +| accessors.js:55:1:55:3 | d.f | accessors.js:34:8:34:12 | () {} | diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.ql b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.ql index e92524adc19..4388a2d388d 100644 --- a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.ql +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.ql @@ -58,20 +58,23 @@ class AnnotatedCall extends DataFlow::Node { string getKind() { result = kind } } -predicate callEdge(AnnotatedCall call, AnnotatedFunction target, int boundArgs) { +predicate callEdge(AnnotatedCall call, Function target, int boundArgs) { FlowSteps::calls(call, target) and boundArgs = -1 or FlowSteps::callsBound(call, target, boundArgs) } -query predicate spuriousCallee( - AnnotatedCall call, AnnotatedFunction target, int boundArgs, string kind -) { +query predicate spuriousCallee(AnnotatedCall call, Function target, int boundArgs, string kind) { callEdge(call, target, boundArgs) and kind = call.getKind() and not ( target = call.getAnExpectedCallee(kind) and boundArgs = call.getBoundArgsOrMinusOne() + ) and + ( + target instanceof AnnotatedFunction + or + call.getCallTargetName() = "NONE" ) } diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/accessors.js b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/accessors.js index f9f4e25d092..5ec118c460b 100644 --- a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/accessors.js +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/accessors.js @@ -50,5 +50,6 @@ obj.f(); /** calls:NONE */ C.f(); +const d = new D(); /** calls:NONE */ -new D().f(); +d.f(); diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js new file mode 100644 index 00000000000..f60181d1cac --- /dev/null +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js @@ -0,0 +1,60 @@ +import 'dummy'; + +class Base { + workInBase() { + /** calls:methodInBase */ + this.methodInBase(); + + /** calls:methodInSub1 calls:methodInSub2 */ + this.methodInSub(); + + /** calls:overriddenInSub0 calls:overriddenInSub1 calls:overriddenInSub2 */ + this.overriddenInSub(); + } + + /** name:methodInBase */ + methodInBase() { + /** calls:methodInSub1 calls:methodInSub2 */ + this.methodInSub(); + } + + /** name:overriddenInSub0 */ + overriddenInSub() { + } +} + +class Subclass1 extends Base { + workInSub() { + /** calls:methodInBase */ + this.methodInBase(); + + /** calls:overriddenInSub1 */ + this.overriddenInSub(); + } + + /** name:methodInSub1 */ + methodInSub() { + } + + /** name:overriddenInSub1 */ + overriddenInSub() { + } +} + +class Subclass2 extends Base { + workInSub() { + /** calls:methodInBase */ + this.methodInBase(); + + /** calls:overriddenInSub2 */ + this.overriddenInSub(); + } + + /** name:methodInSub2 */ + methodInSub() { + } + + /** name:overriddenInSub2 */ + overriddenInSub() { + } +} diff --git a/javascript/ql/test/library-tests/CallGraphs/FullTest/tests.expected b/javascript/ql/test/library-tests/CallGraphs/FullTest/tests.expected index 4d2c78206a4..8185f693b22 100644 --- a/javascript/ql/test/library-tests/CallGraphs/FullTest/tests.expected +++ b/javascript/ql/test/library-tests/CallGraphs/FullTest/tests.expected @@ -56,6 +56,7 @@ test_getAFunctionValue | classes.js:3:10:5:5 | () {\\n ... ;\\n } | classes.js:3:10:5:5 | () {\\n ... ;\\n } | | classes.js:7:6:9:5 | () {\\n ... ;\\n } | classes.js:7:6:9:5 | () {\\n ... ;\\n } | | classes.js:8:7:8:16 | this.hello | classes.js:3:10:5:5 | () {\\n ... ;\\n } | +| classes.js:8:7:8:16 | this.hello | classes.js:13:10:15:5 | () {\\n ... ;\\n } | | classes.js:12:3:16:3 | B | classes.js:12:21:12:20 | (...arg ... rgs); } | | classes.js:12:3:16:3 | class B ... }\\n } | classes.js:12:21:12:20 | (...arg ... rgs); } | | classes.js:12:19:12:19 | A | classes.js:2:11:2:10 | () {} | @@ -447,6 +448,7 @@ test_getACallee | a.js:3:1:3:5 | bar() | b.js:2:8:2:24 | function bar() {} | | a.js:4:1:4:5 | qux() | c.js:2:8:2:24 | function bar() {} | | classes.js:8:7:8:18 | this.hello() | classes.js:3:10:5:5 | () {\\n ... ;\\n } | +| classes.js:8:7:8:18 | this.hello() | classes.js:13:10:15:5 | () {\\n ... ;\\n } | | classes.js:12:21:12:20 | super(...args) | classes.js:2:11:2:10 | () {} | | classes.js:18:3:18:9 | new B() | classes.js:12:21:12:20 | (...arg ... rgs); } | | classes.js:18:3:18:17 | new B().hello() | classes.js:13:10:15:5 | () {\\n ... ;\\n } | diff --git a/javascript/ql/test/library-tests/TripleDot/subclass.js b/javascript/ql/test/library-tests/TripleDot/subclass.js new file mode 100644 index 00000000000..35c722d2df4 --- /dev/null +++ b/javascript/ql/test/library-tests/TripleDot/subclass.js @@ -0,0 +1,34 @@ +import 'dummy'; + +class Base { + baseMethod(x) { + this.subclassMethod(x); + } +} + +class Subclass1 extends Base { + work() { + this.baseMethod(source("sub1")); + } + subclassMethod(x) { + sink(x); // $ hasValueFlow=sub1 + } +} + +class Subclass2 extends Base { + work() { + this.baseMethod(source("sub2")); + } + subclassMethod(x) { + sink(x); // $ hasValueFlow=sub2 + } +} + +class Subclass3 extends Base { + work() { + this.baseMethod("safe"); + } + subclassMethod(x) { + sink(x); + } +}