Ruby: More splat flow (alternative) #14090
Conversation
There was a problem hiding this comment.
This aligns with how I envisioned it in my comment #13974 (review). A few comments.
| exists(c.asCallable()) and // exclude library callables | ||
| isParameterNode(_, c, any(ParameterPosition p | p.isSplat(_))) | ||
| exists(c.asCallable()) // exclude library callables | ||
| // and isParameterNode(_, c, any(ParameterPosition p | p.isSplat(_))) |
| exists(int n, int splatPos, CfgNodes::ExprNodes::CallCfgNode call | | ||
| // Don't propagate taint on the `self` element of the splat | ||
| // since that won't (probably) won't reach the parameters of the callable. | ||
| // This saves a node per call. |
There was a problem hiding this comment.
I'm not sure I understand this comment.
There was a problem hiding this comment.
This was a misunderstanding on my part that content at index -1 represented the value of the container itself. That's why I added the n >= 0 constraint. I've now realised this isn't true and that positional content is only defined for n in [0..10] so I'll remove the constraint and this comment.
| // Don't propagate taint on the `self` element of the splat | ||
| // since that won't (probably) won't reach the parameters of the callable. | ||
| // This saves a node per call. | ||
| n >= 0 and |
| } | ||
|
|
||
| class TSourceParameterNode = | ||
| TNormalParameterNode or TBlockParameterNode or TSelfParameterNode or | ||
| TSynthHashSplatParameterNode or TSynthSplatParameterNode or TSynthSplatArgParameterNode; | ||
| TSynthHashSplatParameterNode or TSynthSplatParameterNode or TSynthSplatArgParameterNode or | ||
| TSynthSplatArgumentElementNode; |
| TSynthSplatArgumentNode(CfgNodes::ExprNodes::CallCfgNode c) or | ||
| TSynthSplatArgumentElementNode(CfgNodes::ExprNodes::CallCfgNode c, int n) { | ||
| n in [0 .. 10] and | ||
| exists(Argument arg, ArgumentPosition pos | pos.isSplat(_) and arg.isArgumentOf(c, pos)) |
There was a problem hiding this comment.
I believe this should be limited to pos.isSplat(any(int splatPos | splatPos > 0))?
There was a problem hiding this comment.
If we do that, then I think it breaks cases like
def f(x, y); end
f(*[taint, 2])because we no longer match the explicit splat arg to the synth splat parameter. If we re-add ppos.isSynthSplat() and apos.isSplat(0) to parameterMatch, then we still have a failure in cases like
def g(x, *y); end
g(*[1, taint])There was a problem hiding this comment.
Yeah, I was thinking we would still have ppos.isSynthSplat() and apos.isSplat(0), since there should be no need to go through a TSynthSplatArgumentElementNode node in that case, as the indices don't need to be adjusted.
There was a problem hiding this comment.
I fixed the missing flow in this case
def g(x, *y); end
g(*[1, taint])by adding a read step from element N of SynthSplatParameterNode to SynthSplatParameterElementNode(N). This means that taint flows to SynthSplatParameterElementNode(1) and then we have an existing store step that will store it from there into the splat parameter at element 0.
I've pushed a change which does this (plus the other changes in this comment thread). Hopefully that does the trick. I am a bit concerned that we're re-using these nodes for multiple things and that can be confusing, but I guess it's necessary to keep performance reasonable.
hmac
force-pushed
the
splat-flow-4
branch
2 times, most recently
from
0ccd5b7 to
93cf226
Compare
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])
In cases such as
def f(x, *y); end
f(*[1, 2])
we don't need any `SynthSplatArgumentElementNodes`. We get flow from the
splat argument to a `SynthSplatParameterNode` via `parameterMatch`, then
from element 0 of the synth splat to the positional param `x` via a
read step.
We add a read step from element 1 to `SynthSplatParameterElementNode(1)`.
From there we get flow to element 0 of `*y` via an existing store step.
- Only step through a `SynthSplatParameterElementNode` when there is a splat parameter at index > 0. - Model read+stores via `SynthSplatArgumentElementNode` as a single read-store step in type tracking.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| not exists(Argument arg, ArgumentPosition pos | pos.isSplat(_) | arg.isArgumentOf(c, pos)) | ||
| TSynthSplatArgumentNode(CfgNodes::ExprNodes::CallCfgNode c) or | ||
| TSynthSplatArgumentElementNode(CfgNodes::ExprNodes::CallCfgNode c, int n) { | ||
| n in [-1 .. 10] and |
There was a problem hiding this comment.
Maybe add a comment saying that we use -1 to represent unknown index.
No description provided.