Merge pull request #18783 from asgerf/js/downward-calls

JS: Resolve calls downward in class hierarchy

Author: asgerf

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)
   }
 
   /**

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
+  compatibleTypesWithAny(t2, t1)
   or
-  compatibleTypesNonSymRefl(t2, t1)
+  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]

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.

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 | () {} |

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"
   )
 }
 

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();

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() {
+    }
+}

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    } |

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);
+    }
+}