Skip to content

Commit a58aa17

Browse files
hmachmac
authored
Merge pull request github#13878 from hmac/splat-flow
Ruby: Track flow from splat arguments to positional parameters
2 parents 2e338cc + b03f6ef commit a58aa17

File tree

7 files changed

+354
-25
lines changed

7 files changed

+354
-25
lines changed

‎ruby/ql/lib/change-notes/2023-08-08-splat-arguments.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+
* Flow between splat arguments (`*args`) and positional parameters is now tracked more precisely.

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,6 +439,7 @@ private module Cached {
439439
} or
440440
THashSplatArgumentPosition() or
441441
TSplatAllArgumentPosition() or
442+
TSplatArgumentPosition(int pos) { exists(Call c | c.getArgument(pos) instanceof SplatExpr) } or
442443
TAnyArgumentPosition() or
443444
TAnyKeywordArgumentPosition()
444445

@@ -468,6 +469,10 @@ private module Cached {
468469
// synthetic parameter position.
469470
TSynthHashSplatParameterPosition() or
470471
TSplatAllParameterPosition() or
472+
TSplatParameterPosition(int pos) {
473+
exists(Parameter p | p.getPosition() = pos and p instanceof SplatParameter)
474+
} or
475+
TSynthSplatParameterPosition() or
471476
TAnyParameterPosition() or
472477
TAnyKeywordParameterPosition()
473478
}
@@ -1288,8 +1293,12 @@ class ParameterPosition extends TParameterPosition {
12881293

12891294
predicate isSynthHashSplat() { this = TSynthHashSplatParameterPosition() }
12901295

1296+
predicate isSynthSplat() { this = TSynthSplatParameterPosition() }
1297+
12911298
predicate isSplatAll() { this = TSplatAllParameterPosition() }
12921299

1300+
predicate isSplat(int n) { this = TSplatParameterPosition(n) }
1301+
12931302
/**
12941303
* Holds if this position represents any parameter, except `self` parameters. This
12951304
* includes both positional, named, and block parameters.
@@ -1320,6 +1329,10 @@ class ParameterPosition extends TParameterPosition {
13201329
this.isAny() and result = "any"
13211330
or
13221331
this.isAnyNamed() and result = "any-named"
1332+
or
1333+
this.isSynthSplat() and result = "synthetic *"
1334+
or
1335+
exists(int pos | this.isSplat(pos) and result = "* (position " + pos + ")")
13231336
}
13241337
}
13251338

@@ -1354,6 +1367,8 @@ class ArgumentPosition extends TArgumentPosition {
13541367

13551368
predicate isSplatAll() { this = TSplatAllArgumentPosition() }
13561369

1370+
predicate isSplat(int n) { this = TSplatArgumentPosition(n) }
1371+
13571372
/** Gets a textual representation of this position. */
13581373
string toString() {
13591374
this.isSelf() and result = "self"
@@ -1371,6 +1386,8 @@ class ArgumentPosition extends TArgumentPosition {
13711386
this.isHashSplat() and result = "**"
13721387
or
13731388
this.isSplatAll() and result = "*"
1389+
or
1390+
exists(int pos | this.isSplat(pos) and result = "* (position " + pos + ")")
13741391
}
13751392
}
13761393

@@ -1401,6 +1418,11 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
14011418
or
14021419
ppos.isSplatAll() and apos.isSplatAll()
14031420
or
1421+
ppos.isSynthSplat() and apos.isSplatAll()
1422+
or
1423+
// Exact splat match
1424+
exists(int n | apos.isSplat(n) and ppos.isSplat(n))
1425+
or
14041426
ppos.isAny() and argumentPositionIsNotSelf(apos)
14051427
or
14061428
apos.isAny() and parameterPositionIsNotSelf(ppos)

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

Lines changed: 96 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,7 @@ private class Argument extends CfgNodes::ExprCfgNode {
245245
not this.getExpr() instanceof BlockArgument and
246246
not this.getExpr().(Pair).getKey().getConstantValue().isSymbol(_) and
247247
not this.getExpr() instanceof HashSplatExpr and
248+
not this.getExpr() instanceof SplatExpr and
248249
arg.isPositional(i)
249250
)
250251
or
@@ -261,8 +262,15 @@ private class Argument extends CfgNodes::ExprCfgNode {
261262
arg.isHashSplat()
262263
or
263264
this = call.getArgument(0) and
265+
not exists(call.getArgument(1)) and
264266
this.getExpr() instanceof SplatExpr and
265267
arg.isSplatAll()
268+
or
269+
exists(int pos | pos > 0 or exists(call.getArgument(pos + 1)) |
270+
this = call.getArgument(pos) and
271+
this.getExpr() instanceof SplatExpr and
272+
arg.isSplat(pos)
273+
)
266274
}
267275

268276
/** Holds if this expression is the `i`th argument of `c`. */
@@ -300,6 +308,10 @@ private module Cached {
300308
TSynthHashSplatParameterNode(DataFlowCallable c) {
301309
isParameterNode(_, c, any(ParameterPosition p | p.isKeyword(_)))
302310
} or
311+
TSynthSplatParameterNode(DataFlowCallable c) {
312+
exists(c.asCallable()) and // exclude library callables
313+
isParameterNode(_, c, any(ParameterPosition p | p.isPositional(_)))
314+
} or
303315
TExprPostUpdateNode(CfgNodes::ExprCfgNode n) {
304316
// filter out nodes that clearly don't need post-update nodes
305317
isNonConstantExpr(n) and
@@ -318,7 +330,7 @@ private module Cached {
318330

319331
class TSourceParameterNode =
320332
TNormalParameterNode or TBlockParameterNode or TSelfParameterNode or
321-
TSynthHashSplatParameterNode;
333+
TSynthHashSplatParameterNode or TSynthSplatParameterNode;
322334

323335
cached
324336
Location getLocation(NodeImpl n) { result = n.getLocationImpl() }
@@ -514,6 +526,8 @@ predicate nodeIsHidden(Node n) {
514526
n instanceof SynthHashSplatParameterNode
515527
or
516528
n instanceof SynthHashSplatArgumentNode
529+
or
530+
n instanceof SynthSplatParameterNode
517531
}
518532

519533
/** An SSA definition, viewed as a node in a data flow graph. */
@@ -610,7 +624,15 @@ private module ParameterNodes {
610624
pos.isHashSplat()
611625
or
612626
parameter = callable.getParameter(0).(SplatParameter) and
627+
not exists(callable.getParameter(1)) and
613628
pos.isSplatAll()
629+
or
630+
exists(int n | n > 0 |
631+
parameter = callable.getParameter(n).(SplatParameter) and
632+
pos.isSplat(n) and
633+
// There are no positional parameters after the splat
634+
not exists(SimpleParameter p, int m | m > n | p = callable.getParameter(m))
635+
)
614636
)
615637
}
616638

@@ -749,6 +771,70 @@ private module ParameterNodes {
749771
final override string toStringImpl() { result = "**kwargs" }
750772
}
751773

774+
/**
775+
* A synthetic data-flow node to allow flow to positional parameters from a splat argument.
776+
*
777+
* For example, in the following code:
778+
*
779+
* ```rb
780+
* def foo(x, y); end
781+
*
782+
* foo(*[a, b])
783+
* ```
784+
*
785+
* We want `a` to flow to `x` and `b` to flow to `y`. We do this by constructing
786+
* a `SynthSplatParameterNode` for the method `foo`, and matching the splat argument to this
787+
* parameter node via `parameterMatch/2`. We then add read steps from this node to parameters
788+
* `x` and `y`, for content at indices 0 and 1 respectively (see `readStep`).
789+
*
790+
* We don't yet correctly handle cases where the splat argument is not the first argument, e.g. in
791+
* ```rb
792+
* foo(a, *[b])
793+
* ```
794+
*/
795+
class SynthSplatParameterNode extends ParameterNodeImpl, TSynthSplatParameterNode {
796+
private DataFlowCallable callable;
797+
798+
SynthSplatParameterNode() { this = TSynthSplatParameterNode(callable) }
799+
800+
/**
801+
* Gets a parameter which will contain the value given by `c`, assuming
802+
* that the method was called with a single splat argument.
803+
* For example, if the synth splat parameter is for the following method
804+
*
805+
* ```rb
806+
* def foo(x, y, a:, *rest)
807+
* end
808+
* ```
809+
*
810+
* Then `getAParameter(element 0) = x` and `getAParameter(element 1) = y`.
811+
*/
812+
ParameterNode getAParameter(ContentSet c) {
813+
exists(int n |
814+
isParameterNode(result, callable, (any(ParameterPosition p | p.isPositional(n)))) and
815+
(
816+
c = getPositionalContent(n)
817+
or
818+
c.isSingleton(TUnknownElementContent())
819+
)
820+
)
821+
}
822+
823+
final override Parameter getParameter() { none() }
824+
825+
final override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
826+
c = callable and pos.isSynthSplat()
827+
}
828+
829+
final override CfgScope getCfgScope() { result = callable.asCallable() }
830+
831+
final override DataFlowCallable getEnclosingCallable() { result = callable }
832+
833+
final override Location getLocationImpl() { result = callable.getLocation() }
834+
835+
final override string toStringImpl() { result = "synthetic *args" }
836+
}
837+
752838
/** A parameter for a library callable with a flow summary. */
753839
class SummaryParameterNode extends ParameterNodeImpl, FlowSummaryNode {
754840
private ParameterPosition pos_;
@@ -1099,6 +1185,13 @@ private ContentSet getKeywordContent(string name) {
10991185
)
11001186
}
11011187

1188+
private ContentSet getPositionalContent(int n) {
1189+
exists(ConstantValue::ConstantIntegerValue i |
1190+
result.isSingleton(TKnownElementContent(i)) and
1191+
i.isInt(n)
1192+
)
1193+
}
1194+
11021195
/**
11031196
* Subset of `storeStep` that should be shared with type-tracking.
11041197
*/
@@ -1187,6 +1280,8 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
11871280
or
11881281
node2 = node1.(SynthHashSplatParameterNode).getAKeywordParameter(c)
11891282
or
1283+
node2 = node1.(SynthSplatParameterNode).getAParameter(c)
1284+
or
11901285
FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(), c,
11911286
node2.(FlowSummaryNode).getSummaryNode())
11921287
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2796,6 +2796,7 @@
27962796
| UseUseExplosion.rb:21:3675:21:3680 | call to use | UseUseExplosion.rb:21:3670:21:3680 | else ... |
27972797
| UseUseExplosion.rb:21:3686:21:3696 | else ... | UseUseExplosion.rb:21:9:21:3700 | if ... |
27982798
| UseUseExplosion.rb:21:3691:21:3696 | call to use | UseUseExplosion.rb:21:3686:21:3696 | else ... |
2799+
| UseUseExplosion.rb:24:5:25:7 | synthetic *args | UseUseExplosion.rb:24:13:24:13 | i |
27992800
| UseUseExplosion.rb:24:5:25:7 | use | UseUseExplosion.rb:1:1:26:3 | C |
28002801
| file://:0:0:0:0 | [summary param] position 0 in & | file://:0:0:0:0 | [summary] read: Argument[0].Element[any] in & |
28012802
| file://:0:0:0:0 | [summary param] position 0 in + | file://:0:0:0:0 | [summary] read: Argument[0].Element[any] in + |
@@ -2840,6 +2841,7 @@
28402841
| file://:0:0:0:0 | [summary] read: Argument[0].Element[any] in Hash[] | file://:0:0:0:0 | [summary] read: Argument[0].Element[any].Element[1] in Hash[] |
28412842
| local_dataflow.rb:1:1:7:3 | self (foo) | local_dataflow.rb:3:8:3:10 | self |
28422843
| local_dataflow.rb:1:1:7:3 | self in foo | local_dataflow.rb:1:1:7:3 | self (foo) |
2844+
| local_dataflow.rb:1:1:7:3 | synthetic *args | local_dataflow.rb:1:9:1:9 | a |
28432845
| local_dataflow.rb:1:1:150:3 | self (local_dataflow.rb) | local_dataflow.rb:49:1:53:3 | self |
28442846
| local_dataflow.rb:1:9:1:9 | a | local_dataflow.rb:1:9:1:9 | a |
28452847
| local_dataflow.rb:1:9:1:9 | a | local_dataflow.rb:2:7:2:7 | a |
@@ -2874,6 +2876,7 @@
28742876
| local_dataflow.rb:10:5:13:3 | __synth__0__1 | local_dataflow.rb:10:5:13:3 | __synth__0__1 |
28752877
| local_dataflow.rb:10:5:13:3 | __synth__0__1 | local_dataflow.rb:10:9:10:9 | x |
28762878
| local_dataflow.rb:10:5:13:3 | call to each | local_dataflow.rb:10:1:13:3 | ... = ... |
2879+
| local_dataflow.rb:10:5:13:3 | synthetic *args | local_dataflow.rb:10:5:13:3 | __synth__0__1 |
28772880
| local_dataflow.rb:10:9:10:9 | x | local_dataflow.rb:12:5:12:5 | x |
28782881
| local_dataflow.rb:10:14:10:18 | [post] array | local_dataflow.rb:15:10:15:14 | array |
28792882
| local_dataflow.rb:10:14:10:18 | array | local_dataflow.rb:15:10:15:14 | array |
@@ -2883,13 +2886,15 @@
28832886
| local_dataflow.rb:15:1:17:3 | __synth__0__1 | local_dataflow.rb:15:1:17:3 | __synth__0__1 |
28842887
| local_dataflow.rb:15:1:17:3 | __synth__0__1 | local_dataflow.rb:15:1:17:3 | __synth__0__1 |
28852888
| local_dataflow.rb:15:1:17:3 | __synth__0__1 | local_dataflow.rb:15:5:15:5 | x |
2889+
| local_dataflow.rb:15:1:17:3 | synthetic *args | local_dataflow.rb:15:1:17:3 | __synth__0__1 |
28862890
| local_dataflow.rb:15:10:15:14 | [post] array | local_dataflow.rb:19:10:19:14 | array |
28872891
| local_dataflow.rb:15:10:15:14 | array | local_dataflow.rb:19:10:19:14 | array |
28882892
| local_dataflow.rb:16:9:16:10 | 10 | local_dataflow.rb:16:3:16:10 | break |
28892893
| local_dataflow.rb:19:1:21:3 | __synth__0__1 | local_dataflow.rb:19:1:21:3 | ... = ... |
28902894
| local_dataflow.rb:19:1:21:3 | __synth__0__1 | local_dataflow.rb:19:1:21:3 | __synth__0__1 |
28912895
| local_dataflow.rb:19:1:21:3 | __synth__0__1 | local_dataflow.rb:19:1:21:3 | __synth__0__1 |
28922896
| local_dataflow.rb:19:1:21:3 | __synth__0__1 | local_dataflow.rb:19:5:19:5 | x |
2897+
| local_dataflow.rb:19:1:21:3 | synthetic *args | local_dataflow.rb:19:1:21:3 | __synth__0__1 |
28932898
| local_dataflow.rb:19:5:19:5 | x | local_dataflow.rb:20:6:20:6 | x |
28942899
| local_dataflow.rb:20:6:20:6 | x | local_dataflow.rb:20:6:20:10 | ... > ... |
28952900
| local_dataflow.rb:20:10:20:10 | 1 | local_dataflow.rb:20:6:20:10 | ... > ... |
@@ -2901,11 +2906,13 @@
29012906
| local_dataflow.rb:30:14:30:20 | "class" | local_dataflow.rb:30:5:30:24 | C |
29022907
| local_dataflow.rb:32:5:32:25 | bar | local_dataflow.rb:32:1:32:1 | x |
29032908
| local_dataflow.rb:32:5:32:25 | bar | local_dataflow.rb:32:1:32:25 | ... = ... |
2909+
| local_dataflow.rb:34:1:39:3 | synthetic *args | local_dataflow.rb:34:7:34:7 | x |
29042910
| local_dataflow.rb:34:7:34:7 | x | local_dataflow.rb:34:7:34:7 | x |
29052911
| local_dataflow.rb:34:7:34:7 | x | local_dataflow.rb:35:6:35:6 | x |
29062912
| local_dataflow.rb:35:6:35:6 | x | local_dataflow.rb:35:6:35:11 | ... == ... |
29072913
| local_dataflow.rb:35:11:35:11 | 4 | local_dataflow.rb:35:6:35:11 | ... == ... |
29082914
| local_dataflow.rb:36:13:36:13 | 7 | local_dataflow.rb:36:6:36:13 | return |
2915+
| local_dataflow.rb:41:1:47:3 | synthetic *args | local_dataflow.rb:41:7:41:7 | x |
29092916
| local_dataflow.rb:41:7:41:7 | x | local_dataflow.rb:41:7:41:7 | x |
29102917
| local_dataflow.rb:41:7:41:7 | x | local_dataflow.rb:42:6:42:6 | x |
29112918
| local_dataflow.rb:42:6:42:6 | x | local_dataflow.rb:42:6:42:11 | ... == ... |
@@ -2924,8 +2931,10 @@
29242931
| local_dataflow.rb:51:20:51:20 | x | local_dataflow.rb:51:20:51:24 | ... < ... |
29252932
| local_dataflow.rb:51:24:51:24 | 9 | local_dataflow.rb:51:20:51:24 | ... < ... |
29262933
| local_dataflow.rb:55:5:55:13 | Array | local_dataflow.rb:55:5:55:13 | call to [] |
2934+
| local_dataflow.rb:57:1:58:3 | synthetic *args | local_dataflow.rb:57:9:57:9 | x |
29272935
| local_dataflow.rb:60:1:90:3 | self (test_case) | local_dataflow.rb:78:12:78:20 | self |
29282936
| local_dataflow.rb:60:1:90:3 | self in test_case | local_dataflow.rb:60:1:90:3 | self (test_case) |
2937+
| local_dataflow.rb:60:1:90:3 | synthetic *args | local_dataflow.rb:60:15:60:15 | x |
29292938
| local_dataflow.rb:60:15:60:15 | x | local_dataflow.rb:60:15:60:15 | x |
29302939
| local_dataflow.rb:60:15:60:15 | x | local_dataflow.rb:61:12:61:12 | x |
29312940
| local_dataflow.rb:61:7:68:5 | SSA phi read(x) | local_dataflow.rb:69:12:69:12 | x |
@@ -3098,6 +3107,7 @@
30983107
| local_dataflow.rb:118:3:118:11 | call to source | local_dataflow.rb:118:3:118:31 | call to tap |
30993108
| local_dataflow.rb:118:3:118:11 | self | local_dataflow.rb:119:3:119:31 | self |
31003109
| local_dataflow.rb:118:17:118:31 | <captured entry> self | local_dataflow.rb:118:23:118:29 | self |
3110+
| local_dataflow.rb:118:17:118:31 | synthetic *args | local_dataflow.rb:118:20:118:20 | x |
31013111
| local_dataflow.rb:118:20:118:20 | x | local_dataflow.rb:118:20:118:20 | x |
31023112
| local_dataflow.rb:118:20:118:20 | x | local_dataflow.rb:118:28:118:28 | x |
31033113
| local_dataflow.rb:119:3:119:31 | [post] self | local_dataflow.rb:119:8:119:16 | self |
@@ -3112,8 +3122,10 @@
31123122
| local_dataflow.rb:123:8:123:20 | call to dup | local_dataflow.rb:123:8:123:45 | call to tap |
31133123
| local_dataflow.rb:123:8:123:45 | call to tap | local_dataflow.rb:123:8:123:49 | call to dup |
31143124
| local_dataflow.rb:123:26:123:45 | <captured entry> self | local_dataflow.rb:123:32:123:43 | self |
3125+
| local_dataflow.rb:123:26:123:45 | synthetic *args | local_dataflow.rb:123:29:123:29 | x |
31153126
| local_dataflow.rb:126:1:128:3 | self (use) | local_dataflow.rb:127:3:127:8 | self |
31163127
| local_dataflow.rb:126:1:128:3 | self in use | local_dataflow.rb:126:1:128:3 | self (use) |
3128+
| local_dataflow.rb:126:1:128:3 | synthetic *args | local_dataflow.rb:126:9:126:9 | x |
31173129
| local_dataflow.rb:130:1:150:3 | self (use_use_madness) | local_dataflow.rb:132:6:132:11 | self |
31183130
| local_dataflow.rb:130:1:150:3 | self in use_use_madness | local_dataflow.rb:130:1:150:3 | self (use_use_madness) |
31193131
| local_dataflow.rb:131:3:131:3 | x | local_dataflow.rb:132:10:132:10 | x |

0 commit comments

Comments
 (0)