Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix infinite loop on mixin self-includes #1611

Merged
merged 1 commit into from
Jul 3, 2021

Conversation

Skipants
Copy link
Contributor

@Skipants Skipants commented Jun 8, 2021

Adds ae37c48:

Fixes a regression in 5.0.2 where a module that had code including itself in a class method would cause Brakeman::Tracker#find_method to infinite loop.

Hi! Thanks for the gem! While upgrading from 5.0.1 to 5.0.2 we had this regression when running brakeman in our CI. I narrowed it down to this unorthodox-but-technically-valid-ruby. We have a module in our codebase that includes itself when building a generic class. It caused brakeman to infinite loop on Brakeman::Tracker#find_method. I turned the smallest PoC I could into a test case.

Another way to solve this might be to prevent a class being added to its own includes collection, but this was the easier fix. Thanks, again!

@presidentbeef
Copy link
Owner

There shouldn't be any regression between 5.0.1->5.0.2. 🤔 Can you try 5.0.4?

@presidentbeef
Copy link
Owner

Oh. Yeah I messed up 5.0.2. You can use 5.0.4 for now and I'll take a look at this code change.

@presidentbeef
Copy link
Owner

Hi @Skipants - thanks for the PR.

I think this should be more general. There are a couple other examples of avoiding infinite recursion loops in Brakeman, for example

def ancestor? parent, seen={}
seen[self.name] = true
if self.parent == parent or seen[self.parent]
true
elsif parent_model = collection[self.parent]
parent_model.ancestor? parent, seen
else
false
end
end

Can you update this PR to use that approach? Thanks!

Fixes inifinite looping on include cycles by only adding classes to the list of includes that are not in the ancestor chain. Also considers the same class an ancestor for this reason.
@Skipants
Copy link
Contributor Author

@presidentbeef I updated and rebased based on your suggestion. I think this is what you meant but let me know. I had to add the condition that a class is its own ancestor because that was the case I ran into. I hope that's not too heavy-handed of a change. All the tests pass for me locally.

@presidentbeef presidentbeef merged commit f5055ea into presidentbeef:main Jul 3, 2021
@presidentbeef
Copy link
Owner

Thanks!

Repository owner locked and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants