Skip to content

Commit 7c35309

Browse files
asgerfasgerf
authored
Merge pull request #15823 from asgerf/js/lift-cg-restriction
JS: Call graph improvements
2 parents 245cd5c + ac4601c commit 7c35309

File tree

4 files changed

+39
-12
lines changed

4 files changed

+39
-12
lines changed

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

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1262,6 +1262,12 @@ module ClassNode {
12621262
result.getFile() = f
12631263
}
12641264

1265+
pragma[nomagic]
1266+
private DataFlow::NewNode getAnInstantiationInFile(string name, File f) {
1267+
result = AccessPath::getAReferenceTo(name).(DataFlow::LocalSourceNode).getAnInstantiation() and
1268+
result.getFile() = f
1269+
}
1270+
12651271
/**
12661272
* Gets a reference to the function `func`, where there exists a read/write of the "prototype" property on that reference.
12671273
*/
@@ -1273,7 +1279,7 @@ module ClassNode {
12731279
}
12741280

12751281
/**
1276-
* A function definition with prototype manipulation as a `ClassNode` instance.
1282+
* A function definition, targeted by a `new`-call or with prototype manipulation, seen as a `ClassNode` instance.
12771283
*/
12781284
class FunctionStyleClass extends Range, DataFlow::ValueNode {
12791285
override Function astNode;
@@ -1284,9 +1290,12 @@ module ClassNode {
12841290
(
12851291
exists(getAFunctionValueWithPrototype(function))
12861292
or
1287-
exists(string name |
1288-
this = AccessPath::getAnAssignmentTo(name) and
1293+
function = any(NewNode new).getCalleeNode().analyze().getAValue()
1294+
or
1295+
exists(string name | this = AccessPath::getAnAssignmentTo(name) |
12891296
exists(getAPrototypeReferenceInFile(name, this.getFile()))
1297+
or
1298+
exists(getAnInstantiationInFile(name, this.getFile()))
12901299
)
12911300
)
12921301
}

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

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -241,24 +241,25 @@ module CallGraph {
241241
)
242242
}
243243

244-
private DataFlow::FunctionNode getAMethodOnPlainObject(DataFlow::SourceNode node) {
244+
private DataFlow::FunctionNode getAMethodOnObject(DataFlow::SourceNode node) {
245245
(
246-
(
247-
node instanceof DataFlow::ObjectLiteralNode
248-
or
249-
node instanceof DataFlow::FunctionNode
250-
) and
251246
result = node.getAPropertySource()
252247
or
253248
result = node.(DataFlow::ObjectLiteralNode).getPropertyGetter(_)
254249
or
255250
result = node.(DataFlow::ObjectLiteralNode).getPropertySetter(_)
256251
) and
257-
not node.getTopLevel().isExterns()
252+
not node.getTopLevel().isExterns() and
253+
// Ignore writes to `this` inside a constructor, since this is already handled by instance method tracking
254+
not exists(DataFlow::ClassNode cls |
255+
node = cls.getConstructor().getReceiver()
256+
or
257+
node = cls.(DataFlow::ClassNode::FunctionStyleClass).getAPrototypeReference()
258+
)
258259
}
259260

260261
private predicate shouldTrackObjectWithMethods(DataFlow::SourceNode node) {
261-
exists(getAMethodOnPlainObject(node))
262+
exists(getAMethodOnObject(node))
262263
}
263264

264265
/**
@@ -292,7 +293,7 @@ module CallGraph {
292293
predicate impliedReceiverStep(DataFlow::SourceNode pred, DataFlow::SourceNode succ) {
293294
exists(DataFlow::SourceNode host |
294295
pred = getAnAllocationSiteRef(host) and
295-
succ = getAMethodOnPlainObject(host).getReceiver()
296+
succ = getAMethodOnObject(host).getReceiver()
296297
)
297298
}
298299
}

‎javascript/ql/src/change-notes/2024-03-07-lift-cg-restriction.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The call graph has been improved, leading to more alerts for data flow based queries.

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,20 +135,31 @@ test_getAFunctionValue
135135
| tst.js:3:1:3:1 | h | tst.js:3:5:3:17 | function() {} |
136136
| tst.js:3:1:3:17 | h = function() {} | tst.js:3:5:3:17 | function() {} |
137137
| tst.js:3:5:3:17 | function() {} | tst.js:3:5:3:17 | function() {} |
138+
| tst.js:4:1:4:1 | k | tst.js:2:9:2:21 | function() {} |
138139
| tst.js:4:1:4:5 | k = g | tst.js:2:9:2:21 | function() {} |
139140
| tst.js:4:5:4:5 | g | tst.js:2:9:2:21 | function() {} |
140141
| tst.js:6:1:6:1 | f | tst.js:1:1:1:15 | function f() {} |
141142
| tst.js:7:1:7:1 | g | tst.js:2:9:2:21 | function() {} |
142143
| tst.js:8:1:8:1 | h | tst.js:3:5:3:17 | function() {} |
143144
| tst.js:9:1:9:1 | k | tst.js:2:9:2:21 | function() {} |
144145
| tst.js:11:1:20:1 | functio ... \\tf();\\n} | tst.js:11:1:20:1 | functio ... \\tf();\\n} |
146+
| tst.js:11:12:11:12 | m | tst.js:2:9:2:21 | function() {} |
147+
| tst.js:11:12:11:12 | m | tst.js:2:9:2:21 | function() {} |
148+
| tst.js:12:6:12:6 | m | tst.js:2:9:2:21 | function() {} |
149+
| tst.js:12:6:12:27 | n | tst.js:2:9:2:21 | function() {} |
145150
| tst.js:12:6:12:27 | n | tst.js:12:15:12:27 | function() {} |
151+
| tst.js:12:10:12:10 | m | tst.js:2:9:2:21 | function() {} |
152+
| tst.js:12:10:12:10 | m | tst.js:2:9:2:21 | function() {} |
153+
| tst.js:12:10:12:10 | m | tst.js:2:9:2:21 | function() {} |
154+
| tst.js:12:10:12:27 | m \|\| function() {} | tst.js:2:9:2:21 | function() {} |
146155
| tst.js:12:10:12:27 | m \|\| function() {} | tst.js:12:15:12:27 | function() {} |
147156
| tst.js:12:15:12:27 | function() {} | tst.js:12:15:12:27 | function() {} |
148157
| tst.js:13:2:13:16 | function p() {} | tst.js:13:2:13:16 | function p() {} |
149158
| tst.js:13:11:13:11 | p | tst.js:13:2:13:16 | function p() {} |
159+
| tst.js:14:2:14:2 | m | tst.js:2:9:2:21 | function() {} |
150160
| tst.js:15:2:15:2 | l | tst.js:11:1:20:1 | functio ... \\tf();\\n} |
151161
| tst.js:16:2:16:17 | arguments.callee | tst.js:11:1:20:1 | functio ... \\tf();\\n} |
162+
| tst.js:17:2:17:2 | n | tst.js:2:9:2:21 | function() {} |
152163
| tst.js:17:2:17:2 | n | tst.js:12:15:12:27 | function() {} |
153164
| tst.js:18:2:18:2 | p | tst.js:13:2:13:16 | function p() {} |
154165
| tst.js:19:2:19:2 | f | tst.js:1:1:1:15 | function f() {} |
@@ -463,8 +474,10 @@ test_getACallee
463474
| tst.js:7:1:7:3 | g() | tst.js:2:9:2:21 | function() {} |
464475
| tst.js:8:1:8:3 | h() | tst.js:3:5:3:17 | function() {} |
465476
| tst.js:9:1:9:3 | k() | tst.js:2:9:2:21 | function() {} |
477+
| tst.js:14:2:14:4 | m() | tst.js:2:9:2:21 | function() {} |
466478
| tst.js:15:2:15:4 | l() | tst.js:11:1:20:1 | functio ... \\tf();\\n} |
467479
| tst.js:16:2:16:19 | arguments.callee() | tst.js:11:1:20:1 | functio ... \\tf();\\n} |
480+
| tst.js:17:2:17:4 | n() | tst.js:2:9:2:21 | function() {} |
468481
| tst.js:17:2:17:4 | n() | tst.js:12:15:12:27 | function() {} |
469482
| tst.js:18:2:18:4 | p() | tst.js:13:2:13:16 | function p() {} |
470483
| tst.js:19:2:19:4 | f() | tst.js:1:1:1:15 | function f() {} |

0 commit comments

Comments
 (0)