Skip to content
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

byte-buddy no longer runs on JDK11 b6 #428

Closed
sdfelts opened this issue Mar 28, 2018 · 11 comments
Closed

byte-buddy no longer runs on JDK11 b6 #428

sdfelts opened this issue Mar 28, 2018 · 11 comments
Assignees
Labels
Milestone

Comments

@sdfelts
Copy link

sdfelts commented Mar 28, 2018

Unsafe.defineClass was removed.
Caused by:
java.lang.UnsupportedOperationException: Cannot define class using
reflection
at
net.bytebuddy.dynamic.loading.ClassInjector$UsingReflection$Dispatcher$Unavail
able.defineClass(ClassInjec
tor.java:821)
at
net.bytebuddy.dynamic.loading.ClassInjector$UsingReflection.inject(ClassInject
or.java:185)

The quick fix to get a running jar again is to remove the JDK9 logic in src/main/java/net/bytebuddy/dynamic/loading/ClassInjector.java

@@ -9,7 +9,6 @@ import net.bytebuddy.dynamic.DynamicType;
import net.bytebuddy.dynamic.scaffold.subclass.ConstructorStrategy;
import net.bytebuddy.implementation.FixedValue;
import net.bytebuddy.implementation.MethodCall;
-import net.bytebuddy.utility.JavaModule;
import net.bytebuddy.utility.JavaType;
import net.bytebuddy.utility.RandomString;
import org.objectweb.asm.Opcodes;
@@ -298,9 +297,10 @@ public interface ClassInjector {
@SuppressFBWarnings(value = "REC_CATCH_EXCEPTION",
justification = "Exception should not be rethrown but trigger a fallback")
public Initializable run() {
try {

  •                    return JavaModule.isSupported() 
    
  •                            ? Dispatcher.Indirect.make() 
    
  •                            : Dispatcher.Direct.make(); 
    
  •                    return // JavaModule.isSupported() 
    
  •                            // ? Dispatcher.Indirect.make() 
    
  •                            // : 
    
  •                           Dispatcher.Direct.make(); 
                   } catch (Exception exception) { 
                       return new Unavailable(exception); 
    
@sdfelts
Copy link
Author

sdfelts commented Mar 28, 2018

Better stack trace:
Caused by:
java.lang.NoSuchMethodException: sun.misc.Unsafe.defineClass(java.lang.String, [B, int, int, java.lang.C
lassLoader, java.security.ProtectionDomain)
at java.base/java.lang.Class.getMethod(Class.java:2067)
at net.bytebuddy.dynamic.loading.ClassInjector$UsingUnsafe$Dispatcher$CreationAction.run(ClassInject
or.java:1260)
at net.bytebuddy.dynamic.loading.ClassInjector$UsingUnsafe$Dispatcher$CreationAction.run(ClassInject
or.java:1248)
at java.base/java.security.AccessController.doPrivileged(Native Method)
at net.bytebuddy.dynamic.loading.ClassInjector$UsingUnsafe.(ClassInjector.java:1127)
... 27 more

@AlanBateman
Copy link

Unsafe.defineClass has been terminally deprecated since JDK 9.

For proxy classes, then maybe the user of Byte Buddy can get the class bytes and use Lookup.defineClass to inject the class (as that is the only supported API for injecting classes).

@raphw
Copy link
Owner

raphw commented Mar 29, 2018

@sdfelts In what context are you experiencing this error? This should only happen if you are using ClassLoadingStrategy.Default.INJECTION which does indeed no longer work. This class loading strategy has been discouraged but there are still scenarios where using it is the best option. For the build, I will create a rule to exclude these tests if sun.misc.Unsafe is not available, only few tests rely on it.

If you are using this strategy, consider replacing it with ClassLoadingStrategy.UsingLookup which is what @AlanBateman suggested. It takes a method handle lookup as an argument and can define classes only in some packages and class loaders unlike the injection strategy.

@AlanBateman Thanks for reaching out directly. Unfortunately, Lookup::defineClass is insufficient in many cases. Using a method handle is for example inapplicable for:

  1. Java agents that need to define auxiliary classes for the instrumented types similar to javac sometimes needs to define inner classes to fulfill method contracts. As of today, only strange workarounds can achieve this as embedding the byte code in the instrumented class's constant pool as base64 string and using the method handle received from within this class. Unfortunately, the class file transformer is not provided an appropriate method handle what makes this necessary.
  2. Test frameworks such as Mockito have a good reason to define classes in foreign packages even without receiving a method handle lookup. This is only applicable in a test scope where security or encapsulation is not a big concern.

There are plenty of other workarounds as of today such as using a Java agent to open the jdk.internal.misc package, using JNI or extracting the IMPL_LOOKUP field from the MethodHandles.Lookup class by shifting some of the class's byte code to extract it to some accessible field. However, I hope that we can get it right this time such that all current useful Java libraries can continue to work without creating new hacks that will cause new disturbance in future Java releases.

I have not too long ago summarized my thoughts about the current state of affairs, knowing most low-level libraries fairly well but did unfortunately not receive any feedback. The easiest would be to add defineClass and newInstance methods to the Instrumentation interface as Java agents can already operate outside of the Java security model. If there was additionally a test module to get hold of Instrumentation where that module was not resolved unless explicitly added, this would allow test libraries like Mockito to get to do what they do today without potentially compromising production environments.

@raphw raphw self-assigned this Mar 29, 2018
@raphw raphw added the build label Mar 29, 2018
@raphw raphw added this to the 1.8.1 milestone Mar 29, 2018
@sdfelts
Copy link
Author

sdfelts commented Mar 29, 2018

This is failing in a nested call from Mockito. I'm not sure what that code is doing.

        at net.bytebuddy.dynamic.DynamicType$Default$Unloaded.load(DynamicType.java:4456)
        at org.mockito.internal.creation.bytebuddy.SubclassBytecodeGenerator.mockClass(SubclassBytecodeGenerator.j

ava:94)
at org.mockito.internal.creation.bytebuddy.TypeCachingBytecodeGenerator$1.call(TypeCachingBytecodeGenerato
r.java:37)
at org.mockito.internal.creation.bytebuddy.TypeCachingBytecodeGenerator$1.call(TypeCachingBytecodeGenerato
r.java:34)
at net.bytebuddy.TypeCache.findOrInsert(TypeCache.java:138)
at net.bytebuddy.TypeCache$WithInlineExpunction.findOrInsert(TypeCache.java:346)
at net.bytebuddy.TypeCache.findOrInsert(TypeCache.java:161)
at net.bytebuddy.TypeCache$WithInlineExpunction.findOrInsert(TypeCache.java:355)
at org.mockito.internal.creation.bytebuddy.TypeCachingBytecodeGenerator.mockClass(TypeCachingBytecodeGener
ator.java:32)
at org.mockito.internal.creation.bytebuddy.SubclassByteBuddyMockMaker.createMockType(SubclassByteBuddyMock
Maker.java:71)
at org.mockito.internal.creation.bytebuddy.SubclassByteBuddyMockMaker.createMock(SubclassByteBuddyMockMake
r.java:42)
at org.mockito.internal.creation.bytebuddy.ByteBuddyMockMaker.createMock(ByteBuddyMockMaker.java:25)
at org.mockito.internal.util.MockUtil.createMock(MockUtil.java:35)
...

@raphw
Copy link
Owner

raphw commented Mar 29, 2018

Mockito is one of the cases where class file injection is still essential and no good alternative is offered. Mockito often needs to define a class within another package in order to allow mocking of package-private classes and mocking of protected methods. Without Unsafe, this is not currently possible in such a general scope.

@sdfelts
Copy link
Author

sdfelts commented Mar 29, 2018

The current state is that byte-buddy doesn't work on JDK11 - there is no work-around because it's calling a method that doesn't exist.
By removing the JDK9 approach and using the pre-JDK9 code, the software works.
Even if JDK11 were locked down, there would be a work-around by using JVM options to open packages.
At this time, that seems to be the best approach.

@AlanBateman
Copy link

@raphw The OpenJDK mailing lists are the right place to bring up additional use-cases. The question of java agents instrumenting classes with references to auxiliary classes is something to bring to serviceability-dev as it's beyond the use-cases that the java.lang.instrument API was intended for. There may be a connection to the work in JEP 181 on nest based access control.

@raphw
Copy link
Owner

raphw commented Mar 31, 2018

@AlanBateman Is that really the right mailing list given that Mockito is more of a code testing tool and less about "debugging, profiling, monitoring, and management"? I have mentioned this topic on various OpenJDK mailing lists in the past but never got a proper response, therefore I am hesistant on using even more time on writing up my thoughts.

My suggestion still is to add defineClass and allocateInstance methods to the Instrumentation interface. Java agents can always get hold of the jdk.internal.misc.Unsafe instance and open the package to access it, this does therefore not add any security holes, quite the contrary, as Java agents are loaded on the class path in the unnamed module, having these methods available might reduce future security issues if agents just naively open unsafe to the entire class path. This way, we could avoid further hacks and finally rely on a stable API. This would really add to the stability of our libraries that are downloaded many million times every month and have become essential to the Java ecosystem. Furthermore, JVMTI agents have access to these methods already.

I find it unfortunate that this method is taken away just like that, especially with the wide use of Java agents that currently rely on this method to implement javac-like behavior and the large amount of testing tools such as Mockito that are now broken. Right now, Mockito does not work at all and the only idea I have is to use the mentioned Java agent to overcome this what would just imply more hacks that break in the future. With only these two methods, I do not think Unsafe would be needed anymore. But at the moment, no project can build on JDK 11 what is a large fraction of Java projects what always puts a very unfortunate pressure from upstream upon us.

Of course, to a given extend, we could use the lookup API but this will not work in many scenarios such as OSGi. I think it is legitimate that some libraries need to get around this.

@mlchung
Copy link

mlchung commented Apr 1, 2018

I have created https://bugs.openjdk.java.net/browse/JDK-8200559 to provide a replacement API for Unsafe::defineClass for java instrumentation agent.

@raphw
Copy link
Owner

raphw commented Apr 1, 2018

Brilliant, adding this method would solve 99% of my use cases and avoid the ugly workaround via jdk.internal.misc.

It also seems like using MethodHandles.Lookup::defineClass works for the majority of our use cases besides some obscure ones involving serialization which I doubt that those impact too many users.

I will try to get a release out asap and ask for some community feedback on the changes.

@raphw
Copy link
Owner

raphw commented Apr 8, 2018

Build does now work again. Mockito is also fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants