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 off-by-one error causing Dir.glob("foo" + "{bar}") to fail if Java asserts are enabled #5612

Closed

Conversation

@ikaronen-relex
Copy link
Contributor

commented Feb 12, 2019

When Java asserts are enabled, Dir.glob and Dir[] crash if given a pattern that ends in a curly brace. However, this only happens if the pattern string has not been interned. (Specifically, the underlying ByteList must have bytes.length == realSize for the bug to trigger. Apparently that's not usually the case for interned string literals.)

Here is a simple command-line test case:

JAVA_OPTS=-ea bin/jruby -J-ea -e 'Dir.glob("foo" + "{bar}")'

(I have no idea why both JAVA_OPTS=-ea and -J-ea are needed to enable Java asserts, but that seems to be the case. That might be a separate issue.)

On JRuby 9.2.6.0, running the command above produces the following output:

Unhandled Java exception: java.lang.AssertionError: Invalid start: 8
java.lang.AssertionError: Invalid start: 8
               append at org/jruby/util/ByteList.java:542
          push_braces at org/jruby/util/Dir.java:473
            push_glob at org/jruby/util/Dir.java:312
                 glob at org/jruby/RubyDir.java:271
                 call at org/jruby/RubyDir$INVOKER$s$0$2$glob.gen:-1
                 call at org/jruby/internal/runtime/methods/DynamicMethod.java:204
                 call at org/jruby/internal/runtime/methods/DynamicMethod.java:200
         cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:346
                 call at org/jruby/runtime/callsite/CachingCallSite.java:172
    invokeOther3:glob at -e:1
               <main> at -e:1
  invokeWithArguments at java/lang/invoke/MethodHandle.java:627
                 load at org/jruby/ir/Compiler.java:94
            runScript at org/jruby/Ruby.java:854
          runNormally at org/jruby/Ruby.java:777
          runNormally at org/jruby/Ruby.java:795
          runFromMain at org/jruby/Ruby.java:607
        doRunFromMain at org/jruby/Main.java:415
          internalRun at org/jruby/Main.java:307
                  run at org/jruby/Main.java:234
                 main at org/jruby/Main.java:206

As it happens, this assertion error is also triggered by running RSpec with no arguments (and with Java asserts enabled), since the glob pattern it uses to find spec files ("spec/{**{,/*/**}/*_spec.rb}"by default) ends in a curly brace (and is constructed via concatenation at runtime, and thus not interned). In particular, running:

JAVA_OPTS=-ea bin/jruby -J-ea -S rspec

fails with:

Unhandled Java exception: java.lang.AssertionError: Invalid start: 27
java.lang.AssertionError: Invalid start: 27
               append at org/jruby/util/ByteList.java:542
          push_braces at org/jruby/util/Dir.java:473
            push_glob at org/jruby/util/Dir.java:312
                 aref at org/jruby/RubyDir.java:230
                 call at org/jruby/RubyDir$INVOKER$s$0$0$aref.gen:-1
                 call at org/jruby/internal/runtime/methods/JavaMethod.java:797
                 call at org/jruby/internal/runtime/methods/DynamicMethod.java:200
                 ...

The cause of this bug is a simple off-by-one error in the assert at ByteList.java line 524 (which was formerly commented out due to CI failures, and reintroduced into JRuby 9.2.6.0 by commit 607998c). Instead of:

assert start >= 0 && (start == 0 || start < moreBytes.length) : "Invalid start: " + start;

the correct assertion would simply be:

assert start >= 0 && start <= moreBytes.length : "Invalid start: " + start;

This change permits the degenerate but valid use case of appending 0 bytes starting at the end of a non-empty byte array.


Ps. Here's one more direct test case that doesn't depend on the specific behavior of the Dir class:

JAVA_OPTS=-ea bin/jruby -J-ea -e 'org.jruby.util.ByteList.new.append("foo".bytes.to_java(:byte), 3, 0)'

I would include this (and the first test case above) as specs in the PR, but it would seem that JRuby specs are currently run with Java asserts disabled by default, and I'm not sure how to change or work around that.

@headius

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

(I have no idea why both JAVA_OPTS=-ea and -J-ea are needed to enable Java asserts, but that seems to be the case. That might be a separate issue.)

Probably because it launches a subprocess. The -J flags only apply to the target process.

You're right, we should run asserts in the specs. That would be tweakable under rakelib/spec.rake I think?

Thanks for the work on this one!

@headius

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

I pushed a tweak to the build that enables assertions across the board: 21e4663.

@headius

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Master is green again with assertions enabled. Perhaps now you can merge or rebase and add specs for the behaviors you fixed?

@headius headius added this to the JRuby 9.2.7.0 milestone Feb 13, 2019

@ikaronen-relex ikaronen-relex force-pushed the ikaronen-relex:glob-curly-at-end-bug branch from fe11a3a to e65a9a4 Feb 13, 2019

@ikaronen-relex

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

Well, I wrote what I thought would be a reasonable basic regression test, but when I try to run it using the instructions at https://github.com/jruby/jruby/blob/master/BUILDING.md, i.e. with the command:

bin/jruby spec/mspec/bin/mspec ci spec/regression/GH-5612_glob_pattern_ending_in_curly_brace_fails_assert_in_ByteList_append_spec.rb 

all I get is:

$ /Users/ilmarikaronen/git/jruby/bin/jruby /Users/ilmarikaronen/git/jruby/spec/mspec/bin/mspec-ci spec/regression/GH-5612_glob_pattern_ending_in_curly_brace_fails_assert_in_ByteList_append_spec.rb
jruby 9.2.6.0-SNAPSHOT (2.5.3) 2018-12-13 976b651 Java HotSpot(TM) 64-Bit Server VM 25.192-b12 on 1.8.0_192-b12 +jit [darwin-x86_64]
                                                                                             
1)
Java::OrgJrubyUtil::ByteList append(byte[], int, int) throws AssertionError on invalid start or length ERROR
NoMethodError: undefined method `expect' for #<MSpecEnv:0xba8d91c>
Did you mean?  exec
/Users/ilmarikaronen/git/jruby/spec/regression/GH-5612_glob_pattern_ending_in_curly_brace_fails_assert_in_ByteList_append_spec.rb:6:in `block in <main>'
org/jruby/RubyBasicObject.java:2606:in `instance_eval'
org/jruby/RubyEnumerable.java:1753:in `all?'
org/jruby/RubyArray.java:1792:in `each'
org/jruby/RubyArray.java:1792:in `each'
/Users/ilmarikaronen/git/jruby/spec/regression/GH-5612_glob_pattern_ending_in_curly_brace_fails_assert_in_ByteList_append_spec.rb:1:in `<main>'
org/jruby/RubyKernel.java:1010:in `load'
org/jruby/RubyBasicObject.java:2606:in `instance_eval'
org/jruby/RubyArray.java:1792:in `each'
                                                                                             
2)
Java::OrgJrubyUtil::ByteList append(byte[], int, int) does not throw for valid start and length ERROR
NoMethodError: undefined method `expect' for #<MSpecEnv:0xba8d91c>
Did you mean?  exec
/Users/ilmarikaronen/git/jruby/spec/regression/GH-5612_glob_pattern_ending_in_curly_brace_fails_assert_in_ByteList_append_spec.rb:15:in `block in <main>'
org/jruby/RubyBasicObject.java:2606:in `instance_eval'
org/jruby/RubyEnumerable.java:1753:in `all?'
org/jruby/RubyArray.java:1792:in `each'
org/jruby/RubyArray.java:1792:in `each'
/Users/ilmarikaronen/git/jruby/spec/regression/GH-5612_glob_pattern_ending_in_curly_brace_fails_assert_in_ByteList_append_spec.rb:1:in `<main>'
org/jruby/RubyKernel.java:1010:in `load'
org/jruby/RubyBasicObject.java:2606:in `instance_eval'
org/jruby/RubyArray.java:1792:in `each'
                                                                                             
3)
Dir does not crash if a glob pattern ends in a curly brace ERROR
NoMethodError: undefined method `expect' for #<MSpecEnv:0xba8d91c>
Did you mean?  exec
/Users/ilmarikaronen/git/jruby/spec/regression/GH-5612_glob_pattern_ending_in_curly_brace_fails_assert_in_ByteList_append_spec.rb:27:in `block in <main>'
org/jruby/RubyBasicObject.java:2606:in `instance_eval'
org/jruby/RubyEnumerable.java:1753:in `all?'
org/jruby/RubyArray.java:1792:in `each'
/Users/ilmarikaronen/git/jruby/spec/regression/GH-5612_glob_pattern_ending_in_curly_brace_fails_assert_in_ByteList_append_spec.rb:25:in `<main>'
org/jruby/RubyKernel.java:1010:in `load'
org/jruby/RubyBasicObject.java:2606:in `instance_eval'
org/jruby/RubyArray.java:1792:in `each'
[| | ==================100%================== | 00:00:00]      0F      3E 

Finished in 0.278223 seconds

1 file, 3 examples, 0 expectations, 0 failures, 3 errors, 0 tagged

It looks as if the modern expect rspec syntax is somehow disabled, even though I see other existing specs using it. I could try to debug this further myself, but honestly it looks like either the rspec config or the documentation I'm trying to follow is somehow messed up.

@headius

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Ah, don't run mspec for those specs. mspec is only for the "Ruby specs" under spec/ruby.

Install rspec and run that for spec/regression, or rake spec:regression.

@headius

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

@ikaronen-relex We have not had time to follow up on this, so punting it for this release. If it's ready to go, we can merge it into 9.2.8.0, due out in May.

@headius headius modified the milestones: JRuby 9.2.7.0, JRuby 9.2.8.0 Apr 9, 2019

kares added a commit to kares/jruby that referenced this pull request Apr 11, 2019

[fix] pass -J-ea down to java as -ea
it did not work for a while (was noticed at jrubyGH-5612)

assertions are enabled in tests since: 21e4663
@kares

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

okay, so I fixed the -ea issue - it wasn't being passed down to launcher and I am seeing failures.
CI got red-ish ... including the issue mentioned here: https://travis-ci.org/kares/jruby/builds/518835139

@headius

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@kares Wow! Ok, I thought I had fixed -ea stuff previously, but I think I only did it for rubyspec (which seems half ok). These all need to be fixed, obviously!

kares added a commit to kares/jruby that referenced this pull request Apr 12, 2019

[fix] pass -J-ea down to java as -ea
it did not work for a while (was noticed at jrubyGH-5612)

assertions are enabled in tests since: 21e4663
@kares

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

the relevant commit is now on master, there were several ByteList failures (as explained here) wout it
43a760d

@kares kares closed this Apr 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.