ReflectUtils breaks with Java 9 build 148+ #93

Closed
uschindler opened this Issue Dec 26, 2016 · 23 comments

Projects

None yet

3 participants

@uschindler
uschindler commented Dec 26, 2016 edited

Since Java 9 build 148, it is no longer possible to do setAccessible on any public Java runtime API class (except sun.misc.Unsafe as special case). This affects ClassLoader#defineClass (protected method).

Because of this any mocking library (Mockito, EasyMock,...) that uses CGLIB breaks with ExceptionOnInitializer.

Example in Apache Solr (Mockito/Easymock):

   [junit4] ERROR   0.38s J2 | TestManagedSchemaThreadSafety.testThreadSafety <<<
   [junit4]    > Throwable #1: java.lang.ExceptionInInitializerError
   [junit4]    >        at __randomizedtesting.SeedInfo.seed([8E654E5E1F32A757:142F5A06CCAD15A1]:0)
   [junit4]    >        at org.mockito.cglib.core.KeyFactory$Generator.generateClass(KeyFactory.java:167)
   [junit4]    >        at org.mockito.cglib.core.DefaultGeneratorStrategy.generate(DefaultGeneratorStrategy.java:25)
   [junit4]    >        at org.mockito.cglib.core.AbstractClassGenerator.create(AbstractClassGenerator.java:217)
   [junit4]    >        at org.mockito.cglib.core.KeyFactory$Generator.create(KeyFactory.java:145)
   [junit4]    >        at org.mockito.cglib.core.KeyFactory.create(KeyFactory.java:117)
   [junit4]    >        at org.mockito.cglib.core.KeyFactory.create(KeyFactory.java:109)
   [junit4]    >        at org.mockito.cglib.core.KeyFactory.create(KeyFactory.java:105)
   [junit4]    >        at org.mockito.cglib.proxy.Enhancer.<clinit>(Enhancer.java:70)
   [junit4]    >        at org.mockito.internal.creation.jmock.ClassImposterizer.createProxyClass(ClassImposterizer.java:85)
   [junit4]    >        at org.mockito.internal.creation.jmock.ClassImposterizer.imposterise(ClassImposterizer.java:62)
   [junit4]    >        at org.mockito.internal.creation.jmock.ClassImposterizer.imposterise(ClassImposterizer.java:56)
   [junit4]    >        at org.mockito.internal.creation.CglibMockMaker.createMock(CglibMockMaker.java:23)
   [junit4]    >        at org.mockito.internal.util.MockUtil.createMock(MockUtil.java:26)
   [junit4]    >        at org.mockito.internal.MockitoCore.mock(MockitoCore.java:51)
   [junit4]    >        at org.mockito.Mockito.mock(Mockito.java:1243)
   [junit4]    >        at org.apache.solr.schema.TestManagedSchemaThreadSafety.createZkController(TestManagedSchemaThreadSafety.java:135)
   [junit4]    >        at org.apache.solr.schema.TestManagedSchemaThreadSafety.testThreadSafety(TestManagedSchemaThreadSafety.java:118)
   [junit4]    >        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
   [junit4]    >        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
   [junit4]    >        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
   [junit4]    >        at java.base/java.lang.reflect.Method.invoke(Method.java:538)
   [junit4]    >        at java.base/java.lang.Thread.run(Thread.java:844)
   [junit4]    > Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make protected final java.lang.Class java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain) throws java.lang.ClassFormatError accessible: module java.base does not "opens java.lang" to unnamed module @4cbd6df7
   [junit4]    >        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:207)
   [junit4]    >        at java.base/java.lang.reflect.Method.checkCanSetAccessible(Method.java:192)
   [junit4]    >        at java.base/java.lang.reflect.Method.setAccessible(Method.java:186)
   [junit4]    >        at org.mockito.cglib.core.ReflectUtils$2.run(ReflectUtils.java:57)
   [junit4]    >        at java.base/java.security.AccessController.doPrivileged(Native Method)
   [junit4]    >        at org.mockito.cglib.core.ReflectUtils.<clinit>(ReflectUtils.java:47)
   [junit4]    >        ... 55 more
   [junit4]   2> 64469 INFO  (SUITE-TestManagedSchemaThreadSafety-seed#[8E654E5E1F32A757]-worker) [    ] o.a.s.c.ZkTestServer connecting to 127.0.0.1:51648 51648
   [junit4]   2> 64474 INFO  (Thread-158) [    ] o.a.s.c.ZkTestServer connecting to 127.0.0.1:51648 51648
   [junit4]   2> 64478 INFO  (SUITE-TestManagedSchemaThreadSafety-seed#[8E654E5E1F32A757]-worker) [    ] o.a.s.SolrTestCaseJ4 ###deleteCore
   [junit4]   2> NOTE: leaving temporary files on disk at: C:\Users\Uwe Schindler\Projects\lucene\trunk-lusolr1\solr\build\solr-core\test\J2\temp\solr.schema.TestManagedSchemaThreadSafety_8E654E5E1F32A757-001
   [junit4]   2> Dez. 26, 2016 8:47:14 NACHM. com.carrotsearch.randomizedtesting.ThreadLeakControl checkThreadLeaks
   [junit4]   2> WARNUNG: Will linger awaiting termination of 2 leaked thread(s).
   [junit4]   2> NOTE: test params are: codec=Lucene70, sim=RandomSimilarity(queryNorm=false): {}, locale=so-KE, timezone=Etc/GMT+4
   [junit4]   2> NOTE: Windows 10 10.0 amd64/Oracle Corporation 9-ea (64-bit)/cpus=4,threads=1,free=188820656,total=266338304
   [junit4]   2> NOTE: All tests run in this JVM: [TestRandomFaceting, TestCharFilters, BlockJoinFacetSimpleTest, SolrCmdDistributorTest, TestTolerantUpdateProcessorRandomCloud, TestManagedSchemaThreadSafety]
   [junit4] Completed [31/670 (1!)] on J2 in 0.83s, 1 test, 1 error <<< FAILURES!

I have no idea why CGLib needs to do this (instead of just subclassing ClassLoader). I have the feeling that mocking libs need to "inject" classes into existing classloaders, so the workaround is needed.

A trick to solve this might be using MethodHandles instead of Reflection using a "temporary" child class of ClassLoader (just to get a MethodHandle to the defineClass method). But this would require minimum Java 7. I may provide a PR to do this.

@raphw
Member
raphw commented Dec 26, 2016 edited

It's not that easy. Creating a new class loader causes the generated code to be loaded by another class loader. Even if the class's package name is the same, the loaded class will exist in a different runtime package what limits the scope of classes that can be subclassed. There is no backwards-compatible way for us to resolve this.

This issue is not easy to workaround and I still hoping that this will not make the final release of Java 9, this change would break thousands of projects. For now, I suggest to wait and see as creating a new class loader every time is a very breaking change.

This is also a major performance issue as class loaders are quite expensive to create.

As for Mockito: It does no longer use cglib in more recent versions but the problem prevails for any other code generation library.

@uschindler

As said before, I have a solution that works without reflection to achieve the same and requires no AccessController checks. Only limitation: Requires Java 7, but otherwise very easy! Maybe I post my idea here, let me just finish something else.

@raphw
Member
raphw commented Dec 26, 2016

I think I know what you mean by that. You want to create a handle to defineMethod within a subclass of class loader where you inherit the privileges of the surrounding class. This way, you can access the method from outside if you expose the reference to this handle.

Quite clever actually, we might pursue this.

@uschindler

Exactly! :-)

@uschindler

Nevertheless, we have a Lucene expressions module that subclasses ClassLoader base class all the time (whenever a user creates a query using the Lucene Expressions syntax). We have performance and garbage collection tests in Lucene and Elasticsearch that never ever caused any additional load. To me those "oh man thats slow" arguments are a no-go unless proven by benchmarks under normal workloads.

@raphw
Member
raphw commented Dec 26, 2016

This is no guess work, I do a lot of code generation (creator of Byte Buddy, do most of the Mockito-related code generation work and do code generation in Hibernate) and I ran a lot of benchmarks in the field of code generation. Creating dedicated class loaders has a very significant impact, I did benchmark this.

@uschindler

With Java 7 / 8?

@raphw
Member
raphw commented Dec 27, 2016 edited

Yes, I remember testing this with a recent Java 8 version. For generating a class with a dedicated class loader, it takes usually twice the time compared to injection.

@uschindler

Ok, for us this is not a problem at all. Securitywise, CGLIB should allow both variants, e.g. use the one with new ClassLoader if SecurityManager does not allow it (e.g., Elasticsearch forbids any setAccessible anywhere in its code or dependend libraries).

@uschindler
uschindler commented Dec 27, 2016 edited

Hi,
I just tried the MethodHandle hack: It does not work, because the Lookup object is "too intelligent". If you try to get a MethodHandle for a virtual method that is protected, it adds an explicit cast of the receiver to our lookup class. So the MH looks like this afterwards: MethodHandle(ClassLoaderAccessor,String,byte[],int,int,ProtectionDomain)Class
The first arg should be ClassLoader but was downcasted to our subclass ClassLoaderAccessor:

import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.ProtectionDomain;

public class Test {
  
  private static abstract class ClassLoaderAccessor extends ClassLoader {
    static final MethodHandle MH_DEFINE_CLASS;
    static {
      try {
        MH_DEFINE_CLASS = MethodHandles.lookup().findVirtual(ClassLoader.class, "defineClass",
            MethodType.methodType(Class.class, String.class, byte[].class, int.class, int.class, ProtectionDomain.class));
        System.out.println(MH_DEFINE_CLASS);
      } catch (NoSuchMethodException | IllegalAccessException e) {
        throw new AssertionError("Cannot find ClassLoader#defineClass(String, byte[], int, int, ProtectionDomain)", e);
      }
    }
  }
  
  public static void main(String[] args) throws Throwable {
    ClassLoaderAccessor.MH_DEFINE_CLASS.invoke(ClassLoader.getSystemClassLoader(), "foo.bar", new byte[10], 0, 10, null);
  }

}

See MethodHandles#restrictProtectedReceiver in JDK source code.

Javadocs in https://docs.oracle.com/javase/7/docs/api/java/lang/invoke/MethodHandles.Lookup.html#findVirtual(java.lang.Class,%20java.lang.String,%20java.lang.invoke.MethodType) says: "The first argument will be of type refc if the lookup class has full privileges to access the member. Otherwise the member must be protected and the first argument will be restricted in type to the lookup class."

@raphw
Member
raphw commented Dec 27, 2016

I stumbled upon the same problem, would have been too good to be true.

@magro magro referenced this issue in magro/kryo-serializers Dec 27, 2016
Open

Java 9 compatibility #71

@raphw
Member
raphw commented Dec 27, 2016

Just FYI, Byte Buddy added a fallback to using Unsafe for defining classes. To make use of this, use the newest version of Mockito 2 where this issue is worked around.

@uschindler
uschindler commented Dec 29, 2016 edited

Hi,
the Unsafe trick is good, because Unsafe is (at least in Java 9) reachable, if you have the right privileges. Cglib, should do the same.

About Byte-Buddy: Looks fine, although it has one big problem: Please use AccessController.doPrivileged when initializing and calling the Unsafe and ClassLoader hacks. Cglib does it, but Byte-Buddy does not: No-go for Elasticsearch or Lucene/Solr, which runs under SecurityManager (also its tests run with SM to ensure that nothing bad happens and we run with lowest possible privileges).

@raphw
Member
raphw commented Dec 29, 2016

The library is called Byte Buddy.

About the security manager issue: It is up to the users of the library to provide correct privileges. Otherwise, giving privileges to Byte Buddy would allow using the class injector to randomly inject classes to any user of the library. If one wants to use a security manager, the consumer of the library should assure that the injector is called with the correct privileges, anything else would offer an attack vector where getting hold of Byte Buddy classes would allow corrupting any privilege check by instrumentation. This is also implied by the documentation.

@uschindler

The problem is that the security check is done inside a static initializer and this caused much headache with Elasticsearch. You cannot really control when and by whom this static init block is called! That's why I am complaining. So correct would be the following: Get Unsafe/ClassLoader privates with own security, so one can grant ByteBuddy the correct privilege to access sun.misc.Unsafe in general.

On every call that causes injection of a class, just ask security manager if this is fine (using a RuntimePermission) and run the code without doPrivileged.

@raphw
Member
raphw commented Dec 29, 2016 edited

This is not true for Byte Buddy but it is for cglib. Byte Buddy attempts to set accessibility when attempting to inject a class. This causes the security manager to be asked.

@uschindler

To get declared fields, you also need privileges. Making them accessible is another thing.

@raphw
Member
raphw commented Jan 3, 2017

Added a fallback to Unsafe.

@raphw raphw closed this Jan 3, 2017
@julianhyde

@raphw Can you tell me which version of Mockito this issue is (or will be) fixed in? It's blocking us (see https://issues.apache.org/jira/browse/CALCITE-1568) and we don't use Cglib directly, only via Mockito.

@raphw
Member
raphw commented Jan 9, 2017

Its fixed in Mockito for a while. Mockito is using Byte Buddy where this issue is resolved since 2.0.

@julianhyde

@raphw I had no idea we were on an old version of Mockito. We'll upgrade. Thanks!

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