Reduce unsafe usages #14855
Reduce unsafe usages #14855
Conversation
| @SuppressWarnings("unused") | ||
| public static class LoadClassAdvice { | ||
|
|
||
| // Class loader stub is shaded back to the real class loader class. It is used to call protected |
There was a problem hiding this comment.
I tried multiple approaches here (check the commits if interested). Firstly I tried creating a MethodHandles.Lookup in the advice and passing it along. This allowed getting access to MethodHandles for findLoadedClass and defineClass. Unfortunately it didn't work on jdk8, apparently ClassLoader is a restricted class there for which you can't create a Lookup for. Didn't like that a special case was needed for jdk8. Next I tried writing the advice with asm. Since we need to call multiple methods it was a sizable chunk of asm so instead I tried what you see here. We use a modified stub ClassLoader class that has findLoadedClass and defineClass as public so we could use them in the advice. During the build we shade the stub to java.lang.ClassLoader. The bytecode works because this advice is inlined into subclasses of ClassLoader that can call these protected methods. It isn't ideal either as it required some build script hacking to make the shading work.
| // TODO by default, we use unsafe to define rest of the classes into boot loader | ||
| // can be disabled with -Dnet.bytebuddy.safe=true | ||
| // use -Dsun.misc.unsafe.memory.access=debug to check where unsafe is used | ||
| if (ClassInjector.UsingUnsafe.isAvailable() && !classnameToBytes.isEmpty()) { |
There was a problem hiding this comment.
The following ClassInjector.UsingInstrumentation works without unsafe but it needs to put the classes in to a jar and append that jar to boot class path. We could use that as a fallback, but the thing is that besides the virtual field classes that we defined above using lookup we have only a couple of helper classes that we need to add to boot loader. Could consider moving those classes to a bootstrap module. With indy we would be loading those helpers into separate class loader and that would also get rid of the unsafe usage here.
|
cc @SylvainJuge @JonasKunz in case you have a chance to review |
| // net.bytebuddy.dynamic.loading.ClassInjector$UsingUnsafe$Dispatcher$CreationAction | ||
| // we don't use byte-buddy nexus so we disable it and later restore the original value for the | ||
| // system property | ||
| String originalNexusDisabled = System.setProperty("net.bytebuddy.nexus.disabled", "true"); |
There was a problem hiding this comment.
[minor] Do you think it would be safer to wrap this in a try/finally or even a dedicated method to make it harder to forget about resetting this in the future ?
I think this could potentially impact the application if it relies on bytebuddy as well.
There was a problem hiding this comment.
moved it to separate method
There was a problem hiding this comment.
I don't know either, let's try to improve that later if we can.
| baseJavaagentLibs(project(":instrumentation:executors:javaagent")) | ||
| baseJavaagentLibs(project(":instrumentation:internal:internal-application-logger:javaagent")) | ||
| baseJavaagentLibs(project(":instrumentation:internal:internal-class-loader:javaagent")) | ||
| baseJavaagentLibs(project(":instrumentation:internal:internal-class-loader:javaagent", configuration = "shaded")) |
There was a problem hiding this comment.
do you think we could make this shaded configuration override the default one so we don't have to propagate similar changes wherever this module is used ? This could also remove the need to selectively exclude the default configuration in the change below.
There was a problem hiding this comment.
I don't know how to do that. It could be beneficial in other places too. If you know how this could be done then that would be awesome.
There was a problem hiding this comment.
This is an idea: #15083 - it might need some polishing, though in principle it should do this work.
4 participants
This PR aims to reduce using unsafe to get us closer to not causing the unsafe warning getting printed with latest java.