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

Brakeman hangs scanning controllers. #36

Closed
codeferret opened this issue Jan 30, 2012 · 14 comments
Closed

Brakeman hangs scanning controllers. #36

codeferret opened this issue Jan 30, 2012 · 14 comments
Labels

Comments

@codeferret
Copy link
Contributor

Placing the following code in test/apps/rails2/app/controllers/application_controller.rb seems to cause brakeman to hang indefinitely when I attempt to run tests. This came up in an app we were testing.

before_filter :awesome

def funky_panda
end

def awesome
  something = if params[:thang]
    params[:thang]
  elsif somevar = "monkeypanda"
    somevar = somevar.split(",").map { |s|
      s += 'stuff' unless s =~ /regex/
      s.split('things')
    }.first
    somevar.first.downcase
  end

  if (some_var = SomeClass.things, something)
    AnotherClass.thang = @thang = some_var.to_sym
  elsif (some_var = find_thang(AppConfig.stuff, something))
    AnotherClass.thang = @thang = some_var.to_sym
  end

  if beta_override && cookies['yummy'] != @thang.to_s
    cookies['yummy'] = { :value => @thang.to_s }
  end

  return true
end
@presidentbeef
Copy link
Owner

Awesome. I'll dig into this.

@presidentbeef
Copy link
Owner

There seem to be a couple things going on here.

First clue is that it's not in an infinite loop, it's just slowing down a ton. The slow down is due to deep copying of the values. The values themselves are growing quite large.

One reason they are growing large is because of assignments inside branches, which are turned into OR'ed values. This has been greatly alleviated with my latest commit.

Another reason they are growing large is because of s += 'stuff'. Each time this is hit, the value assigned to s is growing. Because s shows up in several places, this is multiplied.

I THINK what's happening is that this state is being stored, and every time awesome gets processed as a filter it is growing larger (instead of starting with a blank slate). This is something to look into tomorrow.

However, in the meantime, I've fixed the --no-branching option so if you use that (or --faster) you can at least get around this for now.

@presidentbeef
Copy link
Owner

Okay, scratch my "stored state" theory. The rest is right, though. What's weird is that this is happening when running Brakeman, but not if I run the AliasProcessor (where all this is happening) manually. Strange.

@presidentbeef
Copy link
Owner

It seems like the issue is that awesome is both a filter and an action. Is this the case in your app?

@oreoshake
Copy link
Contributor

Issue still persists

@oreoshake
Copy link
Contributor

Hmm yeah there's a default routes warning, going to fix that and see

@oreoshake
Copy link
Contributor

I was able to work around this with the 1.2.2 code base (without your most recent changes) by using a local variable to do the manipulation inside the map block:

    somevar = somevar.split(",").map { |s|
      s += 'stuff' unless s =~ /regex/
      s.split('things')
    }.first

to

    somevar = somevar.split(",").map { |s|
      tmp = s
      tmp += 'stuff' unless tmp =~ /regex/
      tmp.split('things')
    }.first

I remember having issues with modifying objects when inside a iterator block in Java, I think it would throw ConcurrentModificationException.

I imagine whatever ruby does (probably making a billion copies) to get around Java's limitation is causing the hang. Even with the latest code base, it hangs until I run out of memory ~30 minutes in.

Going to use the workaround for now.

@presidentbeef
Copy link
Owner

Well, how Ruby handles modification of a block parameter (which this code is not really doing) would have nothing to do with Brakeman, as it's not executing the code.

However, this does narrow down the issue. The problem comes from how Brakeman is processing the assignments.

Original:

somevar = (local somevar).split(",").map do |s|
  s = ((local s) + "stuff") unless (local s) =~ /regex/
  ((local s) + "stuffstuffstuffstuffstuffstuffstuffstuff").split("things")
end.first

Using tmp:

somevar = (local somevar).split(",").map do |s|
  tmp = (local s)
  tmp = ((local s) + "stuff") unless (local s) =~ /regex/
  ((local s) or ((local s) + "stuff")).split("things")
end.first

Edit: I should be able to get this fixed tomorrow.

@presidentbeef presidentbeef reopened this Feb 7, 2012
presidentbeef pushed a commit that referenced this issue Feb 8, 2012
to attempt to avoid stuff blowing up ridiculously
which should help #36
@presidentbeef
Copy link
Owner

Okay, I think I've resolved it. Please try the latest.

@oreoshake
Copy link
Contributor

success!

@presidentbeef
Copy link
Owner

Cool...unfortunately, I don't like it. It prevents detection of some simple things. I have a couple other ideas, though.

On Feb 7, 2012, at 5:03 PM, Neil Matatall reply@reply.github.com wrote:

success!


Reply to this email directly or view it on GitHub:
#36 (comment)

@presidentbeef
Copy link
Owner

Hey Neil, can you verify that the latest still works for you? Thanks!

@oreoshake
Copy link
Contributor

Still works, seems faster

@presidentbeef
Copy link
Owner

Okay, cool. Thanks.

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

No branches or pull requests

3 participants