Skip to content

Ruby: Data-flow through hashes#8942

Merged
hvitved merged 5 commits intogithub:mainfrom
hvitved:ruby/dataflow/hashes
May 24, 2022
Merged

Ruby: Data-flow through hashes#8942
hvitved merged 5 commits intogithub:mainfrom
hvitved:ruby/dataflow/hashes

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Apr 28, 2022

This PR is the culmination of a lot of preparatory work (#7750, #7677, #7914, #8641, #8786, #8870, #8898, #8938) required for precisely tracking data-flow through hashes in Ruby. The bulk of the work of this PR is about adding flow summaries (and tests) for the members of the Hash class. For some of the summaries, the method names overlap with existing summaries for the Array class, and for those we have adjusted existing summaries to account for hashes.

For example, the (simplified) original flow summary for the delete_if method was

// flow into first parameter of block
input = "Argument[self].Element[any]" and
output = "Argument[block].Parameter[0]"
or
// array indices may get shifted
input = "Argument[self].Element[any]" and
output = "Argument[self].Element[?]"

It has now been generalized to account for (a) for hashes, the value goes into the second parameter of the block, (b) only array indices may get shifted, all other known indices (such as symbols) will not:

// flow into last parameter of block
input = "Argument[self].Element[any]" and
output = "Argument[block].Parameter[" + lastBlockParam + "]" // lastBlockParam is bound via the call-site
or
// array indices may get shifted
input = "Argument[self].Element[0..]" and
output = "Argument[self].Element[?]"
or
// all other indices (including '?') do not get shifted
input = "Argument[self].WithoutElement[0..].WithElement[any]" and
output = "Argument[self]"

Note that if we are tracking a precise integer index (Element(0..)), then we assume that it will be an array, and hence conservatively convert the index to be unknown, although that wouldn't be the case if it was a known integer key in a hash.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 1 vulnerability.

@hvitved hvitved force-pushed the ruby/dataflow/hashes branch from fad94db to 6159e1c Compare May 10, 2022 12:17
@github github deleted a comment from github-advanced-security bot May 10, 2022
@hvitved hvitved force-pushed the ruby/dataflow/hashes branch from 6159e1c to db92901 Compare May 12, 2022 09:50
@hvitved hvitved changed the title Ruby: Initial data-flow through hashes Ruby: Data-flow through hashes May 12, 2022
@hvitved hvitved force-pushed the ruby/dataflow/hashes branch 5 times, most recently from 64d4e23 to 7800907 Compare May 18, 2022 07:56
@hvitved hvitved marked this pull request as ready for review May 19, 2022 08:19
@hvitved hvitved requested a review from a team as a code owner May 19, 2022 08:19
Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

Looks good to me. Could you add some more QLdoc to the flow summaries?

Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

This looks great to me. A minor nit: I think it would be clearer to use more informative field names for the ConstantValues that represent an array index or hash key.

Is the slowdown in performance within expectations?

aibaars
aibaars previously approved these changes May 24, 2022
@hvitved hvitved force-pushed the ruby/dataflow/hashes branch from 84ee88a to 1ae8087 Compare May 24, 2022 12:29
@hvitved
Copy link
Contributor Author

hvitved commented May 24, 2022

Rebased to resolve merge conflict.

@hvitved hvitved merged commit d61f645 into github:main May 24, 2022
@hvitved hvitved deleted the ruby/dataflow/hashes branch May 24, 2022 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants