Skip to:

  1. Skip to Jira Navigation
  2. Skip to Side Navigation
  3. Skip to Main Content

Reflection on internal classes fails under Java 9

Description

Due to changes in reflective access for the Jigsaw module system in Java 9, the Reflector will now fail on some cases that worked in previous Java versions.

(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))

Here fac will be an instance of com.sun.xml.internal.stream.XMLInputFactoryImpl, which is an extension of javax.xml.stream.XMLInputFactory. In the new java.xml module, javax.xml.stream is an exported package, but the XMLInputFactoryImpl is an internal extension of the public abstract class in that package. The invocation of createXMLStreamReader will be reflective and the Reflector will attempt to invoke the method based on the implementation class, which is not accessible outside the module, yielding:

WARNING: An illegal reflective access operation has occurred WARNING: Illegal reflective access by clojure.lang.Reflector (file:/Users/alex/.m2/repository/org/clojure/clojure/1.10.0-alpha8/clojure-1.10.0-alpha8.jar) to method com.sun.xml.internal.stream.XMLInputFactoryImpl.createXMLStreamReader(java.io.Reader) WARNING: Please consider reporting this to the maintainers of clojure.lang.Reflector WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations WARNING: All illegal access operations will be denied in a future release

A workaround here is to avoid the reflective call by type-hinting to the public exported interface but that defeats the point of reflection support:

(.createXMLStreamReader ^javax.xml.stream.XMLInputFactory fac (java.io.StringReader. ""))

Another (undesirable) workaround is to export the private package from java.xml to the unnamed module (which is the module used when code is loaded from the classpath rather than from a module) when invoking java/javac:

java --add-exports=java.xml/com.sun.xml.internal.stream=ALL-UNNAMED --add-exports=java.xml/com.sun.xml.internal.stream.writers=ALL-UNNAMED --add-exports=java.xml/com.sun.org.apache.xerces.internal.impl=ALL-UNNAMED ...

Alternatives:

We have investigated a number of options to enhance the Reflector. When we are trying to reflect, the only class we know is the class of the target object (here a non-public class, but one we can look at due to the default settings of --illegal-access). Some options considered include:

  • checking canAccess() on the Method (here it returns true, so no help)

  • invoking and looking for IllegalAccessError (pretty gross)

  • traversing the super class and interface hierarchy to find "more public" instances (this really is replicating logic that already exists in the Java method lookup logic)

None of these is "good".

Proposed:

This continues to be a warning on Java 9, 10, and (just released) 11. The warning is accurate - you are using reflection to a method on a non-exported class. The default setting in Java 9, 10, 11 is --illegal-access=permit which allows this but reports a warning on the first occurrence (other settings are warn, debug, and deny). In some future Java, the default may be deny.

If at some point in the future the JVM changes the default to deny, we still want these reflective calls to work. In that case, we will know this situation has occurred because Method#canAccess will return false. When that happens, we need to look up through the superclasses of the target object to find one that has an accessible version of the method.

Patch: clj-2066-6.patch - when invoking an instance method, map each method to an accessible super class method, then filter non-null methods found (presumably all of them). When on Java 8 or when --illegal-access=permit (the default on Java 9, 10, and 11), then the canAccess() check on each method will return true, allowing it to occur. If --illegal-access=deny, then canAccess() will be false and super class methods will be used instead.

Cases to test for screening (all with example at top):

  • Java 8 - should work with no warnings

  • Java 9, 10, and 11 with default JVM options (should succeed, but warn per --illegal-access=permit)

  • Java 9, 10, and 11 with --illegal-access=deny (should succeed, with no warnings)

  • Java 9, 10, and 11 with "--add-opens java.xml/com.sun.xml.internal.stream=ALL-UNNAMED" - should succeed, with no warnings

More info:

Environment

None

Attachments

6
  • 03 Oct 2018, 08:09 PM
  • 02 Oct 2018, 10:14 PM
  • 21 Sep 2018, 09:44 PM
  • 26 Aug 2018, 03:18 AM
  • 07 Sep 2017, 01:23 PM
  • 16 Mar 2017, 03:20 PM

Activity

Show:

Alex MillerOctober 3, 2018 at 8:09 PM

Small tweaks in -6 patch

Alex MillerOctober 2, 2018 at 10:28 PM

Apply the patch and {{mvn clean test }} (with Java 8, since that's our build setup).

To test, I am using a deps.edn in my clojure repo like this:

{:deps {org.clojure/clojure {:mvn/version "1.10.0-alpha8"} org.clojure/test.check {:mvn/version "0.9.0"}} :aliases {:dbg {:classpath-overrides {org.clojure/clojure "/Users/alex/code/clojure/target/classes"}}}} ;; <-- change

You can then test in the various modes (swapping JDK independently) with:

;; Java 8: should succeed with no warnings clj -A:dbg -e '(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))' ;; Java 9, 10, 11: should warn but succeed clj -A:dbg -e '(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))' ;; Java 9, 10, 11: should NOT warn and succeed clj -A:dbg -J--add-opens -Jjava.xml/com.sun.xml.internal.stream=ALL-UNNAMED -e '(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))' ;; Java 9, 10, 11: should NOT warn and succeed (new code exercised in this case) clj -A:dbg -J--illegal-access=deny -e '(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))'

Ghadi ShaybanAugust 26, 2018 at 3:18 AM

Adding a patch with the approach I suggested above. Relevant trace output of the motiving repro case.

Java 10 Denying Access -- Future default setting

/usr/lib/jvm/java-10-openjdk/bin/java --illegal-access=deny -jar ./clojure.jar - <<EOF (def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. "")) EOF looking up createXMLStreamReader on com.sun.xml.internal.stream.XMLInputFactoryImpl looking up createXMLStreamReader on javax.xml.stream.XMLInputFactory found on javax.xml.stream.XMLInputFactory

Java 10 Default Setting

/usr/lib/jvm/java-10-openjdk/bin/java -jar ./clojure.jar - <<EOF (def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. "")) EOF looking up createXMLStreamReader on com.sun.xml.internal.stream.XMLInputFactoryImpl WARNING: An illegal reflective access operation has occurred WARNING: Illegal reflective access by clojure.lang.InjectedInvoker/319670866 (file:/home/ghadi/dev/clojure/clojure.jar) to method com.sun.xml.internal.stream.XMLInputFactoryImpl.createXMLStreamReader(java.io.Reader) WARNING: Please consider reporting this to the maintainers of clojure.lang.InjectedInvoker/319670866 WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations WARNING: All illegal access operations will be denied in a future release found on com.sun.xml.internal.stream.XMLInputFactoryImpl

Java 8

/usr/lib/jvm/java-8-openjdk/bin/java -jar ./clojure.jar - <<EOF (def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. "")) EOF looking up createXMLStreamReader on com.sun.xml.internal.stream.XMLInputFactoryImpl found on com.sun.xml.internal.stream.XMLInputFactoryImpl

%

Ghadi ShaybanAugust 24, 2018 at 8:29 PM

Since illegal reflective ops are going to be denied in the near future, this is still a relevant issue. You can simulate the pending future behavior by adding the switch --illegal-access=deny on JDK9+

Now that Clojure is JDK1.8 minimum, a new way around the IllegalAccess catching is available to us, and it doesn't involve multirelease jars or anything messy like that.

The predicate j.l.reflect.Method::canAccess(targetObj) allows you to determine accessibility, and it respects modules. It will return false if you are calling a non-exported Method (e.g. on the XML impl subclass), but true on the public method upstairs in the chain. pseudocode:

static final MethodHandle ACCESSIBLE; static { ACCESSIBLE = if jdk8? then constantly true else MH.Lookup("canAccess") }

This will help avoid the catching and rethrowing. We'll still need to have the ancestor lookup logic as in the current patch.

tcrawleyNovember 4, 2017 at 1:38 AM

I realized while responding to the mailing-list thread for beta4 that this patch actually does nothing under non-EA builds of Java 9, since it no longer throws on illegal reflective access, it just warns. I believe the only way to avoid the warning (without code compiled for Java 9) would be to calculate the ancestor chain, then start from the top of that, asking for the method. However, we'd have to do that for every reflective call, since we'd have no way to know when we have to do it to avoid the warning.

Completed

Details

Assignee

Reporter

Labels

Approval

Ok

Patch

Code and Test

Priority

Affects versions

Fix versions

Created November 22, 2016 at 4:01 PM
Updated October 5, 2018 at 12:19 AM
Resolved October 5, 2018 at 12:19 AM