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 stack overflow when loading default config - JRuby on Windows 8 #1659

Merged
merged 1 commit into from
Feb 24, 2015

Conversation

pimterry
Copy link

It's impossible to start Rubocop on my machine, with JRuby and Windows 8 (not sure which is the issue). Rubocop version 0.29.1.

If you try to so, you get a stack overflow, with:

SystemStackError: stack level too deep                                                                              
                   each at org/jruby/RubyArray.java:1613                                                            
              partition at org/jruby/RubyEnumerable.java:914                                                        
               validate at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config.rb:102       
      warn_unless_valid at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config.rb:80        
              load_file at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config_loader.rb:41 
           base_configs at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config_loader.rb:69 
                    map at org/jruby/RubyArray.java:2412                                                            
           base_configs at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config_loader.rb:60 
    resolve_inheritance at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config_loader.rb:128
              load_file at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config_loader.rb:29 
  default_configuration at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config_loader.rb:99 
               validate at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config.rb:103       
                   each at org/jruby/RubyArray.java:1613                                                            
              partition at org/jruby/RubyEnumerable.java:914                                                        
               validate at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config.rb:102       
      warn_unless_valid at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config.rb:80        
              load_file at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config_loader.rb:41 
           base_configs at C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/lib/rubocop/config_loader.rb:69 
                    map at org/jruby/RubyArray.java:2412         
                    ....                                                   

This happens because the check that ensures Rubocop does not validate its own config (and fall into an infinite loop) simply checks that RUBOCOP_HOME is not a prefix (via start_with?) of the config path. That's not sufficient.

For this check to be sufficient each path must be in a canonical format first, as otherwise two different (by string comparison) paths might actually refer to the same underlying path, giving false negatives.

In my case the paths being compared are:

C:\jruby-1.7.18\lib\ruby\gems\shared\gems\rubocop-0.29.1/config
C:/jruby-1.7.18/lib/ruby/gems/shared/gems/rubocop-0.29.1/config/enabled.yml

Note the different slashes. Path 1 is in reality a parent directory of path 2, but it is not a string prefix of path 2 (which is what the current test checks for). This seems to happen because File.realpath produces different slashes to File.join, but I don't think the source of this matters.

This patch fixes this generally, by ensuring the paths are always run through File.expand_path first, which ensures you get a canonical format for the path, not just what you were given from elsewhere. I've also put in an actual test for this feature (not validating your built-in config), since there wasn't one.

'config'))
base_config_path = File.expand_path(File.join(ConfigLoader::RUBOCOP_HOME,
'config'))
return if File.expand_path(loaded_path).start_with? base_config_path
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add () for starts_with?.

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 17, 2015

The change looks good to me. Address my tiny remarks and add a changelog entry.

@pimterry pimterry force-pushed the infinite-path-check-loop branch from 6e35555 to e4ecd4c Compare February 21, 2015 18:45
@pimterry
Copy link
Author

Fixes and changelog entry added, and rebased.

@@ -8,6 +8,9 @@
* [#1604](https://github.com/bbatsov/rubocop/issues/1604): Add `IgnoreClassMethods` option to `TrivialAccessors` cop. ([@bbatsov][])
* [#1651](https://github.com/bbatsov/rubocop/issues/1651): The `Style/SpaceAroundOperators` cop now also detects extra spaces around operators. A list of operators that *may* be surrounded by multiple spaces is configurable. ([@bquorning][])

### Bugs fixed
* [#1659](https://github.com/bbatsov/rubocop/pull/1659): Fix stack overflow with JRuby and Windows 8, during initial config validation. ([@pimterry][])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a blank line here.

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 21, 2015

Squash the two commits and we're good to go.

@pimterry pimterry force-pushed the infinite-path-check-loop branch from e4ecd4c to 2b2a0a7 Compare February 21, 2015 23:08
@pimterry
Copy link
Author

Squashed, should be good to go now.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 99.65% when pulling 2b2a0a7 on pimterry:infinite-path-check-loop into 76e8f88 on bbatsov:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 99.65% when pulling 2b2a0a7 on pimterry:infinite-path-check-loop into 76e8f88 on bbatsov:master.

bbatsov added a commit that referenced this pull request Feb 24, 2015
Fix stack overflow when loading default config - JRuby on Windows 8
@bbatsov bbatsov merged commit 2e463b0 into rubocop:master Feb 24, 2015
@bbatsov
Copy link
Collaborator

bbatsov commented Feb 24, 2015

👍

@pimterry pimterry deleted the infinite-path-check-loop branch February 24, 2015 10:34
@pimterry
Copy link
Author

pimterry commented Mar 2, 2015

Thanks for merging this. Any timeline on when it'll be released? I have some developers on Windows machines working with JRuby atm, and Rubocop being unusable in those conditions is causing some problems.

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.

3 participants