Avoiding sun.misc.Unsafe and embracing modules in Java libraries: missing links

[Rafael Winterhalter]

Hei Henri,

for Maven, Gradle and IDEs I think it is sensible to expect that these
runners would resolve such a test module unless it is explicitly specified
otherwise. Already today, those runners patch the module under test in
order to include the test code. I would expect of these runners to adopt
similarly to a test module as they adopted to the module system in general.

As for Spring's use case, I believe that the approach was chosen such that
class proxies resemble interface proxies as much as possible. In general
however, I think that Spring could just generate a subclass proxy and
delegate to the overridden methods instead of delegating to an instance of
the proxied class. If the proxy class mimics the proxied class's
constructors' signatures, this proxy can be instantiated exactly like the
proxied class would have been. This way, using Objenesis and the like is no
longer necessary and I argue that one should avoid exposing this
functionality if there is a reasonable alternative that does not require
any derivation from Javas programming model. As a matter of fact, I often
find developers confused about how fields are not set on such Spring
proxies and I even found possible exploits in code because of it. For
example, imagine a class:

class Foo {
  public final production;
  Foo(boolean production) { this.production = production; } // Only call
from unit test
  public Foo() { this(true); }
  public void doSomethingSensible() {
    if (production) { doSecurityCheck(); }
    // do something ...
  }
}

With Objenesis, you can create an instance of the above class and set it to
test mode with the production field being false as the default value. In
contrast, for the case of serialization, I already argued that support for
serializable types should probably go into MethodHandles.Lookup similar to
how defineClass was added. This does not render an additional security
threat, if Foo was serializable I could just synthetically create a
serialized vale of an instance with production being false and deserialize
this value in a production environment. On a side note, Spring would create
a class with the above field value automatically if Foo was a bean, making
this exploit very easy even when using a security manager as the proxy
instance is normally available to all sorts of code.

Finally, I disagree with the argument that this sort of API should be
accessible to "standard code" only shielded by a security manager because
this feature is useful today. The module system becomes useless the moment
you punch even a little hole into it. Therefore, I find it important to
remove sun.misc.Unsafe as quickly as possible as the module system can be
easily circumvented until it is removed and since the JPMS only show its
full benefit once this is no longer possible. But in order to keep testing
frameworks that have a good reason to break module boundaries up and
running, I think adding a test modules is a reasonable compromise. As an
additional benefit, it would allow libraries like Mockito that should not
be used in production to self-identify.

One could even argue that this ability to instantiate instance is nothing
that needs to be exposed to a Java agent so maybe this functionality could
also be part of the mentioned test module instead of being a part of
instrumentation. On the other hand, if there is a use case not being
considered for a Java agent, this agent might be tempted to open
jdk.internal.misc to gain this access, one might therefore just decide to
add the method to Instrumentation to offer an easy migration path to
applications that aim to migrate their code with a minimal delta.

Best regards, Rafael

2018-04-03 21:26 GMT+02:00 Henri Tremblay <henri.tremblay at gmail.com>:

> Hi Rafael,
>
> I don't like the idea to have to explicitly load a module to create mocks.
>
> Because Unsafe.allocateInstance is of course used to create mock but also
> proxies. For instance, Spring uses it extensively through Objenesis. And
> different frameworks doing serialization.
>
> Also, It means Gradle and Maven would need to be updated to load the
> module automatically in test context. And all IDE. A lot of annoyance for
> not much benefit since everybody is currently happy.
>
> I've been advocating that if creating a class without calling a
> constructor is useless in multiple contexts, it should be easily available
> through the API. I don't know exactly where to put it but it should be
>
>    - Available to all (java.base)
>    - Protected by the security manager
>
> For completeness, there are 4 ways to create a class without calling a
> constructor right now that I'm aware of:
>
>    - Unsafe.allocateInstance
>    - sun.reflect.ReflectionFactory.newConstructorForSerialization (my
>    favorite because it's the fastest one)
>    - Generate an extending class forgetting to call the super constructor
>    (so it's not exactly that same class that is instantiated). It requires
>    -Xverify:none
>    - Generate a class extending MagicAccessorImpl that will then
>    instantiates the wanted class but calling the wrong constructor
>
> Regards,
> Henri
>
>
>
> On 1 April 2018 at 17:02, Rafael Winterhalter <rafael.wth at gmail.com>
> wrote:
>
>> Hello,
>>
>> I am the/a maintainer of the libraries Byte Buddy, cglib and Mockito with
>> countless dependencies upstream and I wanted to give a summary of adopting
>> the JPMS and migrating away from sun.misc.Unsafe.
>>
>> 1. Java agents cannot define auxiliary classes.
>>
>> Byte Buddy does support the JPMS fully, however, it still relies on
>> sun.misc.Unsafe::defineClass for its Java agent API and currently breaks
>> on
>> Java 11 as this method was removed in a recent EA build. The reason for
>> using Unsafe is that many instrumentations need to define auxiliary
>> classes
>> to aid an instrumentation similar to javac which sometimes needs to define
>> anonymous classes or even synthetic classes. For example, if a Java agent
>> wants to register an event listener to some framework, such listeners
>> often
>> declare multiple methods what makes it impossible to fullfil the listener
>> contract using a lambda expression. Instead, one typically injects an
>> additional class into the same package as the instrumented class. In this
>> case, it is not possible to use MethodHandles.Lookup::defineClass as the
>> class file transformer does not necessarily have private access to the
>> lookup of the instrumented class.
>>
>> The current workarounds are:
>>
>> a) Open the package jdk.internal.misc to gain access to this package's
>> Unsafe class. This can be done via Instrumentation::redefineModule.
>> b) Open the java.lang package to access ClassLoader via reflection.
>> c) Open the java.lang package to access the internal lookup with global
>> access rights.
>>
>> Of these solutions only (b) relies on standard API and is guaranteed to
>> function in the future but the solution still feels hacky and does not
>> work
>> for instrumentations of classes on the bootstrap loader. Opening packages
>> also implies a risk of being applied carelessly since opening the package
>> to the agent's module most likely opens the package to the unnamed module
>> of the system class loader what invites to breaches of the JPMS
>> encapsulation by code that does not ship with the agent.
>>
>> To offer a better solution, I would like to suggest one of the following:
>>
>> a) Add a method defineClass(ClassLoader, byte[], ProtectionDomain) to the
>> Instrumentation interface that works similar to Unsafe::defineClass. This
>> would provide a very simple migration path. Since agents have access to
>> jdk.internal.misc, adding this method does not add any capabilities to the
>> agent, it merley avoids using internal API that might change.
>> b) Supply a MethodHandles.Lookup instance to the
>> ClassFileTransformer::transform API where the instance represents the
>> instrumented class's access rights. This does however carry the risk of
>> invoking the lookupClass method which would either load the instrumented
>> class prematurely causing a circularity error or return an unexpected
>> value
>> such as null. Since the lookup API generally relies on loaded types, there
>> are probably other problems such as invoking Lookup::privateLookupIn
>> before
>> all involved types are loaded.
>>
>> For the sake of simlicity and since easy migration paths make a quick
>> adoption easier, I would suggestion solution (a), also in the light that
>> quick and dirty migrations might still choose option (b) to save time and
>> also since (b) might cause problems when types are not yet loaded.
>>
>> 2. Java proxies cannot invoke default methods of proxied interfaces
>>
>> The Java proxy API does not currently allow the invocation of an
>> overridden
>> default method since
>> the InvocationHandler API only supplies an instance of
>> java.lang.reflection.Method. In Java 8, it was always possible to get hold
>> of method handle of any method of the proxied interface and to specialize
>> the handle on the interface type to invoke the default implementation.
>> With
>> the JPMS, even if an interface type is exported, this same type might not
>> be opened to another module. This implies that if an InvocationHandler is
>> defined by this module to which the interface is exported, this module's
>> InvocationHandler cannot resolve a specialized method handle to a default
>> method of the proxied interface. As a matter of fact, such a call is
>> impossible in this scenario whereas the same call is possible if the proxy
>> is implemented manually at compile time.
>>
>> As a solution, I suggest to provide an argument to the InvocationHandler
>> that represents a lookup instance with the access rights of the proxy
>> class. Using this lookup, the specialized method handles could be
>> resolved.
>>
>> 3. Mocking and serialization libraries still require
>> Unsafe::allocateInstance.
>>
>> For Mockito, it is required to instantiate any class without invoking a
>> constructor with potential side-effects. This is of course a grose
>> violation of Java's general contract for class instantiation but this
>> feature is very useful.
>>
>> Using a Java agent, it is already possible to emulate this feature without
>> internal API by instrumenting all constructors of all classes in the
>> hierarchy of a mocked class by transforming all constructors into the
>> following pseudo-code:
>>
>> SomeConstructor(Void arg) {
>>   if (MockitoBootHelper.THREAD_LOCAL.isMockInstantiatingMode()) {
>>     super(null); // any constructor of the super class with default values
>> for all arguments
>>   } else {
>>     // original constructor code...
>>   }
>> }
>>
>> Before instantiating a mock, the thread local value that is held by a
>> bootstrap-loader injected class is set to true such that a side-effect
>> free
>> construction is achieved.
>>
>> This is of course too expensive and has negative effects on performance
>> due
>> to additional branching and JIT-byte code limits such that one would
>> rather
>> open jdk.internal.misc to access the Unsafe instantiation mechanism if a
>> Java agent is already available.
>>
>> However, mocking and serialization libraries are not typically loaded as a
>> Java agent. Also, I think that the actual requirements are different. My
>> suggestion here is:
>>
>> a) For serialization libraries, I think that adding
>> MethodHandles.Lookup::newInstance(Class<? extends Serializable>) with
>> standard deserialization mechanics would be sufficient.
>> b) For mocking libraries, this does not suffice as mocks can be of any
>> class. I understand that this breaks encapsulation but for unit tests, I
>> argue that the benefit of using these libraries outweights the benefit of
>> full encapsulation within a unit test.
>>
>> As Mockito is typically run within a build which uses a JDK, we could
>> attach to the current VM using the attachment API. Since Java 9, this is
>> no
>> longer possible to attach to the same VM but instead we start a helper JVM
>> process that applies the attachment indirectly. Unfortunately, this is a
>> rather costly operation what is especially problematic when running a
>> single unit test. (The difference to this approach over Unsafe is about
>> half a second on average.)
>>
>> To overcome this, I would like to suggest to:
>>
>> a) Add a method Instrumentation::allocateInstance(Class). Java agents can
>> already emulate this privilege as described above, this is therefore
>> merely
>> a convenience.
>> b) Add a module jdk.test to JDK 11 with a class
>> JavaTest::getInstrumentation that returns an instrumentation instance for
>> the current JVM. This module should not be resolved by default but only
>> when requiring it explicitly similar to the EE modules after Java 9.
>>
>> I think this solution carries two benefits:
>>
>> a) Test libraries like Mockito can only be used in a testing scope. We
>> experience regularly that Mockito is used in production environments. The
>> library is not meant for that and we warn and document that this is not
>> intended use but people regularly ignore this directive. By requiring this
>> module, this form of abuse would no longer be possible and the JVM could
>> even give a meaningful error message if this use was intended.
>> b) The Instrumentation instance can be used for other meaningful testing
>> approaches. For example, Mockito offers an inline mock maker where the
>> mocking logic is inlined into a method body rather then creating a
>> subclass. This approach mainly targets final classes which have become
>> more
>> common especially since the Kotlin language became popular. To supply this
>> alternative mock maker, Mockito attempts attaching an agent to the current
>> VM (directly or indirectly, depending on the VM's version) which suffers
>> the additional costs for attaching an agent that I mentioned before.
>>
>> Thanks for reading this and I hope that you can consider these, as of
>> today, very practiced use cases. The JPMS migration has gone quite well, I
>> find. With these outstanding problems, JDK 11 could be the first release
>> where a majority of Java programs would no longer need to rely on internal
>> API.
>>
>> Best regards, Rafael
>>
>
>