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

Wed May 30 04:46:10 UTC 2018

Hi Rafael,

Thanks for the additional use cases of Unsafe::defineClass and looking
through many different agents to identify their migration path.

Summarize the requirements you collected (please correct/add):

1. need a means to define auxiliary classes in the same runtime package
    of an instrumented class, e.g. to bridge non-public members

2. need a means to define agent classes in a module that provides
    similar functionality as appendToBootstrapClassLoader and
    appendToSystemClassLoaderSearch

3. define a class in a different runtime package as the instrumented
    class.  It's still unclear how common this is (inject a subclass
    that depends on a package-private constructor)

The proposed API requires the agent to redefine the class in a
package that the agent intends to inject its class into (e.g.
redefine java.util.List in order to define java.util.AgentDispatcher).
The proposed API requires more work than the existing
Unsafe::defineClass that can define a class in any package but
still plausible, right?

We should explore other alternatives, if any.

Mandy
[1] 
http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-April/023535.html

On 5/29/18 2:17 PM, Rafael Winterhalter wrote:
> 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 
> <mailto: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/
>     <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
>     <http://mail.openjdk.java.net/pipermail/jdk-dev/2018-January/000405.html>
> 
> 