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

[Fixes #253] Avoid 'defined?' buggy behavior in ruby 2.5.0 #271

Merged
merged 1 commit into from
Mar 9, 2018

Conversation

grzuy
Copy link
Collaborator

@grzuy grzuy commented Jan 29, 2018

Fixes #253

Found that defined? is buggy in ruby 2.5.0, which under certain circumstances users using rack-attack can hit (see minimal app to reproduce issue 253).

I already reported (https://bugs.ruby-lang.org/issues/14407) and fixed (ruby/ruby#1800) the underlaying issue in ruby, but i am guessing it's going to take some time before there's a new ruby release including the fix.

So for now, we would need to circumvent this ruby 2.5.0 bug by using Object#const_defined? instead of defined?, at least for this particular case.


More details:

Anyone using:

  • ruby 2.5.0
  • redis
  • rack-attack without redis-store and using at least one throttle
  • having a toplevel class named Store

will hit this ruby 2.5.0 bug https://bugs.ruby-lang.org/issues/14407

That's because of the following buggy behavior of defined? under ruby 2.5.0:

$ ruby -v
ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-linux]

$ irb
> class Redis
> end
=> nil
> class Store
> end
=> nil
> defined?(::Redis::Store)
=> "constant"
> ::Redis::Store
  NameError (uninitialized constant Redis::Store
    Did you mean?  Store)

'defined?' is buggy in ruby 2.5.0, which under certain circumstances
users using rack-attack can hit. See issue rack#253.

I reported (https://bugs.ruby-lang.org/issues/14407) and
fixed (ruby/ruby#1800) the issue in
ruby already, but i guess i would take some time before there's
a new ruby release including that fix.

So for now we would need to circumvent this bug by using
'const_defined?' instead of 'defined?' for this particular case.

More details:

Anyone using:
  * ruby 2.5.0
  * redis
  * rack-attack without redis-store and using at least one throttle
  * having a toplevel class named Store

will hit this ruby 2.5.0 bug https://bugs.ruby-lang.org/issues/14407

That's because of the following buggy behavior of 'defined?' under ruby
2.5:

```
$ ruby -v
ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-linux]

$ irb
> class Redis
> end
=> nil
> class Store
> end
=> nil
> defined?(::Redis::Store)
=> "constant"
> ::Redis::Store
  NameError (uninitialized constant Redis::Store
    Did you mean?  Store)
```
@grzuy
Copy link
Collaborator Author

grzuy commented Jan 29, 2018

For anyone interested in testing it, here is a minimal ruby app that reproduces the issue: https://github.com/grzuy/ruby_app_for_rack_attack_253.

In that app's master branch you can reproduce the issue.
In the fix_attempt branch, which is setting rack-attack's dependency to this PR's branch, you can test that the issue is gone :-)

@grzuy grzuy mentioned this pull request Jan 30, 2018
@grzuy
Copy link
Collaborator Author

grzuy commented Jan 31, 2018

@frewsxcv Do you have any feedback about this one? :-)

@grzuy grzuy changed the title Avoid 'defined?' buggy behavior in ruby 2.5.0. Fixes #253 [Fixes #253] Avoid 'defined?' buggy behavior in ruby 2.5.0 Feb 22, 2018
@grzuy grzuy merged commit bed046e into rack:master Mar 9, 2018
@grzuy grzuy deleted the ruby_2-5 branch March 9, 2018 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ruby 2.5 compatibility?
1 participant