Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary classes

[Rafael Winterhalter]

Hei Mandy,

after further investigation I have found several other use cases of
sun.misc.Unsafe::defineClass for which the proposal for JDK-8200559 would
not currently offer a solution. I will try to describe a generic case of
the problem I found:

Java agents sometimes need to communicate state from instrumented classes
to a dispatcher that comes with the agent. However, a Java agent cannot
generally expect that any agent classes are visible to an application as a
Java agent is loaded on the class path whereas application classes might
not be loaded by the system class loader or any of its children. Therefore,
instrumented classes cannot typically callback into the agent. This is
typically solved by adding classes to the bootstrap class loader which is
universally visible.

Such a callback typically looks like this:

public abstract class Dispatcher {
  public volatile Dispatcher dispatcher;
  public abstract void receiveMessage(Object msg);
  public static void sendMessage(Object msg) {
    Dispatcher d = dispatcher;
    if (d != null) d.receiveMessage(msg);
  }
}

The agent can now register a subclass of the dispatcher in the field and
receive messages from any instrumented code throughout an application
without considering class loaders.

Adding this call would be trivial by using the
Instrumentation::appendToBootSearchPath method. However, using this method
has two major downsides:
1. The method accepts a file which must be stored on the local file system.
This can be really tricky in some cases, it is typically easier to just
define the classes as byte arrays and sun.misc.Unsafe::defineClass is a way
to avoid this problem.
2. Appending to the search path is only possible in the unnamed module
since Java 9. It is no longer possible to define the above dispatcher to be
located in java.util for instance as the new standard class loader does not
consider the new jar for known modules anymore. Defining the dispatcher in
a well known package has two major advantages:
  a) It is not necessary to alter the module graph to make sure that all
modules with instrumented classes read the boot loaders unnamed module.
This requires a lot of additional maintenance which impacts performance and
memory use. Also, bugs can trigger security risks if edges are added
excessively. With defining a class in java.util, this can be avoided since
the java.base module is impliiclty read by any module.
  b) Some class loaders filter packages that are considered to be loaded,
especially OSGi and JBoss modules class loaders. Those class loaders would
for example never attempt to find classes in the com.acme package on the
bootstrap loader which is why it has become a popular practice to add
classes to java.* packages for such universal dispatch. There are dozens of
such restricting class loaders and working around them is nearly impossible
as there are so many.

With the current consideration, it would instead be required to "pseudo
redefine" a class such as java.util.List only to inject the dispatcher into
the desired package. (Alternatively one can also open the module of
java.base and use a method handles lookup but this is a bad idea if the
Java agent runs on the unnamed system class loader.) Note that all this is
possible using standard API. It is even easier to do with JVMTI where class
definition is trivial.

For all the reasons I have mentioned in previous mails and also for this
additional use case I hope that adding a method for defining a class can be
added to the Instrumentation interface, it would definitely allow for the
cleanest, fastest and least error-prone migration while not inviting to the
mentioned work-arounds which are also currently favored by most teams
working with Java agents that I know of where many of them just open
jdk.internal.misc to use the new Unsafe class what is really unfortunate as
this is a chance to avoid use of internal API alltogether. This choice
would also reduce additional time for Java 11 being supported by all major
production monitoring tools that could just add simple switches to their
code to move from Unsafe to Instrumentation.

It would also be trivial to implement with an interface extension like:

interface Instrumentation {
  default Class<?> defineClass(byte[] bytes, ClassLoader loader,
ProtectionDomain pd) { throw new
UnsupportedOperationException("defineClass"); }
}

Thanks for considering my suggestion and your feedback!
Best regards, Rafael



2018-04-15 8:23 GMT+02:00 mandy chung <mandy.chung at oracle.com>:

> Background:
>
> Java agents support both load time and dynamic instrumentation.   At load
> time,
> the agent's ClassFileTransformer is invoked to transform class bytes.
> There is
> no Class objects at this time.  Dynamic instrumentation is when
> redefineClasses
> or retransformClasses is used to redefine an existing loaded class.  The
> ClassFileTransformer is invoked with class bytes where the Class object is
> present.
>
> Java agent doing instrumentation needs a means to define auxiliary classes
> that are visible and accessible to the instrumented class.  Existing agents
> have been using sun.misc.Unsafe::defineClass to define aux classes directly
> or accessing protected ClassLoader::defineClass method with setAccessible
> to
> suppress the language access check (see [1] where this issue was brought
> up).
>
> Instrumentation::appendToBootstrapClassLoaderSearch and
> appendToSystemClassLoaderSearch
> APIs are existing means to supply additional classes.  It's too limited
> for example it can't inject a class in the same runtime package as the
> class
> being transformed.
>
> Proposal:
>
> This proposes to add a new ClassFileTransformer.transform method taking
> additional ClassDefiner parameter.  A transformer can define additional
> classes during the transformation process, i.e.
> when ClassFileTransformer::transform is invoked.  Some details:
>
> 1. ClassDefiner::defineClass defines a class in the same runtime package
>    as the class being transformed.
> 2. The class is defined in the same thread as the transformers are being
>    invoked.   ClassDefiner::defineClass returns Class object directly
>    before the transformed class is defined.
> 3. No transformation is applied to classes defined by
> ClassDefiner::defineClass.
>
> The first prototype we did is to collect the auxiliary classes and define
> them  until all transformers are invoked and have these aux classes to go
> through the transformation pipeline.  Several complicated issues would
> need to be resolved for example timing whether the auxiliary classes
> should
> be defined before the transformed class (otherwise a potential race where
> some other thread references the transformed class and cause the code to
> execute that in turn reference the auxiliary classes.  The current
> implementation has a native reentrancy check that ensure one class is being
> transformed to avoid potential circularity issues.  This may need JVM TI
> support to be reliable.
>
> This proposal would allow java agents to migrate from internal API and
> ClassDefiner to be enhanced in the future.
>
> Webrev:
>    http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8200559/webrev.00/
>
> Mandy
> [1] http://mail.openjdk.java.net/pipermail/jdk-dev/2018-
> January/000405.html
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180529/b834b72a/attachment.html>