Skip to content

Commit

Permalink
Ruby: Rework splat argument/parameter matching
Browse files Browse the repository at this point in the history
  • Loading branch information
@hvitved
hvitved committed Aug 14, 2024
1 parent 4e02e34 commit 973db51
Show file tree
Hide file tree
Showing 8 changed files with 942 additions and 1,581 deletions.
46 changes: 34 additions & 12 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ private module Cached {
THashSplatArgumentPosition() or
TSynthHashSplatArgumentPosition() or
TSplatArgumentPosition(int pos) { exists(Call c | c.getArgument(pos) instanceof SplatExpr) } or
TSynthSplatArgumentPosition() or
TSynthSplatArgumentPosition(Boolean hasActualSplat) or
TAnyArgumentPosition() or
TAnyKeywordArgumentPosition()

Expand All @@ -590,11 +590,11 @@ private module Cached {
THashSplatParameterPosition() or
TSynthHashSplatParameterPosition() or
TSplatParameterPosition(int pos) {
pos = 0
pos = 0 // needed for flow summaries
or
exists(Parameter p | p.getPosition() = pos and p instanceof SplatParameter)
} or
TSynthSplatParameterPosition() or
TSynthSplatParameterPosition(Boolean hasActualSplat) or
TAnyParameterPosition() or
TAnyKeywordParameterPosition()
}
Expand Down Expand Up @@ -1384,7 +1384,9 @@ class ParameterPosition extends TParameterPosition {
predicate isSplat(int n) { this = TSplatParameterPosition(n) }

/** Holds if this position represents a synthetic splat parameter. */
predicate isSynthSplat() { this = TSynthSplatParameterPosition() }
predicate isSynthSplat(boolean hasActualSplat) {
this = TSynthSplatParameterPosition(hasActualSplat)
}

/**
* Holds if this position represents any parameter, except `self` parameters. This
Expand Down Expand Up @@ -1419,7 +1421,11 @@ class ParameterPosition extends TParameterPosition {
or
exists(int pos | this.isSplat(pos) and result = "* (position " + pos + ")")
or
this.isSynthSplat() and result = "synthetic *"
exists(boolean hasActualSplat, string suffix |
this.isSynthSplat(hasActualSplat) and
result = "synthetic *" + suffix and
if hasActualSplat = true then suffix = " (with actual)" else suffix = ""
)
}
}

Expand Down Expand Up @@ -1459,7 +1465,9 @@ class ArgumentPosition extends TArgumentPosition {
predicate isSplat(int n) { this = TSplatArgumentPosition(n) }

/** Holds if this position represents a synthetic splat argument. */
predicate isSynthSplat() { this = TSynthSplatArgumentPosition() }
predicate isSynthSplat(boolean hasActualSplat) {
this = TSynthSplatArgumentPosition(hasActualSplat)
}

/** Gets a textual representation of this position. */
string toString() {
Expand All @@ -1483,7 +1491,11 @@ class ArgumentPosition extends TArgumentPosition {
or
exists(int pos | this.isSplat(pos) and result = "* (position " + pos + ")")
or
this.isSynthSplat() and result = "synthetic *"
exists(boolean hasActualSplat, string suffix |
this.isSynthSplat(hasActualSplat) and
result = "synthetic *" + suffix and
if hasActualSplat = true then suffix = " (with actual)" else suffix = ""
)
}
}

Expand Down Expand Up @@ -1519,16 +1531,26 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
(ppos.isHashSplat() or ppos.isSynthHashSplat()) and
(apos.isHashSplat() or apos.isSynthHashSplat())
or
exists(int pos |
exists(int pos, boolean hasActualSplatParam, boolean hasActualSplatArg |
(
ppos.isSplat(pos)
ppos.isSplat(pos) and
hasActualSplatParam = true // dummy value
or
ppos.isSynthSplat() and pos = 0
ppos.isSynthSplat(hasActualSplatParam) and
pos = 0 and
// prevent synthetic splat parameters from matching synthetic splat arguments
// when direct positional matching is possible
(
hasActualSplatParam = true
or
hasActualSplatArg = true
)
) and
(
apos.isSplat(pos)
apos.isSplat(pos) and
hasActualSplatArg = true // dummy value
or
apos.isSynthSplat() and pos = 0
apos.isSynthSplat(hasActualSplatArg) and pos = 0
)
)
or
Expand Down
95 changes: 20 additions & 75 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ private module Cached {
name = [input, output].regexpFind("(?<=(^|\\.)Field\\[)[^\\]]+(?=\\])", _, _).trim()
)
} or
TSplatContent(int i, Boolean shifted) { i in [0 .. 10] } or
deprecated TSplatContent(int i, Boolean shifted) { i in [0 .. 10] } or
THashSplatContent(ConstantValue::ConstantSymbolValue cv) or
TCapturedVariableContent(VariableCapture::CapturedVariable v) or
// Only used by type-tracking
Expand All @@ -686,7 +686,6 @@ private module Cached {
TUnknownElementContentApprox() or
TKnownIntegerElementContentApprox() or
TKnownElementContentApprox(string approx) { approx = approxKnownElementIndex(_) } or
TSplatContentApprox(Boolean shifted) or
THashSplatContentApprox(string approx) { approx = approxKnownElementIndex(_) } or
TNonElementContentApprox(Content c) { not c instanceof Content::ElementContent } or
TCapturedVariableContentApprox(VariableCapture::CapturedVariable v)
Expand All @@ -701,14 +700,10 @@ private module Cached {
TSynthHashSplatArgumentType(string methodName) {
methodName = any(SynthHashSplatArgumentNode n).getMethodName()
} or
TSynthSplatArgumentType(string methodName) {
methodName = any(SynthSplatArgumentNode n).getMethodName()
} or
TUnknownDataFlowType()
}

class TElementContent =
TKnownElementContent or TUnknownElementContent or TSplatContent or THashSplatContent;
class TElementContent = TKnownElementContent or TUnknownElementContent or THashSplatContent;

import Cached

Expand Down Expand Up @@ -1188,18 +1183,6 @@ private module ParameterNodes {
* by adding read steps out of the synthesized parameter node to the relevant
* positional parameters.
*
* In order to avoid redundancy (and improve performance) in cases like
*
* ```rb
* foo(a, b, c)
* ```
*
* where direct positional matching is possible, we use a special `SplatContent`
* (instead of reusing `KnownElementContent`) when we construct a synthesized
* splat argument (`SynthSplatArgumentNode`) at the call site, and then only
* add read steps out of this node for actual splat arguments (which will use
* `KnownElementContent` or `TSplatContent(_, true)`).
*
* We don't yet correctly handle cases where a positional argument follows the
* splat argument, e.g. in
*
Expand All @@ -1220,9 +1203,6 @@ private module ParameterNodes {
isParameterNode(p, callable, any(ParameterPosition pos | pos.isPositional(n))) and
not exists(int i | splatParameterAt(callable.asCfgScope(), i) and i < n)
|
// Important: do not include `TSplatContent(_, false)` here, as normal parameter matching is possible
c = getSplatContent(n, true)
or
c = getArrayContent(n)
or
c.isSingleton(TUnknownElementContent())
Expand All @@ -1232,7 +1212,13 @@ private module ParameterNodes {
final override Parameter getParameter() { none() }

final override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
c = callable and pos.isSynthSplat()
c = callable and
exists(boolean hasActualSplat |
pos.isSynthSplat(hasActualSplat) and
if exists(TSynthSplatParameterShiftNode(c, _, _))
then hasActualSplat = true
else hasActualSplat = false
)
}

final override CfgScope getCfgScope() { result = callable.asCfgScope() }
Expand Down Expand Up @@ -1271,11 +1257,7 @@ private module ParameterNodes {
*/
predicate readFrom(SynthSplatParameterNode synthSplat, ContentSet cs) {
synthSplat.isParameterOf(callable, _) and
(
cs = getSplatContent(pos + splatPos, _)
or
cs = getArrayContent(pos + splatPos)
)
cs = getArrayContent(pos + splatPos)
}

/**
Expand Down Expand Up @@ -1506,31 +1488,11 @@ module ArgumentNodes {
* `call`, into a synthetic splat argument.
*/
predicate synthSplatStore(CfgNodes::ExprNodes::CallCfgNode call, Argument arg, ContentSet c) {
exists(int n |
exists(ArgumentPosition pos |
arg.isArgumentOf(call, pos) and
pos.isPositional(n) and
not exists(int i | splatArgumentAt(call, i) and i < n)
)
|
if call instanceof CfgNodes::ExprNodes::ArrayLiteralCfgNode
then
/*
* Needed for cases like
*
* ```rb
* arr = [taint, safe]
*
* def foo(a, b)
* sink(a)
* end
*
* foo(*arr)
* ```
*/

c = getArrayContent(n)
else c = getSplatContent(n, false)
exists(int n, ArgumentPosition pos |
arg.isArgumentOf(call, pos) and
pos.isPositional(n) and
not exists(int i | splatArgumentAt(call, i) and i < n) and
c = getArrayContent(n)
)
}

Expand All @@ -1552,7 +1514,9 @@ module ArgumentNodes {

override predicate sourceArgumentOf(CfgNodes::ExprNodes::CallCfgNode call, ArgumentPosition pos) {
call = call_ and
pos.isSynthSplat()
if any(SynthSplatArgumentShiftNode shift).storeInto(this, _)
then pos.isSynthSplat(true)
else pos.isSynthSplat(false)
}

override string toStringImpl() { result = "synthetic splat argument" }
Expand Down Expand Up @@ -1583,8 +1547,6 @@ module ArgumentNodes {
predicate readFrom(Node splatArg, ContentSet cs) {
splatArg.asExpr().(Argument).isArgumentOf(c, any(ArgumentPosition p | p.isSplat(splatPos))) and
(
cs = getSplatContent(n - splatPos, _)
or
cs = getArrayContent(n - splatPos)
or
n = -1 and
Expand All @@ -1599,7 +1561,7 @@ module ArgumentNodes {
predicate storeInto(SynthSplatArgumentNode synthSplat, ContentSet cs) {
synthSplat = TSynthSplatArgumentNode(c) and
(
cs = getSplatContent(n, true)
cs = getArrayContent(n)
or
n = -1 and
cs.isSingleton(TUnknownElementContent())
Expand Down Expand Up @@ -1813,10 +1775,6 @@ private ContentSet getArrayContent(int n) {
)
}

private ContentSet getSplatContent(int n, boolean adjusted) {
result.isSingleton(TSplatContent(n, adjusted))
}

/**
* Subset of `storeStep` that should be shared with type-tracking.
*/
Expand Down Expand Up @@ -1979,11 +1937,9 @@ DataFlowType getNodeType(Node n) {
or
result = TSynthHashSplatArgumentType(n.(SynthHashSplatArgumentNode).getMethodName())
or
result = TSynthSplatArgumentType(n.(SynthSplatArgumentNode).getMethodName())
or
not n instanceof LambdaSelfReferenceNode and
not mustHaveLambdaType(n, _) and
not n instanceof SynthHashSplatOrSplatArgumentNode and
not n instanceof SynthHashSplatArgumentNode and
result = TUnknownDataFlowType()
}

Expand Down Expand Up @@ -2209,12 +2165,6 @@ class ContentApprox extends TContentApprox {
result = "approximated element " + approx
)
or
exists(boolean shifted, string s |
this = TSplatContentApprox(shifted) and
(if shifted = true then s = " (shifted)" else s = "") and
result = "approximated splat position" + s
)
or
exists(string s |
this = THashSplatContentApprox(s) and
result = "approximated hash-splat position " + s
Expand Down Expand Up @@ -2259,11 +2209,6 @@ ContentApprox getContentApprox(Content c) {
result =
TKnownElementContentApprox(approxKnownElementIndex(c.(Content::KnownElementContent).getIndex()))
or
exists(boolean shifted |
c = TSplatContent(_, shifted) and
result = TSplatContentApprox(shifted)
)
or
result = THashSplatContentApprox(approxKnownElementIndex(c.(Content::HashSplatContent).getKey()))
or
result = TNonElementContentApprox(c)
Expand Down
13 changes: 2 additions & 11 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,7 @@ module Content {
*
* we have an implicit splat argument containing `[1, 2, 3]`.
*/
class SplatContent extends ElementContent, TSplatContent {
deprecated class SplatContent extends Content, TSplatContent {
private int i;
private boolean shifted;

Expand Down Expand Up @@ -797,20 +797,14 @@ class ContentSet extends TContentSet {
private Content getAnElementReadContent() {
exists(Content::KnownElementContent c | this.isKnownOrUnknownElement(c) |
result = c or
result = TSplatContent(c.getIndex().getInt(), _) or
result = THashSplatContent(c.getIndex()) or
result = TUnknownElementContent()
)
or
exists(int lower, boolean includeUnknown |
this = TElementLowerBoundContent(lower, includeUnknown)
|
exists(int i |
result.(Content::KnownElementContent).getIndex().isInt(i) or
result = TSplatContent(i, _)
|
i >= lower
)
exists(int i | result.(Content::KnownElementContent).getIndex().isInt(i) | i >= lower)
or
includeUnknown = true and
result = TUnknownElementContent()
Expand All @@ -821,9 +815,6 @@ class ContentSet extends TContentSet {
|
type = result.(Content::KnownElementContent).getIndex().getValueType()
or
type = "int" and
result instanceof Content::SplatContent
or
type = result.(Content::HashSplatContent).getKey().getValueType()
or
includeUnknown = true and
Expand Down
Loading

0 comments on commit 973db51

Please sign in to comment.