Skip to content

Commit 663d4e8

Browse files
authored
Merge pull request github#12592 from erik-krogh/rhsRegress
JS: Fix performance regression in the `GetLaterAccess` module.
2 parents 5dc5c8e + bdab57b commit 663d4e8

File tree

2 files changed

+48
-17
lines changed

2 files changed

+48
-17
lines changed

javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll

Lines changed: 40 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,19 @@ module AccessPath {
270270

271271
/** A module for computing an access to a variable that happens after a property has been written onto it */
272272
private module GetLaterAccess {
273+
/**
274+
* Gets an reference to the SSA variable `variable`.
275+
* Either the definition or a use of the SSA variable
276+
*/
277+
private VarRef getAVariableRef(SsaVariable variable) {
278+
(
279+
result = variable.getAUse()
280+
or
281+
result = variable.getDefinition().(SsaExplicitDefinition).getDef().getTarget()
282+
) and
283+
variable = getARelevantVariableSimple()
284+
}
285+
273286
/**
274287
* Gets an access to a variable that is written to in `write`, where the access is after the write.
275288
*
@@ -286,7 +299,7 @@ module AccessPath {
286299
pragma[noopt]
287300
DataFlow::Node getLaterBaseAccess(DataFlow::PropWrite write) {
288301
exists(
289-
ControlFlowNode writeNode, BindingPattern access, VarRef otherAccess, Variable variable,
302+
ControlFlowNode writeNode, BindingPattern access, VarRef otherAccess, SsaVariable variable,
290303
StmtContainer container
291304
|
292305
access = getBaseVar(write) and
@@ -303,7 +316,7 @@ module AccessPath {
303316
i < j
304317
)
305318
or
306-
otherAccess.getBasicBlock() = getASuccessorBBThatReadsVar(write) // more manual magic - outlined into a helper predicate.
319+
otherAccess.getBasicBlock() = getASuccessorBBThatReadsVar(write)
307320
)
308321
}
309322

@@ -323,24 +336,34 @@ module AccessPath {
323336
}
324337

325338
/** Gets an access to `var` inside `container` where `usedInWrite` indicates whether the access is the base of a property write. */
326-
private VarRef getAnAccessInContainer(Variable var, StmtContainer container, boolean usedInWrite) {
327-
result.getVariable() = var and
339+
private VarRef getAnAccessInContainer(
340+
SsaVariable var, StmtContainer container, boolean usedInWrite
341+
) {
342+
result = getAVariableRef(var) and
328343
result.getContainer() = container and
329-
var.isLocal() and
330344
if result = getBaseVar(_) then usedInWrite = true else usedInWrite = false
331345
}
332346

333-
/** Gets a variable that is relevant for the computations in the `GetLaterAccess` module. */
334-
private Variable getARelevantVariable() {
347+
/**
348+
* Gets a variable that is relevant for the computations in the `GetLaterAccess` module.
349+
* This predicate restricts as much as it can, but without depending on `getAVariableRef`.
350+
*/
351+
pragma[inline]
352+
private SsaVariable getARelevantVariableSimple() {
335353
// The variable might be used where `getLaterBaseAccess()` is called.
336354
exists(DataFlow::Node node |
337355
exists(fromRhs(node, _)) and
338-
node.asExpr().(VarAccess).getVariable() = result
339-
) and
356+
node.asExpr() = result.getAUse()
357+
)
358+
}
359+
360+
/**
361+
* Gets a variable that is relevant for the computations in the `GetLaterAccess` module.
362+
* This predicate depends on `getAVariableRef`, which in turn depends on `getARelevantVariableSimple`.
363+
*/
364+
private SsaVariable getARelevantVariable() {
340365
// There is a write that writes to the variable.
341-
getBaseVar(_).getVariable() = result and
342-
// It's local.
343-
result.isLocal() and // we skip global variables, because that turns messy quick.
366+
getBaseVar(_) = getAVariableRef(result) and
344367
// There is both a "write" and "read" in the same container of the variable.
345368
exists(StmtContainer container |
346369
exists(getAnAccessInContainer(result, container, true)) and // a "write", an access to the variable that is the base of a property reference.
@@ -350,15 +373,15 @@ module AccessPath {
350373

351374
/** Gets a basic-block that has a read of the variable that is written to by `write`, where the basicblock occurs after `start`. */
352375
private ReachableBasicBlock getASuccessorBBThatReadsVar(DataFlow::PropWrite write) {
353-
exists(VarAccess baseExpr, Variable var, ControlFlowNode writeNode |
376+
exists(VarRef baseExpr, SsaVariable var, ControlFlowNode writeNode |
354377
baseExpr = getBaseVar(write) and
355-
var = baseExpr.getVariable() and
378+
getAVariableRef(var) = baseExpr and
356379
var = getARelevantVariable() and
357380
writeNode = write.getWriteNode() and
358-
writeNode.getBasicBlock().(ReachableBasicBlock).strictlyDominates(result) and
359-
// manual magic.
360-
result = getAnAccessInContainer(getARelevantVariable(), _, false).getBasicBlock()
381+
result.getImmediateDominator() = writeNode.getBasicBlock()
361382
)
383+
or
384+
result.getImmediateDominator() = getASuccessorBBThatReadsVar(write)
362385
}
363386
}
364387

javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/UnsafeCodeConstruction.expected

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,11 @@ nodes
4242
| lib/index.js:112:17:112:21 | taint |
4343
| lib/index.js:112:17:112:21 | taint |
4444
| lib/index.js:113:20:113:24 | taint |
45+
| lib/index.js:115:38:115:42 | taint |
4546
| lib/index.js:121:34:121:38 | taint |
4647
| lib/index.js:129:32:129:36 | taint |
48+
| lib/index.js:135:23:135:49 | this.op ... dOption |
49+
| lib/index.js:135:23:135:49 | this.op ... dOption |
4750
| lib/index.js:136:23:136:49 | this.op ... dOption |
4851
| lib/index.js:136:23:136:49 | this.op ... dOption |
4952
| lib/index.js:137:23:137:49 | this.op ... dOption |
@@ -94,12 +97,16 @@ edges
9497
| lib/index.js:98:30:98:34 | taint | lib/index.js:105:21:105:47 | this.op ... dOption |
9598
| lib/index.js:112:17:112:21 | taint | lib/index.js:113:20:113:24 | taint |
9699
| lib/index.js:112:17:112:21 | taint | lib/index.js:113:20:113:24 | taint |
100+
| lib/index.js:112:17:112:21 | taint | lib/index.js:115:38:115:42 | taint |
101+
| lib/index.js:112:17:112:21 | taint | lib/index.js:115:38:115:42 | taint |
97102
| lib/index.js:112:17:112:21 | taint | lib/index.js:121:34:121:38 | taint |
98103
| lib/index.js:112:17:112:21 | taint | lib/index.js:121:34:121:38 | taint |
99104
| lib/index.js:112:17:112:21 | taint | lib/index.js:129:32:129:36 | taint |
100105
| lib/index.js:112:17:112:21 | taint | lib/index.js:129:32:129:36 | taint |
101106
| lib/index.js:113:20:113:24 | taint | lib/index.js:138:23:138:32 | this.taint |
102107
| lib/index.js:113:20:113:24 | taint | lib/index.js:138:23:138:32 | this.taint |
108+
| lib/index.js:115:38:115:42 | taint | lib/index.js:135:23:135:49 | this.op ... dOption |
109+
| lib/index.js:115:38:115:42 | taint | lib/index.js:135:23:135:49 | this.op ... dOption |
103110
| lib/index.js:121:34:121:38 | taint | lib/index.js:136:23:136:49 | this.op ... dOption |
104111
| lib/index.js:121:34:121:38 | taint | lib/index.js:136:23:136:49 | this.op ... dOption |
105112
| lib/index.js:129:32:129:36 | taint | lib/index.js:137:23:137:49 | this.op ... dOption |
@@ -114,6 +121,7 @@ edges
114121
| lib/index.js:104:21:104:47 | this.op ... dOption | lib/index.js:86:15:86:19 | taint | lib/index.js:104:21:104:47 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:86:15:86:19 | taint | library input | lib/index.js:104:10:104:67 | " var ... ing();" | interpreted as code |
115122
| lib/index.js:105:21:105:47 | this.op ... dOption | lib/index.js:86:15:86:19 | taint | lib/index.js:105:21:105:47 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:86:15:86:19 | taint | library input | lib/index.js:105:10:105:67 | " var ... ing();" | interpreted as code |
116123
| lib/index.js:106:21:106:30 | this.taint | lib/index.js:86:15:86:19 | taint | lib/index.js:106:21:106:30 | this.taint | This string concatenation which depends on $@ is later $@. | lib/index.js:86:15:86:19 | taint | library input | lib/index.js:106:10:106:50 | " var ... ing();" | interpreted as code |
124+
| lib/index.js:135:23:135:49 | this.op ... dOption | lib/index.js:112:17:112:21 | taint | lib/index.js:135:23:135:49 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:112:17:112:21 | taint | library input | lib/index.js:135:12:135:69 | " var ... ing();" | interpreted as code |
117125
| lib/index.js:136:23:136:49 | this.op ... dOption | lib/index.js:112:17:112:21 | taint | lib/index.js:136:23:136:49 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:112:17:112:21 | taint | library input | lib/index.js:136:12:136:69 | " var ... ing();" | interpreted as code |
118126
| lib/index.js:137:23:137:49 | this.op ... dOption | lib/index.js:112:17:112:21 | taint | lib/index.js:137:23:137:49 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:112:17:112:21 | taint | library input | lib/index.js:137:12:137:69 | " var ... ing();" | interpreted as code |
119127
| lib/index.js:138:23:138:32 | this.taint | lib/index.js:112:17:112:21 | taint | lib/index.js:138:23:138:32 | this.taint | This string concatenation which depends on $@ is later $@. | lib/index.js:112:17:112:21 | taint | library input | lib/index.js:138:12:138:52 | " var ... ing();" | interpreted as code |

0 commit comments

Comments
 (0)