Skip to content

Commit a1b7096

Browse files
asgerfasgerf
authored
Merge pull request #18783 from asgerf/js/downward-calls
JS: Resolve calls downward in class hierarchy
2 parents 22bf1af + ad4522c commit a1b7096

File tree

9 files changed

+212
-20
lines changed

9 files changed

+212
-20
lines changed

‎javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll‎

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,20 +1066,46 @@ class ClassNode extends DataFlow::SourceNode instanceof ClassNode::Range {
10661066
result = this.getAnInstanceReference(DataFlow::TypeTracker::end())
10671067
}
10681068

1069+
pragma[nomagic]
1070+
private DataFlow::PropRead getAnOwnInstanceMemberAccess(string name, DataFlow::TypeTracker t) {
1071+
result = this.getAnInstanceReference(t.continue()).getAPropertyRead(name)
1072+
}
1073+
1074+
pragma[nomagic]
1075+
private DataFlow::PropRead getAnInstanceMemberAccessOnSubClass(
1076+
string name, DataFlow::TypeTracker t
1077+
) {
1078+
exists(DataFlow::ClassNode subclass |
1079+
subclass = this.getADirectSubClass() and
1080+
not exists(subclass.getInstanceMember(name, _))
1081+
|
1082+
result = subclass.getAnOwnInstanceMemberAccess(name, t)
1083+
or
1084+
result = subclass.getAnInstanceMemberAccessOnSubClass(name, t)
1085+
)
1086+
}
1087+
1088+
pragma[nomagic]
1089+
private DataFlow::PropRead getAnInstanceMemberAccessOnSuperClass(string name) {
1090+
result = this.getADirectSuperClass().getAReceiverNode().getAPropertyRead(name)
1091+
or
1092+
result = this.getADirectSuperClass().getAnInstanceMemberAccessOnSuperClass(name)
1093+
}
1094+
10691095
/**
10701096
* Gets a property read that accesses the property `name` on an instance of this class.
10711097
*
1072-
* Concretely, this holds when the base is an instance of this class or a subclass thereof.
1098+
* This includes accesses on subclasses (if the member is not overridden) and accesses in a base class
1099+
* (only if accessed on `this`).
10731100
*/
10741101
pragma[nomagic]
10751102
DataFlow::PropRead getAnInstanceMemberAccess(string name, DataFlow::TypeTracker t) {
1076-
result = this.getAnInstanceReference(t.continue()).getAPropertyRead(name)
1103+
result = this.getAnOwnInstanceMemberAccess(name, t)
10771104
or
1078-
exists(DataFlow::ClassNode subclass |
1079-
result = subclass.getAnInstanceMemberAccess(name, t) and
1080-
not exists(subclass.getInstanceMember(name, _)) and
1081-
this = subclass.getADirectSuperClass()
1082-
)
1105+
result = this.getAnInstanceMemberAccessOnSubClass(name, t)
1106+
or
1107+
t.start() and
1108+
result = this.getAnInstanceMemberAccessOnSuperClass(name)
10831109
}
10841110

10851111
/**

‎javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll‎

Lines changed: 67 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -558,15 +558,19 @@ DataFlowCallable nodeGetEnclosingCallable(Node node) {
558558

559559
newtype TDataFlowType =
560560
TFunctionType(Function f) or
561+
TInstanceType(DataFlow::ClassNode cls) or
561562
TAnyType()
562563

563564
class DataFlowType extends TDataFlowType {
564565
string toDebugString() {
565-
this instanceof TFunctionType and
566566
result =
567567
"TFunctionType(" + this.asFunction().toString() + ") at line " +
568568
this.asFunction().getLocation().getStartLine()
569569
or
570+
result =
571+
"TInstanceType(" + this.asInstanceOfClass().toString() + ") at line " +
572+
this.asInstanceOfClass().getLocation().getStartLine()
573+
or
570574
this instanceof TAnyType and result = "TAnyType"
571575
}
572576

@@ -575,13 +579,20 @@ class DataFlowType extends TDataFlowType {
575579
}
576580

577581
Function asFunction() { this = TFunctionType(result) }
582+
583+
DataFlow::ClassNode asInstanceOfClass() { this = TInstanceType(result) }
578584
}
579585

580586
/**
581587
* Holds if `t1` is strictly stronger than `t2`.
582588
*/
583589
predicate typeStrongerThan(DataFlowType t1, DataFlowType t2) {
584-
t1 instanceof TFunctionType and t2 = TAnyType()
590+
// 't1' is a subclass of 't2'
591+
t1.asInstanceOfClass() = t2.asInstanceOfClass().getADirectSubClass+()
592+
or
593+
// Ensure all types are stronger than 'any'
594+
not t1 = TAnyType() and
595+
t2 = TAnyType()
585596
}
586597

587598
private DataFlowType getPreciseType(Node node) {
@@ -590,6 +601,9 @@ private DataFlowType getPreciseType(Node node) {
590601
result = TFunctionType(f)
591602
)
592603
or
604+
result.asInstanceOfClass() =
605+
unique(DataFlow::ClassNode cls | cls.getAnInstanceReference().getALocalUse() = node)
606+
or
593607
result = getPreciseType(node.getImmediatePredecessor())
594608
or
595609
result = getPreciseType(node.(PostUpdateNode).getPreUpdateNode())
@@ -683,18 +697,27 @@ predicate neverSkipInPathGraph(Node node) {
683697
string ppReprType(DataFlowType t) { none() }
684698

685699
pragma[inline]
686-
private predicate compatibleTypesNonSymRefl(DataFlowType t1, DataFlowType t2) {
700+
private predicate compatibleTypesWithAny(DataFlowType t1, DataFlowType t2) {
687701
t1 != TAnyType() and
688702
t2 = TAnyType()
689703
}
690704

705+
pragma[nomagic]
706+
private predicate compatibleTypes1(DataFlowType t1, DataFlowType t2) {
707+
t1.asInstanceOfClass().getADirectSubClass+() = t2.asInstanceOfClass()
708+
}
709+
691710
pragma[inline]
692711
predicate compatibleTypes(DataFlowType t1, DataFlowType t2) {
693712
t1 = t2
694713
or
695-
compatibleTypesNonSymRefl(t1, t2)
714+
compatibleTypesWithAny(t1, t2)
715+
or
716+
compatibleTypesWithAny(t2, t1)
696717
or
697-
compatibleTypesNonSymRefl(t2, t1)
718+
compatibleTypes1(t1, t2)
719+
or
720+
compatibleTypes1(t2, t1)
698721
}
699722

700723
predicate forceHighPrecision(Content c) { none() }
@@ -1061,17 +1084,54 @@ DataFlowCallable viableCallable(DataFlowCall node) {
10611084
result.asSourceCallableNotExterns() = node.asImpliedLambdaCall()
10621085
}
10631086

1087+
private DataFlowCall getACallOnThis(DataFlow::ClassNode cls) {
1088+
result.asOrdinaryCall() = cls.getAReceiverNode().getAPropertyRead().getACall()
1089+
or
1090+
result.asAccessorCall() = cls.getAReceiverNode().getAPropertyRead()
1091+
or
1092+
result.asPartialCall().getACallbackNode() = cls.getAReceiverNode().getAPropertyRead()
1093+
}
1094+
1095+
private predicate downwardCall(DataFlowCall call) {
1096+
exists(DataFlow::ClassNode cls |
1097+
call = getACallOnThis(cls) and
1098+
viableCallable(call).asSourceCallable() =
1099+
cls.getADirectSubClass+().getAnInstanceMember().getFunction()
1100+
)
1101+
}
1102+
10641103
/**
10651104
* Holds if the set of viable implementations that can be called by `call`
10661105
* might be improved by knowing the call context.
10671106
*/
1068-
predicate mayBenefitFromCallContext(DataFlowCall call) { none() }
1107+
predicate mayBenefitFromCallContext(DataFlowCall call) { downwardCall(call) }
1108+
1109+
/** Gets the type of the receiver of `call`. */
1110+
private DataFlowType getThisArgumentType(DataFlowCall call) {
1111+
exists(DataFlow::Node node |
1112+
isArgumentNodeImpl(node, call, MkThisParameter()) and
1113+
result = getNodeType(node)
1114+
)
1115+
}
1116+
1117+
/** Gets the type of the 'this' parameter of `call`. */
1118+
private DataFlowType getThisParameterType(DataFlowCallable callable) {
1119+
exists(DataFlow::Node node |
1120+
isParameterNodeImpl(node, callable, MkThisParameter()) and
1121+
result = getNodeType(node)
1122+
)
1123+
}
10691124

10701125
/**
10711126
* Gets a viable dispatch target of `call` in the context `ctx`. This is
10721127
* restricted to those `call`s for which a context might make a difference.
10731128
*/
1074-
DataFlowCallable viableImplInCallContext(DataFlowCall call, DataFlowCall ctx) { none() }
1129+
DataFlowCallable viableImplInCallContext(DataFlowCall call, DataFlowCall ctx) {
1130+
mayBenefitFromCallContext(call) and
1131+
result = viableCallable(call) and
1132+
viableCallable(ctx) = call.getEnclosingCallable() and
1133+
compatibleTypes(getThisArgumentType(ctx), getThisParameterType(result))
1134+
}
10751135

10761136
bindingset[node, fun]
10771137
pragma[inline_late]

‎javascript/ql/src/change-notes/2025-02-17-downward-calls.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
category: majorAnalysis
3+
---
4+
* Improved call resolution logic to better handle calls resolving "downwards", targeting
5+
a method declared in a subclass of the enclosing class. Data flow analysis
6+
has also improved to avoid spurious flow between unrelated classes in the class hierarchy.

‎javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,4 +12,4 @@ accessorCall
1212
| accessors.js:44:1:44:9 | new D().f | accessors.js:37:8:37:13 | (x) {} |
1313
| accessors.js:48:1:48:5 | obj.f | accessors.js:5:8:5:12 | () {} |
1414
| accessors.js:51:1:51:3 | C.f | accessors.js:19:15:19:19 | () {} |
15-
| accessors.js:54:1:54:9 | new D().f | accessors.js:34:8:34:12 | () {} |
15+
| accessors.js:55:1:55:3 | d.f | accessors.js:34:8:34:12 | () {} |

‎javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.ql‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,20 +58,23 @@ class AnnotatedCall extends DataFlow::Node {
5858
string getKind() { result = kind }
5959
}
6060

61-
predicate callEdge(AnnotatedCall call, AnnotatedFunction target, int boundArgs) {
61+
predicate callEdge(AnnotatedCall call, Function target, int boundArgs) {
6262
FlowSteps::calls(call, target) and boundArgs = -1
6363
or
6464
FlowSteps::callsBound(call, target, boundArgs)
6565
}
6666

67-
query predicate spuriousCallee(
68-
AnnotatedCall call, AnnotatedFunction target, int boundArgs, string kind
69-
) {
67+
query predicate spuriousCallee(AnnotatedCall call, Function target, int boundArgs, string kind) {
7068
callEdge(call, target, boundArgs) and
7169
kind = call.getKind() and
7270
not (
7371
target = call.getAnExpectedCallee(kind) and
7472
boundArgs = call.getBoundArgsOrMinusOne()
73+
) and
74+
(
75+
target instanceof AnnotatedFunction
76+
or
77+
call.getCallTargetName() = "NONE"
7578
)
7679
}
7780

‎javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/accessors.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,5 +50,6 @@ obj.f();
5050
/** calls:NONE */
5151
C.f();
5252

53+
const d = new D();
5354
/** calls:NONE */
54-
new D().f();
55+
d.f();

‎javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import 'dummy';
2+
3+
class Base {
4+
workInBase() {
5+
/** calls:methodInBase */
6+
this.methodInBase();
7+
8+
/** calls:methodInSub1 calls:methodInSub2 */
9+
this.methodInSub();
10+
11+
/** calls:overriddenInSub0 calls:overriddenInSub1 calls:overriddenInSub2 */
12+
this.overriddenInSub();
13+
}
14+
15+
/** name:methodInBase */
16+
methodInBase() {
17+
/** calls:methodInSub1 calls:methodInSub2 */
18+
this.methodInSub();
19+
}
20+
21+
/** name:overriddenInSub0 */
22+
overriddenInSub() {
23+
}
24+
}
25+
26+
class Subclass1 extends Base {
27+
workInSub() {
28+
/** calls:methodInBase */
29+
this.methodInBase();
30+
31+
/** calls:overriddenInSub1 */
32+
this.overriddenInSub();
33+
}
34+
35+
/** name:methodInSub1 */
36+
methodInSub() {
37+
}
38+
39+
/** name:overriddenInSub1 */
40+
overriddenInSub() {
41+
}
42+
}
43+
44+
class Subclass2 extends Base {
45+
workInSub() {
46+
/** calls:methodInBase */
47+
this.methodInBase();
48+
49+
/** calls:overriddenInSub2 */
50+
this.overriddenInSub();
51+
}
52+
53+
/** name:methodInSub2 */
54+
methodInSub() {
55+
}
56+
57+
/** name:overriddenInSub2 */
58+
overriddenInSub() {
59+
}
60+
}

‎javascript/ql/test/library-tests/CallGraphs/FullTest/tests.expected‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ test_getAFunctionValue
5656
| classes.js:3:10:5:5 | () {\\n ... ;\\n } | classes.js:3:10:5:5 | () {\\n ... ;\\n } |
5757
| classes.js:7:6:9:5 | () {\\n ... ;\\n } | classes.js:7:6:9:5 | () {\\n ... ;\\n } |
5858
| classes.js:8:7:8:16 | this.hello | classes.js:3:10:5:5 | () {\\n ... ;\\n } |
59+
| classes.js:8:7:8:16 | this.hello | classes.js:13:10:15:5 | () {\\n ... ;\\n } |
5960
| classes.js:12:3:16:3 | B | classes.js:12:21:12:20 | (...arg ... rgs); } |
6061
| classes.js:12:3:16:3 | class B ... }\\n } | classes.js:12:21:12:20 | (...arg ... rgs); } |
6162
| classes.js:12:19:12:19 | A | classes.js:2:11:2:10 | () {} |
@@ -447,6 +448,7 @@ test_getACallee
447448
| a.js:3:1:3:5 | bar() | b.js:2:8:2:24 | function bar() {} |
448449
| a.js:4:1:4:5 | qux() | c.js:2:8:2:24 | function bar() {} |
449450
| classes.js:8:7:8:18 | this.hello() | classes.js:3:10:5:5 | () {\\n ... ;\\n } |
451+
| classes.js:8:7:8:18 | this.hello() | classes.js:13:10:15:5 | () {\\n ... ;\\n } |
450452
| classes.js:12:21:12:20 | super(...args) | classes.js:2:11:2:10 | () {} |
451453
| classes.js:18:3:18:9 | new B() | classes.js:12:21:12:20 | (...arg ... rgs); } |
452454
| 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‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
import 'dummy';
2+
3+
class Base {
4+
baseMethod(x) {
5+
this.subclassMethod(x);
6+
}
7+
}
8+
9+
class Subclass1 extends Base {
10+
work() {
11+
this.baseMethod(source("sub1"));
12+
}
13+
subclassMethod(x) {
14+
sink(x); // $ hasValueFlow=sub1
15+
}
16+
}
17+
18+
class Subclass2 extends Base {
19+
work() {
20+
this.baseMethod(source("sub2"));
21+
}
22+
subclassMethod(x) {
23+
sink(x); // $ hasValueFlow=sub2
24+
}
25+
}
26+
27+
class Subclass3 extends Base {
28+
work() {
29+
this.baseMethod("safe");
30+
}
31+
subclassMethod(x) {
32+
sink(x);
33+
}
34+
}

0 commit comments

Comments
 (0)