-
Notifications
You must be signed in to change notification settings - Fork 1k
Reduce unsafe usages #14855
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
Reduce unsafe usages #14855
Changes from 13 commits
c9f31da
569b517
6006123
ce39147
cf03cc8
8bd3f80
3e640a6
e35564e
e745dd3
c8fac05
a611bf7
ceb08b6
bd7c14e
65edf9a
b5cdeb4
324c20b
bba38fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| plugins { | ||
| id("otel.java-conventions") | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.javaagent.instrumentation.internal.classloader.stub; | ||
|
|
||
| import java.security.ProtectionDomain; | ||
|
|
||
| /** | ||
| * A placeholder for java.lang.ClassLoader to allow compilation of advice classes that invoke | ||
| * protected methods of ClassLoader (like defineClass and findLoadedClass). During the build we'll | ||
| * use shadow plugin to replace reference to this class with the real java.lang.ClassLoader. | ||
| */ | ||
| @SuppressWarnings("JavaLangClash") | ||
| public abstract class ClassLoader { | ||
| public abstract Class<?> findLoadedClass(String name); | ||
|
|
||
| public abstract Class<?> defineClass( | ||
| String name, byte[] b, int off, int len, ProtectionDomain protectionDomain); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,6 +131,16 @@ private static void installBytebuddyAgent( | |
| // https://bugs.openjdk.org/browse/JDK-8164165 | ||
| ThreadLocalRandom.current(); | ||
|
|
||
| // AgentBuilder.Default constructor triggers sun.misc.Unsafe::objectFieldOffset called warning | ||
| // AgentBuilder$Default.<init> | ||
| // -> NexusAccessor.<clinit> | ||
| // -> ClassInjector$UsingReflection.<clinit> | ||
| // -> ClassInjector$UsingUnsafe.<clinit> | ||
| // -> WARNING: sun.misc.Unsafe::objectFieldOffset called by | ||
| // 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"); | ||
|
||
| AgentBuilder agentBuilder = | ||
| new AgentBuilder.Default( | ||
| // default method graph compiler inspects the class hierarchy, we don't need it, so | ||
|
|
@@ -147,6 +157,13 @@ private static void installBytebuddyAgent( | |
| .with(AgentTooling.poolStrategy()) | ||
| .with(AgentTooling.transformListener()) | ||
| .with(AgentTooling.locationStrategy()); | ||
| // restore the original value for the nexus disabled property | ||
| if (originalNexusDisabled != null) { | ||
| System.setProperty("net.bytebuddy.nexus.disabled", originalNexusDisabled); | ||
| } else { | ||
| System.clearProperty("net.bytebuddy.nexus.disabled"); | ||
| } | ||
|
|
||
| if (JavaModule.isSupported()) { | ||
| agentBuilder = agentBuilder.with(new ExposeAgentBootstrapListener(inst)); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried multiple approaches here (check the commits if interested). Firstly I tried creating a
MethodHandles.Lookupin the advice and passing it along. This allowed getting access to MethodHandles forfindLoadedClassanddefineClass. Unfortunately it didn't work on jdk8, apparentlyClassLoaderis a restricted class there for which you can't create aLookupfor. 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 stubClassLoaderclass that hasfindLoadedClassanddefineClassas public so we could use them in the advice. During the build we shade the stub tojava.lang.ClassLoader. The bytecode works because this advice is inlined into subclasses ofClassLoaderthat can call these protected methods. It isn't ideal either as it required some build script hacking to make the shading work.