Skip to content

Commit 3ca4888

Browse files
hvitvedhvitved
committed
Ruby: Rework hash splat argument/parameter matching
1 parent dc8e1f1 commit 3ca4888

File tree

8 files changed

+88
-208
lines changed

8 files changed

+88
-208
lines changed

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

+6-1
Original file line numberDiff line numberDiff line change
@@ -1538,9 +1538,14 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
15381538
or
15391539
exists(string name | ppos.isKeyword(name) and apos.isKeyword(name))
15401540
or
1541-
(ppos.isHashSplat() or ppos.isSynthHashSplat()) and
1541+
ppos.isHashSplat() and
15421542
(apos.isHashSplat() or apos.isSynthHashSplat())
15431543
or
1544+
// no case for `apos.isSynthHashSplat() and ppos.isSynthHashSplat()`, since
1545+
// direct keyword matching is possible
1546+
ppos.isSynthHashSplat() and
1547+
apos.isHashSplat()
1548+
or
15441549
exists(int pos, boolean hasActualSplatParam, boolean hasActualSplatArg |
15451550
(
15461551
ppos.isSplat(pos) and

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

+3-50
Original file line numberDiff line numberDiff line change
@@ -662,7 +662,7 @@ private module Cached {
662662
)
663663
} or
664664
deprecated TSplatContent(int i, Boolean shifted) { i in [0 .. 10] } or
665-
THashSplatContent(ConstantValue::ConstantSymbolValue cv) or
665+
deprecated THashSplatContent(ConstantValue::ConstantSymbolValue cv) or
666666
TCapturedVariableContent(VariableCapture::CapturedVariable v) or
667667
// Only used by type-tracking
668668
TAttributeName(string name) { name = any(SetterMethodCall c).getTargetName() }
@@ -686,24 +686,16 @@ private module Cached {
686686
TUnknownElementContentApprox() or
687687
TKnownIntegerElementContentApprox() or
688688
TKnownElementContentApprox(string approx) { approx = approxKnownElementIndex(_) } or
689-
THashSplatContentApprox(string approx) { approx = approxKnownElementIndex(_) } or
690689
TNonElementContentApprox(Content c) { not c instanceof Content::ElementContent } or
691690
TCapturedVariableContentApprox(VariableCapture::CapturedVariable v)
692691

693692
cached
694693
newtype TDataFlowType =
695694
TLambdaDataFlowType(Callable c) { c = any(LambdaSelfReferenceNode n).getCallable() } or
696-
// In order to reduce the set of cons-candidates, we annotate all implicit (hash) splat
697-
// creations with the name of the method that they are passed into. This includes
698-
// array/hash literals as well (where the name is simply `[]`), because of how they
699-
// are modeled (see `Array.qll` and `Hash.qll`).
700-
TSynthHashSplatArgumentType(string methodName) {
701-
methodName = any(SynthHashSplatArgumentNode n).getMethodName()
702-
} or
703695
TUnknownDataFlowType()
704696
}
705697

706-
class TElementContent = TKnownElementContent or TUnknownElementContent or THashSplatContent;
698+
class TElementContent = TKnownElementContent or TUnknownElementContent;
707699

708700
import Cached
709701

@@ -1118,18 +1110,6 @@ private module ParameterNodes {
11181110
*
11191111
* by adding read steps out of the synthesized parameter node to the relevant
11201112
* keyword parameters.
1121-
*
1122-
* In order to avoid redundancy (and improve performance) in cases like
1123-
*
1124-
* ```rb
1125-
* foo(p1: taint(1), p2: taint(2))
1126-
* ```
1127-
*
1128-
* where direct keyword matching is possible, we use a special `HashSplatContent`
1129-
* (instead of reusing `KnownElementContent`) when we construct a synthesized hash
1130-
* splat argument (`SynthHashSplatArgumentNode`) at the call site, and then only
1131-
* add read steps out of this node for actual hash-splat arguments (which will use
1132-
* a normal `KnownElementContent`).
11331113
*/
11341114
class SynthHashSplatParameterNode extends ParameterNodeImpl, TSynthHashSplatParameterNode {
11351115
private DataFlowCallable callable;
@@ -1436,24 +1416,7 @@ module ArgumentNodes {
14361416
not cv.isSymbol(_)
14371417
)
14381418
|
1439-
if call instanceof CfgNodes::ExprNodes::HashLiteralCfgNode
1440-
then
1441-
/*
1442-
* Needed for cases like
1443-
*
1444-
* ```rb
1445-
* hash = { a: taint, b: safe }
1446-
*
1447-
* def foo(a:, b:)
1448-
* sink(a)
1449-
* end
1450-
*
1451-
* foo(**hash)
1452-
* ```
1453-
*/
1454-
1455-
c.isSingleton(Content::getElementContent(cv))
1456-
else c.isSingleton(THashSplatContent(cv))
1419+
c.isSingleton(Content::getElementContent(cv))
14571420
)
14581421
}
14591422

@@ -1938,11 +1901,8 @@ DataFlowType getNodeType(Node n) {
19381901
result = TLambdaDataFlowType(c)
19391902
)
19401903
or
1941-
result = TSynthHashSplatArgumentType(n.(SynthHashSplatArgumentNode).getMethodName())
1942-
or
19431904
not n instanceof LambdaSelfReferenceNode and
19441905
not mustHaveLambdaType(n, _) and
1945-
not n instanceof SynthHashSplatArgumentNode and
19461906
result = TUnknownDataFlowType()
19471907
}
19481908

@@ -2168,11 +2128,6 @@ class ContentApprox extends TContentApprox {
21682128
result = "approximated element " + approx
21692129
)
21702130
or
2171-
exists(string s |
2172-
this = THashSplatContentApprox(s) and
2173-
result = "approximated hash-splat position " + s
2174-
)
2175-
or
21762131
exists(Content c |
21772132
this = TNonElementContentApprox(c) and
21782133
result = c.toString()
@@ -2212,8 +2167,6 @@ ContentApprox getContentApprox(Content c) {
22122167
result =
22132168
TKnownElementContentApprox(approxKnownElementIndex(c.(Content::KnownElementContent).getIndex()))
22142169
or
2215-
result = THashSplatContentApprox(approxKnownElementIndex(c.(Content::HashSplatContent).getKey()))
2216-
or
22172170
result = TNonElementContentApprox(c)
22182171
}
22192172

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

+1-4
Original file line numberDiff line numberDiff line change
@@ -629,7 +629,7 @@ module Content {
629629
*
630630
* we have an implicit hash-splat argument containing `{:a => 1, :b => 2, :c => 3}`.
631631
*/
632-
class HashSplatContent extends ElementContent, THashSplatContent {
632+
deprecated class HashSplatContent extends Content, THashSplatContent {
633633
private ConstantValue::ConstantSymbolValue cv;
634634

635635
HashSplatContent() { this = THashSplatContent(cv) }
@@ -797,7 +797,6 @@ class ContentSet extends TContentSet {
797797
private Content getAnElementReadContent() {
798798
exists(Content::KnownElementContent c | this.isKnownOrUnknownElement(c) |
799799
result = c or
800-
result = THashSplatContent(c.getIndex()) or
801800
result = TUnknownElementContent()
802801
)
803802
or
@@ -815,8 +814,6 @@ class ContentSet extends TContentSet {
815814
|
816815
type = result.(Content::KnownElementContent).getIndex().getValueType()
817816
or
818-
type = result.(Content::HashSplatContent).getKey().getValueType()
819-
or
820817
includeUnknown = true and
821818
result = TUnknownElementContent()
822819
)

‎ruby/ql/test/library-tests/dataflow/api-graphs/VerifyApiGraphExpectations.expected

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
failures
21
testFailures
2+
failures

‎ruby/ql/test/library-tests/dataflow/flow-summaries/semantics.expected

+12-12
Original file line numberDiff line numberDiff line change
@@ -144,14 +144,14 @@ edges
144144
| semantics.rb:108:5:108:5 | b | semantics.rb:110:27:110:27 | b | provenance | |
145145
| semantics.rb:108:9:108:18 | call to source | semantics.rb:108:5:108:5 | b | provenance | |
146146
| semantics.rb:108:9:108:18 | call to source | semantics.rb:108:5:108:5 | b | provenance | |
147-
| semantics.rb:109:10:109:28 | call to s15 [hash-splat position :foo] | semantics.rb:109:10:109:34 | ...[...] | provenance | |
148-
| semantics.rb:109:10:109:28 | call to s15 [hash-splat position :foo] | semantics.rb:109:10:109:34 | ...[...] | provenance | |
149-
| semantics.rb:109:19:109:19 | a | semantics.rb:109:10:109:28 | call to s15 [hash-splat position :foo] | provenance | |
150-
| semantics.rb:109:19:109:19 | a | semantics.rb:109:10:109:28 | call to s15 [hash-splat position :foo] | provenance | |
151-
| semantics.rb:110:10:110:28 | call to s15 [hash-splat position :bar] | semantics.rb:110:10:110:34 | ...[...] | provenance | |
152-
| semantics.rb:110:10:110:28 | call to s15 [hash-splat position :bar] | semantics.rb:110:10:110:34 | ...[...] | provenance | |
153-
| semantics.rb:110:27:110:27 | b | semantics.rb:110:10:110:28 | call to s15 [hash-splat position :bar] | provenance | |
154-
| semantics.rb:110:27:110:27 | b | semantics.rb:110:10:110:28 | call to s15 [hash-splat position :bar] | provenance | |
147+
| semantics.rb:109:10:109:28 | call to s15 [element :foo] | semantics.rb:109:10:109:34 | ...[...] | provenance | |
148+
| semantics.rb:109:10:109:28 | call to s15 [element :foo] | semantics.rb:109:10:109:34 | ...[...] | provenance | |
149+
| semantics.rb:109:19:109:19 | a | semantics.rb:109:10:109:28 | call to s15 [element :foo] | provenance | |
150+
| semantics.rb:109:19:109:19 | a | semantics.rb:109:10:109:28 | call to s15 [element :foo] | provenance | |
151+
| semantics.rb:110:10:110:28 | call to s15 [element :bar] | semantics.rb:110:10:110:34 | ...[...] | provenance | |
152+
| semantics.rb:110:10:110:28 | call to s15 [element :bar] | semantics.rb:110:10:110:34 | ...[...] | provenance | |
153+
| semantics.rb:110:27:110:27 | b | semantics.rb:110:10:110:28 | call to s15 [element :bar] | provenance | |
154+
| semantics.rb:110:27:110:27 | b | semantics.rb:110:10:110:28 | call to s15 [element :bar] | provenance | |
155155
| semantics.rb:114:5:114:5 | a | semantics.rb:116:14:116:14 | a | provenance | |
156156
| semantics.rb:114:5:114:5 | a | semantics.rb:116:14:116:14 | a | provenance | |
157157
| semantics.rb:114:5:114:5 | a | semantics.rb:119:17:119:17 | a | provenance | |
@@ -1269,14 +1269,14 @@ nodes
12691269
| semantics.rb:108:5:108:5 | b | semmle.label | b |
12701270
| semantics.rb:108:9:108:18 | call to source | semmle.label | call to source |
12711271
| semantics.rb:108:9:108:18 | call to source | semmle.label | call to source |
1272-
| semantics.rb:109:10:109:28 | call to s15 [hash-splat position :foo] | semmle.label | call to s15 [hash-splat position :foo] |
1273-
| semantics.rb:109:10:109:28 | call to s15 [hash-splat position :foo] | semmle.label | call to s15 [hash-splat position :foo] |
1272+
| semantics.rb:109:10:109:28 | call to s15 [element :foo] | semmle.label | call to s15 [element :foo] |
1273+
| semantics.rb:109:10:109:28 | call to s15 [element :foo] | semmle.label | call to s15 [element :foo] |
12741274
| semantics.rb:109:10:109:34 | ...[...] | semmle.label | ...[...] |
12751275
| semantics.rb:109:10:109:34 | ...[...] | semmle.label | ...[...] |
12761276
| semantics.rb:109:19:109:19 | a | semmle.label | a |
12771277
| semantics.rb:109:19:109:19 | a | semmle.label | a |
1278-
| semantics.rb:110:10:110:28 | call to s15 [hash-splat position :bar] | semmle.label | call to s15 [hash-splat position :bar] |
1279-
| semantics.rb:110:10:110:28 | call to s15 [hash-splat position :bar] | semmle.label | call to s15 [hash-splat position :bar] |
1278+
| semantics.rb:110:10:110:28 | call to s15 [element :bar] | semmle.label | call to s15 [element :bar] |
1279+
| semantics.rb:110:10:110:28 | call to s15 [element :bar] | semmle.label | call to s15 [element :bar] |
12801280
| semantics.rb:110:10:110:34 | ...[...] | semmle.label | ...[...] |
12811281
| semantics.rb:110:10:110:34 | ...[...] | semmle.label | ...[...] |
12821282
| semantics.rb:110:27:110:27 | b | semmle.label | b |

0 commit comments

Comments
 (0)