Skip to content

Commit 3f396ac

Browse files
hvitvedhvitved
authored
Merge pull request #371 from github/hvitved/dataflow/arg-sugar
Data flow: Fix bug for sugared call arguments
2 parents e26cf7c + c57b7c5 commit 3f396ac

File tree

7 files changed

+90
-45
lines changed

7 files changed

+90
-45
lines changed

‎ruby/ql/lib/codeql/ruby/ast/Method.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ private import internal.AST
44
private import internal.TreeSitter
55

66
/** A callable. */
7-
class Callable extends Expr, Scope, TCallable {
7+
class Callable extends StmtSequence, Expr, Scope, TCallable {
88
/** Gets the number of parameters of this callable. */
99
final int getNumberOfParameters() { result = count(this.getAParameter()) }
1010

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

Lines changed: 37 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -97,15 +97,20 @@ module LocalFlow {
9797
}
9898
}
9999

100-
/** An argument of a call (including qualifier arguments). */
101-
private class Argument extends Expr {
102-
private Call call;
100+
/** An argument of a call (including qualifier arguments, excluding block arguments). */
101+
private class Argument extends CfgNodes::ExprCfgNode {
102+
private CfgNodes::ExprNodes::CallCfgNode call;
103103
private int arg;
104104

105-
Argument() { this = call.getArgument(arg) }
105+
Argument() {
106+
this = call.getArgument(arg) and
107+
not this.getExpr() instanceof BlockArgument
108+
or
109+
this = call.getReceiver() and arg = -1
110+
}
106111

107112
/** Holds if this expression is the `i`th argument of `c`. */
108-
predicate isArgumentOf(Expr c, int i) { c = call and i = arg }
113+
predicate isArgumentOf(CfgNodes::ExprNodes::CallCfgNode c, int i) { c = call and i = arg }
109114
}
110115

111116
/** A collection of cached types and predicates to be evaluated in the same stage. */
@@ -125,14 +130,7 @@ private module Cached {
125130
TNormalParameterNode(Parameter p) { not p instanceof BlockParameter } or
126131
TSelfParameterNode(MethodBase m) or
127132
TBlockParameterNode(MethodBase m) or
128-
TExprPostUpdateNode(CfgNodes::ExprCfgNode n) {
129-
exists(AstNode node | node = n.getNode() |
130-
node instanceof Argument and
131-
not node instanceof BlockArgument
132-
or
133-
n = any(CfgNodes::ExprNodes::CallCfgNode call).getReceiver()
134-
)
135-
} or
133+
TExprPostUpdateNode(CfgNodes::ExprCfgNode n) { n instanceof Argument } or
136134
TSummaryNode(
137135
FlowSummaryImpl::Public::SummarizedCallable c,
138136
FlowSummaryImpl::Private::SummaryNodeState state
@@ -438,24 +436,18 @@ abstract class ArgumentNode extends Node {
438436
private module ArgumentNodes {
439437
/** A data-flow node that represents an explicit call argument. */
440438
class ExplicitArgumentNode extends ArgumentNode {
441-
ExplicitArgumentNode() {
442-
this.asExpr().getExpr() instanceof Argument and
443-
not this.asExpr().getExpr() instanceof BlockArgument
444-
}
439+
Argument arg;
440+
441+
ExplicitArgumentNode() { this.asExpr() = arg }
445442

446443
override predicate sourceArgumentOf(CfgNodes::ExprNodes::CallCfgNode call, int pos) {
447-
this.asExpr() = call.getArgument(pos)
444+
arg.isArgumentOf(call, pos)
448445
}
449446
}
450447

451448
/** A data-flow node that represents the `self` argument of a call. */
452-
class SelfArgumentNode extends ArgumentNode {
453-
SelfArgumentNode() { this.asExpr() = any(CfgNodes::ExprNodes::CallCfgNode call).getReceiver() }
454-
455-
override predicate sourceArgumentOf(CfgNodes::ExprNodes::CallCfgNode call, int pos) {
456-
this.asExpr() = call.getReceiver() and
457-
pos = -1
458-
}
449+
class SelfArgumentNode extends ExplicitArgumentNode {
450+
SelfArgumentNode() { arg.isArgumentOf(_, -1) }
459451
}
460452

461453
/** A data-flow node that represents a block argument. */
@@ -522,8 +514,8 @@ private module ReturnNodes {
522514
}
523515

524516
/**
525-
* A data-flow node that represents an expression returned by a callable,
526-
* either using an explict `return` statement or as the expression of a method body.
517+
* A data-flow node that represents an expression explicitly returned by
518+
* a callable.
527519
*/
528520
class ExplicitReturnNode extends ReturningNode, ReturningStatementNode {
529521
ExplicitReturnNode() {
@@ -539,11 +531,25 @@ private module ReturnNodes {
539531
}
540532
}
541533

534+
pragma[noinline]
535+
private AstNode implicitReturn(Callable c, ExprNode n) {
536+
exists(CfgNodes::ExprCfgNode en |
537+
en = n.getExprNode() and
538+
en.getASuccessor().(CfgNodes::AnnotatedExitNode).isNormal() and
539+
n.(NodeImpl).getCfgScope() = c and
540+
result = en.getExpr()
541+
)
542+
or
543+
result = implicitReturn(c, n).getParent()
544+
}
545+
546+
/**
547+
* A data-flow node that represents an expression implicitly returned by
548+
* a callable. An implicit return happens when an expression can be the
549+
* last thing that is evaluated in the body of the callable.
550+
*/
542551
class ExprReturnNode extends ReturningNode, ExprNode {
543-
ExprReturnNode() {
544-
this.getExprNode().getASuccessor().(CfgNodes::AnnotatedExitNode).isNormal() and
545-
this.(NodeImpl).getCfgScope() instanceof Callable
546-
}
552+
ExprReturnNode() { exists(Callable c | implicitReturn(c, this) = c.getAStmt()) }
547553

548554
override ReturnKind getKind() { result instanceof NormalReturnKind }
549555
}

‎ruby/ql/test/library-tests/dataflow/local/Nodes.expected

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
ret
2+
| local_dataflow.rb:6:3:6:14 | ... = ... |
3+
| local_dataflow.rb:32:14:32:21 | "method" |
4+
| local_dataflow.rb:36:6:36:13 | return |
5+
| local_dataflow.rb:38:3:38:13 | "reachable" |
6+
| local_dataflow.rb:43:6:43:13 | return |
7+
| local_dataflow.rb:45:3:45:10 | return |
8+
| local_dataflow.rb:50:3:50:13 | next |
9+
| local_dataflow.rb:51:3:51:15 | break |
10+
| local_dataflow.rb:52:3:52:10 | "normal" |
11+
arg
12+
| local_dataflow.rb:3:8:3:10 | self | local_dataflow.rb:3:8:3:10 | call to p | -1 |
13+
| local_dataflow.rb:3:10:3:10 | a | local_dataflow.rb:3:8:3:10 | call to p | 0 |
14+
| local_dataflow.rb:6:8:6:8 | a | local_dataflow.rb:6:10:6:11 | ... + ... | -1 |
15+
| local_dataflow.rb:6:13:6:13 | b | local_dataflow.rb:6:10:6:11 | ... + ... | 0 |
16+
| local_dataflow.rb:9:9:9:15 | Array | local_dataflow.rb:9:9:9:15 | call to [] | -1 |
17+
| local_dataflow.rb:9:10:9:10 | 1 | local_dataflow.rb:9:9:9:15 | call to [] | 0 |
18+
| local_dataflow.rb:9:12:9:12 | 2 | local_dataflow.rb:9:9:9:15 | call to [] | 1 |
19+
| local_dataflow.rb:9:14:9:14 | 3 | local_dataflow.rb:9:9:9:15 | call to [] | 2 |
20+
| local_dataflow.rb:11:1:11:2 | self | local_dataflow.rb:11:1:11:2 | call to do | -1 |
21+
| local_dataflow.rb:12:3:12:5 | self | local_dataflow.rb:12:3:12:5 | call to p | -1 |
22+
| local_dataflow.rb:12:5:12:5 | x | local_dataflow.rb:12:3:12:5 | call to p | 0 |
23+
| local_dataflow.rb:20:6:20:6 | x | local_dataflow.rb:20:6:20:10 | ... > ... | -1 |
24+
| local_dataflow.rb:20:10:20:10 | 1 | local_dataflow.rb:20:6:20:10 | ... > ... | 0 |
25+
| local_dataflow.rb:35:6:35:6 | x | local_dataflow.rb:35:6:35:11 | ... == ... | -1 |
26+
| local_dataflow.rb:35:11:35:11 | 4 | local_dataflow.rb:35:6:35:11 | ... == ... | 0 |
27+
| local_dataflow.rb:42:6:42:6 | x | local_dataflow.rb:42:6:42:11 | ... == ... | -1 |
28+
| local_dataflow.rb:42:11:42:11 | 4 | local_dataflow.rb:42:6:42:11 | ... == ... | 0 |
29+
| local_dataflow.rb:49:1:53:3 | self | local_dataflow.rb:49:1:53:3 | call to m | -1 |
30+
| local_dataflow.rb:49:3:53:3 | do ... end | local_dataflow.rb:49:1:53:3 | call to m | -2 |
31+
| local_dataflow.rb:50:18:50:18 | x | local_dataflow.rb:50:18:50:22 | ... < ... | -1 |
32+
| local_dataflow.rb:50:22:50:22 | 4 | local_dataflow.rb:50:18:50:22 | ... < ... | 0 |
33+
| local_dataflow.rb:51:20:51:20 | x | local_dataflow.rb:51:20:51:24 | ... < ... | -1 |
34+
| local_dataflow.rb:51:24:51:24 | 9 | local_dataflow.rb:51:20:51:24 | ... < ... | 0 |
35+
| local_dataflow.rb:55:1:55:14 | self | local_dataflow.rb:55:1:55:14 | call to foo | -1 |
36+
| local_dataflow.rb:55:5:55:13 | Array | local_dataflow.rb:55:5:55:13 | call to [] | -1 |
37+
| local_dataflow.rb:55:5:55:13 | call to [] | local_dataflow.rb:55:1:55:14 | call to foo | 0 |
38+
| local_dataflow.rb:55:6:55:6 | 1 | local_dataflow.rb:55:5:55:13 | call to [] | 0 |
39+
| local_dataflow.rb:55:9:55:9 | 2 | local_dataflow.rb:55:5:55:13 | call to [] | 1 |
40+
| local_dataflow.rb:55:12:55:12 | 3 | local_dataflow.rb:55:5:55:13 | call to [] | 2 |

‎ruby/ql/test/library-tests/dataflow/local/Nodes.ql

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
import ruby
2+
import codeql.ruby.dataflow.internal.DataFlowPrivate
3+
import codeql.ruby.dataflow.internal.DataFlowDispatch
4+
5+
query predicate ret(ReturningNode node) { any() }
6+
7+
query predicate arg(ArgumentNode n, DataFlowCall call, int pos) { n.argumentOf(call, pos) }

‎ruby/ql/test/library-tests/dataflow/local/ReturnNodes.expected

Lines changed: 0 additions & 9 deletions
This file was deleted.

‎ruby/ql/test/library-tests/dataflow/local/ReturnNodes.ql

Lines changed: 0 additions & 4 deletions
This file was deleted.

‎ruby/ql/test/library-tests/dataflow/local/local_dataflow.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,3 +51,8 @@ def m x
5151
break "break" if x < 9
5252
"normal"
5353
end
54+
55+
foo([1, 2, 3])
56+
57+
def foo x
58+
end

0 commit comments

Comments
 (0)