Skip to content

Commit f0f4fe7

Browse files
hvitvedhvitved
authored
Merge pull request #10444 from hvitved/ruby/stmt-sequence-post-update
Ruby: Add post-update nodes for compound arguments
2 parents 7d0bfe8 + 07f8b35 commit f0f4fe7

File tree

13 files changed

+506
-154
lines changed

13 files changed

+506
-154
lines changed

‎cpp/ql/lib/experimental/semmle/code/cpp/ir/dataflow/internal/DataFlowImplConsistency.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,12 @@ module Consistency {
3232

3333
/** Holds if `n` should be excluded from the consistency test `reverseRead`. */
3434
predicate reverseReadExclude(Node n) { none() }
35+
36+
/** Holds if `n` should be excluded from the consistency test `postHasUniquePre`. */
37+
predicate postHasUniquePreExclude(PostUpdateNode n) { none() }
38+
39+
/** Holds if `n` should be excluded from the consistency test `uniquePostUpdate`. */
40+
predicate uniquePostUpdateExclude(Node n) { none() }
3541
}
3642

3743
private class RelevantNode extends Node {
@@ -166,6 +172,7 @@ module Consistency {
166172
}
167173

168174
query predicate postHasUniquePre(PostUpdateNode n, string msg) {
175+
not any(ConsistencyConfiguration conf).postHasUniquePreExclude(n) and
169176
exists(int c |
170177
c = count(n.getPreUpdateNode()) and
171178
c != 1 and
@@ -174,6 +181,7 @@ module Consistency {
174181
}
175182

176183
query predicate uniquePostUpdate(Node n, string msg) {
184+
not any(ConsistencyConfiguration conf).uniquePostUpdateExclude(n) and
177185
1 < strictcount(PostUpdateNode post | post.getPreUpdateNode() = n) and
178186
msg = "Node has multiple PostUpdateNodes."
179187
}

‎cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowImplConsistency.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,12 @@ module Consistency {
3232

3333
/** Holds if `n` should be excluded from the consistency test `reverseRead`. */
3434
predicate reverseReadExclude(Node n) { none() }
35+
36+
/** Holds if `n` should be excluded from the consistency test `postHasUniquePre`. */
37+
predicate postHasUniquePreExclude(PostUpdateNode n) { none() }
38+
39+
/** Holds if `n` should be excluded from the consistency test `uniquePostUpdate`. */
40+
predicate uniquePostUpdateExclude(Node n) { none() }
3541
}
3642

3743
private class RelevantNode extends Node {
@@ -166,6 +172,7 @@ module Consistency {
166172
}
167173

168174
query predicate postHasUniquePre(PostUpdateNode n, string msg) {
175+
not any(ConsistencyConfiguration conf).postHasUniquePreExclude(n) and
169176
exists(int c |
170177
c = count(n.getPreUpdateNode()) and
171178
c != 1 and
@@ -174,6 +181,7 @@ module Consistency {
174181
}
175182

176183
query predicate uniquePostUpdate(Node n, string msg) {
184+
not any(ConsistencyConfiguration conf).uniquePostUpdateExclude(n) and
177185
1 < strictcount(PostUpdateNode post | post.getPreUpdateNode() = n) and
178186
msg = "Node has multiple PostUpdateNodes."
179187
}

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowImplConsistency.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,12 @@ module Consistency {
3232

3333
/** Holds if `n` should be excluded from the consistency test `reverseRead`. */
3434
predicate reverseReadExclude(Node n) { none() }
35+
36+
/** Holds if `n` should be excluded from the consistency test `postHasUniquePre`. */
37+
predicate postHasUniquePreExclude(PostUpdateNode n) { none() }
38+
39+
/** Holds if `n` should be excluded from the consistency test `uniquePostUpdate`. */
40+
predicate uniquePostUpdateExclude(Node n) { none() }
3541
}
3642

3743
private class RelevantNode extends Node {
@@ -166,6 +172,7 @@ module Consistency {
166172
}
167173

168174
query predicate postHasUniquePre(PostUpdateNode n, string msg) {
175+
not any(ConsistencyConfiguration conf).postHasUniquePreExclude(n) and
169176
exists(int c |
170177
c = count(n.getPreUpdateNode()) and
171178
c != 1 and
@@ -174,6 +181,7 @@ module Consistency {
174181
}
175182

176183
query predicate uniquePostUpdate(Node n, string msg) {
184+
not any(ConsistencyConfiguration conf).uniquePostUpdateExclude(n) and
177185
1 < strictcount(PostUpdateNode post | post.getPreUpdateNode() = n) and
178186
msg = "Node has multiple PostUpdateNodes."
179187
}

‎csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowImplConsistency.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,12 @@ module Consistency {
3232

3333
/** Holds if `n` should be excluded from the consistency test `reverseRead`. */
3434
predicate reverseReadExclude(Node n) { none() }
35+
36+
/** Holds if `n` should be excluded from the consistency test `postHasUniquePre`. */
37+
predicate postHasUniquePreExclude(PostUpdateNode n) { none() }
38+
39+
/** Holds if `n` should be excluded from the consistency test `uniquePostUpdate`. */
40+
predicate uniquePostUpdateExclude(Node n) { none() }
3541
}
3642

3743
private class RelevantNode extends Node {
@@ -166,6 +172,7 @@ module Consistency {
166172
}
167173

168174
query predicate postHasUniquePre(PostUpdateNode n, string msg) {
175+
not any(ConsistencyConfiguration conf).postHasUniquePreExclude(n) and
169176
exists(int c |
170177
c = count(n.getPreUpdateNode()) and
171178
c != 1 and
@@ -174,6 +181,7 @@ module Consistency {
174181
}
175182

176183
query predicate uniquePostUpdate(Node n, string msg) {
184+
not any(ConsistencyConfiguration conf).uniquePostUpdateExclude(n) and
177185
1 < strictcount(PostUpdateNode post | post.getPreUpdateNode() = n) and
178186
msg = "Node has multiple PostUpdateNodes."
179187
}

‎docs/ql-libraries/dataflow/dataflow.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,10 @@ through an additional step targeting a `PostUpdateNode`).
294294

295295
It is recommended to introduce `PostUpdateNode`s for all `ArgumentNode`s (this
296296
can be skipped for immutable arguments), and all field qualifiers for both
297-
reads and stores.
297+
reads and stores. Note also that in the case of compund arguments, such as
298+
`b ? x : y`, it is recommented to have post-update nodes for `x` and `y` (and
299+
not the compound argument itself), and let `[post update] x` have both `x`
300+
and `b ? x : y` as pre-update nodes (and similarly for `[post update] y`).
298301

299302
Remember to define local flow for `PostUpdateNode`s as well in
300303
`simpleLocalFlowStep`. In general out-going local flow from `PostUpdateNode`s

‎java/ql/lib/semmle/code/java/dataflow/internal/DataFlowImplConsistency.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,12 @@ module Consistency {
3232

3333
/** Holds if `n` should be excluded from the consistency test `reverseRead`. */
3434
predicate reverseReadExclude(Node n) { none() }
35+
36+
/** Holds if `n` should be excluded from the consistency test `postHasUniquePre`. */
37+
predicate postHasUniquePreExclude(PostUpdateNode n) { none() }
38+
39+
/** Holds if `n` should be excluded from the consistency test `uniquePostUpdate`. */
40+
predicate uniquePostUpdateExclude(Node n) { none() }
3541
}
3642

3743
private class RelevantNode extends Node {
@@ -166,6 +172,7 @@ module Consistency {
166172
}
167173

168174
query predicate postHasUniquePre(PostUpdateNode n, string msg) {
175+
not any(ConsistencyConfiguration conf).postHasUniquePreExclude(n) and
169176
exists(int c |
170177
c = count(n.getPreUpdateNode()) and
171178
c != 1 and
@@ -174,6 +181,7 @@ module Consistency {
174181
}
175182

176183
query predicate uniquePostUpdate(Node n, string msg) {
184+
not any(ConsistencyConfiguration conf).uniquePostUpdateExclude(n) and
177185
1 < strictcount(PostUpdateNode post | post.getPreUpdateNode() = n) and
178186
msg = "Node has multiple PostUpdateNodes."
179187
}

‎python/ql/lib/semmle/python/dataflow/new/internal/DataFlowImplConsistency.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,12 @@ module Consistency {
3232

3333
/** Holds if `n` should be excluded from the consistency test `reverseRead`. */
3434
predicate reverseReadExclude(Node n) { none() }
35+
36+
/** Holds if `n` should be excluded from the consistency test `postHasUniquePre`. */
37+
predicate postHasUniquePreExclude(PostUpdateNode n) { none() }
38+
39+
/** Holds if `n` should be excluded from the consistency test `uniquePostUpdate`. */
40+
predicate uniquePostUpdateExclude(Node n) { none() }
3541
}
3642

3743
private class RelevantNode extends Node {
@@ -166,6 +172,7 @@ module Consistency {
166172
}
167173

168174
query predicate postHasUniquePre(PostUpdateNode n, string msg) {
175+
not any(ConsistencyConfiguration conf).postHasUniquePreExclude(n) and
169176
exists(int c |
170177
c = count(n.getPreUpdateNode()) and
171178
c != 1 and
@@ -174,6 +181,7 @@ module Consistency {
174181
}
175182

176183
query predicate uniquePostUpdate(Node n, string msg) {
184+
not any(ConsistencyConfiguration conf).uniquePostUpdateExclude(n) and
177185
1 < strictcount(PostUpdateNode post | post.getPreUpdateNode() = n) and
178186
msg = "Node has multiple PostUpdateNodes."
179187
}

‎ruby/ql/consistency-queries/DataFlowConsistency.ql

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import codeql.ruby.AST
2+
import codeql.ruby.CFG
23
import codeql.ruby.DataFlow::DataFlow
34
import codeql.ruby.dataflow.internal.DataFlowPrivate
45
import codeql.ruby.dataflow.internal.DataFlowImplConsistency::Consistency
@@ -13,6 +14,22 @@ private class MyConsistencyConfiguration extends ConsistencyConfiguration {
1314
or
1415
n instanceof SynthHashSplatArgumentNode
1516
or
16-
not isNonConstantExpr(n.asExpr())
17+
not isNonConstantExpr(getAPostUpdateNodeForArg(n.asExpr()))
18+
}
19+
20+
override predicate postHasUniquePreExclude(PostUpdateNode n) {
21+
exists(CfgNodes::ExprCfgNode e, CfgNodes::ExprCfgNode arg |
22+
e = getAPostUpdateNodeForArg(arg) and
23+
e != arg and
24+
n = TExprPostUpdateNode(e)
25+
)
26+
}
27+
28+
override predicate uniquePostUpdateExclude(Node n) {
29+
exists(CfgNodes::ExprCfgNode e, CfgNodes::ExprCfgNode arg |
30+
e = getAPostUpdateNodeForArg(arg) and
31+
e != arg and
32+
n.asExpr() = arg
33+
)
1734
}
1835
}

‎ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowImplConsistency.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,12 @@ module Consistency {
3232

3333
/** Holds if `n` should be excluded from the consistency test `reverseRead`. */
3434
predicate reverseReadExclude(Node n) { none() }
35+
36+
/** Holds if `n` should be excluded from the consistency test `postHasUniquePre`. */
37+
predicate postHasUniquePreExclude(PostUpdateNode n) { none() }
38+
39+
/** Holds if `n` should be excluded from the consistency test `uniquePostUpdate`. */
40+
predicate uniquePostUpdateExclude(Node n) { none() }
3541
}
3642

3743
private class RelevantNode extends Node {
@@ -166,6 +172,7 @@ module Consistency {
166172
}
167173

168174
query predicate postHasUniquePre(PostUpdateNode n, string msg) {
175+
not any(ConsistencyConfiguration conf).postHasUniquePreExclude(n) and
169176
exists(int c |
170177
c = count(n.getPreUpdateNode()) and
171178
c != 1 and
@@ -174,6 +181,7 @@ module Consistency {
174181
}
175182

176183
query predicate uniquePostUpdate(Node n, string msg) {
184+
not any(ConsistencyConfiguration conf).uniquePostUpdateExclude(n) and
177185
1 < strictcount(PostUpdateNode post | post.getPreUpdateNode() = n) and
178186
msg = "Node has multiple PostUpdateNodes."
179187
}

‎ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll

Lines changed: 41 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,32 @@ private class ExprNodeImpl extends ExprNode, NodeImpl {
4343
override string toStringImpl() { result = this.getExprNode().toString() }
4444
}
4545

46+
/**
47+
* Gets a node that may execute last in `n`, and which, when it executes last,
48+
* will be the value of `n`.
49+
*/
50+
private CfgNodes::ExprCfgNode getALastEvalNode(CfgNodes::ExprCfgNode n) {
51+
result = n.(CfgNodes::ExprNodes::StmtSequenceCfgNode).getLastStmt()
52+
or
53+
result = n.(CfgNodes::ExprNodes::ConditionalExprCfgNode).getBranch(_)
54+
or
55+
exists(CfgNodes::AstCfgNode branch |
56+
branch = n.(CfgNodes::ExprNodes::CaseExprCfgNode).getBranch(_)
57+
|
58+
result = branch.(CfgNodes::ExprNodes::InClauseCfgNode).getBody()
59+
or
60+
result = branch.(CfgNodes::ExprNodes::WhenClauseCfgNode).getBody()
61+
or
62+
result = branch
63+
)
64+
}
65+
66+
/** Gets a node for which to construct a post-update node for argument `arg`. */
67+
CfgNodes::ExprCfgNode getAPostUpdateNodeForArg(Argument arg) {
68+
result = getALastEvalNode*(arg) and
69+
not exists(getALastEvalNode(result))
70+
}
71+
4672
/** Provides predicates related to local data flow. */
4773
module LocalFlow {
4874
private import codeql.ruby.dataflow.internal.SsaImpl
@@ -135,19 +161,7 @@ module LocalFlow {
135161
or
136162
nodeFrom.asExpr() = nodeTo.asExpr().(CfgNodes::ExprNodes::BlockArgumentCfgNode).getValue()
137163
or
138-
nodeFrom.asExpr() = nodeTo.asExpr().(CfgNodes::ExprNodes::StmtSequenceCfgNode).getLastStmt()
139-
or
140-
nodeFrom.asExpr() = nodeTo.asExpr().(CfgNodes::ExprNodes::ConditionalExprCfgNode).getBranch(_)
141-
or
142-
exists(CfgNodes::AstCfgNode branch |
143-
branch = nodeTo.asExpr().(CfgNodes::ExprNodes::CaseExprCfgNode).getBranch(_)
144-
|
145-
nodeFrom.asExpr() = branch.(CfgNodes::ExprNodes::InClauseCfgNode).getBody()
146-
or
147-
nodeFrom.asExpr() = branch.(CfgNodes::ExprNodes::WhenClauseCfgNode).getBody()
148-
or
149-
nodeFrom.asExpr() = branch
150-
)
164+
nodeFrom.asExpr() = getALastEvalNode(nodeTo.asExpr())
151165
or
152166
exists(CfgNodes::ExprCfgNode exprTo, ReturningStatementNode n |
153167
nodeFrom = n and
@@ -241,7 +255,8 @@ private module Cached {
241255
// filter out nodes that clearly don't need post-update nodes
242256
isNonConstantExpr(n) and
243257
(
244-
n instanceof Argument or
258+
n = getAPostUpdateNodeForArg(_)
259+
or
245260
n = any(CfgNodes::ExprNodes::InstanceVariableAccessCfgNode v).getReceiver()
246261
)
247262
} or
@@ -1127,7 +1142,18 @@ private module PostUpdateNodes {
11271142

11281143
ExprPostUpdateNode() { this = TExprPostUpdateNode(e) }
11291144

1130-
override ExprNode getPreUpdateNode() { e = result.getExprNode() }
1145+
override ExprNode getPreUpdateNode() {
1146+
// For compund arguments, such as `m(if b then x else y)`, we want the leaf nodes
1147+
// `[post] x` and `[post] y` to have two pre-update nodes: (1) the compund argument,
1148+
// `if b then x else y`; and the (2) the underlying expressions; `x` and `y`,
1149+
// respectively.
1150+
//
1151+
// This ensures that we get flow out of the call into both leafs (1), while still
1152+
// maintaining the invariant that the underlying expression is a pre-update node (2).
1153+
e = getAPostUpdateNodeForArg(result.getExprNode())
1154+
or
1155+
e = result.getExprNode()
1156+
}
11311157

11321158
override CfgScope getCfgScope() { result = e.getExpr().getCfgScope() }
11331159

0 commit comments

Comments
 (0)