Skip to content

Commit

Permalink
Ruby: Handle more splat arg flow
Browse files Browse the repository at this point in the history 
Allow flow from a splat argument to a positional parameter in cases
where there are positional arguments left of the splat. For example:

    def foo(x, y, z); end

    foo(1, *[2, 3])
  • Loading branch information
@hmac
hmac committed Aug 23, 2023
1 parent a5c8917 commit 2376f28
Show file tree
Hide file tree
Showing 6 changed files with 133 additions and 40 deletions.
12 changes: 8 additions & 4 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,11 @@ private module Cached {
TSplatParameterPosition(int pos) {
exists(Parameter p | p.getPosition() = pos and p instanceof SplatParameter)
} or
TSynthSplatParameterPosition() or
TSynthSplatParameterPosition(int pos) {
// `pos` is the position of the splat _argument_ that is matched to the
// `SynthSplatParameterNode` with this position.
exists(ArgumentPosition a | a.isSplat(pos))
} or
TSynthArgSplatParameterPosition() or
TAnyParameterPosition() or
TAnyKeywordParameterPosition()
Expand Down Expand Up @@ -1293,7 +1297,7 @@ class ParameterPosition extends TParameterPosition {

predicate isSynthHashSplat() { this = TSynthHashSplatParameterPosition() }

predicate isSynthSplat() { this = TSynthSplatParameterPosition() }
predicate isSynthSplat(int n) { this = TSynthSplatParameterPosition(n) }

// A fake position to indicate that this parameter node holds content from a synth arg splat node
predicate isSynthArgSplat() { this = TSynthArgSplatParameterPosition() }
Expand Down Expand Up @@ -1329,7 +1333,7 @@ class ParameterPosition extends TParameterPosition {
or
this.isAnyNamed() and result = "any-named"
or
this.isSynthSplat() and result = "synthetic *"
exists(int pos | this.isSynthSplat(pos) and result = "synthetic * (position " + pos + ")")
or
this.isSynthArgSplat() and result = "synthetic * (from *args)"
or
Expand Down Expand Up @@ -1419,7 +1423,7 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
or
ppos.isSplat(0) and apos.isSynthSplat()
or
ppos.isSynthSplat() and apos.isSplat(0)
exists(int n | ppos.isSynthSplat(n) and apos.isSplat(n))
or
apos.isSynthSplat() and ppos.isSynthArgSplat()
or
Expand Down
27 changes: 19 additions & 8 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,9 +335,11 @@ private module Cached {
TSynthHashSplatParameterNode(DataFlowCallable c) {
isParameterNode(_, c, any(ParameterPosition p | p.isKeyword(_)))
} or
TSynthSplatParameterNode(DataFlowCallable c) {
TSynthSplatParameterNode(DataFlowCallable c, int n) {
exists(c.asCallable()) and // exclude library callables
isParameterNode(_, c, any(ParameterPosition p | p.isPositional(_)))
isParameterNode(_, c, any(ParameterPosition p | p.isPositional(_))) and
// `n` is the position of the splat argument that is matched to this node
exists(ArgumentPosition pos | pos.isSplat(n))
} or
TSynthSplatArgParameterNode(DataFlowCallable c) {
exists(c.asCallable()) and // exclude library callables
Expand Down Expand Up @@ -854,11 +856,20 @@ private module ParameterNodes {
* ```rb
* foo(a, *[b])
* ```
*
* TODO: we do now support the above, but we don't support this case:
*
* ```rb
* foo(a, *[b], c)
* ```
*
* Update this documentation.
*/
class SynthSplatParameterNode extends ParameterNodeImpl, TSynthSplatParameterNode {
private DataFlowCallable callable;
private int n;

SynthSplatParameterNode() { this = TSynthSplatParameterNode(callable) }
SynthSplatParameterNode() { this = TSynthSplatParameterNode(callable, n) }

/**
* Gets a parameter which will contain the value given by `c`, assuming
Expand All @@ -870,13 +881,13 @@ private module ParameterNodes {
* end
* ```
*
* Then `getAParameter(element 0) = x` and `getAParameter(element 1) = y`.
* then `getAParameter(element 0) = x` and `getAParameter(element 1) = y`.
*/
ParameterNode getAParameter(ContentSet c) {
exists(int n |
isParameterNode(result, callable, (any(ParameterPosition p | p.isPositional(n)))) and
exists(int m |
isParameterNode(result, callable, (any(ParameterPosition p | p.isPositional(m)))) and
(
c = getPositionalContent(n)
c = getPositionalContent(m - n)
or
c.isSingleton(TUnknownElementContent())
)
Expand All @@ -886,7 +897,7 @@ private module ParameterNodes {
final override Parameter getParameter() { none() }

final override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
c = callable and pos.isSynthSplat()
c = callable and pos.isSynthSplat(n)
}

final override CfgScope getCfgScope() { result = callable.asCallable() }
Expand Down
12 changes: 0 additions & 12 deletions ruby/ql/test/library-tests/dataflow/local/TaintStep.expected
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2796,7 +2796,6 @@
| UseUseExplosion.rb:21:3675:21:3680 | call to use | UseUseExplosion.rb:21:3670:21:3680 | else ... |
| UseUseExplosion.rb:21:3686:21:3696 | else ... | UseUseExplosion.rb:21:9:21:3700 | if ... |
| UseUseExplosion.rb:21:3691:21:3696 | call to use | UseUseExplosion.rb:21:3686:21:3696 | else ... |
| UseUseExplosion.rb:24:5:25:7 | synthetic *args | UseUseExplosion.rb:24:13:24:13 | i |
| UseUseExplosion.rb:24:5:25:7 | use | UseUseExplosion.rb:1:1:26:3 | C |
| file://:0:0:0:0 | [summary param] position 0 in & | file://:0:0:0:0 | [summary] read: Argument[0].Element[any] in & |
| file://:0:0:0:0 | [summary param] position 0 in + | file://:0:0:0:0 | [summary] read: Argument[0].Element[any] in + |
Expand Down Expand Up @@ -2841,7 +2840,6 @@
| 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[] |
| local_dataflow.rb:1:1:7:3 | self (foo) | local_dataflow.rb:3:8:3:10 | self |
| local_dataflow.rb:1:1:7:3 | self in foo | local_dataflow.rb:1:1:7:3 | self (foo) |
| local_dataflow.rb:1:1:7:3 | synthetic *args | local_dataflow.rb:1:9:1:9 | a |
| local_dataflow.rb:1:1:150:3 | <uninitialized> | local_dataflow.rb:10:9:10:9 | x |
| local_dataflow.rb:1:1:150:3 | self (local_dataflow.rb) | local_dataflow.rb:49:1:53:3 | self |
| local_dataflow.rb:1:9:1:9 | a | local_dataflow.rb:1:9:1:9 | a |
Expand Down Expand Up @@ -2879,7 +2877,6 @@
| local_dataflow.rb:10:5:13:3 | __synth__0__1 | local_dataflow.rb:10:5:13:3 | __synth__0__1 |
| local_dataflow.rb:10:5:13:3 | __synth__0__1 | local_dataflow.rb:10:9:10:9 | x |
| local_dataflow.rb:10:5:13:3 | call to each | local_dataflow.rb:10:5:13:3 | ... |
| local_dataflow.rb:10:5:13:3 | synthetic *args | local_dataflow.rb:10:5:13:3 | __synth__0__1 |
| local_dataflow.rb:10:9:10:9 | ... = ... | local_dataflow.rb:10:9:10:9 | if ... |
| local_dataflow.rb:10:9:10:9 | defined? ... | local_dataflow.rb:10:9:10:9 | [false] ! ... |
| local_dataflow.rb:10:9:10:9 | defined? ... | local_dataflow.rb:10:9:10:9 | [true] ! ... |
Expand All @@ -2898,7 +2895,6 @@
| local_dataflow.rb:15:1:17:3 | __synth__0__1 | local_dataflow.rb:15:1:17:3 | __synth__0__1 |
| local_dataflow.rb:15:1:17:3 | __synth__0__1 | local_dataflow.rb:15:5:15:5 | x |
| local_dataflow.rb:15:1:17:3 | call to each | local_dataflow.rb:15:1:17:3 | ... |
| local_dataflow.rb:15:1:17:3 | synthetic *args | local_dataflow.rb:15:1:17:3 | __synth__0__1 |
| local_dataflow.rb:15:5:15:5 | ... = ... | local_dataflow.rb:15:5:15:5 | if ... |
| local_dataflow.rb:15:5:15:5 | defined? ... | local_dataflow.rb:15:5:15:5 | [false] ! ... |
| local_dataflow.rb:15:5:15:5 | defined? ... | local_dataflow.rb:15:5:15:5 | [true] ! ... |
Expand All @@ -2914,7 +2910,6 @@
| local_dataflow.rb:19:1:21:3 | __synth__0__1 | local_dataflow.rb:19:1:21:3 | __synth__0__1 |
| local_dataflow.rb:19:1:21:3 | __synth__0__1 | local_dataflow.rb:19:5:19:5 | x |
| local_dataflow.rb:19:1:21:3 | call to each | local_dataflow.rb:19:1:21:3 | ... |
| local_dataflow.rb:19:1:21:3 | synthetic *args | local_dataflow.rb:19:1:21:3 | __synth__0__1 |
| local_dataflow.rb:19:5:19:5 | ... = ... | local_dataflow.rb:19:5:19:5 | if ... |
| local_dataflow.rb:19:5:19:5 | defined? ... | local_dataflow.rb:19:5:19:5 | [false] ! ... |
| local_dataflow.rb:19:5:19:5 | defined? ... | local_dataflow.rb:19:5:19:5 | [true] ! ... |
Expand All @@ -2933,13 +2928,11 @@
| local_dataflow.rb:30:14:30:20 | "class" | local_dataflow.rb:30:5:30:24 | C |
| local_dataflow.rb:32:5:32:25 | bar | local_dataflow.rb:32:1:32:1 | x |
| local_dataflow.rb:32:5:32:25 | bar | local_dataflow.rb:32:1:32:25 | ... = ... |
| local_dataflow.rb:34:1:39:3 | synthetic *args | local_dataflow.rb:34:7:34:7 | x |
| local_dataflow.rb:34:7:34:7 | x | local_dataflow.rb:34:7:34:7 | x |
| local_dataflow.rb:34:7:34:7 | x | local_dataflow.rb:35:6:35:6 | x |
| local_dataflow.rb:35:6:35:6 | x | local_dataflow.rb:35:6:35:11 | ... == ... |
| local_dataflow.rb:35:11:35:11 | 4 | local_dataflow.rb:35:6:35:11 | ... == ... |
| local_dataflow.rb:36:13:36:13 | 7 | local_dataflow.rb:36:6:36:13 | return |
| local_dataflow.rb:41:1:47:3 | synthetic *args | local_dataflow.rb:41:7:41:7 | x |
| local_dataflow.rb:41:7:41:7 | x | local_dataflow.rb:41:7:41:7 | x |
| local_dataflow.rb:41:7:41:7 | x | local_dataflow.rb:42:6:42:6 | x |
| local_dataflow.rb:42:6:42:6 | x | local_dataflow.rb:42:6:42:11 | ... == ... |
Expand All @@ -2958,10 +2951,8 @@
| local_dataflow.rb:51:20:51:20 | x | local_dataflow.rb:51:20:51:24 | ... < ... |
| local_dataflow.rb:51:24:51:24 | 9 | local_dataflow.rb:51:20:51:24 | ... < ... |
| local_dataflow.rb:55:5:55:13 | Array | local_dataflow.rb:55:5:55:13 | call to [] |
| local_dataflow.rb:57:1:58:3 | synthetic *args | local_dataflow.rb:57:9:57:9 | x |
| local_dataflow.rb:60:1:90:3 | self (test_case) | local_dataflow.rb:78:12:78:20 | self |
| local_dataflow.rb:60:1:90:3 | self in test_case | local_dataflow.rb:60:1:90:3 | self (test_case) |
| local_dataflow.rb:60:1:90:3 | synthetic *args | local_dataflow.rb:60:15:60:15 | x |
| local_dataflow.rb:60:15:60:15 | x | local_dataflow.rb:60:15:60:15 | x |
| local_dataflow.rb:60:15:60:15 | x | local_dataflow.rb:61:12:61:12 | x |
| local_dataflow.rb:61:7:68:5 | SSA phi read(x) | local_dataflow.rb:69:12:69:12 | x |
Expand Down Expand Up @@ -3134,7 +3125,6 @@
| local_dataflow.rb:118:3:118:11 | call to source | local_dataflow.rb:118:3:118:31 | call to tap |
| local_dataflow.rb:118:3:118:11 | self | local_dataflow.rb:119:3:119:31 | self |
| local_dataflow.rb:118:17:118:31 | <captured entry> self | local_dataflow.rb:118:23:118:29 | self |
| local_dataflow.rb:118:17:118:31 | synthetic *args | local_dataflow.rb:118:20:118:20 | x |
| local_dataflow.rb:118:20:118:20 | x | local_dataflow.rb:118:20:118:20 | x |
| local_dataflow.rb:118:20:118:20 | x | local_dataflow.rb:118:28:118:28 | x |
| local_dataflow.rb:119:3:119:31 | [post] self | local_dataflow.rb:119:8:119:16 | self |
Expand All @@ -3149,10 +3139,8 @@
| local_dataflow.rb:123:8:123:20 | call to dup | local_dataflow.rb:123:8:123:45 | call to tap |
| local_dataflow.rb:123:8:123:45 | call to tap | local_dataflow.rb:123:8:123:49 | call to dup |
| local_dataflow.rb:123:26:123:45 | <captured entry> self | local_dataflow.rb:123:32:123:43 | self |
| local_dataflow.rb:123:26:123:45 | synthetic *args | local_dataflow.rb:123:29:123:29 | x |
| local_dataflow.rb:126:1:128:3 | self (use) | local_dataflow.rb:127:3:127:8 | self |
| local_dataflow.rb:126:1:128:3 | self in use | local_dataflow.rb:126:1:128:3 | self (use) |
| local_dataflow.rb:126:1:128:3 | synthetic *args | local_dataflow.rb:126:9:126:9 | x |
| local_dataflow.rb:130:1:150:3 | self (use_use_madness) | local_dataflow.rb:132:6:132:11 | self |
| local_dataflow.rb:130:1:150:3 | self in use_use_madness | local_dataflow.rb:130:1:150:3 | self (use_use_madness) |
| local_dataflow.rb:131:3:131:3 | x | local_dataflow.rb:132:10:132:10 | x |
Expand Down
Loading

0 comments on commit 2376f28

Please sign in to comment.