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

JRuby unable to find methods unless jar is in boot classpath #4266

Closed
jmiettinen opened this Issue Nov 7, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@jmiettinen
Contributor

jmiettinen commented Nov 7, 2016

Environment

  • JRuby 9.1.5.0 or 1.7.23 on OS X (Darwin jmiettinen.local 14.5.0 Darwin Kernel Version 14.5.0: Sun Sep 25 22:07:15 PDT 2016; root:xnu-2782.50.9~1/RELEASE_X86_64 x86_64)
  • No gems, no JAVA_OPTS
  • To reproduce, clone test-case repository and run run.sh which runs both failing and succeeding case.
  • JRuby-complete-jar is used to run org.jruby.Main which then runs a JRuby-script calling a few Java-classes.

Expected Behavior

When running a simple script with just having JRuby in the classpath, not in boot classpath, I would expect the JRuby to correctly resolve Java-methods and call them.

So in that case, the script should print out results of two different builder implementations:

SimpleClassImpl/5
SimpleClassImpl/5
BrokenClassImpl/5
BrokenClassImpl/5

Actual Behavior

What actually happens is that calls succeed when the target Java-class is publicly visible, the calls succeed, but JRuby cannot find the target when implementor of the method is package-private.

SimpleClassImpl/5
SimpleClassImpl/5
NoMethodError: undefined method `set_value' for #<Java::FiRelex::BrokenClass::Builder:0x6fa0450e>
  <main> at src/main/ruby/test.rb:9
@mkristian

This comment has been minimized.

Show comment
Hide comment
@mkristian

mkristian Nov 14, 2016

Member

to get your test case to the point where it fails I applied follow patch

diff --git a/src/main/java/fi/relex/SimpleClassImpl.java b/src/main/java/fi/relex/SimpleClassImpl.java
index 4ea0874..db6978c 100644
--- a/src/main/java/fi/relex/SimpleClassImpl.java
+++ b/src/main/java/fi/relex/SimpleClassImpl.java
@@ -1,10 +1,10 @@
 package fi.relex;

-public class SimpleClassImpl extends SimpleClass {
+class SimpleClassImpl extends SimpleClass {

     private final long value;

-    public SimpleClassImpl(long value) {
+    SimpleClassImpl(long value) {
         this.value = value;
     }

i.e. it is the https://github.com/jmiettinen/jruby-test/blob/master/src/main/java/fi/relex/BrokenClassImpl.java#L15 has a package private Builder whereas https://github.com/jmiettinen/jruby-test/blob/master/src/main/java/fi/relex/SimpleClassImpl.java#L16 is public

weird! I would not ever thought that the bootclassloader can help here - good catch.

Member

mkristian commented Nov 14, 2016

to get your test case to the point where it fails I applied follow patch

diff --git a/src/main/java/fi/relex/SimpleClassImpl.java b/src/main/java/fi/relex/SimpleClassImpl.java
index 4ea0874..db6978c 100644
--- a/src/main/java/fi/relex/SimpleClassImpl.java
+++ b/src/main/java/fi/relex/SimpleClassImpl.java
@@ -1,10 +1,10 @@
 package fi.relex;

-public class SimpleClassImpl extends SimpleClass {
+class SimpleClassImpl extends SimpleClass {

     private final long value;

-    public SimpleClassImpl(long value) {
+    SimpleClassImpl(long value) {
         this.value = value;
     }

i.e. it is the https://github.com/jmiettinen/jruby-test/blob/master/src/main/java/fi/relex/BrokenClassImpl.java#L15 has a package private Builder whereas https://github.com/jmiettinen/jruby-test/blob/master/src/main/java/fi/relex/SimpleClassImpl.java#L16 is public

weird! I would not ever thought that the bootclassloader can help here - good catch.

@jmiettinen

This comment has been minimized.

Show comment
Hide comment
@jmiettinen

jmiettinen Nov 14, 2016

Contributor

Yeah, that's the thing that makes the difference. I should've brought it up more clearly.

As such, being on boot classpath should not have any effect on this as reflection should be able to reach and call both of the classes.
But with JRuby method resolution there seems to be.

Now that I think of it, we've been hit by JRuby being unable call methods from package-private classes before as we're deploying to Tomcat without adding JRuby to boot classpath there.

Contributor

jmiettinen commented Nov 14, 2016

Yeah, that's the thing that makes the difference. I should've brought it up more clearly.

As such, being on boot classpath should not have any effect on this as reflection should be able to reach and call both of the classes.
But with JRuby method resolution there seems to be.

Now that I think of it, we've been hit by JRuby being unable call methods from package-private classes before as we're deploying to Tomcat without adding JRuby to boot classpath there.

@jmiettinen

This comment has been minimized.

Show comment
Hide comment
@jmiettinen

jmiettinen Dec 9, 2016

Contributor

The difference seems to stem from the fact that in JavaUtil there's a check for suppressAccessChecks permission. If that check fails, the code concludes that it cannot call AccessibleObject#setAccessible. This check will succeed if JRuby is in boot classpath and fail otherwise.

This is not true at least if there's no SecurityManager present.

My colleague did some digging and it seems that @headius introduced first ban to not use methods from private classes: ec8d280 as a fix for JRUBY-4799.

This ban seems to have been loosened a bit later on and public methods for private classes are added if JavaUtil.CAN_SET_ACCESSIBLE is true.

I am not very familiar with how the AccessController should be used, but the current usage doesn't seem to match what happens in AccessibleObject#setAccessible.
There code first checks if there is a SecurityManager set and only if so calls SecurityManager#checkPermission(Permission) that delegates to the same AccessController#checkPermission(Permission) as the code in JavaUtil does.

Contributor

jmiettinen commented Dec 9, 2016

The difference seems to stem from the fact that in JavaUtil there's a check for suppressAccessChecks permission. If that check fails, the code concludes that it cannot call AccessibleObject#setAccessible. This check will succeed if JRuby is in boot classpath and fail otherwise.

This is not true at least if there's no SecurityManager present.

My colleague did some digging and it seems that @headius introduced first ban to not use methods from private classes: ec8d280 as a fix for JRUBY-4799.

This ban seems to have been loosened a bit later on and public methods for private classes are added if JavaUtil.CAN_SET_ACCESSIBLE is true.

I am not very familiar with how the AccessController should be used, but the current usage doesn't seem to match what happens in AccessibleObject#setAccessible.
There code first checks if there is a SecurityManager set and only if so calls SecurityManager#checkPermission(Permission) that delegates to the same AccessController#checkPermission(Permission) as the code in JavaUtil does.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Dec 9, 2016

Member

@jmiettinen thanks for the analysis ... your logic totally makes sense, I think there are 2 issues to fix :

  • change

    AccessController.checkPermission(new ReflectPermission("suppressAccessChecks"));
    to do some real code to check accessibility

  • secondly, different methods being installed based on accessibility is quite a 'surprising' feature, JRuby should at least warn or figure out a uniform way to install and than fail at inaccessible methods at call-time (this is not something fixable in a patch release as it needs compatibility)

Member

kares commented Dec 9, 2016

@jmiettinen thanks for the analysis ... your logic totally makes sense, I think there are 2 issues to fix :

  • change

    AccessController.checkPermission(new ReflectPermission("suppressAccessChecks"));
    to do some real code to check accessibility

  • secondly, different methods being installed based on accessibility is quite a 'surprising' feature, JRuby should at least warn or figure out a uniform way to install and than fail at inaccessible methods at call-time (this is not something fixable in a patch release as it needs compatibility)

@kares kares added this to the JRuby 9.1.7.0 milestone Dec 16, 2016

@kares kares closed this Dec 16, 2016

kares added a commit to kares/jruby that referenced this issue Dec 16, 2016

kares added a commit that referenced this issue Dec 17, 2016

jmiettinen added a commit to jmiettinen/jruby that referenced this issue Dec 21, 2016

kares added a commit that referenced this issue Dec 30, 2016

headius added a commit to headius/jruby that referenced this issue May 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment