Skip to content

Rb: add some more flow through splat parameters - #11398

Merged
erik-krogh merged 2 commits into
github:mainfrom
erik-krogh:splat-stuff
Nov 28, 2022
Merged

Rb: add some more flow through splat parameters #11398
erik-krogh merged 2 commits into
github:mainfrom
erik-krogh:splat-stuff

Conversation

@erik-krogh

@erik-krogh erik-krogh commented Nov 23, 2022

Copy link
Copy Markdown
Contributor

This was something I needed in my second-order-command-injection WIP branch.

Precise flow through splat parameters is hard in the general case.
But if we special-case to the situration where both the argument and the parameter are in the first position, then it's easy.

Evaluation looks OK.
The easiest way to see the impact is to look at the new call-edges. Those new edges mostly appear from the receiver being tracked more precisely.
E.g. here where args is a splat parameter that is tracked more precisely with this change.

This is my first venture into the inner workings of the dataflow library, so I hope I got it right.

@github-actions github-actions Bot added the Ruby label Nov 23, 2022
@erik-krogh erik-krogh added the no-change-note-required This PR does not need a change note label Nov 24, 2022
@erik-krogh
erik-krogh marked this pull request as ready for review November 24, 2022 09:21
@erik-krogh
erik-krogh requested a review from a team as a code owner November 24, 2022 09:21

@hvitved hvitved left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not convinced that this adds the type of flow that we want; I think it would be better to add support a la how we do it for hash splats.


args = taint(26)
def splatstuff(*x)
sink x # $ hasValueFlow=26

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want flow in this case; we only want flow to x[n].

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made it similar to how we handle hash-splat, except I still restrict it to the first argument/parameter.

It still gives me the flow I want in the second-order-command-injection PR.

@hvitved hvitved left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good to have some initial support at least for splat parameters. Will you do a final DCA run?

@erik-krogh

erik-krogh commented Nov 28, 2022

Copy link
Copy Markdown
Contributor Author

LGTM, good to have some initial support at least for splat parameters. Will you do a final DCA run?

New evaluation looks about the same as the old evaluation.
Some new call-edges, and a few new results that look good.
I'm also running on rails-projects.yml to be sure.

Edit:
The evaluation on rails-projects.yml also looks clean.
There are a few call-edges that look similar to the new call edges in the above evaluation.

I'm merging.

@erik-krogh
erik-krogh merged commit 0cd50aa into github:main Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-required This PR does not need a change note Ruby

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants