Fix off-by-one error causing Dir.glob("foo" + "{bar}") to fail if Java asserts are enabled by ikaronen-relex · Pull Request #5612 · jruby/jruby

ikaronen-relex · 2019-02-12T18:57:58Z

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 · 2019-02-12T20:23:37Z

(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 · 2019-02-12T20:50:27Z

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

headius · 2019-02-13T05:02:16Z

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

…a asserts are enabled

ikaronen-relex · 2019-02-13T09:23:55Z

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 · 2019-02-14T00:44:26Z

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 · 2019-04-09T00:04:30Z

@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.

it did not work for a while (was noticed at jrubyGH-5612) assertions are enabled in tests since: 21e4663

kares · 2019-04-11T16:48:52Z

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 · 2019-04-12T05:44:11Z

@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!

it did not work for a while (was noticed at jrubyGH-5612) assertions are enabled in tests since: 21e4663

kares · 2019-04-13T06:05:52Z

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

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

ikaronen-relex added 2 commits February 13, 2019 11:08

Fix off-by-one error causing Dir.glob("foo" + "{bar}") to fail if Jav…

5b52ba9

…a asserts are enabled

try to add spec for PR 5612

e65a9a4

ikaronen-relex force-pushed the glob-curly-at-end-bug branch from fe11a3a to e65a9a4 Compare February 13, 2019 09:19

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

91988f1

it did not work for a while (was noticed at jrubyGH-5612) assertions are enabled in tests since: 21e4663

headius mentioned this pull request Apr 12, 2019

Address all tests failing with asserts enabled #5692

Closed

kares mentioned this pull request Apr 12, 2019

pass -ea to JVM and fix asserts for test targets #5693

Merged

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

[fix] pass -J-ea down to java as -ea

8571acc

it did not work for a while (was noticed at jrubyGH-5612) assertions are enabled in tests since: 21e4663

kares closed this Apr 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix off-by-one error causing Dir.glob("foo" + "{bar}") to fail if Java asserts are enabled#5612

Fix off-by-one error causing Dir.glob("foo" + "{bar}") to fail if Java asserts are enabled#5612
ikaronen-relex wants to merge 2 commits intojruby:masterfrom
ikaronen-relex:glob-curly-at-end-bug

ikaronen-relex commented Feb 12, 2019

Uh oh!

headius commented Feb 12, 2019

Uh oh!

headius commented Feb 12, 2019

Uh oh!

headius commented Feb 13, 2019

Uh oh!

ikaronen-relex commented Feb 13, 2019 •

edited

Loading

Uh oh!

headius commented Feb 14, 2019

Uh oh!

headius commented Apr 9, 2019

Uh oh!

kares commented Apr 11, 2019

Uh oh!

headius commented Apr 12, 2019

Uh oh!

kares commented Apr 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ikaronen-relex commented Feb 12, 2019

Uh oh!

headius commented Feb 12, 2019

Uh oh!

headius commented Feb 12, 2019

Uh oh!

headius commented Feb 13, 2019

Uh oh!

ikaronen-relex commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

headius commented Feb 14, 2019

Uh oh!

headius commented Apr 9, 2019

Uh oh!

kares commented Apr 11, 2019

Uh oh!

headius commented Apr 12, 2019

Uh oh!

kares commented Apr 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ikaronen-relex commented Feb 13, 2019 •

edited

Loading